korbit-ai[bot] commented on code in PR #36083:
URL: https://github.com/apache/superset/pull/36083#discussion_r2517653492
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -237,6 +241,30 @@ export default typedMemo(function DataTable<D extends
object>({
...tableHooks,
);
+ const rowSignature = useMemo(
+ // sort the rows by id to ensure the total is not recalculated when the
rows are only reordered
+ () => rows.map(row => row.id).sort().join('|'),
+ [rows],
+ );
Review Comment:
### Unsafe row.id access without type guarantee <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The rowSignature calculation assumes all row objects have an 'id' property,
but this is not guaranteed by the generic type constraint 'D extends object'.
###### Why this matters
This will cause a runtime error when rows don't have an 'id' property, as
accessing undefined.id will throw an error during the map operation.
###### Suggested change ∙ *Feature Preview*
Add a safe accessor or use a different unique identifier:
```typescript
const rowSignature = useMemo(
() => rows.map((row, index) => row.id ?? index).sort().join('|'),
[rows],
);
```
Or use the row index as fallback:
```typescript
const rowSignature = useMemo(
() => rows.map((row, index) => String(row.id || index)).sort().join('|'),
[rows],
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c90d9-2c53-4daf-b656-52ac4ba176d2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c90d9-2c53-4daf-b656-52ac4ba176d2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c90d9-2c53-4daf-b656-52ac4ba176d2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c90d9-2c53-4daf-b656-52ac4ba176d2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c90d9-2c53-4daf-b656-52ac4ba176d2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6144e0ab-3420-4df2-9577-7f2720907085 -->
[](6144e0ab-3420-4df2-9577-7f2720907085)
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -257,6 +258,23 @@ const buildQuery: BuildQuery<TableChartFormData> = (
...moreProps,
};
+ if (formData.server_pagination) {
+ // Add search filter if search text exists
+ if (ownState.searchText && ownState?.searchColumn) {
+ queryObject = {
+ ...queryObject,
+ filters: [
+ ...(queryObject.filters || []),
+ {
+ col: ownState?.searchColumn,
+ op: 'ILIKE',
+ val: `${ownState.searchText}%`,
+ },
+ ],
+ };
+ }
+ }
Review Comment:
### Search filter may be overwritten by cached changes logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The search filter is now applied before the cached changes logic, which may
cause the search filter to be overwritten or reset when cached changes are
processed.
###### Why this matters
This could result in search functionality not working correctly when server
pagination is enabled, as the search filter might be lost during the cached
changes processing that follows immediately after.
###### Suggested change ∙ *Feature Preview*
Move the search filter logic back to after the cached changes processing, or
ensure that the cached changes logic preserves any existing search filters:
```typescript
if (
formData.server_pagination &&
options?.extras?.cachedChanges?.[formData.slice_id] &&
JSON.stringify(options?.extras?.cachedChanges?.[formData.slice_id]) !==
JSON.stringify(queryObject.filters)
) {
queryObject = { ...queryObject, row_offset: 0 };
const modifiedOwnState = {
...options?.ownState,
currentPage: 0,
pageSize: queryObject.row_limit ?? 0,
};
updateTableOwnState(options?.hooks?.setDataMask, modifiedOwnState);
}
// Add search filter after cached changes processing
if (formData.server_pagination) {
if (ownState.searchText && ownState?.searchColumn) {
queryObject = {
...queryObject,
filters: [
...(queryObject.filters || []),
{
col: ownState?.searchColumn,
op: 'ILIKE',
val: `${ownState.searchText}%`,
},
],
};
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d43cf1b-e3d4-4507-bb73-c7942e180513/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d43cf1b-e3d4-4507-bb73-c7942e180513?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d43cf1b-e3d4-4507-bb73-c7942e180513?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d43cf1b-e3d4-4507-bb73-c7942e180513?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d43cf1b-e3d4-4507-bb73-c7942e180513)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d07f9ef2-f2c5-42c6-8fdf-4902e10d95e7 -->
[](d07f9ef2-f2c5-42c6-8fdf-4902e10d95e7)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
