codeant-ai-for-open-source[bot] commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3382272237
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1524,10 +1536,13 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
const modifiedOwnState = {
...serverPaginationData,
sortBy,
+ // Changing the sort re-queries the full dataset, so the
+ // previous page offset is meaningless — return to the first page.
+ currentPage: 0,
};
updateTableOwnState(setDataMask, modifiedOwnState);
},
- [serverPagination, serverPaginationData, setDataMask],
+ [serverPaginationData, setDataMask],
Review Comment:
**Suggestion:** The callback dependency list is incomplete: it reads
`serverPagination` inside `handleSortByChange` but does not include it in the
`useCallback` dependencies. If `serverPagination` flips (for example when
result truncation state changes), this callback can keep a stale value and
either skip required server re-sorts or keep issuing server sort updates when
it should not. Add `serverPagination` to the dependency array so sorting
behavior stays in sync with current mode. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Table chart server-side sorting ignores runtime mode changes.
- ⚠️ Explore users see inconsistent sorting after toggling pagination.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/plugins/plugin-chart-table/src/transformProps.ts`
lines 521–523,
the table chart's `server_pagination` control from `rawFormData` is mapped
into the
boolean `serverPagination` prop returned from `transformProps` and passed
into
`TableChart` via `TableChartTransformedProps` (see `src/types.ts` lines 5–13
where
`serverPagination: boolean; serverPaginationData; setDataMask` are defined).
2. In `superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx`
lines 392–52,
`TableChart` destructures `serverPagination = false`,
`serverPaginationData`, and
`setDataMask` from `props`, and later defines `handleSortByChange` at lines
54–66 (from
the `Read` output) as:
- `if (!serverPagination) return;`
- build `modifiedOwnState` from `serverPaginationData` with the new
`sortBy` and
`currentPage: 0`
- call `updateTableOwnState(setDataMask, modifiedOwnState)`
This callback is memoized with `useCallback` and a dependency array that
currently only
includes `[serverPaginationData, setDataMask]` (line 1545 in the diff).
3. When a user toggles the "Server pagination" control for a table chart in
Explore,
`transformProps` recomputes props with `serverPagination` flipped from
`false` to `true`
(or vice versa) while keeping the same `serverPaginationData` object and
`setDataMask`
hook reference (see `transformProps.ts` lines 36–43 and 39–41 where
`serverPagination`
controls whether `serverPaginationData` comes from ownState or defaults).
Because
`useCallback` in `TableChart.tsx` depends only on `[serverPaginationData,
setDataMask]`,
React reuses the previous `handleSortByChange` function instance, whose
closure still
captures the old `serverPagination` boolean.
4. After this toggle, when the user clicks a column header to sort, the
`DataTable`
component in
`superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx`
lines 142–162 detects a sort change (`sortBy`) and, if `serverPagination` is
true, calls
`handleSortByChange`:
- `const serverSortBy = serverPaginationData?.sortBy || [];`
- `if (serverPagination && !isEqual(sortBy, serverSortBy)) { ...
handleSortByChange([...]); }`
At this point `DataTable` sees `serverPagination === true` (new prop) and
invokes
`handleSortByChange`, but inside that callback the captured
`serverPagination` is still
`false`, so it immediately returns and never calls `updateTableOwnState`.
As a result,
no server-side `sortBy` is pushed into `serverPaginationData`, the
chart-data request
is not reissued, and the "server-side column sorting" path described in
this PR
silently fails after toggling server pagination at runtime. Adding
`serverPagination`
to the `useCallback` dependency array forces `handleSortByChange` to be
recreated when
the mode changes, keeping the sort behavior in sync with the current
pagination mode.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0836872ca2734ae7ba34889484dc2b2c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0836872ca2734ae7ba34889484dc2b2c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 1545:1545
**Comment:**
*Logic Error: The callback dependency list is incomplete: it reads
`serverPagination` inside `handleSortByChange` but does not include it in the
`useCallback` dependencies. If `serverPagination` flips (for example when
result truncation state changes), this callback can keep a stale value and
either skip required server re-sorts or keep issuing server sort updates when
it should not. Add `serverPagination` to the dependency array so sorting
behavior stays in sync with current mode.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=c251767dd6e168555467c1fce79f6e2364278916b46b7653a28c1c141a355853&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=c251767dd6e168555467c1fce79f6e2364278916b46b7653a28c1c141a355853&reaction=dislike'>👎</a>
--
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]
