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></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
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/685f7c92-e877-4305-8391-e3b3ea45e643?what_not_in_standard=true)
[](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></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
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ddd88dd-62be-4fc8-bef4-2c051d0eeb1a?what_not_in_standard=true)
[](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></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
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b304232-f864-48a7-b209-a559264e0460?what_not_in_standard=true)
[](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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
