feat(table v2): add tooltip to table header#39287
Conversation
This reverts commit 35aaeb6.
Code Review Agent Run #0b12a9Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39287 +/- ##
=======================================
Coverage 64.58% 64.58%
=======================================
Files 2564 2564
Lines 133560 133565 +5
Branches 31032 31035 +3
=======================================
+ Hits 86256 86267 +11
+ Misses 45812 45806 -6
Partials 1492 1492
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run #33c9dbActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #05832dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| @@ -384,6 +385,17 @@ const processColumns = memoizeOne(function processColumns( | |||
| ? config.currencyFormat | |||
| : savedCurrency; | |||
|
|
|||
| const metricLookupKey = key.startsWith('%') ? key.slice(1) : key; | |||
| const description = | |||
| rawDatasource.columns?.find( | |||
| (item: { column_name?: string; description?: string | null }) => | |||
| item.column_name === key, | |||
| )?.description ?? | |||
| rawDatasource.metrics?.find( | |||
| (item: { metric_name?: string; description?: string | null }) => | |||
| item.metric_name === metricLookupKey, | |||
| )?.description; | |||
There was a problem hiding this comment.
Suggestion: processColumns is memoized with a custom equality function, but this new logic now depends on rawDatasource and the comparator does not account for rawDatasource.columns/rawDatasource.metrics. When datasource metadata changes (for example, updated column/metric descriptions) while other compared fields stay the same, memoization will incorrectly reuse stale column metadata and tooltip descriptions will not refresh. Include rawDatasource fields in the isEqualColumns comparison (or remove the custom comparator) so description changes invalidate the memoized result. [logic error]
Severity Level: Minor 🧹
- ⚠️ Table V2 header tooltips show outdated column descriptions.
- ⚠️ Dataset metadata edits not reflected until cache invalidation.Steps of Reproduction ✅
1. Render any Table V2 chart (AgGridTable) so that the plugin's `transformProps` runs with
current chart props. `AgGridTableChartPlugin` registers `transformProps` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/index.ts:21-32`, and
`transformProps` calls `processColumns(chartProps)` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:676`.
2. On this first render, `processColumns` is executed and computes column metadata,
including descriptions derived from the dataset metadata (`rawDatasource`). This happens
in `superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:17-31`
(props destructuring including `rawDatasource`) and lines 69-78 where:
- `const description = …` looks up `rawDatasource.columns` and `rawDatasource.metrics`
based on the column key and `metricLookupKey`.
- The resulting `description` is stored on each column object returned from
`processColumns`.
These columns (with `description`) are then passed through `transformProps` into the
chart component.
3. The AG Grid header tooltips for this chart are taken directly from the column
descriptions. In
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:212-229`,
`useColDefs` builds column definitions, and `getCommonColProps` returns a `ColDef` with
`headerTooltip: col.description` at `useColDefs.ts:21-25`. Thus, whatever `processColumns`
computed in step 2 is what the user sees as the header tooltip.
4. Still on the same page (same JS runtime), update only the dataset metadata (for
example, change a column or metric description in the dataset editor) and re-run or
refresh the chart so that new `ChartProps` are constructed with an updated `rawDatasource`
but identical values for the fields compared in `isEqualColumns`. The `processColumns`
memoization uses `memoizeOne(..., isEqualColumns)` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:17-19` and the
comparator in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/isEqualColumns.ts:22-45`
only checks:
- `a.datasource.columnFormats`, `a.datasource.currencyFormats`,
`a.datasource.verboseMap`
- `a.formData.tableTimestampFormat`, `a.formData.timeGrainSqla`
- `a.formData.metrics`, `a.queriesData[0].colnames`, `a.queriesData[0].coltypes`
- `a.formData.extraFilters`, `a.formData.extraFormData`
- `a.rawFormData.column_config`
It never inspects `a.rawDatasource` or `b.rawDatasource`. If the query shape and form
data are unchanged, all these compared fields remain equal even though
`rawDatasource.columns`/`rawDatasource.metrics` now contain new `description` values.
`isEqualColumns` returns `true`, so `memoizeOne` reuses the previous `columns` array
and does not re-run `processColumns`. As a result, `useColDefs` continues to use the
stale `col.description` values for `headerTooltip`, and the user sees outdated tooltips
that do not reflect the updated dataset metadata.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 348:397
**Comment:**
*Logic Error: `processColumns` is memoized with a custom equality function, but this new logic now depends on `rawDatasource` and the comparator does not account for `rawDatasource.columns`/`rawDatasource.metrics`. When datasource metadata changes (for example, updated column/metric descriptions) while other compared fields stay the same, memoization will incorrectly reuse stale column metadata and tooltip descriptions will not refresh. Include `rawDatasource` fields in the `isEqualColumns` comparison (or remove the custom comparator) so description changes invalidate the memoized result.
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
|
Thanks for the contribution! Looks solid — AG Grid's Stale memoization bug ( Percent-metric key stripping: The Otherwise the diff is clean and the feature is a useful parity improvement with the existing table plugin. |
SUMMARY
This Pr adds to AgGridTable tooltip to table header
As previously added to the table in PR #37179
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION