codeant-ai-for-open-source[bot] commented on code in PR #41754:
URL: https://github.com/apache/superset/pull/41754#discussion_r3529558409
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -113,26 +114,55 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
}
}, [columns]);
+ // A single effect owns every ownState write derived from render state.
+ // updateTableOwnState replaces ownState wholesale, so separate effects that
+ // each spread serverPaginationData in the same render would clobber one
+ // another's keys: clamping the current page, priming the raw-mode summary
+ // columns and nudging a re-query for missing totals must be one combined
+ // delta.
useEffect(() => {
- if (!serverPagination || !serverPaginationData || !rowCount) return;
+ const nextOwnState = { ...serverPaginationData };
+ let changed = false;
+
+ if (serverPagination && serverPaginationData && rowCount) {
+ const currentPage = serverPaginationData.currentPage ?? 0;
+ const currentPageSize = serverPaginationData.pageSize ??
serverPageLength;
+ const totalPages = Math.ceil(rowCount / currentPageSize);
+ if (currentPage >= totalPages && totalPages > 0) {
+ nextOwnState.currentPage = Math.max(0, totalPages - 1);
+ changed = true;
Review Comment:
**Suggestion:** The page-clamp condition treats `0` as falsy, so when total
rows drop to zero the clamp logic is skipped entirely. That leaves an
out-of-range `currentPage` in own state (for example after data shrinks),
causing inconsistent pagination state and invalid page display until another
state change fixes it. Guard on defined numeric values instead of truthiness.
[falsy zero check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Server-paginated AG-Grid table shows stale page index.
- ⚠️ Row numbering misaligned when filters yield zero rows.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an AG-Grid table chart with server pagination enabled
(`server_pagination`
true in form data) so that `transformProps` computes a positive `rowCount`
from queries
(`transformProps.ts:47-53` where `rowCount` is set from `countQuery` or
`baseQuery`).
2. Navigate to a later page using the grid pagination, which writes
`currentPage` into own
state via AG-Grid state conversion (`stateConversion.ts:19-21`) and
`updateTableOwnState`
(`utils/externalAPIs.ts:34-40`), so `serverPaginationData.currentPage > 0`.
3. Apply filters that shrink the result set to zero rows; the backend
returns `rowCount =
0` and `transformProps` passes `rowCount` to `TableChart`
(`transformProps.ts:49-53` and
`types.ts:15`, `AgGridTableChart.tsx:1-32`).
4. On this render, the pagination-own-state effect in `TableChart`
(`AgGridTableChart.tsx:64-75`) evaluates `if (serverPagination &&
serverPaginationData &&
rowCount)`. Because `rowCount` is `0` (falsy), the block that clamps
`currentPage` against
`totalPages` is skipped, leaving an out-of-range `currentPage` in own state;
subsequent
queries use this stale page index when calculating `row_offset`
(`buildQuery.ts:216-229`),
and `useColDefs` computes row numbering from the stale `currentPage`
(`useColDefs.ts:18-23, 31-35`), causing pagination UI to show an invalid
page until
another state change resets it.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7be527cf78bb4e77a162e24e2e331bb4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7be527cf78bb4e77a162e24e2e331bb4&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-ag-grid-table/src/AgGridTableChart.tsx
**Line:** 127:133
**Comment:**
*Falsy Zero Check: The page-clamp condition treats `0` as falsy, so
when total rows drop to zero the clamp logic is skipped entirely. That leaves
an out-of-range `currentPage` in own state (for example after data shrinks),
causing inconsistent pagination state and invalid page display until another
state change fixes it. Guard on defined numeric values instead of truthiness.
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%2F41754&comment_hash=bf7ab1d5785d3b005322cecd006903498b518a56ce6f5b0b9010259e1af7fefb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41754&comment_hash=bf7ab1d5785d3b005322cecd006903498b518a56ce6f5b0b9010259e1af7fefb&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -624,11 +624,19 @@ const buildQuery: BuildQuery<TableChartFormData> = (
// Create totals query AFTER all filters (including AG Grid filters) are
applied
// This ensures we can properly exclude AG Grid WHERE filters from the
totals
- if (
+ // In raw records mode the summary is a SUM over the numeric columns primed
+ // into ownState by the chart (see rawSummaryColumns in transformProps).
+ const rawSummaryColumns =
+ queryMode === QueryMode.Raw && formData.show_totals
+ ? ensureIsArray(ownState.rawSummaryColumns as string[] | undefined)
+ : [];
Review Comment:
**Suggestion:** The totals metrics are built directly from persisted
`ownState.rawSummaryColumns`, which can be stale across datasource changes. If
that state still contains column names that do not exist in the current
datasource, the generated SIMPLE metrics will fail backend validation and the
whole chart query can error before the chart has a chance to re-prime own
state. Filter/intersect these columns against current form/query columns before
emitting the totals query. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Raw records summary fails after datasource change.
- ❌ AG-Grid table query errors when summary columns stale.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create an AG-Grid table chart in Raw Records mode with “Show summary”
enabled so
`transformProps` computes `rawSummaryColumns` from the current datasource’s
numeric,
dataset-backed columns (`transformProps.ts:100-113`) and passes them as props
(`types.ts:34`, `AgGridTableChart.tsx:1-32); the chart’s effect primes
`ownState.rawSummaryColumns` via `updateTableOwnState`
(`AgGridTableChart.tsx:78-83`,
`utils/externalAPIs.ts:30-38`).
2. Change the chart’s datasource or column selection to one that does not
contain some of
the previously persisted summary column names, keeping Raw mode and
`show_totals` enabled;
Superset’s query pipeline reuses the prior `ownState` (including
`rawSummaryColumns`) via
`cachedBuildQuery` (`buildQuery.ts:120-137`, which passes
`options?.ownState` into
`buildQuery`).
3. On the first query after the change, before the new chart render can
re-prime own
state, `buildQuery` constructs `rawSummaryColumns` purely from the persisted
`ownState.rawSummaryColumns` when `queryMode === QueryMode.Raw &&
formData.show_totals`
(`buildQuery.ts:629-632`), and, because this array still contains column
names from the
old datasource, it builds a totals extra query with SIMPLE SUM metrics over
those names
(`buildQuery.ts:672-79`).
4. The backend validates these metrics against the new datasource schema;
since some
referenced columns no longer exist, the totals query fails and the entire
chart request
errors out, preventing `transformProps` from running and blocking the chart
from
re-priming `rawSummaryColumns` for the new datasource until the user
disables summary or
resets configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2f571303d8bd435fae5cd5b40dfbfe23&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2f571303d8bd435fae5cd5b40dfbfe23&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-ag-grid-table/src/buildQuery.ts
**Line:** 629:632
**Comment:**
*Api Mismatch: The totals metrics are built directly from persisted
`ownState.rawSummaryColumns`, which can be stale across datasource changes. If
that state still contains column names that do not exist in the current
datasource, the generated SIMPLE metrics will fail backend validation and the
whole chart query can error before the chart has a chance to re-prime own
state. Filter/intersect these columns against current form/query columns before
emitting the totals query.
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%2F41754&comment_hash=d4c8c97137028a7a23cbda6ed02733ad4e38f4e8630811a0b5df33d3cbf34e93&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41754&comment_hash=d4c8c97137028a7a23cbda6ed02733ad4e38f4e8630811a0b5df33d3cbf34e93&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]
