fix(ag-grid): fix conditional formatting theme colors and module extensibility (#35605)

superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts

@@ -241,10 +241,21 @@ export const getColorFormatters = memoizeOne(
   (
     columnConfig: ConditionalFormattingConfig[] | undefined,
     data: DataRecord[],
+    theme?: Record<string, any>,
     alpha?: boolean,
   ) =>
     columnConfig?.reduce(
       (acc: ColorFormatters, config: ConditionalFormattingConfig) => {
+        let resolvedColorScheme = config.colorScheme;
+        if (
+          theme &&
+          typeof config.colorScheme === 'string' &&
+          config.colorScheme.startsWith('color') &&
+          theme[config.colorScheme]
+        ) {
+          resolvedColorScheme = theme[config.colorScheme] as string;
+        }
+
         if (
           config?.column !== undefined &&
           (config?.operator === Comparator.None ||
@@ -257,7 +268,7 @@ export const getColorFormatters = memoizeOne(
           acc.push({
             column: config?.column,
             getColorFromValue: getColorFunction(
-              config,
+              { ...config, colorScheme: resolvedColorScheme },
               data.map(row => row[config.column!] as number),
               alpha,
             ),

superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx

@@ -218,3 +218,29 @@ test('handles missing theme gracefully', () => {
   // Should still render without crashing
   expect(screen.getByTestId('ag-grid-react')).toBeInTheDocument();
 });
+
+test('merges theme overrides with default theme parameters', () => {
+  const themeOverrides = {
+    fontSize: 16,
+    headerBackgroundColor: '#custom-color',
+  };
+
+  render(
+    <ThemedAgGridReact
+      rowData={mockRowData}
+      columnDefs={mockColumnDefs}
+      themeOverrides={themeOverrides}
+    />,
+  );
+
+  const agGrid = screen.getByTestId('ag-grid-react');
+  const theme = JSON.parse(agGrid.getAttribute('data-theme') || '{}');
+
+  // Custom overrides should be applied
+  expect(theme.fontSize).toBe(16);
+  expect(theme.headerBackgroundColor).toBe('#custom-color');
+
+  // Default theme parameters should still be present
+  expect(theme.foregroundColor).toBeDefined();
+  expect(theme.borderColor).toBeDefined();
+});

superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx

@@ -186,5 +186,5 @@ export {
 // Re-export AgGridReact for ref types
 export { AgGridReact } from 'ag-grid-react';
 
-// Export the setup function for AG-Grid modules
-export { setupAGGridModules } from './setupAGGridModules';
+// Export the setup function and default modules for AG-Grid
+export { setupAGGridModules, defaultModules } from './setupAGGridModules';

superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.test.ts

@@ -0,0 +1,110 @@
+/**
+ * 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 { ModuleRegistry } from 'ag-grid-community';
+import { setupAGGridModules, defaultModules } from './setupAGGridModules';
+
+jest.mock('ag-grid-community', () => ({
+  ModuleRegistry: {
+    registerModules: jest.fn(),
+  },
+  ColumnAutoSizeModule: { moduleName: 'ColumnAutoSizeModule' },
+  ColumnHoverModule: { moduleName: 'ColumnHoverModule' },
+  RowAutoHeightModule: { moduleName: 'RowAutoHeightModule' },
+  RowStyleModule: { moduleName: 'RowStyleModule' },
+  PaginationModule: { moduleName: 'PaginationModule' },
+  CellStyleModule: { moduleName: 'CellStyleModule' },
+  TextFilterModule: { moduleName: 'TextFilterModule' },
+  NumberFilterModule: { moduleName: 'NumberFilterModule' },
+  DateFilterModule: { moduleName: 'DateFilterModule' },
+  ExternalFilterModule: { moduleName: 'ExternalFilterModule' },
+  CsvExportModule: { moduleName: 'CsvExportModule' },
+  ColumnApiModule: { moduleName: 'ColumnApiModule' },
+  RowApiModule: { moduleName: 'RowApiModule' },
+  CellApiModule: { moduleName: 'CellApiModule' },
+  RenderApiModule: { moduleName: 'RenderApiModule' },
+  ClientSideRowModelModule: { moduleName: 'ClientSideRowModelModule' },
+  CustomFilterModule: { moduleName: 'CustomFilterModule' },
+}));
+
+beforeEach(() => {
+  jest.clearAllMocks();
+});
+
+test('defaultModules exports an array of AG Grid modules', () => {
+  expect(Array.isArray(defaultModules)).toBe(true);
+  expect(defaultModules.length).toBeGreaterThan(0);
+
+  // Verify it contains expected modules
+  const moduleNames = defaultModules.map((m: any) => m.moduleName);
+  expect(moduleNames).toContain('ColumnAutoSizeModule');
+  expect(moduleNames).toContain('PaginationModule');
+  expect(moduleNames).toContain('ClientSideRowModelModule');
+});
+
+test('setupAGGridModules registers default modules when called without arguments', () => {
+  setupAGGridModules();
+
+  expect(ModuleRegistry.registerModules).toHaveBeenCalledTimes(1);
+  expect(ModuleRegistry.registerModules).toHaveBeenCalledWith(defaultModules);
+});
+
+test('setupAGGridModules registers default + additional modules when provided', () => {
+  const mockEnterpriseModule1 = { moduleName: 'MultiFilterModule' };
+  const mockEnterpriseModule2 = { moduleName: 'PivotModule' };
+  const additionalModules = [mockEnterpriseModule1, mockEnterpriseModule2];
+
+  setupAGGridModules(additionalModules);
+
+  expect(ModuleRegistry.registerModules).toHaveBeenCalledTimes(1);
+
+  const registeredModules = (ModuleRegistry.registerModules as jest.Mock).mock
+    .calls[0][0];
+
+  // Should contain all default modules
+  defaultModules.forEach(module => {
+    expect(registeredModules).toContain(module);
+  });
+
+  // Should contain additional modules
+  expect(registeredModules).toContain(mockEnterpriseModule1);
+  expect(registeredModules).toContain(mockEnterpriseModule2);
+
+  // Total length should be default + additional
+  expect(registeredModules.length).toBe(
+    defaultModules.length + additionalModules.length,
+  );
+});
+
+test('setupAGGridModules handles empty additional modules array', () => {
+  setupAGGridModules([]);
+
+  expect(ModuleRegistry.registerModules).toHaveBeenCalledTimes(1);
+  expect(ModuleRegistry.registerModules).toHaveBeenCalledWith(defaultModules);
+});
+
+test('setupAGGridModules does not mutate defaultModules array', () => {
+  const originalLength = defaultModules.length;
+  const mockEnterpriseModule = { moduleName: 'EnterpriseModule' };
+
+  setupAGGridModules([mockEnterpriseModule]);
+
+  // defaultModules should remain unchanged
+  expect(defaultModules.length).toBe(originalLength);
+  expect(defaultModules).not.toContain(mockEnterpriseModule);
+});

superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.ts

@@ -19,6 +19,7 @@
 
 import {
   ModuleRegistry,
+  type Module,
   ColumnAutoSizeModule,
   ColumnHoverModule,
   RowAutoHeightModule,
@@ -39,27 +40,29 @@ import {
 } from 'ag-grid-community';
 
 /**
- * Registers the AG-Grid modules required for Superset's table functionality.
- * This should be called once during application initialization.
+ * Default AG Grid modules that are registered by default.
+ * These modules provide core AG Grid functionality.
  */
-export const setupAGGridModules = () => {
-  ModuleRegistry.registerModules([
-    ColumnAutoSizeModule,
-    ColumnHoverModule,
-    RowAutoHeightModule,
-    RowStyleModule,
-    PaginationModule,
-    CellStyleModule,
-    TextFilterModule,
-    NumberFilterModule,
-    DateFilterModule,
-    ExternalFilterModule,
-    CsvExportModule,
-    ColumnApiModule,
-    RowApiModule,
-    CellApiModule,
-    RenderApiModule,
-    ClientSideRowModelModule,
-    CustomFilterModule,
-  ]);
+export const defaultModules: Module[] = [
+  ColumnAutoSizeModule,
+  ColumnHoverModule,
+  RowAutoHeightModule,
+  RowStyleModule,
+  PaginationModule,
+  CellStyleModule,
+  TextFilterModule,
+  NumberFilterModule,
+  DateFilterModule,
+  ExternalFilterModule,
+  CsvExportModule,
+  ColumnApiModule,
+  RowApiModule,
+  CellApiModule,
+  RenderApiModule,
+  ClientSideRowModelModule,
+  CustomFilterModule,
+];
+
+export const setupAGGridModules = (additionalModules: Module[] = []) => {
+  ModuleRegistry.registerModules([...defaultModules, ...additionalModules]);
 };

superset-frontend/packages/superset-ui-core/src/components/index.ts

@@ -181,6 +181,7 @@ export {
   ThemedAgGridReact,
   type ThemedAgGridReactProps,
   setupAGGridModules,
+  defaultModules,
 } from './ThemedAgGridReact';
 export {
   CodeEditor,

superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts

@@ -74,6 +74,23 @@ function isPositiveNumber(value: string | number | null | undefined) {
   );
 }
 
+const calculateDifferences = (
+  originalValue: number,
+  comparisonValue: number,
+) => {
+  const valueDifference = originalValue - comparisonValue;
+  let percentDifferenceNum;
+  if (!originalValue && !comparisonValue) {
+    percentDifferenceNum = 0;
+  } else if (!originalValue || !comparisonValue) {
+    percentDifferenceNum = originalValue ? 1 : -1;
+  } else {
+    percentDifferenceNum =
+      (originalValue - comparisonValue) / Math.abs(comparisonValue);
+  }
+  return { valueDifference, percentDifferenceNum };
+};
+
 const processComparisonTotals = (
   comparisonSuffix: string,
   totals?: DataRecord[],
@@ -149,23 +166,6 @@ const getComparisonColFormatter = (
   return formatter;
 };
 
-const calculateDifferences = (
-  originalValue: number,
-  comparisonValue: number,
-) => {
-  const valueDifference = originalValue - comparisonValue;
-  let percentDifferenceNum;
-  if (!originalValue && !comparisonValue) {
-    percentDifferenceNum = 0;
-  } else if (!originalValue || !comparisonValue) {
-    percentDifferenceNum = originalValue ? 1 : -1;
-  } else {
-    percentDifferenceNum =
-      (originalValue - comparisonValue) / Math.abs(comparisonValue);
-  }
-  return { valueDifference, percentDifferenceNum };
-};
-
 const processComparisonDataRecords = memoizeOne(
   function processComparisonDataRecords(
     originalData: DataRecord[] | undefined,
@@ -471,6 +471,7 @@ const transformProps = (
     filterState,
     hooks: { setDataMask = () => {} },
     emitCrossFilters,
+    theme,
   } = chartProps;
 
   const {
@@ -494,6 +495,36 @@ const transformProps = (
 
   const allowRearrangeColumns = true;
 
+  // Calculate time comparison settings early since they're used in multiple places
+  const isUsingTimeComparison =
+    !isEmpty(time_compare) &&
+    queryMode === QueryMode.Aggregate &&
+    comparison_type === ComparisonType.Values &&
+    isFeatureEnabled(FeatureFlag.TableV2TimeComparisonEnabled);
+
+  const nonCustomNorInheritShifts = ensureIsArray(formData.time_compare).filter(
+    (shift: string) => shift !== 'custom' && shift !== 'inherit',
+  );
+  const customOrInheritShifts = ensureIsArray(formData.time_compare).filter(
+    (shift: string) => shift === 'custom' || shift === 'inherit',
+  );
+
+  let timeOffsets: string[] = [];
+
+  if (isUsingTimeComparison && !isEmpty(nonCustomNorInheritShifts)) {
+    timeOffsets = nonCustomNorInheritShifts;
+  }
+
+  // Shifts for custom or inherit time comparison
+  if (isUsingTimeComparison && !isEmpty(customOrInheritShifts)) {
+    if (customOrInheritShifts.includes('custom')) {
+      timeOffsets = timeOffsets.concat([formData.start_date_offset]);
+    }
+    if (customOrInheritShifts.includes('inherit')) {
+      timeOffsets = timeOffsets.concat(['inherit']);
+    }
+  }
+
   const calculateBasicStyle = (
     percentDifferenceNum: number,
     colorOption: ColorSchemeEnum,
@@ -606,12 +637,6 @@ const transformProps = (
       : undefined;
   };
 
-  const isUsingTimeComparison =
-    !isEmpty(time_compare) &&
-    queryMode === QueryMode.Aggregate &&
-    comparison_type === ComparisonType.Values &&
-    isFeatureEnabled(FeatureFlag.TableV2TimeComparisonEnabled);
-
   let hasServerPageLengthChanged = false;
 
   const pageLengthFromMap = serverPageLengthMap.get(slice_id);
@@ -624,28 +649,6 @@ const transformProps = (
 
   const timeGrain = extractTimegrain(formData);
 
-  const nonCustomNorInheritShifts = ensureIsArray(formData.time_compare).filter(
-    (shift: string) => shift !== 'custom' && shift !== 'inherit',
-  );
-  const customOrInheritShifts = ensureIsArray(formData.time_compare).filter(
-    (shift: string) => shift === 'custom' || shift === 'inherit',
-  );
-
-  let timeOffsets: string[] = [];
-
-  if (isUsingTimeComparison && !isEmpty(nonCustomNorInheritShifts)) {
-    timeOffsets = nonCustomNorInheritShifts;
-  }
-
-  // Shifts for custom or inherit time comparison
-  if (isUsingTimeComparison && !isEmpty(customOrInheritShifts)) {
-    if (customOrInheritShifts.includes('custom')) {
-      timeOffsets = timeOffsets.concat([formData.start_date_offset]);
-    }
-    if (customOrInheritShifts.includes('inherit')) {
-      timeOffsets = timeOffsets.concat(['inherit']);
-    }
-  }
   const comparisonSuffix = isUsingTimeComparison
     ? ensureIsArray(timeOffsets)[0]
     : '';
@@ -685,7 +688,7 @@ const transformProps = (
   const basicColorFormatters =
     comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns);
   const columnColorFormatters =
-    getColorFormatters(conditionalFormatting, passedData) ?? [];
+    getColorFormatters(conditionalFormatting, passedData, theme) ?? [];
 
   const basicColorColumnFormatters = getBasicColorFormatterForColumn(
     baseQuery?.data,

superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts

@@ -43,6 +43,7 @@ export default function transformProps(
     rawFormData,
     hooks,
     datasource: { currencyFormats = {}, columnFormats = {} },
+    theme,
   } = chartProps;
   const {
     metricNameFontSize,
@@ -105,7 +106,7 @@ export default function transformProps(
   const defaultColorFormatters = [] as ColorFormatters;
 
   const colorThresholdFormatters =
-    getColorFormatters(conditionalFormatting, data, false) ??
+    getColorFormatters(conditionalFormatting, data, theme, false) ??
     defaultColorFormatters;
   return {
     width,

superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts

@@ -81,6 +81,7 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
     filterState,
     datasource: { verboseMap = {}, columnFormats = {}, currencyFormats = {} },
     emitCrossFilters,
+    theme,
   } = chartProps;
   const { data, colnames, coltypes } = queriesData[0];
   const {
@@ -141,7 +142,11 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
       },
       {},
     );
-  const metricColorFormatters = getColorFormatters(conditionalFormatting, data);
+  const metricColorFormatters = getColorFormatters(
+    conditionalFormatting,
+    data,
+    theme,
+  );
 
   return {
     width,

superset-frontend/plugins/plugin-chart-table/src/transformProps.ts

@@ -469,6 +469,7 @@ const transformProps = (
       onContextMenu,
     },
     emitCrossFilters,
+    theme,
   } = chartProps;
 
   const formData = merge(
@@ -682,7 +683,7 @@ const transformProps = (
   const basicColorFormatters =
     comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns);
   const columnColorFormatters =
-    getColorFormatters(conditionalFormatting, passedData) ??
+    getColorFormatters(conditionalFormatting, passedData, theme) ??
     defaultColorFormatters;
 
   const basicColorColumnFormatters = getBasicColorFormatterForColumn(

superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx

@@ -76,7 +76,7 @@ export default memo(function ColumnConfigItem({
     alignItems: 'center',
     position: 'absolute',
     right: 3 * sizeUnit,
-    top: 3 * sizeUnit,
+    top: 4 * sizeUnit,
     transform: 'translateY(-50%)',
     gap: sizeUnit,
     color: theme.colorTextSecondary,

superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx

@@ -17,7 +17,7 @@
  * under the License.
  */
 import { useMemo, useState, useEffect } from 'react';
-import { styled, SupersetTheme, t, useTheme } from '@superset-ui/core';
+import { styled, t } from '@superset-ui/core';
 import { GenericDataType } from '@apache-superset/core/api/core';
 import {
   Comparator,
@@ -56,10 +56,11 @@ const JustifyEnd = styled.div`
   justify-content: flex-end;
 `;
 
-const colorSchemeOptions = (theme: SupersetTheme) => [
-  { value: theme.colorSuccessBg, label: t('success') },
-  { value: theme.colorWarningBg, label: t('alert') },
-  { value: theme.colorErrorBg, label: t('error') },
+// Use theme token names instead of hex values to support theme switching
+const colorSchemeOptions = () => [
+  { value: 'colorSuccessBg', label: t('success') },
+  { value: 'colorWarningBg', label: t('alert') },
+  { value: 'colorErrorBg', label: t('error') },
 ];
 
 const operatorOptions = [
@@ -238,9 +239,8 @@ export const FormattingPopoverContent = ({
   columns: { label: string; value: string; dataType: GenericDataType }[];
   extraColorChoices?: { label: string; value: string }[];
 }) => {
-  const theme = useTheme();
   const [form] = Form.useForm();
-  const colorScheme = colorSchemeOptions(theme);
+  const colorScheme = colorSchemeOptions();
   const [showOperatorFields, setShowOperatorFields] = useState(
     config === undefined ||
       (config?.colorScheme !== ColorSchemeEnum.Green &&