codeant-ai-for-open-source[bot] commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3590346995
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -416,6 +422,41 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps>
= memo(
serverPaginationData?.agGridFilterModel,
]);
+ // Captures the "current view" (post-filter/sort, all rows across all
+ // pages) for the "Export Current View" menu, mirroring Table V1's
+ // clientView snapshot. Client-side mode only: in server pagination mode
+ // the grid only ever holds a single page's rows, so a client-derived
+ // snapshot can't represent the full filtered/sorted result and export
+ // falls back to a fresh backend query instead (see
useExploreAdditionalActionsMenu).
+ const lastClientViewSignatureRef = useRef<string | null>(null);
+ const handleModelUpdated = useCallback(() => {
+ if (serverPagination || !onClientViewChange || !gridRef.current?.api) {
+ return;
+ }
+ const { api } = gridRef.current;
+ const displayedColumns = api
+ .getAllDisplayedColumns()
+ .filter(column => column.getColId() !== ROW_NUMBER_COL_ID);
+ const columns = displayedColumns.map(column => ({
+ key: column.getColId(),
+ label: column.getColDef().headerName || column.getColId(),
+ }));
+
+ const rows: Record<string, unknown>[] = [];
+ api.forEachNodeAfterFilterAndSort(node => {
+ if (node.data) {
+ rows.push(node.data);
+ }
+ });
+
+ const signature = `${rows.length}|${columns.map(c => c.key).join(',')}`;
+ if (signature === lastClientViewSignatureRef.current) {
+ return;
+ }
+ lastClientViewSignatureRef.current = signature;
+ onClientViewChange({ rows, columns, count: rows.length });
Review Comment:
**Suggestion:** The client-view deduplication key only uses row count and
column keys, so it misses updates when row values/order change but the count
and visible columns stay the same. This leaves `ownState.clientView` stale and
makes “Export Current View” download outdated data after sorts/filters that
keep the same cardinality. Include row content/order (or a robust hash of the
filtered+sorted node IDs/data) in the signature, or remove this early-return
optimization. [cache]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Exported CSV data mismatches current table view.
- ⚠️ Client-side table exports ignore latest sort/filter changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a Table V2 (AG Grid) chart in Explore using viz_type
"ag-grid-table"; the plugin
wiring is defined in
superset-frontend/plugins/plugin-chart-ag-grid-table/src/index.ts:16-27 where
AgGridTableChartPlugin registers AgGridTableChart and buildQuery.
2. Observe that AgGridTableChart renders AgGridDataTable and passes
onClientViewChange
(handleClientViewChange) into it at
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:21-30
and
48-89, and ThemedAgGridReact wires onModelUpdated={handleModelUpdated} at
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:555-579.
3. In client-side pagination mode (formData.server_pagination is false, so
serverPagination prop is false), sort a column without changing filters; Ag
Grid fires
onModelUpdated, which calls handleModelUpdated at
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:53-79.
This
builds rows via api.forEachNodeAfterFilterAndSort and then computes
signature =
`${rows.length}|${columns.map(c => c.key).join(',')}` at lines 73-78.
Because a pure sort
leaves rows.length and displayed column keys unchanged, signature matches
lastClientViewSignatureRef.current and the function returns early without
calling
onClientViewChange.
4. Trigger "Export Current View" from the Explore additional actions menu;
the hook in
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx:117-120
detects EXPORT_CURRENT_VIEW behavior and passes ownState (including
clientView) into
exportChart in exportCSV at lines 56-115, and exportChart in
superset-frontend/src/explore/exploreUtils/index.ts:20-50 forwards ownState
into
buildV1ChartDataPayload at lines 132-152 and 339-50. Because
onClientViewChange was
skipped, ownState.clientView still reflects the previous unsorted view, so
the CSV
download rows/order differ from the currently sorted grid.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48e27666b11747c3bbc9c816a585c2d1&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=48e27666b11747c3bbc9c816a585c2d1&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/AgGridTable/index.tsx
**Line:** 452:457
**Comment:**
*Cache: The client-view deduplication key only uses row count and
column keys, so it misses updates when row values/order change but the count
and visible columns stay the same. This leaves `ownState.clientView` stale and
makes “Export Current View” download outdated data after sorts/filters that
keep the same cardinality. Include row content/order (or a robust hash of the
filtered+sorted node IDs/data) in the signature, or remove this early-return
optimization.
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=cb31d30ea7fed048e06a7f4c1d762945ec2bcc2912b29e1fdf761d7a29f136a3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=cb31d30ea7fed048e06a7f4c1d762945ec2bcc2912b29e1fdf761d7a29f136a3&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -395,6 +395,25 @@ const buildQuery: BuildQuery<TableChartFormData> = (
const extraQueries: QueryObject[] = [];
+ const calculationMode = formData.percent_metric_calculation || 'row_limit';
+
+ if (
+ calculationMode === 'all_records' &&
+ percentMetrics &&
+ percentMetrics.length > 0
+ ) {
+ extraQueries.push({
+ ...queryObject,
+ columns: [],
+ metrics: percentMetrics,
+ post_processing: [],
+ row_limit: 0,
+ row_offset: 0,
+ orderby: [],
+ is_timeseries: false,
+ });
+ }
Review Comment:
**Suggestion:** The percent-metric “all records” extra query is built too
early from an intermediate `queryObject` before later mutations add interactive
group-by/search/AG Grid filter SQL clauses, so the extra query can run with
different filters than the main query. Build this extra query from the
finalized `queryObject` after all query mutations are applied to keep
percent-metric denominators consistent with the displayed result set. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Percent metrics use wrong denominators when filters applied.
- ⚠️ Exported CSV values differ from onscreen percentage view.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Table V2 (AG Grid) chart with percent metrics and set the
"Percent metric
calculation" control to "All records"; this control is wired in
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:448-450
where
name "percent_metric_calculation" is bound to
percentMetricCalculationControl, producing
formData.percent_metric_calculation === 'all_records'.
2. Enable server pagination for the chart and interactively filter or search
using the AG
Grid UI so that ownState carries searchText/searchColumn and AG Grid
filters; these are
consumed in buildQuery via ownState.searchText, ownState.searchColumn,
ownState.agGridSimpleFilters, ownState.agGridComplexWhere, and
ownState.sqlClauses in
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:23-69
and 133-169,
172-200.
3. From Explore, either run the chart or use the CSV export action;
useExploreAdditionalActionsMenu calls exportChart with latestQueryFormData
and ownState at
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx:56-99,
and exportChart delegates to buildV1ChartDataPayload, which retrieves the
chart plugin's
buildQuery function from the registry and invokes
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts via
getChartBuildQueryRegistry (exploreUtils/index.ts:132-152, 339-50; plugin
registration at
plugins/plugin-chart-ag-grid-table/src/index.ts:16-27).
4. Inside AG Grid buildQuery, extraQueries for percent metrics are created
immediately
after queryObject initialization using the intermediate queryObject at
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:37-56,
before
interactive_groupby is merged (lines 58-63) and before server-pagination
search and AG
Grid WHERE/HAVING SQL clauses are applied (lines 23-69 and 133-169,
172-200). The main
queryObject is later mutated to include those filters, but the previously
built extra
percent-metric query retains the unfiltered state. As a result, when
calculationMode ===
'all_records' with percentMetrics, the denominators used for percent-metric
contribution
post-processing are based on a different filtered dataset than the displayed
result set,
leading to inconsistent percent values in the chart and exported CSV.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6b3efcc4726548e4af61870035258e7a&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=6b3efcc4726548e4af61870035258e7a&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:** 398:415
**Comment:**
*Logic Error: The percent-metric “all records” extra query is built too
early from an intermediate `queryObject` before later mutations add interactive
group-by/search/AG Grid filter SQL clauses, so the extra query can run with
different filters than the main query. Build this extra query from the
finalized `queryObject` after all query mutations are applied to keep
percent-metric denominators consistent with the displayed result set.
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=56a7868d3a6fa4906262a4cdb62a876384a5e80425acd8bd8d68207656c01fae&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=56a7868d3a6fa4906262a4cdb62a876384a5e80425acd8bd8d68207656c01fae&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]
