(superset) branch master updated: fix(ag-grid): keep basic conditional formatting aligned after sort (#41390)

[en...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

EnxDev pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git


The following commit(s) were added to refs/heads/master by this push:
     new ffd09829423 fix(ag-grid): keep basic conditional formatting aligned after sort (#41390)
ffd09829423 is described below

commit ffd098294234583562675d40ffed3260509875e6
Author: amaannawab923 <am...@gmail.com>
AuthorDate: Tue Jul 7 18:42:44 2026 +0530

    fix(ag-grid): keep basic conditional formatting aligned after sort (#41390)
    
    Co-authored-by: Enzo Martellucci <52...@users.noreply.github.com>
    Co-authored-by: Enzo Martellucci <en...@gmail.com>
    Co-authored-by: Claude Opus 4.8 (1M context) <no...@anthropic.com>
---
 .../plugin-chart-ag-grid-table/src/consts.ts       |  8 +++
 .../src/renderers/NumericCellRenderer.tsx          | 15 ++--
 .../src/transformProps.ts                          | 19 +++++
 .../src/utils/getCellStyle.ts                      |  8 ++-
 .../src/utils/getRowBasicColorFormatter.ts         | 50 +++++++++++++
 .../test/utils/getRowBasicColorFormatter.test.ts   | 81 ++++++++++++++++++++++
 6 files changed, 172 insertions(+), 9 deletions(-)

diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts
index 888686e450a..a8fc68223f8 100644
--- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts
@@ -44,3 +44,11 @@ export const FILTER_CONDITION_BODY_INDEX = {
 } as const;
 
 export const ROW_NUMBER_COL_ID = '__row_number__';
+
+// Symbol key used to attach a row's basic (increase/decrease) color formatter
+// to the row data object so it travels with the row through AG Grid client-side
+// sorting (#105973). A Symbol guarantees the key can never collide with a real
+// dataset column (even one literally named "__basicColorFormatters__") and is
+// automatically excluded from Object.keys, JSON serialization and spreads, so
+// it never leaks into exports or cross-filters.
+export const BASIC_COLOR_FORMATTERS_ROW_KEY = Symbol('basicColorFormatters');
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx
index 87529673110..11d40569065 100644
--- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx
@@ -24,6 +24,7 @@ import {
 import { CustomCellRendererProps } from '@superset-ui/core/components/ThemedAgGridReact';
 import { BasicColorFormatterType, InputColumn, ValueRange } from '../types';
 import { useIsDark } from '../utils/useTableTheme';
+import getRowBasicColorFormatter from '../utils/getRowBasicColorFormatter';
 
 const StyledTotalCell = styled.div`
   ${() => `
@@ -163,13 +164,13 @@ export const NumericCellRenderer = (
   let arrow = '';
   let arrowColor = '';
   if (hasBasicColorFormatters && col?.metricName) {
-    arrow =
-      basicColorFormatters?.[node?.rowIndex as number]?.[col.metricName]
-        ?.mainArrow;
-    arrowColor =
-      basicColorFormatters?.[node?.rowIndex as number]?.[
-        col.metricName
-      ]?.arrowColor?.toLowerCase();
+    const rowFormatter = getRowBasicColorFormatter(
+      node,
+      node?.rowIndex,
+      basicColorFormatters,
+    )?.[col.metricName];
+    arrow = rowFormatter?.mainArrow ?? '';
+    arrowColor = rowFormatter?.arrowColor?.toLowerCase() ?? '';
   }
 
   const alignment =
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
index 656f139e85a..d6bcffb7cc6 100644
--- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
@@ -46,6 +46,7 @@ import {
 } from '@superset-ui/chart-controls';
 import isEqualColumns from './utils/isEqualColumns';
 import DateWithFormatter from './utils/DateWithFormatter';
+import { BASIC_COLOR_FORMATTERS_ROW_KEY } from './consts';
 import {
   DataColumnMeta,
   TableChartProps,
@@ -703,6 +704,24 @@ const transformProps = (
 
   const basicColorFormatters =
     comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns);
+
+  // Attach each row's basic (increase/decrease) color formatter to the row data
+  // object so it travels with the row through AG Grid client-side sorting.
+  // basicColorFormatters is built in the original query order and was previously
+  // read positionally by the displayed rowIndex, which applied colors to the
+  // wrong rows once the table was sorted (#105973). The key is a Symbol so it
+  // can never collide with a real dataset column and never leaks into exports,
+  // cross-filters or spreads.
+  if (basicColorFormatters) {
+    passedData.forEach((row, index) => {
+      Object.defineProperty(row, BASIC_COLOR_FORMATTERS_ROW_KEY, {
+        value: basicColorFormatters[index],
+        enumerable: false,
+        configurable: true,
+        writable: true,
+      });
+    });
+  }
   const columnColorFormatters =
     getColorFormatters(conditionalFormatting, passedData, theme) ?? [];
 
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts
index ac5eb26b54a..665909c6f7b 100644
--- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts
@@ -24,6 +24,7 @@ import {
 } from '@superset-ui/chart-controls';
 import { CellClassParams } from '@superset-ui/core/components/ThemedAgGridReact';
 import { BasicColorFormatterType, InputColumn } from '../types';
+import getRowBasicColorFormatter from './getRowBasicColorFormatter';
 
 type CellStyleParams = CellClassParams & {
   hasColumnColorFormatters: boolean | undefined;
@@ -84,8 +85,11 @@ const getCellStyle = (params: CellStyleParams) => {
     col?.metricName &&
     node?.rowPinned !== 'bottom'
   ) {
-    backgroundColor =
-      basicColorFormatters?.[rowIndex]?.[col.metricName]?.backgroundColor;
+    backgroundColor = getRowBasicColorFormatter(
+      node,
+      rowIndex,
+      basicColorFormatters,
+    )?.[col.metricName]?.backgroundColor;
   }
 
   const textAlign =
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getRowBasicColorFormatter.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getRowBasicColorFormatter.ts
new file mode 100644
index 00000000000..ad7e7cea411
--- /dev/null
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getRowBasicColorFormatter.ts
@@ -0,0 +1,50 @@
+/**
+ * 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 { BASIC_COLOR_FORMATTERS_ROW_KEY } from '../consts';
+import { BasicColorFormatterType } from '../types';
+
+export type RowFormatters = { [key: string]: BasicColorFormatterType };
+
+/**
+ * Resolves the basic (increase/decrease) color formatters for a given AG Grid
+ * row node.
+ *
+ * The formatter is attached to the row data object itself (see transformProps),
+ * so it follows the row through client-side sorting. Looking it up positionally
+ * by the displayed `rowIndex` was wrong once the user sorted the table, because
+ * the displayed index no longer matched the original data order (#105973).
+ *
+ * Falls back to the positional array for safety when no attached formatter is
+ * present.
+ */
+export default function getRowBasicColorFormatter(
+  node: { data?: Record<string | symbol, unknown> } | undefined,
+  rowIndex: number | null | undefined,
+  basicColorFormatters: RowFormatters[] | undefined,
+): RowFormatters | undefined {
+  const attached = node?.data?.[BASIC_COLOR_FORMATTERS_ROW_KEY] as
+    RowFormatters | undefined;
+  if (attached) {
+    return attached;
+  }
+  if (rowIndex == null) {
+    return undefined;
+  }
+  return basicColorFormatters?.[rowIndex];
+}
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getRowBasicColorFormatter.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getRowBasicColorFormatter.test.ts
new file mode 100644
index 00000000000..63933c676a7
--- /dev/null
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getRowBasicColorFormatter.test.ts
@@ -0,0 +1,81 @@
+/**
+ * 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 getRowBasicColorFormatter, {
+  RowFormatters,
+} from '../../src/utils/getRowBasicColorFormatter';
+import { BASIC_COLOR_FORMATTERS_ROW_KEY } from '../../src/consts';
+
+const red: RowFormatters = {
+  sales: { backgroundColor: 'red', mainArrow: '↓', arrowColor: 'red' },
+};
+const green: RowFormatters = {
+  sales: { backgroundColor: 'green', mainArrow: '↑', arrowColor: 'green' },
+};
+
+// Positional array in the original (unsorted) query order: row 0 -> green, row 1 -> red.
+const positional: RowFormatters[] = [green, red];
+
+test('uses the formatter attached to the row, not the displayed rowIndex (#105973)', () => {
+  // After sorting, the row whose original formatter is `red` is displayed first
+  // (rowIndex 0). The positional lookup would wrongly return `green`.
+  const rowData: Record<string | symbol, unknown> = { sales: 5 };
+  rowData[BASIC_COLOR_FORMATTERS_ROW_KEY] = red;
+  const node = { data: rowData };
+
+  expect(getRowBasicColorFormatter(node, 0, positional)).toBe(red);
+  expect(
+    getRowBasicColorFormatter(node, 0, positional)?.sales.backgroundColor,
+  ).toBe('red');
+});
+
+test('falls back to positional lookup when no formatter is attached', () => {
+  const node = { data: { sales: 5 } };
+  expect(getRowBasicColorFormatter(node, 1, positional)).toBe(red);
+});
+
+test('returns undefined when nothing matches', () => {
+  expect(
+    getRowBasicColorFormatter(undefined, null, positional),
+  ).toBeUndefined();
+  expect(
+    getRowBasicColorFormatter({ data: {} }, null, positional),
+  ).toBeUndefined();
+});
+
+test('attached formatter is a Symbol so it does not leak into the row', () => {
+  const rowData: Record<string | symbol, unknown> = { sales: 5 };
+  rowData[BASIC_COLOR_FORMATTERS_ROW_KEY] = green;
+  // Symbol keys are excluded from Object.keys, JSON serialization and spreads.
+  expect(Object.keys(rowData)).toEqual(['sales']);
+  expect(JSON.stringify(rowData)).toBe('{"sales":5}');
+});
+
+test('does not collide with a real column named "__basicColorFormatters__"', () => {
+  // The old string key would have overwritten this user column; the Symbol key
+  // leaves it untouched while still resolving the attached formatter (#105973).
+  const rowData: Record<string | symbol, unknown> = {
+    sales: 5,
+    __basicColorFormatters__: 'user data',
+  };
+  rowData[BASIC_COLOR_FORMATTERS_ROW_KEY] = red;
+  const node = { data: rowData };
+
+  expect(node.data.__basicColorFormatters__).toBe('user data');
+  expect(getRowBasicColorFormatter(node, 0, positional)).toBe(red);
+});