codeant-ai-for-open-source[bot] commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3590363334
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -439,6 +557,18 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
[setDataMask, serverPagination],
);
+ // Feeds the "Export Current View" menu item (EXPORT_CURRENT_VIEW behavior),
+ // mirroring Table V1's clientView snapshot on ownState.
+ const handleClientViewChange = useCallback(
+ (clientView: ClientViewSnapshot) => {
+ updateTableOwnState(setDataMask, {
+ ...serverPaginationData,
+ clientView,
+ });
+ },
Review Comment:
**Suggestion:** `updateTableOwnState` replaces `ownState` wholesale, so
writing `clientView` from a render-captured `serverPaginationData` snapshot can
overwrite newer `ownState` keys written by other handlers/effects (for example
pagination/filter/totals flags) when events interleave. Merge against the
latest `ownState` at write time (or isolate `clientView` updates so they cannot
clobber unrelated keys). [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Export Current View snapshot may omit latest filters.
- ⚠️ Pagination/filter ownState can diverge from chartState.
- ⚠️ TotalsRequested flag may be reset unexpectedly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an AG Grid Table V2 chart, which renders via `TableChart` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:64-101`
and
receives `serverPaginationData` and `setDataMask` as props representing the
chart's
`ownState`.
2. Trigger a server-side filter change (e.g., update a column filter in the
grid). This
calls `handleFilterChanged` at `AgGridTableChart.tsx:202-248`, which builds
`modifiedOwnState = { ...serverPaginationData, agGridFilterModel,
agGridSimpleFilters,
agGridComplexWhere, agGridHavingClause, lastFilteredColumn,
lastFilteredInputPosition,
currentPage, metricSqlExpressions }` and then calls
`updateTableOwnState(setDataMask,
modifiedOwnState)` at `AgGridTableChart.tsx:238`, causing
`updateTableOwnState` in
`utils/externalAPIs.ts:46-51` to replace `ownState` wholesale.
3. Before a re-render updates `serverPaginationData` to include those new
filter-related
keys, the grid triggers a client-view snapshot update (for
export-current-view behavior).
This calls `handleClientViewChange` at `AgGridTableChart.tsx:163-170`, which
closes over
the previous `serverPaginationData` and calls
`updateTableOwnState(setDataMask, {
...serverPaginationData, clientView })` using that stale snapshot.
4. Because `updateTableOwnState` simply sets `ownState: modifiedOwnState`
(see
`externalAPIs.ts:46-51`) and does not merge with the latest `ownState`, the
second write
built from the older `serverPaginationData` overwrites newer fields (like
`agGridFilterModel`, `totalsRequested`, or pagination adjustments) written by
`handleFilterChanged` or the render-driven `useEffect` at
`AgGridTableChart.tsx:127-180`,
leaving `ownState` inconsistent with the user's actual filters/pagination.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9fafbc7169634dec8ff2347af2b16514&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=9fafbc7169634dec8ff2347af2b16514&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:** 565:568
**Comment:**
*Race Condition: `updateTableOwnState` replaces `ownState` wholesale,
so writing `clientView` from a render-captured `serverPaginationData` snapshot
can overwrite newer `ownState` keys written by other handlers/effects (for
example pagination/filter/totals flags) when events interleave. Merge against
the latest `ownState` at write time (or isolate `clientView` updates so they
cannot clobber unrelated keys).
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%2F42088&comment_hash=6f4fc63c7de173666fb4c35923719eeed79c50e962c860ea0bd9495716e68bea&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=6f4fc63c7de173666fb4c35923719eeed79c50e962c860ea0bd9495716e68bea&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -18,8 +18,10 @@
*/
import { t } from '@apache-superset/core/translation';
import {
Review Comment:
**Suggestion:** The temporal drill-to-detail path assumes every non-null
temporal value is a valid date; if parsing produces an invalid date,
`toISOString()` throws and the context menu action crashes. Validate the parsed
date before building the temporal range (and fall back to a safe equality/null
filter when invalid). [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Drill-to-detail context menu crashes on malformed temporal values.
- ⚠️ Users lose drill actions for affected table charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an AG Grid Table V2 chart using a temporal column configured with
`dataType ===
GenericDataType.Temporal` and a `timeGrain` (the drill columns are computed
in
`TableChart` at `AgGridTableChart.tsx:381-383` and the temporal branch
executes inside
`handleContextMenu`).
2. Ensure the backend returns a row where that temporal field is a non-null
value that
cannot be parsed into a valid JavaScript `Date` (for example, a malformed
timestamp
string). When a context-menu is opened on any cell, `handleContextMenu` at
`AgGridTableChart.tsx:385-88` iterates `drillColumns`, obtains
`dataRecordValue =
rowData[col.key]`, and for temporal columns runs the branch at
`AgGridTableChart.tsx:19-29`.
3. In that branch, `startTime` is computed as `dataRecordValue instanceof
Date ?
dataRecordValue : new Date(dataRecordValue as string | number)`. For the
malformed value,
`new Date(...)` yields an `Invalid Date` instance; this is then passed to
`getTimeRangeFromGranularity(startTime, timeGrain)` at
`AgGridTableChart.tsx:25-28`, which
in `utils/getTimeRangeFromGranularity.ts:25-79` calls `startTime.getTime()`
and constructs
new `Date` objects that remain invalid.
4. `handleContextMenu` then builds `timeRangeValue =
\`${rangeStartTime.toISOString()} :
${rangeEndTime.toISOString()}\`` at `AgGridTableChart.tsx:29`. Calling
`.toISOString()` on
an `Invalid Date` throws a `RangeError: Invalid time value`, which bubbles
out of
`handleContextMenu`, causing the right-click drill-to-detail action to crash
for that
chart instance.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a751202d83384917b78451095fc16528&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=a751202d83384917b78451095fc16528&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:** 20:29
**Comment:**
*Logic Error: The temporal drill-to-detail path assumes every non-null
temporal value is a valid date; if parsing produces an invalid date,
`toISOString()` throws and the context menu action crashes. Validate the parsed
date before building the temporal range (and fall back to a safe equality/null
filter when invalid).
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%2F42088&comment_hash=c285523f3b26b32db821eee930cefbd61c6191b347eaed878f8c241fec99bc64&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=c285523f3b26b32db821eee930cefbd61c6191b347eaed878f8c241fec99bc64&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]
