fix(theming): address PR comments + some fixes and improvements

Author: gabotorresruiz

superset-frontend/packages/superset-ui-core/src/theme/Theme.test.tsx

@@ -427,8 +427,8 @@ describe('Theme', () => {
     });
   });
 
-  describe('BASE_THEME integration tests', () => {
-    it('merges BASE_THEME tokens with empty user theme', () => {
+  describe('base theme integration tests', () => {
+    it('merges base theme tokens with empty user theme', () => {
       const baseTheme: AnyThemeConfig = {
         token: {
           colorPrimary: '#2893B3',
@@ -452,7 +452,7 @@ describe('Theme', () => {
       expect(serialized.algorithm).toBe(ThemeAlgorithm.DEFAULT);
     });
 
-    it('allows user theme to override specific BASE_THEME tokens', () => {
+    it('allows user theme to override specific base theme tokens', () => {
       const baseTheme: AnyThemeConfig = {
         token: {
           colorPrimary: '#2893B3',
@@ -481,7 +481,7 @@ describe('Theme', () => {
       expect(theme.theme.borderRadius).toBe(4);
     });
 
-    it('handles BASE_THEME_DARK with dark algorithm correctly', () => {
+    it('handles base theme with dark algorithm correctly', () => {
       const baseTheme: AnyThemeConfig = {
         token: {
           colorPrimary: '#2893B3',
@@ -508,8 +508,8 @@ describe('Theme', () => {
       expect(serialized.algorithm).toBe(ThemeAlgorithm.DARK);
     });
 
-    it('works with real-world Superset BASE_THEME configuration', () => {
-      // Simulate actual Superset BASE_THEME
+    it('works with real-world Superset base theme configuration', () => {
+      // Simulate actual Superset base theme (THEME_DEFAULT/THEME_DARK from config)
       const supersetBaseTheme: AnyThemeConfig = {
         token: {
           colorPrimary: '#2893B3',
@@ -548,7 +548,7 @@ describe('Theme', () => {
       expect(darkSerialized.algorithm).toBe(ThemeAlgorithm.DARK);
     });
 
-    it('handles component overrides in BASE_THEME', () => {
+    it('handles component overrides in base theme', () => {
       const baseTheme: AnyThemeConfig = {
         token: {
           colorPrimary: '#2893B3',
@@ -704,5 +704,85 @@ describe('Theme', () => {
       // fontFamily reverts to Ant Design default since base theme is not reapplied
       expect(theme.theme.fontFamily).not.toBe('Inter');
     });
+
+    it('minimal theme preserves ALL base theme tokens except overridden ones', () => {
+      // Simulate a comprehensive base theme with many tokens
+      const baseTheme: AnyThemeConfig = {
+        token: {
+          colorPrimary: '#2893B3',
+          colorError: '#e04355',
+          colorWarning: '#fcc700',
+          colorSuccess: '#5ac189',
+          colorInfo: '#66bcfe',
+          fontFamily: 'Inter, Helvetica',
+          fontSize: 14,
+          borderRadius: 4,
+          lineWidth: 1,
+          controlHeight: 32,
+          // Custom Superset tokens
+          brandLogoAlt: 'CustomLogo',
+          menuHoverBackgroundColor: '#eeeeee',
+        } as Record<string, any>,
+        algorithm: antdThemeImport.defaultAlgorithm,
+      };
+
+      // Minimal theme that only overrides primary color and algorithm
+      const minimalTheme: AnyThemeConfig = {
+        token: {
+          colorPrimary: '#ff05dd', // Only override this
+        },
+        algorithm: antdThemeImport.darkAlgorithm, // Change to dark
+      };
+
+      const theme = Theme.fromConfig(minimalTheme, baseTheme);
+
+      // User's override should apply
+      expect(theme.theme.colorPrimary).toBe('#ff05dd');
+
+      // ALL base theme tokens should be preserved
+      expect(theme.theme.colorError).toBe('#e04355');
+      expect(theme.theme.colorWarning).toBe('#fcc700');
+      expect(theme.theme.colorSuccess).toBe('#5ac189');
+      expect(theme.theme.colorInfo).toBe('#66bcfe');
+      expect(theme.theme.fontFamily).toBe('Inter, Helvetica');
+      expect(theme.theme.fontSize).toBe(14);
+      expect(theme.theme.borderRadius).toBe(4);
+      expect(theme.theme.lineWidth).toBe(1);
+      expect(theme.theme.controlHeight).toBe(32);
+
+      // Custom tokens should also be preserved
+      expect((theme.theme as any).brandLogoAlt).toBe('CustomLogo');
+      expect((theme.theme as any).menuHoverBackgroundColor).toBe('#eeeeee');
+
+      // Algorithm should be updated
+      const serialized = theme.toSerializedConfig();
+      expect(serialized.algorithm).toBe(ThemeAlgorithm.DARK);
+    });
+
+    it('arrays in themes are replaced entirely, not merged by index', () => {
+      const baseTheme: AnyThemeConfig = {
+        token: {
+          colorPrimary: '#2893B3',
+        },
+        algorithm: [
+          antdThemeImport.compactAlgorithm,
+          antdThemeImport.defaultAlgorithm,
+        ],
+      };
+
+      const userTheme: AnyThemeConfig = {
+        algorithm: [antdThemeImport.darkAlgorithm], // Replace with single item array
+      };
+
+      const theme = Theme.fromConfig(userTheme, baseTheme);
+      const serialized = theme.toSerializedConfig();
+
+      // User's array should completely replace base array
+      expect(Array.isArray(serialized.algorithm)).toBe(true);
+      expect(serialized.algorithm).toHaveLength(1);
+      expect(serialized.algorithm).toContain(ThemeAlgorithm.DARK);
+      expect(serialized.algorithm).not.toContain(ThemeAlgorithm.COMPACT);
+      expect(serialized.algorithm).not.toContain(ThemeAlgorithm.DEFAULT);
+    });
   });
 });

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

@@ -25,7 +25,7 @@ import {
   CacheProvider as EmotionCacheProvider,
 } from '@emotion/react';
 import createCache from '@emotion/cache';
-import { noop } from 'lodash';
+import { noop, mergeWith } from 'lodash';
 import { GlobalStyles } from './GlobalStyles';
 import {
   AntdThemeConfig,
@@ -63,33 +63,9 @@ export class Theme {
     let mergedConfig: AnyThemeConfig | undefined = config;
 
     if (baseTheme && config) {
-      mergedConfig = {
-        ...baseTheme,
-        ...config,
-      } as AnyThemeConfig;
-
-      if (baseTheme.token || config.token) {
-        (mergedConfig as any).token = {
-          ...(baseTheme.token || {}),
-          ...(config.token || {}),
-        };
-      }
-
-      if (baseTheme.components || config.components) {
-        const mergedComponents: any = { ...(baseTheme.components || {}) };
-
-        if (config.components) {
-          const configComponents = config.components as any;
-          Object.keys(configComponents).forEach(componentName => {
-            mergedComponents[componentName] = {
-              ...(mergedComponents[componentName] || {}),
-              ...configComponents[componentName],
-            };
-          });
-        }
-
-        (mergedConfig as any).components = mergedComponents;
-      }
+      mergedConfig = mergeWith({}, baseTheme, config, (objValue, srcValue) =>
+        Array.isArray(srcValue) ? srcValue : undefined,
+      );
     } else if (baseTheme && !config) {
       mergedConfig = baseTheme;
     }

superset-frontend/packages/superset-ui-core/src/theme/utils/themeUtils.test.ts

@@ -16,7 +16,13 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { getFontSize, getColorVariants, isThemeDark } from './themeUtils';
+import { theme as antdTheme } from 'antd';
+import {
+  getFontSize,
+  getColorVariants,
+  isThemeDark,
+  isThemeConfigDark,
+} from './themeUtils';
 import { Theme } from '../Theme';
 import { ThemeAlgorithm } from '../types';
 
@@ -130,4 +136,111 @@ describe('themeUtils', () => {
       expect(variants.bg).toBeUndefined();
     });
   });
+
+  describe('isThemeConfigDark', () => {
+    it('returns true for config with dark algorithm', () => {
+      const config = {
+        algorithm: antdTheme.darkAlgorithm,
+      };
+      expect(isThemeConfigDark(config)).toBe(true);
+    });
+
+    it('returns true for config with dark algorithm in array', () => {
+      const config = {
+        algorithm: [antdTheme.darkAlgorithm, antdTheme.compactAlgorithm],
+      };
+      expect(isThemeConfigDark(config)).toBe(true);
+    });
+
+    it('returns false for config without dark algorithm', () => {
+      const config = {
+        algorithm: antdTheme.defaultAlgorithm,
+      };
+      expect(isThemeConfigDark(config)).toBe(false);
+    });
+
+    it('returns false for config with no algorithm', () => {
+      const config = {
+        token: {
+          colorPrimary: '#1890ff',
+        },
+      };
+      expect(isThemeConfigDark(config)).toBe(false);
+    });
+
+    it('detects manually-created dark theme without dark algorithm', () => {
+      // This is the edge case: dark colors without dark algorithm
+      const config = {
+        token: {
+          colorBgContainer: '#1a1a1a', // Dark background
+          colorBgBase: '#0a0a0a', // Dark base
+          colorText: '#ffffff', // Light text
+        },
+      };
+      expect(isThemeConfigDark(config)).toBe(true);
+    });
+
+    it('does not false-positive on light theme with custom colors', () => {
+      const config = {
+        token: {
+          colorBgContainer: '#ffffff', // Light background
+          colorBgBase: '#f5f5f5', // Light base
+          colorText: '#000000', // Dark text
+        },
+      };
+      expect(isThemeConfigDark(config)).toBe(false);
+    });
+
+    it('handles partial color tokens gracefully', () => {
+      // With actual theme computation, a dark colorBgContainer results in a dark theme
+      const config = {
+        token: {
+          colorBgContainer: '#1a1a1a', // Dark background
+          // Missing other color tokens
+        },
+      };
+      expect(isThemeConfigDark(config)).toBe(true);
+    });
+
+    it('respects colorBgContainer as the primary indicator', () => {
+      // The computed theme uses colorBgContainer as the main background
+      const darkConfig = {
+        token: {
+          colorBgContainer: '#1a1a1a', // Dark background
+          colorText: '#000000', // Dark text (unusual but doesn't override)
+        },
+      };
+      expect(isThemeConfigDark(darkConfig)).toBe(true);
+
+      const lightConfig = {
+        token: {
+          colorBgContainer: '#ffffff', // Light background
+          colorText: '#ffffff', // Light text (unusual but doesn't override)
+        },
+      };
+      expect(isThemeConfigDark(lightConfig)).toBe(false);
+    });
+
+    it('handles non-string color tokens gracefully', () => {
+      const config = {
+        token: {
+          colorBgContainer: undefined,
+          colorText: null,
+          colorBgBase: 123, // Invalid type
+        },
+      };
+      expect(isThemeConfigDark(config)).toBe(false);
+    });
+
+    it('returns false for empty config', () => {
+      expect(isThemeConfigDark({})).toBe(false);
+    });
+
+    it('returns false for config with empty token object', () => {
+      const config = {
+        token: {},
+      };
+      expect(isThemeConfigDark(config)).toBe(false);
+    });
+  });
 });

superset-frontend/packages/superset-ui-core/src/theme/utils/themeUtils.ts

@@ -18,7 +18,14 @@
  */
 import tinycolor from 'tinycolor2';
 import { useTheme as useEmotionTheme } from '@emotion/react';
-import type { SupersetTheme, FontSizeKey, ColorVariants } from '../types';
+import { theme as antdTheme } from 'antd';
+import type {
+  SupersetTheme,
+  FontSizeKey,
+  ColorVariants,
+  AnyThemeConfig,
+} from '../types';
+import { normalizeThemeConfig } from '../utils';
 
 const fontSizeMap: Record<FontSizeKey, keyof SupersetTheme> = {
   xs: 'fontSizeXS',
@@ -113,6 +120,22 @@ export function isThemeDark(theme: SupersetTheme): boolean {
   return tinycolor(theme.colorBgContainer).isDark();
 }
 
+/**
+ * Check if a theme configuration results in a dark theme
+ * @param config - The theme configuration to check
+ * @returns true if the config results in a dark theme, false otherwise
+ */
+export function isThemeConfigDark(config: AnyThemeConfig): boolean {
+  try {
+    const normalizedConfig = normalizeThemeConfig(config);
+    const themeConfig = antdTheme.getDesignToken(normalizedConfig);
+
+    return tinycolor(themeConfig.colorBgContainer).isDark();
+  } catch {
+    return false;
+  }
+}
+
 /**
  * Hook to determine if the current theme is dark mode
  * @returns true if theme is dark, false if light