codeant-ai-for-open-source[bot] commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3624023428
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -368,75 +395,200 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
[emitCrossFilters, setDataMask, timeGrain, timestampFormatter],
);
+ const drillColumns = isUsingTimeComparison
+ ? (filteredColumns as InputColumn[])
+ : (columns as InputColumn[]);
+
+ const handleContextMenu = useCallback(
+ (event: CellContextMenuEvent) => {
+ if (!onContextMenu || isRawRecords || !event.column || !event.data) {
+ return;
+ }
+ const nativeEvent = event.event as MouseEvent | null | undefined;
+ if (!nativeEvent) return;
+ nativeEvent.preventDefault();
+ nativeEvent.stopPropagation();
+
+ const rowData = event.data as Record<string, DataRecordValue>;
+ const key = event.column.getColId();
+ const cellValue = event.value as DataRecordValue;
+ const colDef = event.column.getColDef();
+ const isMetric = Boolean(
+ colDef.context?.isMetric || colDef.context?.isPercentMetric,
+ );
+
+ const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
+ drillColumns.forEach(col => {
+ if (col.isMetric || col.isPercentMetric) return;
+ const dataRecordValue = rowData[col.key];
+
+ if (
+ dataRecordValue == null ||
+ (dataRecordValue instanceof DateWithFormatter &&
+ dataRecordValue.input == null)
+ ) {
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'IS NULL' as any,
+ val: null,
+ });
+ } else if (col.dataType === GenericDataType.Temporal && timeGrain) {
+ const startTime =
+ dataRecordValue instanceof Date
+ ? dataRecordValue
+ : new Date(dataRecordValue as string | number);
+
+ if (Number.isNaN(startTime.getTime())) {
Review Comment:
**Suggestion:** Temporal drill-to-detail parsing treats any non-`Date` value
as `string | number`, but `DateWithFormatter` instances can reach this branch
and `new Date(object)` becomes invalid. That silently downgrades temporal
filters to equality filters instead of the expected `TEMPORAL_RANGE`. Unwrap
`DateWithFormatter` to its underlying input before constructing the Date.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Drill-to-detail on temporal columns uses equality, not range.
- ⚠️ Time-grain aware filtering parity with Table V1 is broken.
- ⚠️ Users may see unexpected or empty drill detail results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or open an AG Grid table chart with at least one temporal
dimension (e.g., a
timestamp column) using the `plugin-chart-ag-grid-table` plugin
(superset-frontend/plugins/plugin-chart-ag-grid-table/src/index.ts:75).
2. Confirm that temporal cell values are wrapped as `DateWithFormatter`
instances in the
grid row data (class imported in AgGridTableChart.tsx:52 from
`./utils/DateWithFormatter`
and produced by value formatting utilities in `utils/formatValue.ts`).
3. In the rendered chart, right-click a non-null cell in the temporal column
to open the
context menu; this invokes `handleContextMenu` in
AgGridTableChart.tsx:403-507 with
`dataRecordValue` set to a `DateWithFormatter` instance.
4. Within the temporal branch at AgGridTableChart.tsx:438-456,
`dataRecordValue` is not a
`Date`, so `startTime` is computed as `new Date(dataRecordValue as string |
number)` (line
441), yielding an `Invalid Date`. The subsequent
`Number.isNaN(startTime.getTime())` check
treats it as malformed and incorrectly falls back to building an equality
filter (`op:
'=='`) instead of the expected `TEMPORAL_RANGE`, so drill-to-detail uses a
point equality
filter rather than a grain-based time range.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b76d7db95a094e4d91a8a263d68cbfc4&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=b76d7db95a094e4d91a8a263d68cbfc4&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:** 438:441
**Comment:**
*Logic Error: Temporal drill-to-detail parsing treats any non-`Date`
value as `string | number`, but `DateWithFormatter` instances can reach this
branch and `new Date(object)` becomes invalid. That silently downgrades
temporal filters to equality filters instead of the expected `TEMPORAL_RANGE`.
Unwrap `DateWithFormatter` to its underlying input before constructing the Date.
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=af2ad69a316abe37a0a4ac069b21b15f3473b76948e9bc0fb3ebd93385aa3cb2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=af2ad69a316abe37a0a4ac069b21b15f3473b76948e9bc0fb3ebd93385aa3cb2&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -368,75 +395,200 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
[emitCrossFilters, setDataMask, timeGrain, timestampFormatter],
);
+ const drillColumns = isUsingTimeComparison
+ ? (filteredColumns as InputColumn[])
+ : (columns as InputColumn[]);
+
+ const handleContextMenu = useCallback(
+ (event: CellContextMenuEvent) => {
+ if (!onContextMenu || isRawRecords || !event.column || !event.data) {
+ return;
+ }
+ const nativeEvent = event.event as MouseEvent | null | undefined;
+ if (!nativeEvent) return;
+ nativeEvent.preventDefault();
+ nativeEvent.stopPropagation();
+
+ const rowData = event.data as Record<string, DataRecordValue>;
+ const key = event.column.getColId();
+ const cellValue = event.value as DataRecordValue;
+ const colDef = event.column.getColDef();
+ const isMetric = Boolean(
+ colDef.context?.isMetric || colDef.context?.isPercentMetric,
+ );
+
+ const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
+ drillColumns.forEach(col => {
+ if (col.isMetric || col.isPercentMetric) return;
+ const dataRecordValue = rowData[col.key];
+
+ if (
+ dataRecordValue == null ||
+ (dataRecordValue instanceof DateWithFormatter &&
+ dataRecordValue.input == null)
+ ) {
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'IS NULL' as any,
+ val: null,
+ });
+ } else if (col.dataType === GenericDataType.Temporal && timeGrain) {
+ const startTime =
+ dataRecordValue instanceof Date
+ ? dataRecordValue
+ : new Date(dataRecordValue as string | number);
+
+ if (Number.isNaN(startTime.getTime())) {
+ // Malformed temporal value: fall back to an equality filter
+ // instead of building a TEMPORAL_RANGE, since toISOString()
+ // throws on an Invalid Date and would crash the context menu.
+ const sanitizedValue = extractTextFromHTML(dataRecordValue);
+ drillToDetailFilters.push({
+ col: col.key,
+ op: '==',
+ val: sanitizedValue as string | number | boolean,
+ formattedVal: formatColumnValue(col, sanitizedValue)[1],
+ });
+ } else {
+ const [rangeStartTime, rangeEndTime] = getTimeRangeFromGranularity(
+ startTime,
+ timeGrain,
+ );
+ const timeRangeValue = `${rangeStartTime.toISOString()} :
${rangeEndTime.toISOString()}`;
+
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'TEMPORAL_RANGE',
+ val: timeRangeValue,
+ grain: timeGrain,
+ formattedVal: formatColumnValue(col, dataRecordValue)[1],
+ });
+ }
+ } else {
+ const sanitizedValue = extractTextFromHTML(dataRecordValue);
+ drillToDetailFilters.push({
+ col: col.key,
+ op: '==',
+ val: sanitizedValue as string | number | boolean,
+ formattedVal: formatColumnValue(col, sanitizedValue)[1],
+ });
+ }
+ });
+
+ onContextMenu(nativeEvent.clientX, nativeEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: isMetric
+ ? undefined
+ : getCrossFilterDataMask({
+ key,
+ value: cellValue,
+ filters,
+ timeGrain,
+ isActiveFilterValue,
+ timestampFormatter,
+ }),
+ drillBy: isMetric
+ ? undefined
+ : {
+ filters: [
+ {
+ col: key,
+ op: (cellValue == null ||
+ (cellValue instanceof DateWithFormatter &&
+ cellValue.input == null)
+ ? 'IS NULL'
+ : '==') as any,
+ val: extractTextFromHTML(cellValue),
+ },
+ ],
Review Comment:
**Suggestion:** When building the drill-by filter, the null branch sets
operator `IS NULL` but still passes a non-null `val` derived from
`extractTextFromHTML(cellValue)`. This creates an inconsistent filter payload
and can break drill behavior for null cells. Set `val` to `null` when using `IS
NULL`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Drill-by payload for null cells is internally inconsistent.
- ⚠️ Potential confusion for downstream filter handling logic.
- ⚠️ Could mislabel filters in future UI consumers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard containing an AG Grid table chart using the
`plugin-chart-ag-grid-table` plugin
(superset-frontend/plugins/plugin-chart-ag-grid-table/src/index.ts:75),
which renders via
`TableChart` (AgGridTableChart.tsx:87).
2. Ensure the underlying query returns a column with some NULL values (so
cells in that
column render as null) and load the chart.
3. Right-click a cell whose value is null in a non-metric column to trigger
`handleContextMenu` in AgGridTableChart.tsx:403-507; this builds the
`drillBy` filter
array.
4. Inspect the filter object passed through `onContextMenu` (e.g., using
browser devtools)
and observe that for the null cell branch `op` is `IS NULL`
(AgGridTableChart.tsx:498-502)
while `val` is set to `extractTextFromHTML(cellValue)`
(AgGridTableChart.tsx:503),
producing an inconsistent payload where a null operator carries a non-null
value.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f4d4cd1c04e747bbb365608ccb4f6268&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=f4d4cd1c04e747bbb365608ccb4f6268&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:** 498:503
**Comment:**
*Logic Error: When building the drill-by filter, the null branch sets
operator `IS NULL` but still passes a non-null `val` derived from
`extractTextFromHTML(cellValue)`. This creates an inconsistent filter payload
and can break drill behavior for null cells. Set `val` to `null` when using `IS
NULL`.
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=35b4e66f057945f4bb4c4e664d3b3018abca85a0507ee015d57c77e1206ca307&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=35b4e66f057945f4bb4c4e664d3b3018abca85a0507ee015d57c77e1206ca307&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:
##########
@@ -717,24 +742,68 @@ const config: ControlPanelConfig = {
: [];
const chartStatus = chart?.chartStatus;
+ // Normalize legacy `toAllRow`/`toTextColor` flags saved before
+ // `columnFormatting`/`objectFormatting` existed, so "entire
row"
+ // formatters set under the old schema keep working.
+ const value = _?.value ?? [];
+ if (value && Array.isArray(value)) {
+ value.forEach(
+ (item: ConditionalFormattingConfig, index, array) => {
+ if (
+ item.colorScheme &&
+ !['Green', 'Red'].includes(item.colorScheme)
+ ) {
+ if (item.columnFormatting === undefined) {
+ // eslint-disable-next-line no-param-reassign
+ array[index] = {
+ ...item,
+ ...(item.toTextColor === true && {
+ objectFormatting:
ObjectFormattingEnum.TEXT_COLOR,
+ }),
+ ...(item.toAllRow === true && {
+ columnFormatting:
ObjectFormattingEnum.ENTIRE_ROW,
+ }),
+ };
+ }
+ }
+ },
+ );
+ }
const { colnames, coltypes } =
chart?.queriesResponse?.[0] ?? {};
- const numericColumns =
- Array.isArray(colnames) && Array.isArray(coltypes)
- ? colnames
- .filter(
- (colname: string, index: number) =>
- coltypes[index] === GenericDataType.Numeric,
- )
- .map((colname: string) => ({
- value: colname,
- label: Array.isArray(verboseMap)
- ? colname
- : (verboseMap[colname] ?? colname),
- dataType:
- colnames && coltypes[colnames?.indexOf(colname)],
- }))
- : [];
+ const hasColumns =
+ Array.isArray(colnames) && Array.isArray(coltypes);
+ const allColumns = hasColumns
+ ? [
+ {
+ value: ObjectFormattingEnum.ENTIRE_ROW,
+ label: t('entire row'),
+ dataType: GenericDataType.String,
+ },
+ ...colnames.map((colname: string, index: number) => ({
+ value: colname,
+ label: Array.isArray(verboseMap)
+ ? colname
+ : (verboseMap[colname] ?? colname),
+ dataType: coltypes[index],
+ })),
+ ]
+ : [];
+ const numericColumns = hasColumns
+ ? colnames
+ .filter(
+ (colname: string, index: number) =>
+ coltypes[index] === GenericDataType.Numeric,
+ )
+ .map((colname: string) => ({
+ value: colname,
+ label: Array.isArray(verboseMap)
+ ? colname
+ : (verboseMap[colname] ?? colname),
+ dataType:
+ colnames && coltypes[colnames?.indexOf(colname)],
+ }))
Review Comment:
**Suggestion:** This computes `dataType` via `indexOf(colname)` while
iterating `colnames`, which returns the first match and gives incorrect types
when duplicate column names exist. Use the current iteration index to keep
column/type alignment stable. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Conditional formatting may misclassify columns with duplicate names.
- ⚠️ Numeric-only formatting options could appear for non-numeric columns.
- ⚠️ Type-driven UI behavior becomes inconsistent for aliased columns.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dataset or query for an AG Grid table chart where
`queriesResponse[0].colnames` contains the same column name more than once
but with
differing types in `coltypes` (e.g., a dimension and metric both aliased as
`value`) as
returned to `controlPanel.tsx`.
2. Open the chart's edit view so the AG Grid table control panel renders
using `config` in
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:433-826.
3. In the conditional formatting section, `numericColumns` is built at
controlPanel.tsx:775-806 by filtering `colnames` using the iteration index
to test
`coltypes[index] === GenericDataType.Numeric`.
4. During the subsequent `map`, the code computes each option's `dataType`
via
`coltypes[colnames?.indexOf(colname)]` (controlPanel.tsx:804-805), which
always uses the
first occurrence of that name. For duplicate names, this can assign a
non-numeric type to
a numeric column (or vice versa), misaligning data types used by downstream
formatting
logic that relies on `dataType` for behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=08e74a728f9240b7b1a81cd6b3d6c0c9&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=08e74a728f9240b7b1a81cd6b3d6c0c9&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/controlPanel.tsx
**Line:** 804:805
**Comment:**
*Logic Error: This computes `dataType` via `indexOf(colname)` while
iterating `colnames`, which returns the first match and gives incorrect types
when duplicate column names exist. Use the current iteration index to keep
column/type alignment stable.
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=f98e668c007d40d26d74b4b0e71acafec4c8f9a34f765ca36bebcbf805d53b72&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=f98e668c007d40d26d74b4b0e71acafec4c8f9a34f765ca36bebcbf805d53b72&reaction=dislike'>👎</a>
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -148,6 +150,14 @@ def _migrate_temporal_filter(self, rv_data: dict[str,
Any]) -> None:
def upgrade_slice(cls, slc: Slice) -> None:
try:
clz = cls(slc.params)
+ # Some charts don't carry a "datasource" key in params — outside
+ # of migrations, callers always read it via Slice.form_data,
+ # which injects "datasource" from the datasource_id/
+ # datasource_type columns on every access. _build_query() (and
+ # anything else touching self.data) needs that same key, so
+ # synthesize it here the same way for the charts missing it.
+ if "datasource" not in clz.data and slc.datasource_id is not None:
+ clz.data["datasource"] =
f"{slc.datasource_id}__{slc.datasource_type}"
Review Comment:
**Suggestion:** The synthesized datasource token only checks `datasource_id`
and can still format a value like `"123__None"` when `datasource_type` is
missing, which creates an invalid datasource reference and leads to incorrect
query-context generation. Require both parts before synthesizing the datasource
string. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Migrated slices reference invalid datasource tokens in params.
- ⚠️ Some charts fail when loading migrated query_context.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or identify a slice row in the `slices` table (defined in
`superset/migrations/shared/migrate_viz/base.py:35-44`) where
`datasource_id` is set
(e.g., 123) but `datasource_type` is NULL.
2. Ensure the slice `params` JSON does not contain a `datasource` key; this
is common for
older charts that relied on `Slice.form_data` to inject it.
3. Run the `superset migrate_viz` CLI so it calls
`MigrateViz.upgrade_slice()` at
`superset/migrations/shared/migrate_viz/base.py:150` with that `Slice`
instance.
4. Inside `upgrade_slice`, at lines 159-160, the condition `if "datasource"
not in
clz.data and slc.datasource_id is not None:` passes and
`clz.data["datasource"]` is set to
`f"{slc.datasource_id}__{slc.datasource_type}"`, producing a token like
`"123__None"`,
which is an invalid datasource reference that is then persisted into the
migrated slice’s
`params`/`query_context`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=12adf6f61ede462abed7ad5c5be31db8&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=12adf6f61ede462abed7ad5c5be31db8&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/migrations/shared/migrate_viz/base.py
**Line:** 159:160
**Comment:**
*Possible Bug: The synthesized datasource token only checks
`datasource_id` and can still format a value like `"123__None"` when
`datasource_type` is missing, which creates an invalid datasource reference and
leads to incorrect query-context generation. Require both parts before
synthesizing the datasource string.
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=6b310424fd4dca0c4d7456584ab5919cdc7bdc5ddd84eb6098360b4d481c6353&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=6b310424fd4dca0c4d7456584ab5919cdc7bdc5ddd84eb6098360b4d481c6353&reaction=dislike'>👎</a>
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -642,3 +644,254 @@ def process(base_query_object: dict[str, Any]) ->
list[dict[str, Any]]:
return [result]
return build_query_context(self.data, process)
+
+
+def _get_table_chart_time_offsets(form_data: dict[str, Any]) -> list[Any]:
+ """
+ Resolve time_compare into the list of shifts buildQuery.ts sends as
+ time_offsets. table charts use a single-select time_compare control
+ whose choices include the special 'custom'/'inherit' shifts, which
+ resolve to start_date_offset/'inherit' rather than being used verbatim.
+ """
+ time_compare_shifts = ensure_is_array(form_data.get("time_compare"))
+ non_custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift not in ("custom",
"inherit")
+ ]
+ custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift in ("custom",
"inherit")
+ ]
+
+ time_offsets: list[Any] = list(non_custom_or_inherit_shifts)
+ if "custom" in custom_or_inherit_shifts:
+ time_offsets.append(form_data.get("start_date_offset"))
+ if "inherit" in custom_or_inherit_shifts:
+ time_offsets.append("inherit")
+
+ # Dashboard filter override - allows dashboard-level time shifts to
+ # OVERRIDE chart-level time shift settings, mirroring buildQuery.ts.
+ extra_form_data_time_compare = (form_data.get("extra_form_data") or
{}).get(
+ "time_compare"
+ )
+ if extra_form_data_time_compare:
+ time_offsets = [extra_form_data_time_compare]
Review Comment:
**Suggestion:** The dashboard override for time comparison wraps
`extra_form_data.time_compare` in another list, so if the override is already a
list the migrated query gets a nested list (for example `[[...]]`) instead of a
flat list of offsets. This breaks downstream time-offset handling and
comparison label generation. Normalize the override with the same array helper
used elsewhere before assigning it. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Time comparison post-processing receives nested time_offsets lists.
- ⚠️ Comparison metric labels rendered incorrectly in ag-grid table.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dashboard-level time comparison filter that injects
`extra_form_data.time_compare` as a list (e.g., `["1 week ago"]`) into the
table chart’s
form data; this is stored with the slice used by migrations.
2. Run the `superset migrate_viz` CLI to migrate that table chart to
`ag-grid-table`,
which invokes `MigrateViz.upgrade_slice()` in
`superset/migrations/shared/migrate_viz/base.py:150` and constructs a
`MigrateTableChart`
with `self.data` containing `extra_form_data["time_compare"]` as a list.
3. During migration, `MigrateTableChart._build_query()` at
`superset/migrations/shared/migrate_viz/processors.py:857` calls
`_get_table_chart_time_offsets(self.data)` defined at lines 649-677 in the
same file.
4. In `_get_table_chart_time_offsets`, lines 672-676 read the list-valued
override into
`extra_form_data_time_compare` and then execute `time_offsets =
[extra_form_data_time_compare]`, yielding a nested structure like `[["1 week
ago"]]`. This
malformed `time_offsets` is then used in `_build_aggregate_mode_query()` at
lines 732-802
(e.g., in the list comprehension at 758-767), breaking time-offset handling
and causing
incorrect comparison column labels in the migrated query.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2986da0eb86049519633817d86b6abe1&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=2986da0eb86049519633817d86b6abe1&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/migrations/shared/migrate_viz/processors.py
**Line:** 672:676
**Comment:**
*Logic Error: The dashboard override for time comparison wraps
`extra_form_data.time_compare` in another list, so if the override is already a
list the migrated query gets a nested list (for example `[[...]]`) instead of a
flat list of offsets. This breaks downstream time-offset handling and
comparison label generation. Normalize the override with the same array helper
used elsewhere before assigning it.
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=71336db063d7e0131e3f263a8352e6193c8f8ccde0c3741f08840e520b33258d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=71336db063d7e0131e3f263a8352e6193c8f8ccde0c3741f08840e520b33258d&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -416,6 +422,46 @@ 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>[] = [];
+ const rowIds: string[] = [];
+ api.forEachNodeAfterFilterAndSort(node => {
+ if (node.data) {
+ rows.push(node.data);
+ rowIds.push(node.id ?? '');
+ }
Review Comment:
**Suggestion:** This handler rebuilds the full filtered/sorted row snapshot
on every model update, which is O(n) per grid update and can severely degrade
interactivity on large tables. Restrict recomputation to events that actually
affect export content (filter/sort/column visibility) or debounce/throttle
snapshot generation. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Large client-side tables experience slower interactions.
- ⚠️ Export-snapshot maintenance adds O(n) per model update.
- ⚠️ Dashboard users may see lag during filtering/sorting.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the AG Grid-based table chart so that `AgGridDataTable` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:124`
is
rendered with a large client-side dataset (many hundreds or thousands of
rows) and
`serverPagination` set to false, enabling the client-view snapshot path
described in the
comment at lines 425–431.
2. Confirm that `onClientViewChange` is provided by the parent (props
defined at lines
117–118) so that `handleModelUpdated` (lines 432–463) will execute its
snapshot logic when
AG Grid fires `onModelUpdated`, wired at lines 572–578.
3. Interact with the grid in ways that trigger frequent model updates
(sorting, filtering,
changing column visibility). Each such interaction causes
`handleModelUpdated` to rebuild
the snapshot by iterating all nodes via `api.forEachNodeAfterFilterAndSort`
at lines
447–452, pushing every row into `rows` and `rowIds`. This is O(n) work per
model update,
where n is the number of filtered rows.
4. On very large tables or rapid interactions (for example, typing into a
quick filter),
repeated full traversals of the filtered/sorted model in
`handleModelUpdated` increase CPU
usage on the main thread and can degrade perceived grid responsiveness, even
if the
deduplication check at lines 457–459 prevents some `onClientViewChange`
calls, because the
expensive traversal still runs on every `onModelUpdated` event.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=76c37827d5cf4fd1b8b03316a869c317&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=76c37827d5cf4fd1b8b03316a869c317&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:** 445:451
**Comment:**
*Possible Bug: This handler rebuilds the full filtered/sorted row
snapshot on every model update, which is O(n) per grid update and can severely
degrade interactivity on large tables. Restrict recomputation to events that
actually affect export content (filter/sort/column visibility) or
debounce/throttle snapshot generation.
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=fe8cb746a02937256f146ab6605e35533a21276f29bf43edb1b8816902b885e0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=fe8cb746a02937256f146ab6605e35533a21276f29bf43edb1b8816902b885e0&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]
