Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
19151d1
fix: fixed Add Metrics to Tree Chart (#29158)
SBIN2010 Oct 22, 2024
35aaeb6
fix: clear modal after CSS templates is added
SBIN2010 Oct 22, 2024
db3afda
Revert "fix: clear modal after CSS templates is added"
SBIN2010 Oct 23, 2024
9eea2e6
fix: tree metrics exclude validator
SBIN2010 Nov 25, 2024
3930c95
Merge branch 'apache:master' into master
SBIN2010 Mar 11, 2025
b355212
Merge branch 'apache:master' into master
SBIN2010 Mar 18, 2025
7200d6c
Merge branch 'apache:master' into master
SBIN2010 Mar 20, 2025
3049a3d
Merge branch 'apache:master' into master
SBIN2010 Mar 25, 2025
247aef8
Merge branch 'apache:master' into master
SBIN2010 Apr 2, 2025
b4678ec
Merge branch 'apache:master' into master
SBIN2010 Apr 4, 2025
0ae844d
Merge branch 'apache:master' into master
SBIN2010 Jul 1, 2025
49fc529
Merge branch 'apache:master' into master
SBIN2010 Jul 10, 2025
601dde2
fix: revert #30679
SBIN2010 Mar 25, 2025
66823a5
Merge branch 'apache:master' into master
SBIN2010 Jul 12, 2025
4f59629
Merge branch 'apache:master' into master
SBIN2010 Jul 16, 2025
a7ce4e9
Merge branch 'apache:master' into master
SBIN2010 Jul 20, 2025
1bf4fdd
fix
SBIN2010 Jul 20, 2025
1241867
Merge branch 'apache:master' into master
SBIN2010 Jul 31, 2025
045cf6b
Merge branch 'apache:master' into master
SBIN2010 Aug 2, 2025
acdba8a
Merge branch 'apache:master' into master
SBIN2010 Aug 10, 2025
4fb89c2
Merge branch 'apache:master' into master
SBIN2010 Aug 15, 2025
f47d28b
Merge branch 'apache:master' into master
SBIN2010 Aug 18, 2025
3bdf337
Merge branch 'apache:master' into master
SBIN2010 Aug 19, 2025
780a5d6
Merge branch 'apache:master' into master
SBIN2010 Aug 24, 2025
3c339df
Merge branch 'apache:master' into master
SBIN2010 Aug 25, 2025
798b7a6
Merge branch 'apache:master' into master
SBIN2010 Aug 30, 2025
2db564e
Merge branch 'apache:master' into master
SBIN2010 Sep 3, 2025
a58cfad
Merge branch 'apache:master' into master
SBIN2010 Sep 5, 2025
9ea0d2f
Merge branch 'apache:master' into master
SBIN2010 Sep 8, 2025
c6bbf53
Merge branch 'apache:master' into master
SBIN2010 Sep 9, 2025
5214e7a
Merge branch 'apache:master' into master
SBIN2010 Sep 15, 2025
d98105c
Merge branch 'apache:master' into master
SBIN2010 Sep 17, 2025
30b1576
Merge branch 'apache:master' into master
SBIN2010 Sep 19, 2025
4aaa27c
Merge branch 'apache:master' into master
SBIN2010 Sep 23, 2025
25c1987
Merge branch 'apache:master' into master
SBIN2010 Sep 24, 2025
c3854cb
Merge branch 'apache:master' into master
SBIN2010 Oct 1, 2025
e1c4909
Merge branch 'apache:master' into master
SBIN2010 Oct 1, 2025
c60bda8
Merge branch 'apache:master' into master
SBIN2010 Oct 2, 2025
964307e
Merge branch 'apache:master' into master
SBIN2010 Oct 14, 2025
df97cb7
Merge branch 'apache:master' into master
SBIN2010 Oct 22, 2025
6ef82fa
Merge branch 'apache:master' into master
SBIN2010 Oct 27, 2025
d0ae96c
Merge branch 'apache:master' into master
SBIN2010 Oct 29, 2025
725039c
Merge branch 'apache:master' into master
SBIN2010 Oct 29, 2025
727c59f
Merge branch 'apache:master' into master
SBIN2010 Nov 2, 2025
eeb04da
Merge branch 'apache:master' into master
SBIN2010 Nov 7, 2025
ca12644
Merge branch 'apache:master' into master
SBIN2010 Nov 10, 2025
45c1a1a
Merge branch 'apache:master' into master
SBIN2010 Nov 13, 2025
44708bf
Merge branch 'apache:master' into master
SBIN2010 Nov 14, 2025
59f2832
Merge branch 'apache:master' into master
SBIN2010 Nov 29, 2025
4cf50b1
Merge branch 'apache:master' into master
SBIN2010 Dec 18, 2025
e34d80e
Merge branch 'apache:master' into master
SBIN2010 Dec 22, 2025
a48fb9c
Merge branch 'apache:master' into master
SBIN2010 Jan 10, 2026
5100172
Merge branch 'apache:master' into master
SBIN2010 Jan 12, 2026
0bd47e9
Merge branch 'apache:master' into master
SBIN2010 Jan 15, 2026
17614b2
Merge branch 'apache:master' into master
SBIN2010 Jan 30, 2026
7143f13
Merge branch 'apache:master' into master
SBIN2010 Feb 2, 2026
d5776e6
Merge branch 'apache:master' into master
SBIN2010 Feb 3, 2026
8a5617e
Merge branch 'apache:master' into master
SBIN2010 Feb 6, 2026
a8dd1df
Merge branch 'apache:master' into master
SBIN2010 Feb 9, 2026
85a18a4
Merge branch 'apache:master' into master
SBIN2010 Feb 11, 2026
e15912c
Merge branch 'apache:master' into master
SBIN2010 Feb 15, 2026
e465243
Merge branch 'apache:master' into master
SBIN2010 Feb 17, 2026
20110eb
Merge branch 'apache:master' into master
SBIN2010 Feb 27, 2026
effbe6d
Merge branch 'apache:master' into master
SBIN2010 Apr 1, 2026
f9ed68e
Merge branch 'apache:master' into master
SBIN2010 Apr 10, 2026
838215a
feat: add add tooltip to table header
SBIN2010 Apr 10, 2026
67c3ac4
fix: conmments ai
SBIN2010 Apr 10, 2026
ce41a74
fix: add test to transformProps.test
SBIN2010 Apr 11, 2026
2d73c46
fix: test type
SBIN2010 Apr 11, 2026
f3a8d39
Merge branch 'master' into feature/add_AgGredTable_tooltip_to_table_h…
SBIN2010 Apr 22, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,7 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
paginationPageSizeSelector={PAGE_SIZE_OPTIONS}
suppressDragLeaveHidesColumns
pinnedBottomRowData={showTotals ? [cleanedTotals] : undefined}
tooltipShowDelay={500}
localeText={{
// Pagination controls
next: t('Next'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ const processColumns = memoizeOne(function processColumns(
column_config: columnConfig = {},
query_mode: queryMode,
},
rawDatasource,
queriesData,
} = props;
const granularity = extractTimegrain(props.rawFormData);
Expand Down Expand Up @@ -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;
Comment on lines 348 to +397
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
👍 | 👎


let formatter;

if (isTime || config.d3TimeFormat) {
Expand Down Expand Up @@ -430,6 +442,7 @@ const processColumns = memoizeOne(function processColumns(
isPercentMetric,
formatter,
config,
description,
};
})
.sort((a, b) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ export interface InputColumn {
| CurrencyFormatter;
originalLabel?: string;
metricName?: string;
description?: string;
}

export type ValueRange = [number, number] | null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ export const useColDefs = ({
return {
field: colId,
headerName: getHeaderLabel(col),
headerTooltip: col.description,
valueFormatter: p => valueFormatter(p, col),
valueGetter: p => valueGetter(p, col),
cellStyle: p => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,266 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import transformProps from '../src/transformProps';
import { TableChartProps } from '../src/types';
import { GenericDataType } from '@apache-superset/core/common';
import { QueryMode } from '@superset-ui/core';

function createMockChartProps(
overrides: Partial<TableChartProps> = {},
): TableChartProps {
const defaultProps = {
height: 400,
width: 800,
rawFormData: {
viz_type: 'table',
datasource: '1__table',
query_mode: QueryMode.Aggregate,
metrics: [],
percent_metrics: [],
column_config: {},
table_timestamp_format: '',
granularity_sqla: 'day',
time_range: 'No filter',
},
queriesData: [
{
data: [],
colnames: [],
coltypes: [],
rowcount: 0,
applied_filters: [],
rejected_filters: [],
},
],
datasource: {
columns: [],
metrics: [],
columnFormats: {},
currencyFormats: {},
verboseMap: {},
},
rawDatasource: {
columns: [],
metrics: [],
},
filterState: {},
hooks: { setDataMask: jest.fn(), onChartStateChange: jest.fn() },
ownState: {},
emitCrossFilters: false,
theme: {},
...overrides,
};
return defaultProps as unknown as TableChartProps;
}

test('extracts description from datasource.columns for a regular column', () => {
const props = createMockChartProps({
queriesData: [
{
data: [{ col1: 'value' }],
colnames: ['col1'],
coltypes: [GenericDataType.String],
rowcount: 1,
applied_filters: [],
rejected_filters: [],
} as unknown as TableChartProps['queriesData'][number],
],
rawDatasource: {
columns: [
{ column_name: 'col1', description: 'This is a column description' },
],
metrics: [],
},
});

const result = transformProps(props);
const { columns } = result;
const columnMeta = columns.find(c => c.key === 'col1');
expect(columnMeta).toBeDefined();
expect(columnMeta!.description).toBe('This is a column description');
});

test('extracts description from datasource.metrics for a metric column', () => {
const props = createMockChartProps({
rawFormData: {
viz_type: 'table',
datasource: '1__table',
query_mode: QueryMode.Aggregate,
metrics: ['sum_sales'],
percent_metrics: [],
column_config: {},
table_timestamp_format: '',
granularity_sqla: 'day',
time_range: 'No filter',
},
queriesData: [
{
data: [{ sum_sales: 100 }],
colnames: ['sum_sales'],
coltypes: [GenericDataType.Numeric],
rowcount: 1,
applied_filters: [],
rejected_filters: [],
},
] as unknown as TableChartProps['queriesData'],
rawDatasource: {
columns: [],
metrics: [
{ metric_name: 'sum_sales', description: 'Total sales amount' },
],
},
});

const result = transformProps(props);
const { columns } = result;
const columnMeta = columns.find(c => c.key === 'sum_sales');
expect(columnMeta).toBeDefined();
expect(columnMeta!.description).toBe('Total sales amount');
});

test('prefers column description over metric description when both exist with same key', () => {
const props = createMockChartProps({
rawFormData: {
viz_type: 'table',
datasource: '1__table',
query_mode: QueryMode.Aggregate,
metrics: ['revenue'],
percent_metrics: [],
column_config: {},
table_timestamp_format: '',
granularity_sqla: 'day',
time_range: 'No filter',
},
queriesData: [
{
data: [{ revenue: 500 }],
colnames: ['revenue'],
coltypes: [GenericDataType.Numeric],
rowcount: 1,
applied_filters: [],
rejected_filters: [],
},
] as unknown as TableChartProps['queriesData'],
rawDatasource: {
columns: [{ column_name: 'revenue', description: 'Column desc' }],
metrics: [{ metric_name: 'revenue', description: 'Metric desc' }],
},
});

const result = transformProps(props);
const { columns } = result;
const columnMeta = columns.find(c => c.key === 'revenue');
expect(columnMeta!.description).toBe('Column desc');
});

test('handles percent metrics correctly – uses base metric name for lookup', () => {
const props = createMockChartProps({
rawFormData: {
viz_type: 'table',
datasource: '1__table',
query_mode: QueryMode.Aggregate,
metrics: ['profit'],
percent_metrics: ['profit'],
column_config: {},
table_timestamp_format: '',
granularity_sqla: 'day',
time_range: 'No filter',
},
queriesData: [
{
data: [{ '%profit': 0.15 }],
colnames: ['%profit'],
coltypes: [GenericDataType.Numeric],
rowcount: 1,
applied_filters: [],
rejected_filters: [],
},
] as unknown as TableChartProps['queriesData'],
rawDatasource: {
columns: [],
metrics: [
{ metric_name: 'profit', description: 'Profit margin percent' },
],
},
});

const result = transformProps(props);
const { columns } = result;
const columnMeta = columns.find(c => c.key === '%profit');
expect(columnMeta).toBeDefined();
expect(columnMeta!.description).toBe('Profit margin percent');
});

test('sets description to undefined when no matching column or metric is found', () => {
const props = createMockChartProps({
queriesData: [
{
data: [{ unknown_col: 'x' }],
colnames: ['unknown_col'],
coltypes: [GenericDataType.String],
rowcount: 1,
applied_filters: [],
rejected_filters: [],
},
] as unknown as TableChartProps['queriesData'],
rawDatasource: {
columns: [],
metrics: [],
},
});

const result = transformProps(props);
const { columns } = result;
const columnMeta = columns.find(c => c.key === 'unknown_col');
expect(columnMeta!.description).toBeUndefined();
});

test('uses description from column even when verboseMap renames the column', () => {
const props = createMockChartProps({
queriesData: [
{
data: [{ col_x: 10 }],
colnames: ['col_x'],
coltypes: [GenericDataType.Numeric],
rowcount: 1,
applied_filters: [],
rejected_filters: [],
},
] as unknown as TableChartProps['queriesData'],
datasource: {
columns: [],
metrics: [],
columnFormats: {},
currencyFormats: {},
verboseMap: { col_x: 'Custom Label' },
} as unknown as TableChartProps['datasource'],
rawDatasource: {
columns: [
{ column_name: 'col_x', description: 'Original column description' },
],
metrics: [],
},
});

const result = transformProps(props);
const { columns } = result;
const columnMeta = columns.find(c => c.key === 'col_x');
expect(columnMeta!.label).toBe('Custom Label');
expect(columnMeta!.description).toBe('Original column description');
});
Loading