[PR] feat(theme): introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz opened a new pull request, #34144:
URL: https://github.com/apache/superset/pull/34144

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   This change adds full support for Superset’s bootstrap-provided theme objects (default, dark, and settings) and integrates them into the existing ThemeController API.
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   1. Pass a valid `Theme` dict to `THEME_DEFAULT` and or `THEME_DARK` (you can generate one in the Ant Design theme editor: https://ant.design/theme-editor)
   2. Reload the frontend app
   3. Watch the magic happens 
   
   For `THEME_SETTINGS`:
   1. Pass a valid `ThemeSettings` dict
   E.g.:
   ```
   THEME_SETTINGS = {
       "enforced": True,            # If True, forces the default theme and ignores user preferences
       "allowSwitching": True,      # Allow user to switch between themes (default and dark)
       "allowOSPreference": True,   # Allow the app to Auto-detect and set system theme preference
   }
   ```
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [x] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [x] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2206090130


##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -45,202 +81,618 @@ export class ThemeController {
 
   private storage: ThemeStorage;
 
-  private storageKey: string;
-
   private modeStorageKey: string;
 
   private defaultTheme: AnyThemeConfig;
 
-  private systemMode: ThemeMode.DARK | ThemeMode.LIGHT;
+  private darkTheme: AnyThemeConfig | null;
 
-  private currentMode: ThemeMode;
+  private themeSettings: SerializableThemeSettings;
 
-  private customizations: AnyThemeConfig = {};
+  private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT;
 
-  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
+  private currentMode: ThemeMode;
 
-  private canUpdateThemeFn: () => boolean;
+  private readonly hasBootstrapThemes: boolean;
 
-  private canUpdateModeFn: () => boolean;
+  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
 
   private mediaQuery: MediaQueryList;
 
-  constructor(options: ThemeControllerOptions = { themeObject }) {
-    this.storage = options.storage || new LocalStorageAdapter();
-    this.storageKey = options.storageKey || 'superset-theme';
-    this.modeStorageKey = options.modeStorageKey || `${this.storageKey}-mode`;
-    this.defaultTheme = options.defaultTheme || {};
-    this.themeObject = options.themeObject;
-
-    // Load customizations from storage
-    const savedThemeJson = this.storage.getItem(this.storageKey);
-    if (savedThemeJson) {
-      try {
-        this.customizations = JSON.parse(savedThemeJson);
-      } catch (e) {
-        console.error('Failed to parse saved theme:', e);
-        this.storage.removeItem(this.storageKey);
-      }
+  constructor(options: ThemeControllerOptions = {}) {
+    const {
+      storage = new LocalStorageAdapter(),
+      modeStorageKey = STORAGE_KEYS.THEME_MODE,
+      themeObject: fallbackThemeObject = supersetThemeObject,
+      defaultTheme = (supersetThemeObject.theme as AnyThemeConfig) ?? {},
+      onChange = null,
+    } = options;
+
+    this.storage = storage;
+    this.modeStorageKey = modeStorageKey;
+
+    // Initialize bootstrap data and themes
+    const {
+      bootstrapDefaultTheme,
+      bootstrapDarkTheme,
+      bootstrapThemeSettings,
+      hasBootstrapThemes,
+    }: BootstrapThemeData = this.loadBootstrapData();
+
+    this.hasBootstrapThemes = hasBootstrapThemes;
+    this.themeSettings = bootstrapThemeSettings || {};
+
+    // Set themes based on bootstrap data availability
+    if (this.hasBootstrapThemes) {
+      this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null;
+      this.defaultTheme =
+        bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme;
+    } else {
+      this.darkTheme = null;
+      this.defaultTheme = defaultTheme;
     }
 
-    // Determine initial mode
-    this.systemMode = ThemeController.getSystemMode();
-    const savedMode = this.storage.getItem(this.modeStorageKey) as ThemeMode;
-    this.currentMode = savedMode || ThemeMode.SYSTEM;
+    // Initialize system theme detection
+    this.systemMode = ThemeController.getSystemPreferredMode();
 
-    // Apply the initial theme and mode
-    this.applyTheme();
+    // Only initialize media query listener if OS preference is allowed
+    if (this.shouldInitializeMediaQueryListener())
+      this.initializeMediaQueryListener();
 
-    if (options.onChange) {
-      this.onChangeCallbacks.add(options.onChange);
-    }
-    this.canUpdateThemeFn = options.canUpdateTheme || (() => true);
-    this.canUpdateModeFn = options.canUpdateMode || (() => true);
+    // Initialize theme and mode
+    this.currentMode = this.determineInitialMode();
+    const { theme, themeObject } =
+      this.createInitialThemeObject(fallbackThemeObject);
 
-    // Listen for system theme changes to enable dynamic `SYSTEM` mode
-    this.mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
-    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+    this.themeObject = themeObject;
+
+    // Setup change callback
+    if (onChange) this.onChangeCallbacks.add(onChange);
+
+    // Apply initial theme and persist mode
+    this.applyTheme(theme);
+    this.persistMode();
   }
 
+  // Public Methods
+
   /**
-   * Cleans up listeners. Should be called when the controller is no longer needed.
+   * Cleans up listeners and references. Should be called when the controller is no longer needed.
    */
   public destroy(): void {
     this.mediaQuery.removeEventListener('change', this.handleSystemThemeChange);
+    this.onChangeCallbacks.clear();
   }
 
-  public canUpdateTheme(): boolean {
-    return this.canUpdateThemeFn();
+  /**
+   * Check if the user can update the theme.
+   */
+  public canSetTheme(): boolean {
+    return !this.themeSettings?.enforced;
   }
 
-  public canUpdateMode(): boolean {
-    return this.canUpdateModeFn();
+  /**
+   * Check if the user can update the theme mode.
+   */
+  public canSetMode(): boolean {
+    return this.isModeUpdatable();
   }
 
+  /**
+   * Returns the current theme object.
+   */
   public getTheme(): Theme {
     return this.themeObject;
   }
 
+  /**
+   * Returns the current theme mode.
+   */
   public getCurrentMode(): ThemeMode {
     return this.currentMode;
   }
 
   /**
-   * Sets new theme customizations (e.g., from a JSON editor).
-   * This method updates the theme's appearance but preserves the current mode.
+   * Sets new theme.
+   * @param theme - The new theme to apply
+   * @throws {Error} If the user does not have permission to update the theme
    */
-  public setTheme(newCustomizations: AnyThemeConfig): void {
-    if (!this.canUpdateTheme()) {
-      throw new Error('User does not have permission to update the theme');
-    }
-    this.customizations = newCustomizations;
+  public setTheme(theme: AnyThemeConfig): void {
+    this.validateThemeUpdatePermission();
 
-    if (!newCustomizations.algorithm) {
-      this.currentMode = ThemeMode.LIGHT;
-    }
+    const { mode, normalizedTheme } = this.normalizeTheme(theme);
+    this.currentMode = mode;
+
+    this.updateTheme(normalizedTheme);
+  }
 
-    if (newCustomizations?.algorithm) {
-      this.currentMode = newCustomizations.algorithm as ThemeMode;
+  /**
+   * Sets the theme mode (light, dark, or system).
+   * @param mode - The new theme mode to apply
+   * @throws {Error} If the user does not have permission to update the theme mode
+   */
+  public setThemeMode(mode: ThemeMode): void {
+    this.validateModeUpdatePermission(mode);
+
+    if (this.currentMode === mode) return;
+
+    const theme: AnyThemeConfig | null = this.getThemeForMode(mode);
+    if (!theme) {
+      console.warn(`Theme for mode ${mode} not found, falling back to default`);
+      this.fallbackToDefaultMode();
+      return;
     }
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+    this.currentMode = mode;
+    this.updateTheme(theme);
   }
 
   /**
-   * Changes the theme mode (light, dark, or system).
-   * This is for the mode switch.
+   * Resets the theme to the default theme.
    */
-  public changeThemeMode(newMode: ThemeMode): void {
-    if (!this.canUpdateMode()) {
-      throw new Error('User does not have permission to update the theme mode');
+  public resetTheme(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.updateTheme(defaultTheme);
+  }
+
+  /**
+   * Handles system theme changes with error recovery.
+   */
+  private handleSystemThemeChange = (): void => {
+    try {
+      const newSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK =
+        ThemeController.getSystemPreferredMode();
+
+      // Update systemMode regardless of current mode
+      const oldSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK = this.systemMode;
+      this.systemMode = newSystemMode;
+
+      // Only update theme if currently in SYSTEM mode and the preference changed
+      if (
+        this.currentMode === ThemeMode.SYSTEM &&
+        oldSystemMode !== newSystemMode
+      ) {
+        const newTheme: AnyThemeConfig | null = this.getThemeForMode(
+          ThemeMode.SYSTEM,
+        );
+
+        if (newTheme) this.updateTheme(newTheme);
+      }
+    } catch (error) {
+      console.error('Failed to handle system theme change:', error);
     }
-    if (this.currentMode === newMode) return;
+  };
 
-    this.currentMode = newMode;
+  /**
+   * Updates the theme.
+   * @param theme - The new theme to apply
+   */
+  private updateTheme(theme?: AnyThemeConfig): void {
+    try {
+      // If no config provided, use current mode to get theme
+      const config: AnyThemeConfig =
+        theme || this.getThemeForMode(this.currentMode) || this.defaultTheme;
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+      // Normalize the theme
+      const { normalizedTheme } = this.normalizeTheme(config);
+
+      this.applyTheme(normalizedTheme);
+      this.persistMode();
+      this.notifyListeners();
+    } catch (error) {
+      console.error('Failed to update theme:', error);
+      this.fallbackToDefaultMode();
+    }
   }
 
-  public resetTheme(): void {
-    this.customizations = this.defaultTheme;
-    this.applyTheme();
-    this.persist();
+  /**
+   * Fallback to default mode with error recovery.
+   */
+  private fallbackToDefaultMode(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+
+    // Get the default theme which will have the correct algorithm
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.applyTheme(defaultTheme);
+    this.persistMode();
     this.notifyListeners();
   }
 
+  /**
+   * Registers a callback to be called when the theme changes.
+   * @param callback - The callback to be called on theme change
+   * @returns A function to unsubscribe from the theme change events
+   */
   public onChange(callback: (theme: Theme) => void): () => void {
     this.onChangeCallbacks.add(callback);
-    return () => {
-      this.onChangeCallbacks.delete(callback);
+    return () => this.onChangeCallbacks.delete(callback);
+  }
+
+  // Private Helper Methods
+
+  /**
+   * Determines whether the MediaQueryList listener for system theme changes should be initialized.
+   * This checks if OS preference detection is enabled in the theme settings.
+   * @returns {boolean} True if the media query listener should be initialized, false otherwise
+   */
+  private shouldInitializeMediaQueryListener(): boolean {
+    const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+      this.themeSettings || {};
+
+    return allowOSPreference === true;
+  }
+
+  /**
+   * Initializes media query listeners if OS preference is allowed
+   */
+  private initializeMediaQueryListener(): void {
+    this.mediaQuery = window.matchMedia(MEDIA_QUERY_DARK_SCHEME);
+    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+  }
+
+  /**
+   * Loads and validates bootstrap theme data.
+   */
+  private loadBootstrapData(): BootstrapThemeData {
+    const {
+      common: { theme = {} as BootstrapThemeDataConfig },
+    } = getBootstrapData();
+
+    const {
+      default: defaultTheme,
+      dark: darkTheme,
+      settings: themeSettings,
+    } = theme;
+
+    const hasValidDefault: boolean = this.hasKeys(defaultTheme);
+    const hasValidDark: boolean = this.hasKeys(darkTheme);
+    const hasValidSettings: boolean = this.hasKeys(themeSettings);
+
+    return {
+      bootstrapDefaultTheme: hasValidDefault ? defaultTheme : null,
+      bootstrapDarkTheme: hasValidDark ? darkTheme : null,
+      bootstrapThemeSettings: hasValidSettings ? themeSettings : null,
+      hasBootstrapThemes: hasValidDefault || hasValidDark,
     };
   }
 
-  private handleSystemThemeChange = (): void => {
-    const newSystemMode = ThemeController.getSystemMode();
-    if (this.systemMode === newSystemMode) return;
+  /**
+   * Checks if an object has keys (not empty).
+   */
+  private hasKeys(obj: Record<string, any> | undefined | null): boolean {
+    return Boolean(
+      obj && typeof obj === 'object' && Object.keys(obj).length > 0,
+    );
+  }
 
-    this.systemMode = newSystemMode;
-    // If the current mode is SYSTEM, we need to re-apply the theme
-    if (this.currentMode === ThemeMode.SYSTEM) {
-      this.applyTheme();
-      this.notifyListeners();
+  /**
+   * Determines if mode updates are allowed.
+   */
+  private isModeUpdatable(): boolean {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings || {};
+
+    return !enforced && allowSwitching;
+  }
+
+  /**
+   * Normalizes the theme configuration to ensure it has a valid algorithm.
+   * @param theme - The theme configuration to normalize
+   * @returns An object with normalized mode and algorithm.
+   */
+  private normalizeTheme(theme: AnyThemeConfig): {
+    mode: ThemeMode;
+    normalizedTheme: AnyThemeConfig;
+  } {
+    const algorithm: ThemeAlgorithm | ThemeAlgorithm[] = this.getValidAlgorithm(
+      (theme?.algorithm as ThemeAlgorithm | ThemeAlgorithm[]) ||
+        ThemeAlgorithm.DEFAULT,
+    );
+
+    // Extract the mode from the valid algorithm
+    let mode: ThemeMode;
+
+    if (Array.isArray(algorithm)) {
+      const foundAlgorithm =
+        algorithm.find(
+          (m: ThemeAlgorithm) =>
+            m === ThemeAlgorithm.DARK || m === ThemeAlgorithm.DEFAULT,
+        ) ?? ThemeAlgorithm.DEFAULT;
+
+      mode = this.algorithmToMode(foundAlgorithm);
+    } else mode = this.algorithmToMode(algorithm);
+
+    return {
+      mode,
+      normalizedTheme: {
+        ...theme,
+        algorithm,
+      } as AnyThemeConfig,
+    };
+  }
+
+  /**
+   * Converts a ThemeAlgorithm to its corresponding ThemeMode.
+   * @param algorithm - The theme algorithm to convert
+   * @returns The corresponding theme mode
+   */
+  private algorithmToMode(algorithm: ThemeAlgorithm): ThemeMode {
+    switch (algorithm) {
+      case ThemeAlgorithm.DARK:
+        return ThemeMode.DARK;
+      case ThemeAlgorithm.DEFAULT:
+        return ThemeMode.DEFAULT;
+      case ThemeAlgorithm.COMPACT:
+        return ThemeMode.DEFAULT;
+      default:
+        return ThemeMode.DEFAULT;
     }
-  };
+  }
+
+  /**
+   * Returns the appropriate theme configuration for a given mode.
+   * @param mode - The theme mode to get the configuration for
+   * @returns The theme configuration for the specified mode or null if not available
+   */
+  private getThemeForMode(mode: ThemeMode): AnyThemeConfig | null {
+    const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+      this.themeSettings;
+
+    let resolvedMode: ThemeMode = mode;
+
+    if (mode === ThemeMode.SYSTEM) {
+      if (!allowOSPreference) return null;
+      resolvedMode = ThemeController.getSystemPreferredMode();
+    }
+
+    // When we don't have bootstrap themes, we need to create variants using algorithm
+    if (!this.hasBootstrapThemes) {
+      if (resolvedMode === ThemeMode.DARK)
+        return {
+          ...this.defaultTheme,
+          algorithm: ThemeAlgorithm.DARK,
+        };
+
+      return {
+        ...this.defaultTheme,
+        algorithm: ThemeAlgorithm.DEFAULT,
+      };
+    }
+
+    // When we have bootstrap themes, use them
+    if (resolvedMode === ThemeMode.DARK)
+      return this.darkTheme || this.defaultTheme;
+
+    return this.defaultTheme;
+  }
+
+  /**
+   * Creates the initial theme object.
+   * This sets the theme based on the current mode and ensures it has the correct algorithm.
+   * @param defaultThemeObject - The fallback theme object to use if no theme is set
+   * @returns An object containing the theme and the themeObject
+   */
+  private createInitialThemeObject(defaultThemeObject: Theme): {
+    theme: AnyThemeConfig;
+    themeObject: Theme;
+  } {
+    let theme: AnyThemeConfig | null = this.getThemeForMode(this.currentMode);
+    theme = theme || (defaultThemeObject.theme as AnyThemeConfig);
+
+    const { normalizedTheme } = this.normalizeTheme(theme);
+
+    return {
+      theme: normalizedTheme,
+      themeObject: Theme.fromConfig(normalizedTheme),
+    };
+  }
+
+  /**
+   * Determines the initial theme mode with error recovery.
+   */
+  private determineInitialMode(): ThemeMode {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings;
+
+    // Enforced mode always takes precedence
+    if (enforced) {
+      this.storage.removeItem(this.modeStorageKey);
+      return ThemeMode.DEFAULT;
+    }
+
+    // When OS preference is allowed but switching is not
+    // This means the user MUST follow OS preference and cannot override it
+    if (allowOSPreference && !allowSwitching) {
+      // Clear any saved preference since switching is not allowed
+      this.storage.removeItem(this.modeStorageKey);
+      return ThemeMode.SYSTEM;
+    }
+
+    // Try to restore saved mode
+    const savedMode: ThemeMode | null = this.loadSavedMode();
+    if (savedMode && this.isValidThemeMode(savedMode)) return savedMode;
+
+    // Fallback to system preference if allowed and available
+    if (allowOSPreference && this.getThemeForMode(this.systemMode))
+      return ThemeMode.SYSTEM;
+
+    return ThemeMode.DEFAULT;
+  }
+
+  /**
+   * Safely loads saved theme mode from storage.
+   */
+  private loadSavedMode(): ThemeMode | null {
+    try {
+      const stored: string | null = this.storage.getItem(this.modeStorageKey);
+      if (stored && Object.values(ThemeMode).includes(stored as ThemeMode))
+        return stored as ThemeMode;
+
+      return null;
+    } catch (error) {
+      console.warn('Failed to load saved theme mode:', error);
+      return null;
+    }
+  }
 
   /**
-   * Centralized method to apply the current customizations and mode.
+   * Validates if a theme mode is valid and supported.
+   * This checks if the mode is one of the known ThemeMode values.
+   * @param mode - The theme mode to validate
+   * @returns {boolean} True if the mode is valid, false otherwise
    */
-  private applyTheme(): void {
-    const newConfig = { ...this.customizations };
+  private isValidThemeMode(mode: ThemeMode): boolean {
+    if (!Object.values(ThemeMode).includes(mode)) return false;
 
-    switch (this.currentMode) {
+    // Validate that we have the required theme data for the mode
+    switch (mode) {
       case ThemeMode.DARK:
-        newConfig.algorithm = 'dark';
-        break;
-      case ThemeMode.LIGHT:
-        newConfig.algorithm = 'default';
-        break;
+        return !!(this.darkTheme || this.defaultTheme);
+      case ThemeMode.DEFAULT:
+        return !!this.defaultTheme;
       case ThemeMode.SYSTEM:
-        newConfig.algorithm =
-          this.systemMode === ThemeMode.DARK ? 'dark' : 'default';
-        break;
-      case ThemeMode.COMPACT:
-        newConfig.algorithm = 'compact';
-        break;
+        return this.themeSettings?.allowOSPreference !== false;
       default:
-        newConfig.algorithm = 'default';
-        break;
+        return true;
     }
+  }
 
-    this.themeObject.setConfig(newConfig);
+  /**
+   * Validates permission to update theme.
+   */
+  private validateThemeUpdatePermission(): void {
+    if (!this.canSetTheme())
+      throw new Error('User does not have permission to update the theme');
   }
 
-  private persist(): void {
-    this.storage.setItem(this.modeStorageKey, this.currentMode);
+  /**
+   * Validates permission to update mode.
+   * @param newMode - The new mode to validate
+   * @throws {Error} If the user does not have permission to update the theme mode
+   * @throws {Error} If the new mode is SYSTEM and OS preference is not allowed
+   */
+  private validateModeUpdatePermission(newMode: ThemeMode): void {
+    const {
+      allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings;
+
+    // If OS preference is allowed but switching is not,
+    // don't allow any mode changes
+    if (allowOSPreference && !allowSwitching)
+      throw new Error(
+        'Theme mode changes are not allowed when OS preference is enforced',
+      );
+
+    // Check if user can set a new theme mode
+    if (!this.canSetMode())
+      throw new Error('User does not have permission to update the theme mode');
+
+    // Check if user has permissions to set OS preference as a theme mode
+    if (newMode === ThemeMode.SYSTEM && !allowOSPreference)
+      throw new Error('System theme mode is not allowed');
+  }
 
-    const { algorithm, ...persistedCustomizations } = this.customizations;
-    this.storage.setItem(
-      this.storageKey,
-      JSON.stringify(persistedCustomizations),
+  /**
+   * Validates theme algorithm combinations.
+   * This checks if the provided algorithm is a valid combination of ThemeMode values.
+   * @param algorithm - The theme algorithm to validate
+   * @returns {boolean} True if the algorithms combination is valid, false otherwise
+   */
+  private isValidAlgorithmCombination(algorithm: ThemeAlgorithm[]): boolean {
+    const inputSet = new Set(algorithm);
+    return VALID_ALGORITHM_COMBINATIONS.some(
+      validCombination =>
+        inputSet.size === validCombination.size &&
+        [...inputSet].every(item => validCombination.has(item)),
     );
   }
 
+  /**
+   * Checks if the algorithm is a valid combination or a simple one.
+   * @param algorithm - The theme mode or combination to convert
+   * @returns A valid ThemeAlgorithm or ThemeAlgorithm[]
+   */
+  private getValidAlgorithm(

Review Comment:
   Totally agree, I'll fix this ASAP



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["mistercrunch (via GitHub)" <gi...@apache.org>]

mistercrunch commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2202087778


##########
superset-frontend/packages/superset-ui-core/src/theme/types.ts:
##########
@@ -33,18 +33,33 @@ import { Theme } from '.';
 export type AntdTokens = ReturnType<typeof antdThemeImport.getDesignToken>;
 export type AntdThemeConfig = ThemeConfig;
 
+/**
+ * A combination of theme modes that can be used in theme config.
+ * This is used to define which algorithms are allow to be applied together.
+ */
+export type ThemeAlgorithmCombination = (

Review Comment:
   conceptually we're kind of mixing some types that aren't the same, might want to decouple `Algorithms` as supported by antd ('default', 'dark', 'compact') from our `ThemeDarknessSetting` ('default', 'dark', 'system')



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["mistercrunch (via GitHub)" <gi...@apache.org>]

mistercrunch merged PR #34144:
URL: https://github.com/apache/superset/pull/34144


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theme): introduce bootstrap-driven Superset theme configurations [superset]

["korbit-ai[bot] (via GitHub)" <gi...@apache.org>]

korbit-ai[bot] commented on PR #34144:
URL: https://github.com/apache/superset/pull/34144#issuecomment-3063960322

   Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
   
   > Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
   
   
   
   [](d591012b-e4c2-49ad-be99-95748047833a)


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theme): introduce bootstrap-driven Superset theme configurations [superset]

["codecov[bot] (via GitHub)" <gi...@apache.org>]

codecov[bot] commented on PR #34144:
URL: https://github.com/apache/superset/pull/34144#issuecomment-3063969172

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/34144?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: Patch coverage is `50.00000%` with `44 lines` in your changes missing coverage. Please review.
   > Project coverage is 51.68%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`aff0a79`)](https://app.codecov.io/gh/apache/superset/commit/aff0a79d7b0b10c25f7f9d0dd81e1c98c09c9f6e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 2116 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/34144?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/themes/utils.py](https://app.codecov.io/gh/apache/superset/pull/34144?src=pr&el=tree&filepath=superset%2Fthemes%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGhlbWVzL3V0aWxzLnB5) | 22.22% | [31 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/34144?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [superset/views/base.py](https://app.codecov.io/gh/apache/superset/pull/34144?src=pr&el=tree&filepath=superset%2Fviews%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | 43.75% | [6 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/34144?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #34144      +/-   ##
   ==========================================
   - Coverage   60.48%   51.68%   -8.81%     
   ==========================================
     Files        1931      561    -1370     
     Lines       76236    40605   -35631     
     Branches     8568     4284    -4284     
   ==========================================
   - Hits        46114    20985   -25129     
   + Misses      28017    19051    -8966     
   + Partials     2105      569    -1536     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/34144/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/34144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.14% <50.00%> (-2.01%)` | :arrow_down: |
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/34144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/34144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.89% <50.00%> (-2.92%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/34144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `51.67% <50.00%> (-11.84%)` | :arrow_down: |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/34144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.36%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/34144?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["mistercrunch (via GitHub)" <gi...@apache.org>]

mistercrunch commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205938335


##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -45,202 +81,618 @@ export class ThemeController {
 
   private storage: ThemeStorage;
 
-  private storageKey: string;
-
   private modeStorageKey: string;
 
   private defaultTheme: AnyThemeConfig;
 
-  private systemMode: ThemeMode.DARK | ThemeMode.LIGHT;
+  private darkTheme: AnyThemeConfig | null;
 
-  private currentMode: ThemeMode;
+  private themeSettings: SerializableThemeSettings;
 
-  private customizations: AnyThemeConfig = {};
+  private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT;
 
-  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
+  private currentMode: ThemeMode;
 
-  private canUpdateThemeFn: () => boolean;
+  private readonly hasBootstrapThemes: boolean;
 
-  private canUpdateModeFn: () => boolean;
+  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
 
   private mediaQuery: MediaQueryList;
 
-  constructor(options: ThemeControllerOptions = { themeObject }) {
-    this.storage = options.storage || new LocalStorageAdapter();
-    this.storageKey = options.storageKey || 'superset-theme';
-    this.modeStorageKey = options.modeStorageKey || `${this.storageKey}-mode`;
-    this.defaultTheme = options.defaultTheme || {};
-    this.themeObject = options.themeObject;
-
-    // Load customizations from storage
-    const savedThemeJson = this.storage.getItem(this.storageKey);
-    if (savedThemeJson) {
-      try {
-        this.customizations = JSON.parse(savedThemeJson);
-      } catch (e) {
-        console.error('Failed to parse saved theme:', e);
-        this.storage.removeItem(this.storageKey);
-      }
+  constructor(options: ThemeControllerOptions = {}) {
+    const {
+      storage = new LocalStorageAdapter(),
+      modeStorageKey = STORAGE_KEYS.THEME_MODE,
+      themeObject: fallbackThemeObject = supersetThemeObject,
+      defaultTheme = (supersetThemeObject.theme as AnyThemeConfig) ?? {},
+      onChange = null,
+    } = options;
+
+    this.storage = storage;
+    this.modeStorageKey = modeStorageKey;
+
+    // Initialize bootstrap data and themes
+    const {
+      bootstrapDefaultTheme,
+      bootstrapDarkTheme,
+      bootstrapThemeSettings,
+      hasBootstrapThemes,
+    }: BootstrapThemeData = this.loadBootstrapData();
+
+    this.hasBootstrapThemes = hasBootstrapThemes;
+    this.themeSettings = bootstrapThemeSettings || {};
+
+    // Set themes based on bootstrap data availability
+    if (this.hasBootstrapThemes) {
+      this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null;
+      this.defaultTheme =
+        bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme;
+    } else {
+      this.darkTheme = null;
+      this.defaultTheme = defaultTheme;
     }
 
-    // Determine initial mode
-    this.systemMode = ThemeController.getSystemMode();
-    const savedMode = this.storage.getItem(this.modeStorageKey) as ThemeMode;
-    this.currentMode = savedMode || ThemeMode.SYSTEM;
+    // Initialize system theme detection
+    this.systemMode = ThemeController.getSystemPreferredMode();
 
-    // Apply the initial theme and mode
-    this.applyTheme();
+    // Only initialize media query listener if OS preference is allowed
+    if (this.shouldInitializeMediaQueryListener())
+      this.initializeMediaQueryListener();
 
-    if (options.onChange) {
-      this.onChangeCallbacks.add(options.onChange);
-    }
-    this.canUpdateThemeFn = options.canUpdateTheme || (() => true);
-    this.canUpdateModeFn = options.canUpdateMode || (() => true);
+    // Initialize theme and mode
+    this.currentMode = this.determineInitialMode();
+    const { theme, themeObject } =
+      this.createInitialThemeObject(fallbackThemeObject);
 
-    // Listen for system theme changes to enable dynamic `SYSTEM` mode
-    this.mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
-    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+    this.themeObject = themeObject;
+
+    // Setup change callback
+    if (onChange) this.onChangeCallbacks.add(onChange);
+
+    // Apply initial theme and persist mode
+    this.applyTheme(theme);
+    this.persistMode();
   }
 
+  // Public Methods
+
   /**
-   * Cleans up listeners. Should be called when the controller is no longer needed.
+   * Cleans up listeners and references. Should be called when the controller is no longer needed.
    */
   public destroy(): void {
     this.mediaQuery.removeEventListener('change', this.handleSystemThemeChange);
+    this.onChangeCallbacks.clear();
   }
 
-  public canUpdateTheme(): boolean {
-    return this.canUpdateThemeFn();
+  /**
+   * Check if the user can update the theme.
+   */
+  public canSetTheme(): boolean {
+    return !this.themeSettings?.enforced;
   }
 
-  public canUpdateMode(): boolean {
-    return this.canUpdateModeFn();
+  /**
+   * Check if the user can update the theme mode.
+   */
+  public canSetMode(): boolean {
+    return this.isModeUpdatable();
   }
 
+  /**
+   * Returns the current theme object.
+   */
   public getTheme(): Theme {
     return this.themeObject;
   }
 
+  /**
+   * Returns the current theme mode.
+   */
   public getCurrentMode(): ThemeMode {
     return this.currentMode;
   }
 
   /**
-   * Sets new theme customizations (e.g., from a JSON editor).
-   * This method updates the theme's appearance but preserves the current mode.
+   * Sets new theme.
+   * @param theme - The new theme to apply
+   * @throws {Error} If the user does not have permission to update the theme
    */
-  public setTheme(newCustomizations: AnyThemeConfig): void {
-    if (!this.canUpdateTheme()) {
-      throw new Error('User does not have permission to update the theme');
-    }
-    this.customizations = newCustomizations;
+  public setTheme(theme: AnyThemeConfig): void {
+    this.validateThemeUpdatePermission();
 
-    if (!newCustomizations.algorithm) {
-      this.currentMode = ThemeMode.LIGHT;
-    }
+    const { mode, normalizedTheme } = this.normalizeTheme(theme);
+    this.currentMode = mode;
+
+    this.updateTheme(normalizedTheme);
+  }
 
-    if (newCustomizations?.algorithm) {
-      this.currentMode = newCustomizations.algorithm as ThemeMode;
+  /**
+   * Sets the theme mode (light, dark, or system).
+   * @param mode - The new theme mode to apply
+   * @throws {Error} If the user does not have permission to update the theme mode
+   */
+  public setThemeMode(mode: ThemeMode): void {
+    this.validateModeUpdatePermission(mode);
+
+    if (this.currentMode === mode) return;
+
+    const theme: AnyThemeConfig | null = this.getThemeForMode(mode);
+    if (!theme) {
+      console.warn(`Theme for mode ${mode} not found, falling back to default`);
+      this.fallbackToDefaultMode();
+      return;
     }
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+    this.currentMode = mode;
+    this.updateTheme(theme);
   }
 
   /**
-   * Changes the theme mode (light, dark, or system).
-   * This is for the mode switch.
+   * Resets the theme to the default theme.
    */
-  public changeThemeMode(newMode: ThemeMode): void {
-    if (!this.canUpdateMode()) {
-      throw new Error('User does not have permission to update the theme mode');
+  public resetTheme(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.updateTheme(defaultTheme);
+  }
+
+  /**
+   * Handles system theme changes with error recovery.
+   */
+  private handleSystemThemeChange = (): void => {
+    try {
+      const newSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK =
+        ThemeController.getSystemPreferredMode();
+
+      // Update systemMode regardless of current mode
+      const oldSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK = this.systemMode;
+      this.systemMode = newSystemMode;
+
+      // Only update theme if currently in SYSTEM mode and the preference changed
+      if (
+        this.currentMode === ThemeMode.SYSTEM &&
+        oldSystemMode !== newSystemMode
+      ) {
+        const newTheme: AnyThemeConfig | null = this.getThemeForMode(
+          ThemeMode.SYSTEM,
+        );
+
+        if (newTheme) this.updateTheme(newTheme);
+      }
+    } catch (error) {
+      console.error('Failed to handle system theme change:', error);
     }
-    if (this.currentMode === newMode) return;
+  };
 
-    this.currentMode = newMode;
+  /**
+   * Updates the theme.
+   * @param theme - The new theme to apply
+   */
+  private updateTheme(theme?: AnyThemeConfig): void {
+    try {
+      // If no config provided, use current mode to get theme
+      const config: AnyThemeConfig =
+        theme || this.getThemeForMode(this.currentMode) || this.defaultTheme;
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+      // Normalize the theme
+      const { normalizedTheme } = this.normalizeTheme(config);
+
+      this.applyTheme(normalizedTheme);
+      this.persistMode();
+      this.notifyListeners();
+    } catch (error) {
+      console.error('Failed to update theme:', error);
+      this.fallbackToDefaultMode();
+    }
   }
 
-  public resetTheme(): void {
-    this.customizations = this.defaultTheme;
-    this.applyTheme();
-    this.persist();
+  /**
+   * Fallback to default mode with error recovery.
+   */
+  private fallbackToDefaultMode(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+
+    // Get the default theme which will have the correct algorithm
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.applyTheme(defaultTheme);
+    this.persistMode();
     this.notifyListeners();
   }
 
+  /**
+   * Registers a callback to be called when the theme changes.
+   * @param callback - The callback to be called on theme change
+   * @returns A function to unsubscribe from the theme change events
+   */
   public onChange(callback: (theme: Theme) => void): () => void {
     this.onChangeCallbacks.add(callback);
-    return () => {
-      this.onChangeCallbacks.delete(callback);
+    return () => this.onChangeCallbacks.delete(callback);
+  }
+
+  // Private Helper Methods
+
+  /**
+   * Determines whether the MediaQueryList listener for system theme changes should be initialized.
+   * This checks if OS preference detection is enabled in the theme settings.
+   * @returns {boolean} True if the media query listener should be initialized, false otherwise
+   */
+  private shouldInitializeMediaQueryListener(): boolean {
+    const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+      this.themeSettings || {};
+
+    return allowOSPreference === true;
+  }
+
+  /**
+   * Initializes media query listeners if OS preference is allowed
+   */
+  private initializeMediaQueryListener(): void {
+    this.mediaQuery = window.matchMedia(MEDIA_QUERY_DARK_SCHEME);
+    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+  }
+
+  /**
+   * Loads and validates bootstrap theme data.
+   */
+  private loadBootstrapData(): BootstrapThemeData {
+    const {
+      common: { theme = {} as BootstrapThemeDataConfig },
+    } = getBootstrapData();
+
+    const {
+      default: defaultTheme,
+      dark: darkTheme,
+      settings: themeSettings,
+    } = theme;
+
+    const hasValidDefault: boolean = this.hasKeys(defaultTheme);
+    const hasValidDark: boolean = this.hasKeys(darkTheme);
+    const hasValidSettings: boolean = this.hasKeys(themeSettings);
+
+    return {
+      bootstrapDefaultTheme: hasValidDefault ? defaultTheme : null,
+      bootstrapDarkTheme: hasValidDark ? darkTheme : null,
+      bootstrapThemeSettings: hasValidSettings ? themeSettings : null,
+      hasBootstrapThemes: hasValidDefault || hasValidDark,
     };
   }
 
-  private handleSystemThemeChange = (): void => {
-    const newSystemMode = ThemeController.getSystemMode();
-    if (this.systemMode === newSystemMode) return;
+  /**
+   * Checks if an object has keys (not empty).
+   */
+  private hasKeys(obj: Record<string, any> | undefined | null): boolean {
+    return Boolean(
+      obj && typeof obj === 'object' && Object.keys(obj).length > 0,
+    );
+  }
 
-    this.systemMode = newSystemMode;
-    // If the current mode is SYSTEM, we need to re-apply the theme
-    if (this.currentMode === ThemeMode.SYSTEM) {
-      this.applyTheme();
-      this.notifyListeners();
+  /**
+   * Determines if mode updates are allowed.
+   */
+  private isModeUpdatable(): boolean {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings || {};
+
+    return !enforced && allowSwitching;
+  }
+
+  /**
+   * Normalizes the theme configuration to ensure it has a valid algorithm.
+   * @param theme - The theme configuration to normalize
+   * @returns An object with normalized mode and algorithm.
+   */
+  private normalizeTheme(theme: AnyThemeConfig): {

Review Comment:
   Isn't this duplicated logic from what's in `utils.ts`?  https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/theme/utils.ts#L111-L116
   
   Seems ThemeController should probably only deal with `AnyThemeConfig` and assume that the Theme class will always handle things properly or error out if there's a bad theme object (?)



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["korbit-ai[bot] (via GitHub)" <gi...@apache.org>]

korbit-ai[bot] commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205626371


##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -45,202 +81,570 @@
 
   private storage: ThemeStorage;
 
-  private storageKey: string;
-
   private modeStorageKey: string;
 
   private defaultTheme: AnyThemeConfig;
 
-  private systemMode: ThemeMode.DARK | ThemeMode.LIGHT;
+  private darkTheme: AnyThemeConfig | null;
 
-  private currentMode: ThemeMode;
+  private themeSettings: SerializableThemeSettings;
 
-  private customizations: AnyThemeConfig = {};
+  private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT;
 
-  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
+  private currentMode: ThemeMode;
 
-  private canUpdateThemeFn: () => boolean;
+  private readonly hasBootstrapThemes: boolean;
 
-  private canUpdateModeFn: () => boolean;
+  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
 
   private mediaQuery: MediaQueryList;
 
-  constructor(options: ThemeControllerOptions = { themeObject }) {
-    this.storage = options.storage || new LocalStorageAdapter();
-    this.storageKey = options.storageKey || 'superset-theme';
-    this.modeStorageKey = options.modeStorageKey || `${this.storageKey}-mode`;
-    this.defaultTheme = options.defaultTheme || {};
-    this.themeObject = options.themeObject;
-
-    // Load customizations from storage
-    const savedThemeJson = this.storage.getItem(this.storageKey);
-    if (savedThemeJson) {
-      try {
-        this.customizations = JSON.parse(savedThemeJson);
-      } catch (e) {
-        console.error('Failed to parse saved theme:', e);
-        this.storage.removeItem(this.storageKey);
-      }
+  constructor(options: ThemeControllerOptions = {}) {
+    const {
+      storage = new LocalStorageAdapter(),
+      modeStorageKey = STORAGE_KEYS.THEME_MODE,
+      themeObject: fallbackThemeObject = supersetThemeObject,
+      defaultTheme = (supersetThemeObject.theme as AnyThemeConfig) ?? {},
+      onChange = null,
+    } = options;
+
+    this.storage = storage;
+    this.modeStorageKey = modeStorageKey;
+
+    // Initialize bootstrap data and themes
+    const {
+      bootstrapDefaultTheme,
+      bootstrapDarkTheme,
+      bootstrapThemeSettings,
+      hasBootstrapThemes,
+    }: BootstrapThemeData = this.loadBootstrapData();
+
+    this.hasBootstrapThemes = hasBootstrapThemes;
+    this.themeSettings = bootstrapThemeSettings || {};
+
+    // Set themes based on bootstrap data availability
+    if (this.hasBootstrapThemes) {
+      this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null;
+      this.defaultTheme =
+        bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme;
+    } else {
+      this.darkTheme = null;
+      this.defaultTheme = defaultTheme;
     }

Review Comment:
   ### Premature MediaQuery Listener <sub>![category Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The MediaQueryList listener is created immediately in the constructor without checking if system preferences are allowed or needed.
   
   
   ###### Why this matters
   Unnecessary event listener registration and processing when system preferences are not used, leading to wasted resources and unnecessary system theme checks.
   
   ###### Suggested change ∙ *Feature Preview*
   Create the MediaQuery listener only when needed:
   ```typescript
   private initializeSystemThemeDetection(): void {
     if (this.themeSettings?.allowOSPreference) {
       this.mediaQuery = window.matchMedia(MEDIA_QUERY_DARK_SCHEME);
       this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
     }
   }
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:1e385654-1187-44a6-b933-64a0b1b2bf91 -->
   
   
   [](1e385654-1187-44a6-b933-64a0b1b2bf91)



##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -45,202 +81,570 @@ export class ThemeController {
 
   private storage: ThemeStorage;
 
-  private storageKey: string;
-
   private modeStorageKey: string;
 
   private defaultTheme: AnyThemeConfig;
 
-  private systemMode: ThemeMode.DARK | ThemeMode.LIGHT;
+  private darkTheme: AnyThemeConfig | null;
 
-  private currentMode: ThemeMode;
+  private themeSettings: SerializableThemeSettings;
 
-  private customizations: AnyThemeConfig = {};
+  private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT;
 
-  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
+  private currentMode: ThemeMode;
 
-  private canUpdateThemeFn: () => boolean;
+  private readonly hasBootstrapThemes: boolean;
 
-  private canUpdateModeFn: () => boolean;
+  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
 
   private mediaQuery: MediaQueryList;
 
-  constructor(options: ThemeControllerOptions = { themeObject }) {
-    this.storage = options.storage || new LocalStorageAdapter();
-    this.storageKey = options.storageKey || 'superset-theme';
-    this.modeStorageKey = options.modeStorageKey || `${this.storageKey}-mode`;
-    this.defaultTheme = options.defaultTheme || {};
-    this.themeObject = options.themeObject;
-
-    // Load customizations from storage
-    const savedThemeJson = this.storage.getItem(this.storageKey);
-    if (savedThemeJson) {
-      try {
-        this.customizations = JSON.parse(savedThemeJson);
-      } catch (e) {
-        console.error('Failed to parse saved theme:', e);
-        this.storage.removeItem(this.storageKey);
-      }
+  constructor(options: ThemeControllerOptions = {}) {
+    const {
+      storage = new LocalStorageAdapter(),
+      modeStorageKey = STORAGE_KEYS.THEME_MODE,
+      themeObject: fallbackThemeObject = supersetThemeObject,
+      defaultTheme = (supersetThemeObject.theme as AnyThemeConfig) ?? {},
+      onChange = null,
+    } = options;
+
+    this.storage = storage;
+    this.modeStorageKey = modeStorageKey;
+
+    // Initialize bootstrap data and themes
+    const {
+      bootstrapDefaultTheme,
+      bootstrapDarkTheme,
+      bootstrapThemeSettings,
+      hasBootstrapThemes,
+    }: BootstrapThemeData = this.loadBootstrapData();
+
+    this.hasBootstrapThemes = hasBootstrapThemes;
+    this.themeSettings = bootstrapThemeSettings || {};
+
+    // Set themes based on bootstrap data availability
+    if (this.hasBootstrapThemes) {
+      this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null;
+      this.defaultTheme =
+        bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme;
+    } else {
+      this.darkTheme = null;
+      this.defaultTheme = defaultTheme;
     }
 
-    // Determine initial mode
-    this.systemMode = ThemeController.getSystemMode();
-    const savedMode = this.storage.getItem(this.modeStorageKey) as ThemeMode;
-    this.currentMode = savedMode || ThemeMode.SYSTEM;
+    // Initialize system theme detection
+    this.systemMode = ThemeController.getSystemPreferredMode();
+    this.mediaQuery = window.matchMedia(MEDIA_QUERY_DARK_SCHEME);
+    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
 
-    // Apply the initial theme and mode
-    this.applyTheme();
+    // Initialize theme and mode
+    this.currentMode = this.determineInitialMode();
+    const { theme, themeObject } =
+      this.createInitialThemeObject(fallbackThemeObject);
 
-    if (options.onChange) {
-      this.onChangeCallbacks.add(options.onChange);
-    }
-    this.canUpdateThemeFn = options.canUpdateTheme || (() => true);
-    this.canUpdateModeFn = options.canUpdateMode || (() => true);
+    this.themeObject = themeObject;
 
-    // Listen for system theme changes to enable dynamic `SYSTEM` mode
-    this.mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
-    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+    // Setup change callback
+    if (onChange) this.onChangeCallbacks.add(onChange);
+
+    // Apply initial theme and persist mode
+    this.applyTheme(theme);
+    this.persistMode();
   }
 
+  // Public Methods
+
   /**
-   * Cleans up listeners. Should be called when the controller is no longer needed.
+   * Cleans up listeners and references. Should be called when the controller is no longer needed.
    */
   public destroy(): void {
     this.mediaQuery.removeEventListener('change', this.handleSystemThemeChange);
+    this.onChangeCallbacks.clear();
   }
 
-  public canUpdateTheme(): boolean {
-    return this.canUpdateThemeFn();
+  /**
+   * Check if the user can update the theme.
+   */
+  public canSetTheme(): boolean {
+    return !this.themeSettings?.enforced;
   }
 
-  public canUpdateMode(): boolean {
-    return this.canUpdateModeFn();
+  /**
+   * Check if the user can update the theme mode.
+   */
+  public canSetMode(): boolean {
+    return this.isModeUpdatable();
   }
 
+  /**
+   * Returns the current theme object.
+   */
   public getTheme(): Theme {
     return this.themeObject;
   }
 
+  /**
+   * Returns the current theme mode.
+   */
   public getCurrentMode(): ThemeMode {
     return this.currentMode;
   }
 
   /**
-   * Sets new theme customizations (e.g., from a JSON editor).
-   * This method updates the theme's appearance but preserves the current mode.
+   * Sets new theme.
+   * @param theme - The new theme to apply
+   * @throws {Error} If the user does not have permission to update the theme
    */
-  public setTheme(newCustomizations: AnyThemeConfig): void {
-    if (!this.canUpdateTheme()) {
-      throw new Error('User does not have permission to update the theme');
-    }
-    this.customizations = newCustomizations;
+  public setTheme(theme: AnyThemeConfig): void {
+    this.validateThemeUpdatePermission();
 
-    if (!newCustomizations.algorithm) {
-      this.currentMode = ThemeMode.LIGHT;
-    }
+    const { mode, normalizedTheme } = this.normalizeTheme(theme);
+    this.currentMode = mode;
+
+    this.updateTheme(normalizedTheme);
+  }
 
-    if (newCustomizations?.algorithm) {
-      this.currentMode = newCustomizations.algorithm as ThemeMode;
+  /**
+   * Sets the theme mode (light, dark, or system).
+   * @param mode - The new theme mode to apply
+   * @throws {Error} If the user does not have permission to update the theme mode
+   */
+  public setThemeMode(mode: ThemeMode): void {
+    this.validateModeUpdatePermission(mode);
+
+    if (this.currentMode === mode) return;
+
+    const theme: AnyThemeConfig | null = this.getThemeForMode(mode);
+    if (!theme) {
+      console.warn(`Theme for mode ${mode} not found, falling back to default`);
+      this.fallbackToDefaultMode();
+      return;
     }
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+    this.currentMode = mode;
+    this.updateTheme(theme);
   }
 
   /**
-   * Changes the theme mode (light, dark, or system).
-   * This is for the mode switch.
+   * Resets the theme to the default theme.
    */
-  public changeThemeMode(newMode: ThemeMode): void {
-    if (!this.canUpdateMode()) {
-      throw new Error('User does not have permission to update the theme mode');
+  public resetTheme(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.updateTheme(defaultTheme);
+  }
+
+  /**
+   * Handles system theme changes with error recovery.
+   */
+  private handleSystemThemeChange = (): void => {
+    try {
+      const newSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK =
+        ThemeController.getSystemPreferredMode();
+
+      // Update systemMode regardless of current mode
+      const oldSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK = this.systemMode;
+      this.systemMode = newSystemMode;
+
+      // Only update theme if currently in SYSTEM mode and the preference changed
+      if (
+        this.currentMode === ThemeMode.SYSTEM &&
+        oldSystemMode !== newSystemMode
+      ) {
+        const newTheme: AnyThemeConfig | null = this.getThemeForMode(
+          ThemeMode.SYSTEM,
+        );
+
+        if (newTheme) this.updateTheme(newTheme);
+      }
+    } catch (error) {
+      console.error('Failed to handle system theme change:', error);
     }
-    if (this.currentMode === newMode) return;
+  };
 
-    this.currentMode = newMode;
+  /**
+   * Updates the theme.
+   * @param theme - The new theme to apply
+   */
+  private updateTheme(theme?: AnyThemeConfig): void {
+    try {
+      // If no config provided, use current mode to get theme
+      const config: AnyThemeConfig =
+        theme || this.getThemeForMode(this.currentMode) || this.defaultTheme;
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+      // Normalize the theme
+      const { normalizedTheme } = this.normalizeTheme(config);
+
+      this.applyTheme(normalizedTheme);
+      this.persistMode();
+      this.notifyListeners();
+    } catch (error) {
+      console.error('Failed to update theme:', error);
+      this.fallbackToDefaultMode();
+    }
   }
 
-  public resetTheme(): void {
-    this.customizations = this.defaultTheme;
-    this.applyTheme();
-    this.persist();
+  /**
+   * Fallback to default mode with error recovery.
+   */
+  private fallbackToDefaultMode(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+
+    // Get the default theme which will have the correct algorithm
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.applyTheme(defaultTheme);
+    this.persistMode();
     this.notifyListeners();
   }
 
+  /**
+   * Registers a callback to be called when the theme changes.
+   * @param callback - The callback to be called on theme change
+   * @returns A function to unsubscribe from the theme change events
+   */
   public onChange(callback: (theme: Theme) => void): () => void {
     this.onChangeCallbacks.add(callback);
-    return () => {
-      this.onChangeCallbacks.delete(callback);
+    return () => this.onChangeCallbacks.delete(callback);
+  }
+
+  // Private Helper Methods
+
+  /**
+   * Loads and validates bootstrap theme data.
+   */
+  private loadBootstrapData(): BootstrapThemeData {
+    const {
+      common: { theme = {} as BootstrapThemeDataConfig },
+    } = getBootstrapData();
+
+    const {
+      default: defaultTheme,
+      dark: darkTheme,
+      settings: themeSettings,
+    } = theme;
+
+    const hasValidDefault: boolean = this.hasKeys(defaultTheme);
+    const hasValidDark: boolean = this.hasKeys(darkTheme);
+    const hasValidSettings: boolean = this.hasKeys(themeSettings);
+
+    return {
+      bootstrapDefaultTheme: hasValidDefault ? defaultTheme : null,
+      bootstrapDarkTheme: hasValidDark ? darkTheme : null,
+      bootstrapThemeSettings: hasValidSettings ? themeSettings : null,
+      hasBootstrapThemes: hasValidDefault || hasValidDark,
     };
   }
 
-  private handleSystemThemeChange = (): void => {
-    const newSystemMode = ThemeController.getSystemMode();
-    if (this.systemMode === newSystemMode) return;
+  /**
+   * Checks if an object has keys (not empty).
+   */
+  private hasKeys(obj: Record<string, any> | undefined | null): boolean {
+    return Boolean(
+      obj && typeof obj === 'object' && Object.keys(obj).length > 0,
+    );
+  }
 
-    this.systemMode = newSystemMode;
-    // If the current mode is SYSTEM, we need to re-apply the theme
-    if (this.currentMode === ThemeMode.SYSTEM) {
-      this.applyTheme();
-      this.notifyListeners();
+  /**
+   * Determines if mode updates are allowed.
+   */
+  private isModeUpdatable(): boolean {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings || {};
+
+    return !enforced && allowSwitching;
+  }
+
+  /**
+   * Normalizes the theme configuration to ensure it has a valid algorithm.
+   * @param theme - The theme configuration to normalize
+   * @returns An object with normalized mode and algorithm.
+   */
+  private normalizeTheme(theme: AnyThemeConfig): {
+    mode: ThemeMode;
+    normalizedTheme: AnyThemeConfig;
+  } {
+    const algorithm: ThemeAlgorithm | ThemeAlgorithm[] = this.getValidAlgorithm(
+      (theme?.algorithm as ThemeAlgorithm | ThemeAlgorithm[]) ||
+        ThemeAlgorithm.DEFAULT,
+    );
+
+    // Extract the mode from the valid algorithm
+    let mode: ThemeMode;
+
+    if (Array.isArray(algorithm))
+      mode =
+        (algorithm.find(
+          (m: ThemeAlgorithm) =>
+            m === ThemeAlgorithm.DARK || m === ThemeAlgorithm.DEFAULT,
+        ) as unknown as ThemeMode) || ThemeMode.DEFAULT;
+    else mode = algorithm as unknown as ThemeMode;
+
+    return {
+      mode,
+      normalizedTheme: {
+        ...theme,
+        algorithm,
+      } as AnyThemeConfig,
+    };
+  }
+
+  /**
+   * Returns the appropriate theme configuration for a given mode.
+   * @param mode - The theme mode to get the configuration for
+   * @returns The theme configuration for the specified mode or null if not available
+   */
+  private getThemeForMode(mode: ThemeMode): AnyThemeConfig | null {
+    const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+      this.themeSettings;
+
+    let resolvedMode: ThemeMode = mode;
+
+    if (mode === ThemeMode.SYSTEM) {
+      if (!allowOSPreference) return null;
+      resolvedMode = ThemeController.getSystemPreferredMode();
     }
-  };
+
+    // When we don't have bootstrap themes, we need to create variants using algorithm
+    if (!this.hasBootstrapThemes) {
+      if (resolvedMode === ThemeMode.DARK)
+        return {
+          ...this.defaultTheme,
+          algorithm: ThemeAlgorithm.DARK,
+        };
+
+      return {
+        ...this.defaultTheme,
+        algorithm: ThemeAlgorithm.DEFAULT,
+      };
+    }
+
+    // When we have bootstrap themes, use them
+    if (resolvedMode === ThemeMode.DARK)
+      return this.darkTheme || this.defaultTheme;
+
+    return this.defaultTheme;
+  }
+
+  /**
+   * Creates the initial theme object.
+   * This sets the theme based on the current mode and ensures it has the correct algorithm.
+   * @param defaultThemeObject - The fallback theme object to use if no theme is set
+   * @returns An object containing the theme and the themeObject
+   */
+  private createInitialThemeObject(defaultThemeObject: Theme): {
+    theme: AnyThemeConfig;
+    themeObject: Theme;
+  } {
+    let theme: AnyThemeConfig | null = this.getThemeForMode(this.currentMode);
+    theme = theme || (defaultThemeObject.theme as AnyThemeConfig);
+
+    const { normalizedTheme } = this.normalizeTheme(theme);
+
+    return {
+      theme: normalizedTheme,
+      themeObject: Theme.fromConfig(normalizedTheme),
+    };
+  }
+
+  /**
+   * Determines the initial theme mode with error recovery.
+   */
+  private determineInitialMode(): ThemeMode {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings;
+
+    // Enforced mode always takes precedence
+    if (enforced) {
+      this.storage.removeItem(this.modeStorageKey);
+      return ThemeMode.DEFAULT;
+    }
+
+    // When OS preference is allowed but switching is not
+    // This means the user MUST follow OS preference and cannot override it
+    if (allowOSPreference && !allowSwitching) {
+      // Clear any saved preference since switching is not allowed
+      this.storage.removeItem(this.modeStorageKey);
+      return ThemeMode.SYSTEM;
+    }
+
+    // Try to restore saved mode
+    const savedMode: ThemeMode | null = this.loadSavedMode();
+    if (savedMode && this.isValidThemeMode(savedMode)) return savedMode;
+
+    // Fallback to system preference if allowed and available
+    if (allowOSPreference && this.getThemeForMode(this.systemMode))
+      return ThemeMode.SYSTEM;
+
+    return ThemeMode.DEFAULT;
+  }
+
+  /**
+   * Safely loads saved theme mode from storage.
+   */
+  private loadSavedMode(): ThemeMode | null {
+    try {
+      return this.storage.getItem(this.modeStorageKey) as ThemeMode;
+    } catch (error) {
+      console.warn('Failed to load saved theme mode:', error);
+      return null;
+    }
+  }
 
   /**
-   * Centralized method to apply the current customizations and mode.
+   * Validates if a theme mode is valid and supported.
+   * This checks if the mode is one of the known ThemeMode values.
+   * @param mode - The theme mode to validate
+   * @returns {boolean} True if the mode is valid, false otherwise
    */
-  private applyTheme(): void {
-    const newConfig = { ...this.customizations };
+  private isValidThemeMode(mode: ThemeMode): boolean {
+    if (!Object.values(ThemeMode).includes(mode)) return false;
 
-    switch (this.currentMode) {
+    // Validate that we have the required theme data for the mode
+    switch (mode) {
       case ThemeMode.DARK:
-        newConfig.algorithm = 'dark';
-        break;
-      case ThemeMode.LIGHT:
-        newConfig.algorithm = 'default';
-        break;
+        return !!(this.darkTheme || this.defaultTheme);
+      case ThemeMode.DEFAULT:
+        return !!this.defaultTheme;
       case ThemeMode.SYSTEM:
-        newConfig.algorithm =
-          this.systemMode === ThemeMode.DARK ? 'dark' : 'default';
-        break;
-      case ThemeMode.COMPACT:
-        newConfig.algorithm = 'compact';
-        break;
+        return this.themeSettings?.allowOSPreference !== false;
       default:
-        newConfig.algorithm = 'default';
-        break;
+        return true;
     }
+  }
 
-    this.themeObject.setConfig(newConfig);
+  /**
+   * Validates permission to update theme.
+   */
+  private validateThemeUpdatePermission(): void {
+    if (!this.canSetTheme())
+      throw new Error('User does not have permission to update the theme');
   }
 
-  private persist(): void {
-    this.storage.setItem(this.modeStorageKey, this.currentMode);
+  /**
+   * Validates permission to update mode.
+   * @param newMode - The new mode to validate
+   * @throws {Error} If the user does not have permission to update the theme mode
+   * @throws {Error} If the new mode is SYSTEM and OS preference is not allowed
+   */
+  private validateModeUpdatePermission(newMode: ThemeMode): void {
+    const {
+      allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings;
+
+    // If OS preference is allowed but switching is not,
+    // don't allow any mode changes
+    if (allowOSPreference && !allowSwitching)
+      throw new Error(
+        'Theme mode changes are not allowed when OS preference is enforced',
+      );
+
+    // Check if user can set a new theme mode
+    if (!this.canSetMode())
+      throw new Error('User does not have permission to update the theme mode');
+
+    // Check if user has permissions to set OS preference as a theme mode
+    if (newMode === ThemeMode.SYSTEM && !allowOSPreference)
+      throw new Error('System theme mode is not allowed');
+  }
 
-    const { algorithm, ...persistedCustomizations } = this.customizations;
-    this.storage.setItem(
-      this.storageKey,
-      JSON.stringify(persistedCustomizations),
+  /**
+   * Validates theme algorithm combinations.
+   * This checks if the provided algorithm is a valid combination of ThemeMode values.
+   * @param algorithm - The theme algorithm to validate
+   * @returns {boolean} True if the algorithms combination is valid, false otherwise
+   */
+  private isValidAlgorithmCombination(algorithm: ThemeAlgorithm[]): boolean {
+    const inputSet = new Set(algorithm);
+    return VALID_ALGORITHM_COMBINATIONS.some(
+      validCombination =>
+        inputSet.size === validCombination.size &&
+        [...inputSet].every(item => validCombination.has(item)),
     );
   }
 
+  /**
+   * Checks if the algorithm is a valid combination or a simple one.
+   * @param algorithm - The theme mode or combination to convert
+   * @returns A valid ThemeAlgorithm or ThemeAlgorithm[]
+   */
+  private getValidAlgorithm(
+    algorithm: ThemeAlgorithm | ThemeMode | ThemeAlgorithm[],
+  ): ThemeAlgorithm | ThemeAlgorithm[] {
+    if (Array.isArray(algorithm) && this.isValidAlgorithmCombination(algorithm))
+      return algorithm as ThemeAlgorithm[];
+
+    switch (algorithm) {
+      case ThemeAlgorithm.DARK:
+      case ThemeAlgorithm.COMPACT:
+        return algorithm;
+      case ThemeMode.SYSTEM:
+        return this.systemMode as unknown as ThemeAlgorithm;

Review Comment:
   ### Unsafe Algorithm Type Casting <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Unsafe type casting from ThemeMode.SYSTEM to ThemeAlgorithm without proper validation can lead to runtime errors.
   
   
   ###### Why this matters
   The direct type casting assumes systemMode values map directly to ThemeAlgorithm values, which may not be true. This could cause runtime errors if the values don't align.
   
   ###### Suggested change ∙ *Feature Preview*
   Add explicit mapping between ThemeMode and ThemeAlgorithm:
   ```typescript
   case ThemeMode.SYSTEM:
     return this.systemMode === ThemeMode.DARK
       ? ThemeAlgorithm.DARK
       : ThemeAlgorithm.DEFAULT;
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:a970960a-2b52-4e8a-ad31-122cae9b2481 -->
   
   
   [](a970960a-2b52-4e8a-ad31-122cae9b2481)



##########
superset-frontend/packages/superset-ui-core/src/theme/index.tsx:
##########
@@ -56,10 +56,12 @@ export function useTheme() {
   return theme;
 }
 
-const styled = emotionStyled;
+const styled: CreateStyled = emotionStyled;
 
 // launching in in dark mode for now while iterating
-const themeObject = Theme.fromConfig({ algorithm: 'default' });
+const themeObject: Theme = Theme.fromConfig({
+  algorithm: ThemeAlgorithm.DEFAULT,
+});
 
 const { theme } = themeObject;
 const supersetTheme = theme;

Review Comment:
   ### Redundant Theme Variable Declaration <sub>![category Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Redundant theme variable declaration creates confusion about which variable should be used.
   
   
   ###### Why this matters
   Having two variables referencing the same theme object can lead to inconsistent usage across the codebase and make maintenance more difficult.
   
   ###### Suggested change ∙ *Feature Preview*
   Remove the redundant supersetTheme variable and export theme directly:
   ```typescript
   const { theme } = themeObject;
   
   export { Theme, themeObject, styled, theme };
   ```
   
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:2548c5e6-d3c7-46e7-a395-49806a93a732 -->
   
   
   [](2548c5e6-d3c7-46e7-a395-49806a93a732)



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2206089798


##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -45,202 +81,618 @@ export class ThemeController {
 
   private storage: ThemeStorage;
 
-  private storageKey: string;
-
   private modeStorageKey: string;
 
   private defaultTheme: AnyThemeConfig;
 
-  private systemMode: ThemeMode.DARK | ThemeMode.LIGHT;
+  private darkTheme: AnyThemeConfig | null;
 
-  private currentMode: ThemeMode;
+  private themeSettings: SerializableThemeSettings;
 
-  private customizations: AnyThemeConfig = {};
+  private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT;
 
-  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
+  private currentMode: ThemeMode;
 
-  private canUpdateThemeFn: () => boolean;
+  private readonly hasBootstrapThemes: boolean;
 
-  private canUpdateModeFn: () => boolean;
+  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
 
   private mediaQuery: MediaQueryList;
 
-  constructor(options: ThemeControllerOptions = { themeObject }) {
-    this.storage = options.storage || new LocalStorageAdapter();
-    this.storageKey = options.storageKey || 'superset-theme';
-    this.modeStorageKey = options.modeStorageKey || `${this.storageKey}-mode`;
-    this.defaultTheme = options.defaultTheme || {};
-    this.themeObject = options.themeObject;
-
-    // Load customizations from storage
-    const savedThemeJson = this.storage.getItem(this.storageKey);
-    if (savedThemeJson) {
-      try {
-        this.customizations = JSON.parse(savedThemeJson);
-      } catch (e) {
-        console.error('Failed to parse saved theme:', e);
-        this.storage.removeItem(this.storageKey);
-      }
+  constructor(options: ThemeControllerOptions = {}) {
+    const {
+      storage = new LocalStorageAdapter(),
+      modeStorageKey = STORAGE_KEYS.THEME_MODE,
+      themeObject: fallbackThemeObject = supersetThemeObject,
+      defaultTheme = (supersetThemeObject.theme as AnyThemeConfig) ?? {},
+      onChange = null,
+    } = options;
+
+    this.storage = storage;
+    this.modeStorageKey = modeStorageKey;
+
+    // Initialize bootstrap data and themes
+    const {
+      bootstrapDefaultTheme,
+      bootstrapDarkTheme,
+      bootstrapThemeSettings,
+      hasBootstrapThemes,
+    }: BootstrapThemeData = this.loadBootstrapData();
+
+    this.hasBootstrapThemes = hasBootstrapThemes;
+    this.themeSettings = bootstrapThemeSettings || {};
+
+    // Set themes based on bootstrap data availability
+    if (this.hasBootstrapThemes) {
+      this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null;
+      this.defaultTheme =
+        bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme;
+    } else {
+      this.darkTheme = null;
+      this.defaultTheme = defaultTheme;
     }
 
-    // Determine initial mode
-    this.systemMode = ThemeController.getSystemMode();
-    const savedMode = this.storage.getItem(this.modeStorageKey) as ThemeMode;
-    this.currentMode = savedMode || ThemeMode.SYSTEM;
+    // Initialize system theme detection
+    this.systemMode = ThemeController.getSystemPreferredMode();
 
-    // Apply the initial theme and mode
-    this.applyTheme();
+    // Only initialize media query listener if OS preference is allowed
+    if (this.shouldInitializeMediaQueryListener())
+      this.initializeMediaQueryListener();
 
-    if (options.onChange) {
-      this.onChangeCallbacks.add(options.onChange);
-    }
-    this.canUpdateThemeFn = options.canUpdateTheme || (() => true);
-    this.canUpdateModeFn = options.canUpdateMode || (() => true);
+    // Initialize theme and mode
+    this.currentMode = this.determineInitialMode();
+    const { theme, themeObject } =
+      this.createInitialThemeObject(fallbackThemeObject);
 
-    // Listen for system theme changes to enable dynamic `SYSTEM` mode
-    this.mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
-    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+    this.themeObject = themeObject;
+
+    // Setup change callback
+    if (onChange) this.onChangeCallbacks.add(onChange);
+
+    // Apply initial theme and persist mode
+    this.applyTheme(theme);
+    this.persistMode();
   }
 
+  // Public Methods
+
   /**
-   * Cleans up listeners. Should be called when the controller is no longer needed.
+   * Cleans up listeners and references. Should be called when the controller is no longer needed.
    */
   public destroy(): void {
     this.mediaQuery.removeEventListener('change', this.handleSystemThemeChange);
+    this.onChangeCallbacks.clear();
   }
 
-  public canUpdateTheme(): boolean {
-    return this.canUpdateThemeFn();
+  /**
+   * Check if the user can update the theme.
+   */
+  public canSetTheme(): boolean {
+    return !this.themeSettings?.enforced;
   }
 
-  public canUpdateMode(): boolean {
-    return this.canUpdateModeFn();
+  /**
+   * Check if the user can update the theme mode.
+   */
+  public canSetMode(): boolean {
+    return this.isModeUpdatable();
   }
 
+  /**
+   * Returns the current theme object.
+   */
   public getTheme(): Theme {
     return this.themeObject;
   }
 
+  /**
+   * Returns the current theme mode.
+   */
   public getCurrentMode(): ThemeMode {
     return this.currentMode;
   }
 
   /**
-   * Sets new theme customizations (e.g., from a JSON editor).
-   * This method updates the theme's appearance but preserves the current mode.
+   * Sets new theme.
+   * @param theme - The new theme to apply
+   * @throws {Error} If the user does not have permission to update the theme
    */
-  public setTheme(newCustomizations: AnyThemeConfig): void {
-    if (!this.canUpdateTheme()) {
-      throw new Error('User does not have permission to update the theme');
-    }
-    this.customizations = newCustomizations;
+  public setTheme(theme: AnyThemeConfig): void {
+    this.validateThemeUpdatePermission();
 
-    if (!newCustomizations.algorithm) {
-      this.currentMode = ThemeMode.LIGHT;
-    }
+    const { mode, normalizedTheme } = this.normalizeTheme(theme);
+    this.currentMode = mode;
+
+    this.updateTheme(normalizedTheme);
+  }
 
-    if (newCustomizations?.algorithm) {
-      this.currentMode = newCustomizations.algorithm as ThemeMode;
+  /**
+   * Sets the theme mode (light, dark, or system).
+   * @param mode - The new theme mode to apply
+   * @throws {Error} If the user does not have permission to update the theme mode
+   */
+  public setThemeMode(mode: ThemeMode): void {
+    this.validateModeUpdatePermission(mode);
+
+    if (this.currentMode === mode) return;
+
+    const theme: AnyThemeConfig | null = this.getThemeForMode(mode);
+    if (!theme) {
+      console.warn(`Theme for mode ${mode} not found, falling back to default`);
+      this.fallbackToDefaultMode();
+      return;
     }
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+    this.currentMode = mode;
+    this.updateTheme(theme);
   }
 
   /**
-   * Changes the theme mode (light, dark, or system).
-   * This is for the mode switch.
+   * Resets the theme to the default theme.
    */
-  public changeThemeMode(newMode: ThemeMode): void {
-    if (!this.canUpdateMode()) {
-      throw new Error('User does not have permission to update the theme mode');
+  public resetTheme(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.updateTheme(defaultTheme);
+  }
+
+  /**
+   * Handles system theme changes with error recovery.
+   */
+  private handleSystemThemeChange = (): void => {
+    try {
+      const newSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK =
+        ThemeController.getSystemPreferredMode();
+
+      // Update systemMode regardless of current mode
+      const oldSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK = this.systemMode;
+      this.systemMode = newSystemMode;
+
+      // Only update theme if currently in SYSTEM mode and the preference changed
+      if (
+        this.currentMode === ThemeMode.SYSTEM &&
+        oldSystemMode !== newSystemMode
+      ) {
+        const newTheme: AnyThemeConfig | null = this.getThemeForMode(
+          ThemeMode.SYSTEM,
+        );
+
+        if (newTheme) this.updateTheme(newTheme);
+      }
+    } catch (error) {
+      console.error('Failed to handle system theme change:', error);
     }
-    if (this.currentMode === newMode) return;
+  };
 
-    this.currentMode = newMode;
+  /**
+   * Updates the theme.
+   * @param theme - The new theme to apply
+   */
+  private updateTheme(theme?: AnyThemeConfig): void {
+    try {
+      // If no config provided, use current mode to get theme
+      const config: AnyThemeConfig =
+        theme || this.getThemeForMode(this.currentMode) || this.defaultTheme;
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+      // Normalize the theme
+      const { normalizedTheme } = this.normalizeTheme(config);
+
+      this.applyTheme(normalizedTheme);
+      this.persistMode();
+      this.notifyListeners();
+    } catch (error) {
+      console.error('Failed to update theme:', error);
+      this.fallbackToDefaultMode();
+    }
   }
 
-  public resetTheme(): void {
-    this.customizations = this.defaultTheme;
-    this.applyTheme();
-    this.persist();
+  /**
+   * Fallback to default mode with error recovery.
+   */
+  private fallbackToDefaultMode(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+
+    // Get the default theme which will have the correct algorithm
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.applyTheme(defaultTheme);
+    this.persistMode();
     this.notifyListeners();
   }
 
+  /**
+   * Registers a callback to be called when the theme changes.
+   * @param callback - The callback to be called on theme change
+   * @returns A function to unsubscribe from the theme change events
+   */
   public onChange(callback: (theme: Theme) => void): () => void {
     this.onChangeCallbacks.add(callback);
-    return () => {
-      this.onChangeCallbacks.delete(callback);
+    return () => this.onChangeCallbacks.delete(callback);
+  }
+
+  // Private Helper Methods
+
+  /**
+   * Determines whether the MediaQueryList listener for system theme changes should be initialized.
+   * This checks if OS preference detection is enabled in the theme settings.
+   * @returns {boolean} True if the media query listener should be initialized, false otherwise
+   */
+  private shouldInitializeMediaQueryListener(): boolean {
+    const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+      this.themeSettings || {};
+
+    return allowOSPreference === true;
+  }
+
+  /**
+   * Initializes media query listeners if OS preference is allowed
+   */
+  private initializeMediaQueryListener(): void {
+    this.mediaQuery = window.matchMedia(MEDIA_QUERY_DARK_SCHEME);
+    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+  }
+
+  /**
+   * Loads and validates bootstrap theme data.
+   */
+  private loadBootstrapData(): BootstrapThemeData {
+    const {
+      common: { theme = {} as BootstrapThemeDataConfig },
+    } = getBootstrapData();
+
+    const {
+      default: defaultTheme,
+      dark: darkTheme,
+      settings: themeSettings,
+    } = theme;
+
+    const hasValidDefault: boolean = this.hasKeys(defaultTheme);
+    const hasValidDark: boolean = this.hasKeys(darkTheme);
+    const hasValidSettings: boolean = this.hasKeys(themeSettings);
+
+    return {
+      bootstrapDefaultTheme: hasValidDefault ? defaultTheme : null,
+      bootstrapDarkTheme: hasValidDark ? darkTheme : null,
+      bootstrapThemeSettings: hasValidSettings ? themeSettings : null,
+      hasBootstrapThemes: hasValidDefault || hasValidDark,
     };
   }
 
-  private handleSystemThemeChange = (): void => {
-    const newSystemMode = ThemeController.getSystemMode();
-    if (this.systemMode === newSystemMode) return;
+  /**
+   * Checks if an object has keys (not empty).
+   */
+  private hasKeys(obj: Record<string, any> | undefined | null): boolean {
+    return Boolean(
+      obj && typeof obj === 'object' && Object.keys(obj).length > 0,
+    );
+  }
 
-    this.systemMode = newSystemMode;
-    // If the current mode is SYSTEM, we need to re-apply the theme
-    if (this.currentMode === ThemeMode.SYSTEM) {
-      this.applyTheme();
-      this.notifyListeners();
+  /**
+   * Determines if mode updates are allowed.
+   */
+  private isModeUpdatable(): boolean {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings || {};
+
+    return !enforced && allowSwitching;
+  }
+
+  /**
+   * Normalizes the theme configuration to ensure it has a valid algorithm.
+   * @param theme - The theme configuration to normalize
+   * @returns An object with normalized mode and algorithm.
+   */
+  private normalizeTheme(theme: AnyThemeConfig): {

Review Comment:
   You're right, let me fix this



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theme): introduce bootstrap-driven Superset theme configurations [superset]

["korbit-ai[bot] (via GitHub)" <gi...@apache.org>]

korbit-ai[bot] commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2201967671


##########
superset/views/base.py:
##########
@@ -292,6 +293,51 @@
     }
 
 
+def get_theme_bootstrap_data() -> dict[str, Any]:
+    """
+    Returns the theme data to be sent to the client.
+    """
+    # Get theme configs
+    default_theme = (
+        conf["THEME_DEFAULT"]()
+        if callable(conf["THEME_DEFAULT"])
+        else conf["THEME_DEFAULT"]
+    )
+    dark_theme = (
+        conf["THEME_DARK"]() if callable(conf["THEME_DARK"]) else conf["THEME_DARK"]
+    )
+    theme_settings = (
+        conf["THEME_SETTINGS"]()
+        if callable(conf["THEME_SETTINGS"])
+        else conf["THEME_SETTINGS"]
+    )
+
+    # Validate and warn if invalid
+    if not is_valid_theme(default_theme):
+        logger.warning("Invalid THEME_DEFAULT configuration, using empty theme")
+        default_theme = {}
+
+    if not is_valid_theme(dark_theme):
+        logger.warning("Invalid THEME_DARK configuration, using empty theme")
+        dark_theme = {}

Review Comment:
   ### Missing Valid Theme Fallback Structure <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   When an invalid dark theme is detected, the code sets it to an empty dictionary rather than a valid default theme structure, which could cause client-side rendering issues.
   
   
   ###### Why this matters
   Using an empty dictionary as a fallback for an invalid theme configuration may lead to incomplete or broken theme rendering on the client side, as the theme structure expected by the frontend would be missing.
   
   ###### Suggested change ∙ *Feature Preview*
   Instead of using an empty dictionary, provide a minimal valid theme structure as fallback:
   ```python
   if not is_valid_theme(dark_theme):
       logger.warning("Invalid THEME_DARK configuration, using default theme structure")
       dark_theme = {
           "colors": {},
           "typography": {},
           "borderRadius": 0,
           "spacing": {}
       }
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1bc8bfb2-cf25-4ea3-a2b6-664bf0a344fe/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1bc8bfb2-cf25-4ea3-a2b6-664bf0a344fe?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1bc8bfb2-cf25-4ea3-a2b6-664bf0a344fe?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1bc8bfb2-cf25-4ea3-a2b6-664bf0a344fe?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1bc8bfb2-cf25-4ea3-a2b6-664bf0a344fe)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:d827cd69-f1d9-4154-ab1c-f595e61bfbc1 -->
   
   
   [](d827cd69-f1d9-4154-ab1c-f595e61bfbc1)



##########
superset/views/base.py:
##########
@@ -292,6 +293,51 @@
     }
 
 
+def get_theme_bootstrap_data() -> dict[str, Any]:
+    """
+    Returns the theme data to be sent to the client.
+    """
+    # Get theme configs
+    default_theme = (
+        conf["THEME_DEFAULT"]()
+        if callable(conf["THEME_DEFAULT"])
+        else conf["THEME_DEFAULT"]
+    )
+    dark_theme = (
+        conf["THEME_DARK"]() if callable(conf["THEME_DARK"]) else conf["THEME_DARK"]
+    )
+    theme_settings = (
+        conf["THEME_SETTINGS"]()
+        if callable(conf["THEME_SETTINGS"])
+        else conf["THEME_SETTINGS"]
+    )

Review Comment:
   ### Repetitive config value resolution pattern <sub>![category Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Repeated pattern of checking if config value is callable and executing it if so. This pattern appears 3 times with similar structure.
   
   
   ###### Why this matters
   The repetitive pattern makes the code harder to read and maintain. A utility function would make the intent clearer and reduce cognitive load.
   
   ###### Suggested change ∙ *Feature Preview*
   Extract to a helper function:
   ```python
   def get_config_value(key: str) -> Any:
       value = conf[key]
       return value() if callable(value) else value
   
   # Then use as:
   default_theme = get_config_value("THEME_DEFAULT")
   dark_theme = get_config_value("THEME_DARK")
   theme_settings = get_config_value("THEME_SETTINGS")
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268a3164-a9bb-4266-8abc-64f83ad8a2aa/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268a3164-a9bb-4266-8abc-64f83ad8a2aa?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268a3164-a9bb-4266-8abc-64f83ad8a2aa?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268a3164-a9bb-4266-8abc-64f83ad8a2aa?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268a3164-a9bb-4266-8abc-64f83ad8a2aa)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:0f32d74c-8af8-4190-b830-1426c0de8d42 -->
   
   
   [](0f32d74c-8af8-4190-b830-1426c0de8d42)



##########
superset/views/base.py:
##########
@@ -292,6 +293,51 @@
     }
 
 
+def get_theme_bootstrap_data() -> dict[str, Any]:
+    """
+    Returns the theme data to be sent to the client.
+    """
+    # Get theme configs
+    default_theme = (
+        conf["THEME_DEFAULT"]()
+        if callable(conf["THEME_DEFAULT"])
+        else conf["THEME_DEFAULT"]
+    )
+    dark_theme = (
+        conf["THEME_DARK"]() if callable(conf["THEME_DARK"]) else conf["THEME_DARK"]
+    )
+    theme_settings = (
+        conf["THEME_SETTINGS"]()
+        if callable(conf["THEME_SETTINGS"])
+        else conf["THEME_SETTINGS"]
+    )
+
+    # Validate and warn if invalid
+    if not is_valid_theme(default_theme):
+        logger.warning("Invalid THEME_DEFAULT configuration, using empty theme")
+        default_theme = {}
+
+    if not is_valid_theme(dark_theme):
+        logger.warning("Invalid THEME_DARK configuration, using empty theme")
+        dark_theme = {}
+
+    if not is_valid_theme_settings(theme_settings):
+        logger.warning("Invalid THEME_SETTINGS configuration, using defaults")
+        theme_settings = {
+            "enforced": False,
+            "allowSwitching": True,
+            "allowOSPreference": True,
+        }

Review Comment:
   ### Hardcoded theme settings defaults <sub>![category Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Magic default values are embedded directly in the code without being defined as constants or configuration values.
   
   
   ###### Why this matters
   Hardcoded default values make it difficult to understand their significance and make future changes more error-prone.
   
   ###### Suggested change ∙ *Feature Preview*
   Define defaults as constants:
   ```python
   DEFAULT_THEME_SETTINGS = {
       "enforced": False,
       "allowSwitching": True,
       "allowOSPreference": True,
   }
   
   # Then use as:
   theme_settings = DEFAULT_THEME_SETTINGS
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/328e078c-6b73-4b20-806f-d84fc2411f32/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/328e078c-6b73-4b20-806f-d84fc2411f32?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/328e078c-6b73-4b20-806f-d84fc2411f32?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/328e078c-6b73-4b20-806f-d84fc2411f32?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/328e078c-6b73-4b20-806f-d84fc2411f32)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:8f899d76-7c0f-4f53-9c82-51a1dc5287f8 -->
   
   
   [](8f899d76-7c0f-4f53-9c82-51a1dc5287f8)



##########
superset/themes/utils.py:
##########
@@ -0,0 +1,97 @@
+# 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.
+from typing import Any, Dict
+
+from superset.themes.types import ThemeMode, ThemeSettingsKey
+
+
+def _is_valid_theme_mode(mode: str) -> bool:
+    """Check if a string is a valid theme mode"""
+    try:
+        ThemeMode(mode)
+        return True
+    except ValueError:
+        return False
+
+
+def _is_valid_algorithm(algorithm: Any) -> bool:

Review Comment:
   ### Overly permissive type hint <sub>![category Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The function uses 'Any' type hint for the algorithm parameter, which is too permissive and obscures the expected input types.
   
   
   ###### Why this matters
   Using 'Any' makes it harder for developers to understand what types are actually valid without reading the implementation.
   
   ###### Suggested change ∙ *Feature Preview*
   Use a more specific type hint:
   ```python
   from typing import Union, List
   
   def _is_valid_algorithm(algorithm: Union[str, List[str]]) -> bool:
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/917acad8-52e3-49e0-9ccf-6574c044f41a/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/917acad8-52e3-49e0-9ccf-6574c044f41a?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/917acad8-52e3-49e0-9ccf-6574c044f41a?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/917acad8-52e3-49e0-9ccf-6574c044f41a?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/917acad8-52e3-49e0-9ccf-6574c044f41a)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:ffc4585b-4bdd-410c-aa19-32e48601fc59 -->
   
   
   [](ffc4585b-4bdd-410c-aa19-32e48601fc59)



##########
superset/themes/utils.py:
##########
@@ -0,0 +1,97 @@
+# 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.
+from typing import Any, Dict
+
+from superset.themes.types import ThemeMode, ThemeSettingsKey
+
+
+def _is_valid_theme_mode(mode: str) -> bool:
+    """Check if a string is a valid theme mode"""
+    try:
+        ThemeMode(mode)
+        return True
+    except ValueError:
+        return False
+
+
+def _is_valid_algorithm(algorithm: Any) -> bool:
+    """Helper function to validate theme algorithm"""
+    if isinstance(algorithm, str):
+        return _is_valid_theme_mode(algorithm) or algorithm == ThemeMode.SYSTEM
+    elif isinstance(algorithm, list):
+        return all(
+            isinstance(alg, str) and _is_valid_theme_mode(alg) for alg in algorithm
+        )
+    else:
+        return False
+
+
+def is_valid_theme(theme: Dict[str, Any]) -> bool:
+    """Check if a theme is valid"""
+    try:
+        if not isinstance(theme, dict):
+            return False
+
+        # Empty dict is valid
+        if not theme:
+            return True
+
+        # Validate each field type
+        validations = [
+            ("token", dict),
+            ("components", dict),
+            ("hashed", bool),
+            ("inherit", bool),
+        ]
+
+        for field, expected_type in validations:
+            if field in theme and not isinstance(theme[field], expected_type):
+                return False
+
+        # Validate algorithm field separately due to its complexity
+        if "algorithm" in theme and not _is_valid_algorithm(theme["algorithm"]):
+            return False
+
+        return True
+    except Exception:
+        return False
+
+
+def is_valid_theme_settings(settings: Dict[str, Any]) -> bool:
+    """Check if theme settings are valid"""
+    try:
+        if not isinstance(settings, dict):
+            return False
+
+        # Empty dict is valid
+        if not settings:
+            return True
+
+        # Check if all keys are valid
+        valid_keys = {setting.value for setting in ThemeSettingsKey}

Review Comment:
   ### Inefficient repeated set creation from static enum <sub>![category Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Creating a new set of valid keys on every function call is inefficient as ThemeSettingsKey is an enumeration that doesn't change at runtime.
   
   
   ###### Why this matters
   Repeatedly creating this set for each validation adds unnecessary computation overhead, especially if the function is called frequently.
   
   ###### Suggested change ∙ *Feature Preview*
   Move the valid keys set creation to module level as a constant:
   ```python
   VALID_THEME_SETTINGS = {setting.value for setting in ThemeSettingsKey}
   
   def is_valid_theme_settings(settings: Dict[str, Any]) -> bool:
       # Use VALID_THEME_SETTINGS instead of creating new set
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2627bd-7c04-4781-8fb9-d74e201612ba/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2627bd-7c04-4781-8fb9-d74e201612ba?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2627bd-7c04-4781-8fb9-d74e201612ba?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2627bd-7c04-4781-8fb9-d74e201612ba?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2627bd-7c04-4781-8fb9-d74e201612ba)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:f414d907-4ac7-48ae-8d72-25c7d788f9ad -->
   
   
   [](f414d907-4ac7-48ae-8d72-25c7d788f9ad)



##########
superset-frontend/src/types/bootstrapTypes.ts:
##########
@@ -143,6 +147,18 @@
   };
 }
 
+export interface SerializableThemeSettings {
+  enforced?: boolean;
+  allowSwitching?: boolean;
+  allowOSPreference?: boolean;
+}
+
+export interface BootstrapThemeDataConfig {
+  default: SerializableThemeConfig | {};
+  dark: SerializableThemeConfig | {};
+  settings: SerializableThemeSettings | {};
+}

Review Comment:
   ### Invalid Theme Configuration Type Definition <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The union type with empty object ({}) in theme configurations allows for potentially invalid theme data to be passed through the type system.
   
   
   ###### Why this matters
   Empty objects could be passed instead of valid theme configurations, leading to runtime errors when the theme system attempts to use the configuration.
   
   ###### Suggested change ∙ *Feature Preview*
   Remove the empty object option to ensure only valid theme configurations are accepted:
   ```typescript
   export interface BootstrapThemeDataConfig {
     default: SerializableThemeConfig;
     dark: SerializableThemeConfig;
     settings: SerializableThemeSettings;
   }
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/107d67b6-5b1c-4323-8e13-8176ba58a43b/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/107d67b6-5b1c-4323-8e13-8176ba58a43b?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/107d67b6-5b1c-4323-8e13-8176ba58a43b?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/107d67b6-5b1c-4323-8e13-8176ba58a43b?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/107d67b6-5b1c-4323-8e13-8176ba58a43b)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:ed4154c9-cd44-4a1f-ae99-59feed20cb82 -->
   
   
   [](ed4154c9-cd44-4a1f-ae99-59feed20cb82)



##########
superset/themes/utils.py:
##########
@@ -0,0 +1,97 @@
+# 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.
+from typing import Any, Dict
+
+from superset.themes.types import ThemeMode, ThemeSettingsKey
+
+
+def _is_valid_theme_mode(mode: str) -> bool:
+    """Check if a string is a valid theme mode"""
+    try:
+        ThemeMode(mode)
+        return True
+    except ValueError:
+        return False
+
+
+def _is_valid_algorithm(algorithm: Any) -> bool:
+    """Helper function to validate theme algorithm"""
+    if isinstance(algorithm, str):
+        return _is_valid_theme_mode(algorithm) or algorithm == ThemeMode.SYSTEM
+    elif isinstance(algorithm, list):
+        return all(
+            isinstance(alg, str) and _is_valid_theme_mode(alg) for alg in algorithm
+        )
+    else:
+        return False
+
+
+def is_valid_theme(theme: Dict[str, Any]) -> bool:
+    """Check if a theme is valid"""

Review Comment:
   ### Incomplete theme validation docstring <sub>![category Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The docstring lacks information about the expected theme structure and validation rules.
   
   
   ###### Why this matters
   Developers need to read through the implementation to understand what makes a theme valid, making the code harder to use correctly.
   
   ###### Suggested change ∙ *Feature Preview*
   """Validate theme dictionary structure and types.
   
   A valid theme can be empty or must contain properly typed fields:
   - token (dict)
   - components (dict)
   - hashed (bool)
   - inherit (bool)
   - algorithm (str or list of str matching ThemeMode)
   
   Returns:
       bool: True if theme structure is valid, False otherwise
   """
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd750467-f48d-40ef-8624-26c2a3a35277/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd750467-f48d-40ef-8624-26c2a3a35277?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd750467-f48d-40ef-8624-26c2a3a35277?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd750467-f48d-40ef-8624-26c2a3a35277?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd750467-f48d-40ef-8624-26c2a3a35277)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:3ac493a8-a79d-4348-ac9b-68d262c66836 -->
   
   
   [](3ac493a8-a79d-4348-ac9b-68d262c66836)



##########
superset-frontend/src/types/bootstrapTypes.ts:
##########
@@ -143,6 +147,18 @@ export interface MenuData {
   };
 }
 
+export interface SerializableThemeSettings {
+  enforced?: boolean;
+  allowSwitching?: boolean;
+  allowOSPreference?: boolean;
+}

Review Comment:
   ### Missing theme settings interface documentation <sub>![category Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The interface SerializableThemeSettings lacks documentation explaining the purpose and impact of its configuration flags.
   
   
   ###### Why this matters
   Without clear documentation, developers may incorrectly configure theme settings, leading to unexpected theming behavior in the application.
   
   ###### Suggested change ∙ *Feature Preview*
   /**
    * Theme configuration settings that control theme behavior.
    * - enforced: Forces the use of the default theme
    * - allowSwitching: Enables manual theme switching
    * - allowOSPreference: Enables automatic theme switching based on OS preferences
    */
   export interface SerializableThemeSettings {
     enforced?: boolean;
     allowSwitching?: boolean;
     allowOSPreference?: boolean;
   }
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a6f35c2-d14d-4092-85b8-87403370e7c7/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a6f35c2-d14d-4092-85b8-87403370e7c7?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a6f35c2-d14d-4092-85b8-87403370e7c7?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a6f35c2-d14d-4092-85b8-87403370e7c7?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a6f35c2-d14d-4092-85b8-87403370e7c7)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:1dc05624-4302-4256-99a0-4678f911ce9e -->
   
   
   [](1dc05624-4302-4256-99a0-4678f911ce9e)



##########
superset/views/base.py:
##########
@@ -292,6 +293,51 @@
     }
 
 
+def get_theme_bootstrap_data() -> dict[str, Any]:
+    """
+    Returns the theme data to be sent to the client.
+    """
+    # Get theme configs
+    default_theme = (
+        conf["THEME_DEFAULT"]()
+        if callable(conf["THEME_DEFAULT"])
+        else conf["THEME_DEFAULT"]
+    )
+    dark_theme = (
+        conf["THEME_DARK"]() if callable(conf["THEME_DARK"]) else conf["THEME_DARK"]
+    )
+    theme_settings = (
+        conf["THEME_SETTINGS"]()
+        if callable(conf["THEME_SETTINGS"])
+        else conf["THEME_SETTINGS"]
+    )
+
+    # Validate and warn if invalid
+    if not is_valid_theme(default_theme):
+        logger.warning("Invalid THEME_DEFAULT configuration, using empty theme")
+        default_theme = {}
+
+    if not is_valid_theme(dark_theme):
+        logger.warning("Invalid THEME_DARK configuration, using empty theme")
+        dark_theme = {}
+
+    if not is_valid_theme_settings(theme_settings):
+        logger.warning("Invalid THEME_SETTINGS configuration, using defaults")

Review Comment:
   ### Insufficient Error Context in Theme Validation Logs <sub>![category Logging](https://img.shields.io/badge/Logging-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Warning logs for invalid theme configurations lack context about what made the theme invalid, making troubleshooting difficult.
   
   
   ###### Why this matters
   Without detailed error information in the logs, administrators would struggle to identify and fix incorrect theme configurations.
   
   ###### Suggested change ∙ *Feature Preview*
   ```python
   if not is_valid_theme(default_theme):
       logger.warning(
           "Invalid THEME_DEFAULT configuration: %s, using empty theme",
           default_theme
       )
       default_theme = {}
   
   if not is_valid_theme(dark_theme):
       logger.warning(
           "Invalid THEME_DARK configuration: %s, using empty theme",
           dark_theme
       )
       dark_theme = {}
   
   if not is_valid_theme_settings(theme_settings):
       logger.warning(
           "Invalid THEME_SETTINGS configuration: %s, using defaults",
           theme_settings
       )
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/68982dc7-8810-4817-a5d5-03042e60f062/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/68982dc7-8810-4817-a5d5-03042e60f062?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/68982dc7-8810-4817-a5d5-03042e60f062?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/68982dc7-8810-4817-a5d5-03042e60f062?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/68982dc7-8810-4817-a5d5-03042e60f062)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:d89e5f12-4069-4143-93e3-bffd287a0853 -->
   
   
   [](d89e5f12-4069-4143-93e3-bffd287a0853)



##########
superset/themes/utils.py:
##########
@@ -0,0 +1,97 @@
+# 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.
+from typing import Any, Dict
+
+from superset.themes.types import ThemeMode, ThemeSettingsKey
+
+
+def _is_valid_theme_mode(mode: str) -> bool:
+    """Check if a string is a valid theme mode"""

Review Comment:
   ### Insufficient docstring for theme mode validation <sub>![category Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The docstring only describes what the function does, but not the expected input/output or why it's needed.
   
   
   ###### Why this matters
   Without understanding valid theme modes and their purpose, developers may misuse the function or have to dig through other files to understand its usage.
   
   ###### Suggested change ∙ *Feature Preview*
   """Validate if a string represents a valid theme mode.
   
   Args:
       mode: String to validate against ThemeMode enum
   
   Returns:
       bool: True if mode is a valid ThemeMode value, False otherwise
   """
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9975ca46-eb9d-4b4c-b5d6-cc45e21d784a/upvote) [![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9975ca46-eb9d-4b4c-b5d6-cc45e21d784a?what_not_true=true)  [![Not in Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9975ca46-eb9d-4b4c-b5d6-cc45e21d784a?what_out_of_scope=true) [![Not in coding standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9975ca46-eb9d-4b4c-b5d6-cc45e21d784a?what_not_in_standard=true) [![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9975ca46-eb9d-4b4c-b5d6-cc45e21d784a)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:025b67b9-d52b-480f-abb9-e713e4da5e4f -->
   
   
   [](025b67b9-d52b-480f-abb9-e713e4da5e4f)



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205550596


##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -160,7 +164,7 @@ export class Theme {
    * Dark mode should be specified via the algorithm property in the config
    */
   setConfig(config: AnyThemeConfig): void {
-    const antdConfig = normalizeThemeConfig(config);
+    const antdConfig: ThemeConfig = normalizeThemeConfig(config);

Review Comment:
   I think it makes sense



##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -169,7 +173,7 @@ export class Theme {
     };
 
     // First phase: Let Ant Design compute the tokens
-    const tokens = Theme.getFilteredAntdTheme(antdConfig);
+    const tokens: SharedAntdTokens = Theme.getFilteredAntdTheme(antdConfig);

Review Comment:
   Yep, I think it makes sense



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["mistercrunch (via GitHub)" <gi...@apache.org>]

mistercrunch commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2202066882


##########
superset-frontend/packages/superset-ui-core/src/theme/types.ts:
##########
@@ -33,18 +33,33 @@ import { Theme } from '.';
 export type AntdTokens = ReturnType<typeof antdThemeImport.getDesignToken>;
 export type AntdThemeConfig = ThemeConfig;
 
+/**
+ * A combination of theme modes that can be used in theme config.
+ * This is used to define which algorithms are allow to be applied together.
+ */
+export type ThemeAlgorithmCombination = (

Review Comment:
   I think there's a drier way to write all this, maybe something like:
   ```ts
   export enum ThemeMode {
     DEFAULT = 'default',
     DARK = 'dark',
     COMPACT = 'compact',
   }
   export type ThemeAlgorithmCombination = ThemeMode[];
   export type ThemeAlgorithmOption = ThemeMode | ThemeAlgorithmCombination;
   ```
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205560970


##########
superset-frontend/packages/superset-ui-core/src/theme/utils.ts:
##########
@@ -38,9 +40,8 @@ export function isSerializableConfig(
 
   if (algorithm === undefined) return true;
 
-  if (Array.isArray(algorithm)) {
+  if (Array.isArray(algorithm))

Review Comment:
   I understand your point, but having a `single-line if statement` at the top and then a `multiline if statement` for just one condition feels inconsistent.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["korbit-ai[bot] (via GitHub)" <gi...@apache.org>]

korbit-ai[bot] commented on PR #34144:
URL: https://github.com/apache/superset/pull/34144#issuecomment-3070641886

   I was unable to post the issues I found. This could be because a force push or squash has changed the commit history since I scanned this pull request. You can get another review by commenting `/korbit-review`.
   
   [](3bf49d86-832d-46e8-a16b-c5c973408e1d)


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205581290


##########
superset-frontend/packages/superset-ui-core/src/theme/types.ts:
##########
@@ -33,18 +33,33 @@ import { Theme } from '.';
 export type AntdTokens = ReturnType<typeof antdThemeImport.getDesignToken>;
 export type AntdThemeConfig = ThemeConfig;
 
+/**
+ * A combination of theme modes that can be used in theme config.
+ * This is used to define which algorithms are allow to be applied together.
+ */
+export type ThemeAlgorithmCombination = (

Review Comment:
   Done!



##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -160,7 +164,7 @@ export class Theme {
    * Dark mode should be specified via the algorithm property in the config
    */
   setConfig(config: AnyThemeConfig): void {
-    const antdConfig = normalizeThemeConfig(config);
+    const antdConfig: ThemeConfig = normalizeThemeConfig(config);

Review Comment:
   Done!



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on PR #34144:
URL: https://github.com/apache/superset/pull/34144#issuecomment-3070644025

   /korbit-review


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["mistercrunch (via GitHub)" <gi...@apache.org>]

mistercrunch commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205941823


##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -45,202 +81,618 @@ export class ThemeController {
 
   private storage: ThemeStorage;
 
-  private storageKey: string;
-
   private modeStorageKey: string;
 
   private defaultTheme: AnyThemeConfig;
 
-  private systemMode: ThemeMode.DARK | ThemeMode.LIGHT;
+  private darkTheme: AnyThemeConfig | null;
 
-  private currentMode: ThemeMode;
+  private themeSettings: SerializableThemeSettings;
 
-  private customizations: AnyThemeConfig = {};
+  private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT;
 
-  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
+  private currentMode: ThemeMode;
 
-  private canUpdateThemeFn: () => boolean;
+  private readonly hasBootstrapThemes: boolean;
 
-  private canUpdateModeFn: () => boolean;
+  private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
 
   private mediaQuery: MediaQueryList;
 
-  constructor(options: ThemeControllerOptions = { themeObject }) {
-    this.storage = options.storage || new LocalStorageAdapter();
-    this.storageKey = options.storageKey || 'superset-theme';
-    this.modeStorageKey = options.modeStorageKey || `${this.storageKey}-mode`;
-    this.defaultTheme = options.defaultTheme || {};
-    this.themeObject = options.themeObject;
-
-    // Load customizations from storage
-    const savedThemeJson = this.storage.getItem(this.storageKey);
-    if (savedThemeJson) {
-      try {
-        this.customizations = JSON.parse(savedThemeJson);
-      } catch (e) {
-        console.error('Failed to parse saved theme:', e);
-        this.storage.removeItem(this.storageKey);
-      }
+  constructor(options: ThemeControllerOptions = {}) {
+    const {
+      storage = new LocalStorageAdapter(),
+      modeStorageKey = STORAGE_KEYS.THEME_MODE,
+      themeObject: fallbackThemeObject = supersetThemeObject,
+      defaultTheme = (supersetThemeObject.theme as AnyThemeConfig) ?? {},
+      onChange = null,
+    } = options;
+
+    this.storage = storage;
+    this.modeStorageKey = modeStorageKey;
+
+    // Initialize bootstrap data and themes
+    const {
+      bootstrapDefaultTheme,
+      bootstrapDarkTheme,
+      bootstrapThemeSettings,
+      hasBootstrapThemes,
+    }: BootstrapThemeData = this.loadBootstrapData();
+
+    this.hasBootstrapThemes = hasBootstrapThemes;
+    this.themeSettings = bootstrapThemeSettings || {};
+
+    // Set themes based on bootstrap data availability
+    if (this.hasBootstrapThemes) {
+      this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null;
+      this.defaultTheme =
+        bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme;
+    } else {
+      this.darkTheme = null;
+      this.defaultTheme = defaultTheme;
     }
 
-    // Determine initial mode
-    this.systemMode = ThemeController.getSystemMode();
-    const savedMode = this.storage.getItem(this.modeStorageKey) as ThemeMode;
-    this.currentMode = savedMode || ThemeMode.SYSTEM;
+    // Initialize system theme detection
+    this.systemMode = ThemeController.getSystemPreferredMode();
 
-    // Apply the initial theme and mode
-    this.applyTheme();
+    // Only initialize media query listener if OS preference is allowed
+    if (this.shouldInitializeMediaQueryListener())
+      this.initializeMediaQueryListener();
 
-    if (options.onChange) {
-      this.onChangeCallbacks.add(options.onChange);
-    }
-    this.canUpdateThemeFn = options.canUpdateTheme || (() => true);
-    this.canUpdateModeFn = options.canUpdateMode || (() => true);
+    // Initialize theme and mode
+    this.currentMode = this.determineInitialMode();
+    const { theme, themeObject } =
+      this.createInitialThemeObject(fallbackThemeObject);
 
-    // Listen for system theme changes to enable dynamic `SYSTEM` mode
-    this.mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
-    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+    this.themeObject = themeObject;
+
+    // Setup change callback
+    if (onChange) this.onChangeCallbacks.add(onChange);
+
+    // Apply initial theme and persist mode
+    this.applyTheme(theme);
+    this.persistMode();
   }
 
+  // Public Methods
+
   /**
-   * Cleans up listeners. Should be called when the controller is no longer needed.
+   * Cleans up listeners and references. Should be called when the controller is no longer needed.
    */
   public destroy(): void {
     this.mediaQuery.removeEventListener('change', this.handleSystemThemeChange);
+    this.onChangeCallbacks.clear();
   }
 
-  public canUpdateTheme(): boolean {
-    return this.canUpdateThemeFn();
+  /**
+   * Check if the user can update the theme.
+   */
+  public canSetTheme(): boolean {
+    return !this.themeSettings?.enforced;
   }
 
-  public canUpdateMode(): boolean {
-    return this.canUpdateModeFn();
+  /**
+   * Check if the user can update the theme mode.
+   */
+  public canSetMode(): boolean {
+    return this.isModeUpdatable();
   }
 
+  /**
+   * Returns the current theme object.
+   */
   public getTheme(): Theme {
     return this.themeObject;
   }
 
+  /**
+   * Returns the current theme mode.
+   */
   public getCurrentMode(): ThemeMode {
     return this.currentMode;
   }
 
   /**
-   * Sets new theme customizations (e.g., from a JSON editor).
-   * This method updates the theme's appearance but preserves the current mode.
+   * Sets new theme.
+   * @param theme - The new theme to apply
+   * @throws {Error} If the user does not have permission to update the theme
    */
-  public setTheme(newCustomizations: AnyThemeConfig): void {
-    if (!this.canUpdateTheme()) {
-      throw new Error('User does not have permission to update the theme');
-    }
-    this.customizations = newCustomizations;
+  public setTheme(theme: AnyThemeConfig): void {
+    this.validateThemeUpdatePermission();
 
-    if (!newCustomizations.algorithm) {
-      this.currentMode = ThemeMode.LIGHT;
-    }
+    const { mode, normalizedTheme } = this.normalizeTheme(theme);
+    this.currentMode = mode;
+
+    this.updateTheme(normalizedTheme);
+  }
 
-    if (newCustomizations?.algorithm) {
-      this.currentMode = newCustomizations.algorithm as ThemeMode;
+  /**
+   * Sets the theme mode (light, dark, or system).
+   * @param mode - The new theme mode to apply
+   * @throws {Error} If the user does not have permission to update the theme mode
+   */
+  public setThemeMode(mode: ThemeMode): void {
+    this.validateModeUpdatePermission(mode);
+
+    if (this.currentMode === mode) return;
+
+    const theme: AnyThemeConfig | null = this.getThemeForMode(mode);
+    if (!theme) {
+      console.warn(`Theme for mode ${mode} not found, falling back to default`);
+      this.fallbackToDefaultMode();
+      return;
     }
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+    this.currentMode = mode;
+    this.updateTheme(theme);
   }
 
   /**
-   * Changes the theme mode (light, dark, or system).
-   * This is for the mode switch.
+   * Resets the theme to the default theme.
    */
-  public changeThemeMode(newMode: ThemeMode): void {
-    if (!this.canUpdateMode()) {
-      throw new Error('User does not have permission to update the theme mode');
+  public resetTheme(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.updateTheme(defaultTheme);
+  }
+
+  /**
+   * Handles system theme changes with error recovery.
+   */
+  private handleSystemThemeChange = (): void => {
+    try {
+      const newSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK =
+        ThemeController.getSystemPreferredMode();
+
+      // Update systemMode regardless of current mode
+      const oldSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK = this.systemMode;
+      this.systemMode = newSystemMode;
+
+      // Only update theme if currently in SYSTEM mode and the preference changed
+      if (
+        this.currentMode === ThemeMode.SYSTEM &&
+        oldSystemMode !== newSystemMode
+      ) {
+        const newTheme: AnyThemeConfig | null = this.getThemeForMode(
+          ThemeMode.SYSTEM,
+        );
+
+        if (newTheme) this.updateTheme(newTheme);
+      }
+    } catch (error) {
+      console.error('Failed to handle system theme change:', error);
     }
-    if (this.currentMode === newMode) return;
+  };
 
-    this.currentMode = newMode;
+  /**
+   * Updates the theme.
+   * @param theme - The new theme to apply
+   */
+  private updateTheme(theme?: AnyThemeConfig): void {
+    try {
+      // If no config provided, use current mode to get theme
+      const config: AnyThemeConfig =
+        theme || this.getThemeForMode(this.currentMode) || this.defaultTheme;
 
-    this.applyTheme();
-    this.persist();
-    this.notifyListeners();
+      // Normalize the theme
+      const { normalizedTheme } = this.normalizeTheme(config);
+
+      this.applyTheme(normalizedTheme);
+      this.persistMode();
+      this.notifyListeners();
+    } catch (error) {
+      console.error('Failed to update theme:', error);
+      this.fallbackToDefaultMode();
+    }
   }
 
-  public resetTheme(): void {
-    this.customizations = this.defaultTheme;
-    this.applyTheme();
-    this.persist();
+  /**
+   * Fallback to default mode with error recovery.
+   */
+  private fallbackToDefaultMode(): void {
+    this.currentMode = ThemeMode.DEFAULT;
+
+    // Get the default theme which will have the correct algorithm
+    const defaultTheme: AnyThemeConfig =
+      this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+    this.applyTheme(defaultTheme);
+    this.persistMode();
     this.notifyListeners();
   }
 
+  /**
+   * Registers a callback to be called when the theme changes.
+   * @param callback - The callback to be called on theme change
+   * @returns A function to unsubscribe from the theme change events
+   */
   public onChange(callback: (theme: Theme) => void): () => void {
     this.onChangeCallbacks.add(callback);
-    return () => {
-      this.onChangeCallbacks.delete(callback);
+    return () => this.onChangeCallbacks.delete(callback);
+  }
+
+  // Private Helper Methods
+
+  /**
+   * Determines whether the MediaQueryList listener for system theme changes should be initialized.
+   * This checks if OS preference detection is enabled in the theme settings.
+   * @returns {boolean} True if the media query listener should be initialized, false otherwise
+   */
+  private shouldInitializeMediaQueryListener(): boolean {
+    const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+      this.themeSettings || {};
+
+    return allowOSPreference === true;
+  }
+
+  /**
+   * Initializes media query listeners if OS preference is allowed
+   */
+  private initializeMediaQueryListener(): void {
+    this.mediaQuery = window.matchMedia(MEDIA_QUERY_DARK_SCHEME);
+    this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+  }
+
+  /**
+   * Loads and validates bootstrap theme data.
+   */
+  private loadBootstrapData(): BootstrapThemeData {
+    const {
+      common: { theme = {} as BootstrapThemeDataConfig },
+    } = getBootstrapData();
+
+    const {
+      default: defaultTheme,
+      dark: darkTheme,
+      settings: themeSettings,
+    } = theme;
+
+    const hasValidDefault: boolean = this.hasKeys(defaultTheme);
+    const hasValidDark: boolean = this.hasKeys(darkTheme);
+    const hasValidSettings: boolean = this.hasKeys(themeSettings);
+
+    return {
+      bootstrapDefaultTheme: hasValidDefault ? defaultTheme : null,
+      bootstrapDarkTheme: hasValidDark ? darkTheme : null,
+      bootstrapThemeSettings: hasValidSettings ? themeSettings : null,
+      hasBootstrapThemes: hasValidDefault || hasValidDark,
     };
   }
 
-  private handleSystemThemeChange = (): void => {
-    const newSystemMode = ThemeController.getSystemMode();
-    if (this.systemMode === newSystemMode) return;
+  /**
+   * Checks if an object has keys (not empty).
+   */
+  private hasKeys(obj: Record<string, any> | undefined | null): boolean {
+    return Boolean(
+      obj && typeof obj === 'object' && Object.keys(obj).length > 0,
+    );
+  }
 
-    this.systemMode = newSystemMode;
-    // If the current mode is SYSTEM, we need to re-apply the theme
-    if (this.currentMode === ThemeMode.SYSTEM) {
-      this.applyTheme();
-      this.notifyListeners();
+  /**
+   * Determines if mode updates are allowed.
+   */
+  private isModeUpdatable(): boolean {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings || {};
+
+    return !enforced && allowSwitching;
+  }
+
+  /**
+   * Normalizes the theme configuration to ensure it has a valid algorithm.
+   * @param theme - The theme configuration to normalize
+   * @returns An object with normalized mode and algorithm.
+   */
+  private normalizeTheme(theme: AnyThemeConfig): {
+    mode: ThemeMode;
+    normalizedTheme: AnyThemeConfig;
+  } {
+    const algorithm: ThemeAlgorithm | ThemeAlgorithm[] = this.getValidAlgorithm(
+      (theme?.algorithm as ThemeAlgorithm | ThemeAlgorithm[]) ||
+        ThemeAlgorithm.DEFAULT,
+    );
+
+    // Extract the mode from the valid algorithm
+    let mode: ThemeMode;
+
+    if (Array.isArray(algorithm)) {
+      const foundAlgorithm =
+        algorithm.find(
+          (m: ThemeAlgorithm) =>
+            m === ThemeAlgorithm.DARK || m === ThemeAlgorithm.DEFAULT,
+        ) ?? ThemeAlgorithm.DEFAULT;
+
+      mode = this.algorithmToMode(foundAlgorithm);
+    } else mode = this.algorithmToMode(algorithm);
+
+    return {
+      mode,
+      normalizedTheme: {
+        ...theme,
+        algorithm,
+      } as AnyThemeConfig,
+    };
+  }
+
+  /**
+   * Converts a ThemeAlgorithm to its corresponding ThemeMode.
+   * @param algorithm - The theme algorithm to convert
+   * @returns The corresponding theme mode
+   */
+  private algorithmToMode(algorithm: ThemeAlgorithm): ThemeMode {
+    switch (algorithm) {
+      case ThemeAlgorithm.DARK:
+        return ThemeMode.DARK;
+      case ThemeAlgorithm.DEFAULT:
+        return ThemeMode.DEFAULT;
+      case ThemeAlgorithm.COMPACT:
+        return ThemeMode.DEFAULT;
+      default:
+        return ThemeMode.DEFAULT;
     }
-  };
+  }
+
+  /**
+   * Returns the appropriate theme configuration for a given mode.
+   * @param mode - The theme mode to get the configuration for
+   * @returns The theme configuration for the specified mode or null if not available
+   */
+  private getThemeForMode(mode: ThemeMode): AnyThemeConfig | null {
+    const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+      this.themeSettings;
+
+    let resolvedMode: ThemeMode = mode;
+
+    if (mode === ThemeMode.SYSTEM) {
+      if (!allowOSPreference) return null;
+      resolvedMode = ThemeController.getSystemPreferredMode();
+    }
+
+    // When we don't have bootstrap themes, we need to create variants using algorithm
+    if (!this.hasBootstrapThemes) {
+      if (resolvedMode === ThemeMode.DARK)
+        return {
+          ...this.defaultTheme,
+          algorithm: ThemeAlgorithm.DARK,
+        };
+
+      return {
+        ...this.defaultTheme,
+        algorithm: ThemeAlgorithm.DEFAULT,
+      };
+    }
+
+    // When we have bootstrap themes, use them
+    if (resolvedMode === ThemeMode.DARK)
+      return this.darkTheme || this.defaultTheme;
+
+    return this.defaultTheme;
+  }
+
+  /**
+   * Creates the initial theme object.
+   * This sets the theme based on the current mode and ensures it has the correct algorithm.
+   * @param defaultThemeObject - The fallback theme object to use if no theme is set
+   * @returns An object containing the theme and the themeObject
+   */
+  private createInitialThemeObject(defaultThemeObject: Theme): {
+    theme: AnyThemeConfig;
+    themeObject: Theme;
+  } {
+    let theme: AnyThemeConfig | null = this.getThemeForMode(this.currentMode);
+    theme = theme || (defaultThemeObject.theme as AnyThemeConfig);
+
+    const { normalizedTheme } = this.normalizeTheme(theme);
+
+    return {
+      theme: normalizedTheme,
+      themeObject: Theme.fromConfig(normalizedTheme),
+    };
+  }
+
+  /**
+   * Determines the initial theme mode with error recovery.
+   */
+  private determineInitialMode(): ThemeMode {
+    const {
+      enforced = DEFAULT_THEME_SETTINGS.enforced,
+      allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings;
+
+    // Enforced mode always takes precedence
+    if (enforced) {
+      this.storage.removeItem(this.modeStorageKey);
+      return ThemeMode.DEFAULT;
+    }
+
+    // When OS preference is allowed but switching is not
+    // This means the user MUST follow OS preference and cannot override it
+    if (allowOSPreference && !allowSwitching) {
+      // Clear any saved preference since switching is not allowed
+      this.storage.removeItem(this.modeStorageKey);
+      return ThemeMode.SYSTEM;
+    }
+
+    // Try to restore saved mode
+    const savedMode: ThemeMode | null = this.loadSavedMode();
+    if (savedMode && this.isValidThemeMode(savedMode)) return savedMode;
+
+    // Fallback to system preference if allowed and available
+    if (allowOSPreference && this.getThemeForMode(this.systemMode))
+      return ThemeMode.SYSTEM;
+
+    return ThemeMode.DEFAULT;
+  }
+
+  /**
+   * Safely loads saved theme mode from storage.
+   */
+  private loadSavedMode(): ThemeMode | null {
+    try {
+      const stored: string | null = this.storage.getItem(this.modeStorageKey);
+      if (stored && Object.values(ThemeMode).includes(stored as ThemeMode))
+        return stored as ThemeMode;
+
+      return null;
+    } catch (error) {
+      console.warn('Failed to load saved theme mode:', error);
+      return null;
+    }
+  }
 
   /**
-   * Centralized method to apply the current customizations and mode.
+   * Validates if a theme mode is valid and supported.
+   * This checks if the mode is one of the known ThemeMode values.
+   * @param mode - The theme mode to validate
+   * @returns {boolean} True if the mode is valid, false otherwise
    */
-  private applyTheme(): void {
-    const newConfig = { ...this.customizations };
+  private isValidThemeMode(mode: ThemeMode): boolean {
+    if (!Object.values(ThemeMode).includes(mode)) return false;
 
-    switch (this.currentMode) {
+    // Validate that we have the required theme data for the mode
+    switch (mode) {
       case ThemeMode.DARK:
-        newConfig.algorithm = 'dark';
-        break;
-      case ThemeMode.LIGHT:
-        newConfig.algorithm = 'default';
-        break;
+        return !!(this.darkTheme || this.defaultTheme);
+      case ThemeMode.DEFAULT:
+        return !!this.defaultTheme;
       case ThemeMode.SYSTEM:
-        newConfig.algorithm =
-          this.systemMode === ThemeMode.DARK ? 'dark' : 'default';
-        break;
-      case ThemeMode.COMPACT:
-        newConfig.algorithm = 'compact';
-        break;
+        return this.themeSettings?.allowOSPreference !== false;
       default:
-        newConfig.algorithm = 'default';
-        break;
+        return true;
     }
+  }
 
-    this.themeObject.setConfig(newConfig);
+  /**
+   * Validates permission to update theme.
+   */
+  private validateThemeUpdatePermission(): void {
+    if (!this.canSetTheme())
+      throw new Error('User does not have permission to update the theme');
   }
 
-  private persist(): void {
-    this.storage.setItem(this.modeStorageKey, this.currentMode);
+  /**
+   * Validates permission to update mode.
+   * @param newMode - The new mode to validate
+   * @throws {Error} If the user does not have permission to update the theme mode
+   * @throws {Error} If the new mode is SYSTEM and OS preference is not allowed
+   */
+  private validateModeUpdatePermission(newMode: ThemeMode): void {
+    const {
+      allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference,
+      allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+    } = this.themeSettings;
+
+    // If OS preference is allowed but switching is not,
+    // don't allow any mode changes
+    if (allowOSPreference && !allowSwitching)
+      throw new Error(
+        'Theme mode changes are not allowed when OS preference is enforced',
+      );
+
+    // Check if user can set a new theme mode
+    if (!this.canSetMode())
+      throw new Error('User does not have permission to update the theme mode');
+
+    // Check if user has permissions to set OS preference as a theme mode
+    if (newMode === ThemeMode.SYSTEM && !allowOSPreference)
+      throw new Error('System theme mode is not allowed');
+  }
 
-    const { algorithm, ...persistedCustomizations } = this.customizations;
-    this.storage.setItem(
-      this.storageKey,
-      JSON.stringify(persistedCustomizations),
+  /**
+   * Validates theme algorithm combinations.
+   * This checks if the provided algorithm is a valid combination of ThemeMode values.
+   * @param algorithm - The theme algorithm to validate
+   * @returns {boolean} True if the algorithms combination is valid, false otherwise
+   */
+  private isValidAlgorithmCombination(algorithm: ThemeAlgorithm[]): boolean {
+    const inputSet = new Set(algorithm);
+    return VALID_ALGORITHM_COMBINATIONS.some(
+      validCombination =>
+        inputSet.size === validCombination.size &&
+        [...inputSet].every(item => validCombination.has(item)),
     );
   }
 
+  /**
+   * Checks if the algorithm is a valid combination or a simple one.
+   * @param algorithm - The theme mode or combination to convert
+   * @returns A valid ThemeAlgorithm or ThemeAlgorithm[]
+   */
+  private getValidAlgorithm(

Review Comment:
   Similar to above comment, this looks like dup logic that probably either already exists or belongs directly in Theme or utils



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["msyavuz (via GitHub)" <gi...@apache.org>]

msyavuz commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205451326


##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -160,7 +164,7 @@ export class Theme {
    * Dark mode should be specified via the algorithm property in the config
    */
   setConfig(config: AnyThemeConfig): void {
-    const antdConfig = normalizeThemeConfig(config);
+    const antdConfig: ThemeConfig = normalizeThemeConfig(config);

Review Comment:
   We don't have a standard for this, but i personally prefer implicit types in places like this where the function's return is already typed



##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -169,7 +173,7 @@ export class Theme {
     };
 
     // First phase: Let Ant Design compute the tokens
-    const tokens = Theme.getFilteredAntdTheme(antdConfig);
+    const tokens: SharedAntdTokens = Theme.getFilteredAntdTheme(antdConfig);

Review Comment:
   Same discussion here



##########
superset-frontend/packages/superset-ui-core/src/theme/utils.ts:
##########
@@ -38,9 +40,8 @@ export function isSerializableConfig(
 
   if (algorithm === undefined) return true;
 
-  if (Array.isArray(algorithm)) {
+  if (Array.isArray(algorithm))

Review Comment:
   This is another personal preference but i think multiline if statements are better for these since refactoring and debugging is a bit easier.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205581680


##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -169,7 +173,7 @@ export class Theme {
     };
 
     // First phase: Let Ant Design compute the tokens
-    const tokens = Theme.getFilteredAntdTheme(antdConfig);
+    const tokens: SharedAntdTokens = Theme.getFilteredAntdTheme(antdConfig);

Review Comment:
   Done!



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theming): Introduce bootstrap-driven Superset theme configurations [superset]

["mistercrunch (via GitHub)" <gi...@apache.org>]

mistercrunch commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205944937


##########
superset/config.py:
##########
@@ -677,18 +678,58 @@ class D3TimeFormat(TypedDict, total=False):
 # This is merely a default
 EXTRA_CATEGORICAL_COLOR_SCHEMES: list[dict[str, Any]] = []
 
-# THEME is used for setting a custom theme to Superset, it follows the ant design
-# theme structure
-# You can use the AntDesign theme editor to generate a theme structure
-# https://ant.design/theme-editor
+# ---------------------------------------------------

Review Comment:
   seems we may need to touch up the related comment in `UPDATING.md`. I think I added a comment in there, but we may want to clarify further what people will need to do. The comment that's there might be ok already.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(theme): introduce bootstrap-driven Superset theme configurations [superset]

["gabotorresruiz (via GitHub)" <gi...@apache.org>]

gabotorresruiz commented on PR #34144:
URL: https://github.com/apache/superset/pull/34144#issuecomment-3063962840

   /korbit-review


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org