(superset) branch fix/105973-basic-conditional-formatting-sort created (now 348d924c92a)

[am...@apache.org]

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

amaannawab923 pushed a change to branch fix/105973-basic-conditional-formatting-sort
in repository https://gitbox.apache.org/repos/asf/superset.git


      at 348d924c92a fix(plugin-chart-ag-grid-table): keep basic conditional formatting aligned after sort (#105973)

This branch includes the following new commits:

     new 348d924c92a fix(plugin-chart-ag-grid-table): keep basic conditional formatting aligned after sort (#105973)

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

(superset) 01/01: fix(plugin-chart-ag-grid-table): keep basic conditional formatting aligned after sort (#105973)

[am...@apache.org]

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

amaannawab923 pushed a commit to branch fix/105973-basic-conditional-formatting-sort
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 348d924c92a503014b6b7f3cf1a8cc2c17d72db9
Author: amaannawab923 <am...@gmail.com>
AuthorDate: Wed Jun 24 17:34:48 2026 +0530

    fix(plugin-chart-ag-grid-table): keep basic conditional formatting aligned after sort (#105973)
    
    The basic (increase/decrease) color formatters were built in the original
    query order and read positionally by the displayed AG Grid rowIndex. Once the
    table was sorted client-side, the displayed index no longer matched the data
    order, so the green/red background and arrow indicators were applied to the
    wrong rows.
    
    Attach each row's formatter to its row data object (non-enumerable, so it never
    leaks into exports/cross-filters) so it travels with the row through sorting,
    and resolve it via getRowBasicColorFormatter in both getCellStyle and
    NumericCellRenderer, falling back to the positional lookup. Add unit tests.
---
 .../plugin-chart-ag-grid-table/src/consts.ts       |  5 ++
 .../src/renderers/NumericCellRenderer.tsx          | 15 ++---
 .../src/transformProps.ts                          | 18 ++++++
 .../src/utils/getCellStyle.ts                      |  8 ++-
 .../src/utils/getRowBasicColorFormatter.ts         | 51 +++++++++++++++++
 .../test/utils/getRowBasicColorFormatter.test.ts   | 65 ++++++++++++++++++++++
 6 files changed, 153 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..b53785d847e 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,8 @@ export const FILTER_CONDITION_BODY_INDEX = {
 } as const;
 
 export const ROW_NUMBER_COL_ID = '__row_number__';
+
+// Non-enumerable 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).
+export const BASIC_COLOR_FORMATTERS_ROW_KEY = '__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..32a8d2cd597 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 1ad19b06b57..4135dc2df8f 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,23 @@ 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 property is
+  // non-enumerable so it 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..254510be752
--- /dev/null
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getRowBasicColorFormatter.ts
@@ -0,0 +1,51 @@
+/**
+ * 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';
+
+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, 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..2c5016cbc68
--- /dev/null
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getRowBasicColorFormatter.test.ts
@@ -0,0 +1,65 @@
+/**
+ * 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 from '../../src/utils/getRowBasicColorFormatter';
+import { BASIC_COLOR_FORMATTERS_ROW_KEY } from '../../src/consts';
+
+const red = { sales: { backgroundColor: 'red', mainArrow: '↓', arrowColor: 'red' } };
+const green = {
+  sales: { backgroundColor: 'green', mainArrow: '↑', arrowColor: 'green' },
+};
+
+// Positional array in the original (unsorted) query order: row 0 -> green, row 1 -> red.
+const positional = [green, red] as any;
+
+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, unknown> = { sales: 5 };
+  Object.defineProperty(rowData, BASIC_COLOR_FORMATTERS_ROW_KEY, {
+    value: red,
+    enumerable: false,
+  });
+  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 non-enumerable so it does not leak into the row', () => {
+  const rowData: Record<string, unknown> = { sales: 5 };
+  Object.defineProperty(rowData, BASIC_COLOR_FORMATTERS_ROW_KEY, {
+    value: green,
+    enumerable: false,
+  });
+  expect(Object.keys(rowData)).toEqual(['sales']);
+});