korbit-ai[bot] commented on code in PR #33420: URL: https://github.com/apache/superset/pull/33420#discussion_r2086526532
########## superset-frontend/packages/superset-ui-core/src/theme/ThemeController.tsx: ########## @@ -0,0 +1,188 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { theme as antdThemeImport } from 'antd-v5'; +import { Theme } from './Theme'; +import { AnyThemeConfig } from './types'; + +export interface ThemeStorage { + getItem(key: string): string | null; + setItem(key: string, value: string): void; + removeItem(key: string): void; +} + +export class LocalStorageAdapter implements ThemeStorage { + getItem(key: string): string | null { + return localStorage.getItem(key); + } + + setItem(key: string, value: string): void { + localStorage.setItem(key, value); + } + + removeItem(key: string): void { + localStorage.removeItem(key); + } +} + +export interface ThemeControllerOptions { + storage?: ThemeStorage; + storageKey?: string; + defaultTheme?: AnyThemeConfig; + onChange?: (theme: Theme) => void; +} + +export class ThemeController { + private static instance: ThemeController | null = null; + + private theme: Theme; + + private storage: ThemeStorage; + + private storageKey: string; + + private defaultTheme: AnyThemeConfig; + + private customizations: AnyThemeConfig = {}; + + private onChangeCallbacks: Set<(theme: Theme) => void> = new Set(); + + private constructor(options: ThemeControllerOptions = {}) { + this.storage = options.storage || new LocalStorageAdapter(); + this.storageKey = options.storageKey || 'superset-theme'; + this.defaultTheme = options.defaultTheme || {}; + + const savedThemeJson = this.storage.getItem(this.storageKey); + let initialCustomizations: AnyThemeConfig = {}; + + if (savedThemeJson) { + try { + initialCustomizations = JSON.parse(savedThemeJson); + this.customizations = initialCustomizations; + } catch (e) { + console.error('Failed to parse saved theme:', e); + initialCustomizations = {}; + } Review Comment: ### Missing Theme JSON in Parse Error <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The error message in the catch block doesn't include the actual theme JSON that failed to parse. ###### Why this matters Without the actual JSON string that caused the parsing error, debugging invalid theme configurations becomes more challenging. ###### Suggested change ∙ *Feature Preview* Include the problematic JSON in the error message: ```typescript try { initialCustomizations = JSON.parse(savedThemeJson); this.customizations = initialCustomizations; } catch (e) { console.error('Failed to parse saved theme:', e, '\nInvalid JSON:', savedThemeJson); initialCustomizations = {}; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b97223a6-1d34-41b9-afd7-dcd545c3d97e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b97223a6-1d34-41b9-afd7-dcd545c3d97e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b97223a6-1d34-41b9-afd7-dcd545c3d97e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b97223a6-1d34-41b9-afd7-dcd545c3d97e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b97223a6-1d34-41b9-afd7-dcd545c3d97e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a230aefb-7817-4a0e-9a54-d5d151ecd5ac --> [](a230aefb-7817-4a0e-9a54-d5d151ecd5ac) ########## superset-frontend/packages/superset-ui-core/src/theme/index.tsx: ########## @@ -51,11 +52,13 @@ export function useTheme() { const styled = emotionStyled; +const themeController = ThemeController.getInstance(); // launching in in dark mode for now while iterating Review Comment: ### Misleading and typo-containing comment <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Comment has a typo ('in in') and contains temporary/outdated information about dark mode iteration that doesn't match the actual code implementation ###### Why this matters Outdated or inaccurate comments can mislead developers and make code harder to understand, especially when the comment contradicts the actual implementation ###### Suggested change ∙ *Feature Preview* Remove the outdated comment since it no longer reflects the current implementation: ```typescript const themeController = ThemeController.getInstance(); const themeObject = themeController.getTheme(); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c157983d-39bf-4cc5-b27d-a39374353d02/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c157983d-39bf-4cc5-b27d-a39374353d02?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c157983d-39bf-4cc5-b27d-a39374353d02?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c157983d-39bf-4cc5-b27d-a39374353d02?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c157983d-39bf-4cc5-b27d-a39374353d02) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:341abce0-8c0d-4ac3-a740-e1eaf5e4b19e --> [](341abce0-8c0d-4ac3-a740-e1eaf5e4b19e) ########## superset-frontend/src/components/ThemeEditor/index.tsx: ########## @@ -22,24 +22,23 @@ import { themeObject, exampleThemes, SerializableThemeConfig, - SupersetTheme, + useThemeContext, } from '@superset-ui/core'; import { useState } from 'react'; import { Icons } from 'src/components/Icons'; interface ThemeEditorProps { - initialTheme?: SupersetTheme; tooltipTitle?: string; modalTitle?: string; } const ThemeEditor: React.FC<ThemeEditorProps> = ({ - initialTheme = {}, tooltipTitle = 'Edit Theme', modalTitle = 'Theme Editor', }) => { + const { theme, setTheme } = useThemeContext(); const [isModalOpen, setIsModalOpen] = useState<boolean>(false); - const jsonTheme: string = themeObject.json(); + const jsonTheme: string = JSON.stringify(theme.toSerializedConfig(), null, 2); Review Comment: ### Redundant JSON Processing <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Unnecessary JSON stringification on component initialization. The jsonTheme variable is only used to initialize jsonMetadata state, which is immediately overwritten when the modal opens. ###### Why this matters This creates unnecessary processing overhead on every component render, even when the theme editor is not being used. ###### Suggested change ∙ *Feature Preview* Remove the jsonTheme variable and initialize jsonMetadata with an empty string or move the JSON stringification to only occur when needed: ```typescript const [jsonMetadata, setJsonMetadata] = useState<string>(''); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4792ee1a-9ba4-4c41-8bd8-4bebdb2eaa09/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4792ee1a-9ba4-4c41-8bd8-4bebdb2eaa09?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4792ee1a-9ba4-4c41-8bd8-4bebdb2eaa09?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4792ee1a-9ba4-4c41-8bd8-4bebdb2eaa09?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4792ee1a-9ba4-4c41-8bd8-4bebdb2eaa09) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:53195dcd-4912-4ad6-b503-2c7ba61e7a25 --> [](53195dcd-4912-4ad6-b503-2c7ba61e7a25) ########## superset-frontend/packages/superset-ui-core/src/theme/ThemeController.tsx: ########## @@ -0,0 +1,188 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { theme as antdThemeImport } from 'antd-v5'; +import { Theme } from './Theme'; +import { AnyThemeConfig } from './types'; + +export interface ThemeStorage { + getItem(key: string): string | null; + setItem(key: string, value: string): void; + removeItem(key: string): void; +} + +export class LocalStorageAdapter implements ThemeStorage { + getItem(key: string): string | null { + return localStorage.getItem(key); + } + + setItem(key: string, value: string): void { + localStorage.setItem(key, value); + } + + removeItem(key: string): void { + localStorage.removeItem(key); + } +} + +export interface ThemeControllerOptions { + storage?: ThemeStorage; + storageKey?: string; + defaultTheme?: AnyThemeConfig; + onChange?: (theme: Theme) => void; +} + +export class ThemeController { + private static instance: ThemeController | null = null; + + private theme: Theme; + + private storage: ThemeStorage; + + private storageKey: string; + + private defaultTheme: AnyThemeConfig; + + private customizations: AnyThemeConfig = {}; + + private onChangeCallbacks: Set<(theme: Theme) => void> = new Set(); + + private constructor(options: ThemeControllerOptions = {}) { + this.storage = options.storage || new LocalStorageAdapter(); + this.storageKey = options.storageKey || 'superset-theme'; + this.defaultTheme = options.defaultTheme || {}; + + const savedThemeJson = this.storage.getItem(this.storageKey); + let initialCustomizations: AnyThemeConfig = {}; + + if (savedThemeJson) { + try { + initialCustomizations = JSON.parse(savedThemeJson); + this.customizations = initialCustomizations; + } catch (e) { + console.error('Failed to parse saved theme:', e); + initialCustomizations = {}; + } + } + + this.theme = Theme.fromConfig(initialCustomizations); + + if (options.onChange) { + this.onChangeCallbacks.add(options.onChange); + } + } + + /** + * Get the singleton instance of ThemeController + */ + public static getInstance(options?: ThemeControllerOptions): ThemeController { + if (!ThemeController.instance) { + ThemeController.instance = new ThemeController(options); + } else if (options) { + if (options.storage) { + ThemeController.instance.storage = options.storage; + } + if (options.storageKey) { + ThemeController.instance.storageKey = options.storageKey; + } + if (options.defaultTheme) { + ThemeController.instance.defaultTheme = options.defaultTheme; + } + if (options.onChange) { + ThemeController.instance.onChangeCallbacks.add(options.onChange); + } + } + return ThemeController.instance; + } + + /** + * Get the current theme + */ + public getTheme(): Theme { + return this.theme; + } + + /** + * Set a new theme configuration + */ + public setTheme(config: AnyThemeConfig): void { + this.customizations = config; + this.theme.setConfig(config); + this.persistTheme(); + this.notifyListeners(); + } + + /** + * Toggle dark mode + */ + public toggleDarkMode(isDark: boolean): void { + this.customizations.algorithm = isDark + ? antdThemeImport.darkAlgorithm + : antdThemeImport.defaultAlgorithm; + + if (isDark) { + this.customizations = { + ...this.customizations, + algorithm: 'dark', + }; + } else { + const { algorithm, ...rest } = this.customizations; + this.customizations = Object.keys(rest).length > 0 ? { ...rest } : {}; + } + + this.theme.setConfig(this.customizations); + + this.persistTheme(); + this.notifyListeners(); + } + + /** + * Reset to default theme + */ + public resetTheme(): void { + this.customizations = {}; + this.theme.setConfig(this.defaultTheme); + this.persistTheme(); + this.notifyListeners(); + } + + /** + * Register a callback to be called when theme changes + */ + public onChange(callback: (theme: Theme) => void): () => void { + this.onChangeCallbacks.add(callback); + + return () => { + this.onChangeCallbacks.delete(callback); + }; + } + + private persistTheme(): void { + this.storage.setItem(this.storageKey, JSON.stringify(this.customizations)); + } + + private notifyListeners(): void { + this.onChangeCallbacks.forEach(callback => { + try { + callback(this.theme); + } catch (e) { + console.error('Error in theme change callback:', e); + } + }); + } Review Comment: ### Synchronous Callback Execution <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The notifyListeners method executes callbacks sequentially, which could block the main thread if there are many listeners or if callbacks are computationally expensive. ###### Why this matters Sequential execution of callbacks can lead to UI jank and poor responsiveness, especially when theme changes trigger multiple component re-renders. ###### Suggested change ∙ *Feature Preview* Use Promise.all with setTimeout to defer callbacks to the next event loop iteration: ```typescript private async notifyListeners(): Promise<void> { await Promise.all( Array.from(this.onChangeCallbacks).map(callback => setTimeout(() => { try { callback(this.theme); } catch (e) { console.error('Error in theme change callback:', e); } }, 0) ) ); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c560852-2143-4302-8d05-1b7ccc850c30/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c560852-2143-4302-8d05-1b7ccc850c30?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c560852-2143-4302-8d05-1b7ccc850c30?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c560852-2143-4302-8d05-1b7ccc850c30?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c560852-2143-4302-8d05-1b7ccc850c30) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:375e17dd-5843-4c79-a5f2-478a7862cac3 --> [](375e17dd-5843-4c79-a5f2-478a7862cac3) ########## superset-frontend/packages/superset-ui-core/src/theme/ThemeController.tsx: ########## @@ -0,0 +1,188 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { theme as antdThemeImport } from 'antd-v5'; +import { Theme } from './Theme'; +import { AnyThemeConfig } from './types'; + +export interface ThemeStorage { + getItem(key: string): string | null; + setItem(key: string, value: string): void; + removeItem(key: string): void; +} + +export class LocalStorageAdapter implements ThemeStorage { + getItem(key: string): string | null { + return localStorage.getItem(key); + } + + setItem(key: string, value: string): void { + localStorage.setItem(key, value); + } + + removeItem(key: string): void { + localStorage.removeItem(key); + } +} + +export interface ThemeControllerOptions { + storage?: ThemeStorage; + storageKey?: string; + defaultTheme?: AnyThemeConfig; + onChange?: (theme: Theme) => void; +} + +export class ThemeController { + private static instance: ThemeController | null = null; + + private theme: Theme; + + private storage: ThemeStorage; + + private storageKey: string; + + private defaultTheme: AnyThemeConfig; + + private customizations: AnyThemeConfig = {}; + + private onChangeCallbacks: Set<(theme: Theme) => void> = new Set(); + + private constructor(options: ThemeControllerOptions = {}) { + this.storage = options.storage || new LocalStorageAdapter(); + this.storageKey = options.storageKey || 'superset-theme'; + this.defaultTheme = options.defaultTheme || {}; + + const savedThemeJson = this.storage.getItem(this.storageKey); + let initialCustomizations: AnyThemeConfig = {}; + + if (savedThemeJson) { + try { + initialCustomizations = JSON.parse(savedThemeJson); + this.customizations = initialCustomizations; + } catch (e) { + console.error('Failed to parse saved theme:', e); + initialCustomizations = {}; + } + } + + this.theme = Theme.fromConfig(initialCustomizations); + + if (options.onChange) { + this.onChangeCallbacks.add(options.onChange); + } + } + + /** + * Get the singleton instance of ThemeController + */ + public static getInstance(options?: ThemeControllerOptions): ThemeController { + if (!ThemeController.instance) { + ThemeController.instance = new ThemeController(options); + } else if (options) { + if (options.storage) { + ThemeController.instance.storage = options.storage; + } + if (options.storageKey) { + ThemeController.instance.storageKey = options.storageKey; + } + if (options.defaultTheme) { + ThemeController.instance.defaultTheme = options.defaultTheme; + } + if (options.onChange) { + ThemeController.instance.onChangeCallbacks.add(options.onChange); + } + } + return ThemeController.instance; + } + + /** + * Get the current theme + */ + public getTheme(): Theme { + return this.theme; + } + + /** + * Set a new theme configuration + */ + public setTheme(config: AnyThemeConfig): void { + this.customizations = config; + this.theme.setConfig(config); + this.persistTheme(); + this.notifyListeners(); + } + + /** + * Toggle dark mode + */ + public toggleDarkMode(isDark: boolean): void { + this.customizations.algorithm = isDark + ? antdThemeImport.darkAlgorithm + : antdThemeImport.defaultAlgorithm; + + if (isDark) { + this.customizations = { + ...this.customizations, + algorithm: 'dark', + }; + } else { + const { algorithm, ...rest } = this.customizations; + this.customizations = Object.keys(rest).length > 0 ? { ...rest } : {}; + } Review Comment: ### Redundant Theme Object Operations <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The toggleDarkMode method performs redundant object spread operations and reassignments of the algorithm property. ###### Why this matters Unnecessary object operations and property reassignments cause additional memory allocations and CPU cycles, especially if this method is called frequently during theme switches. ###### Suggested change ∙ *Feature Preview* Simplify the toggleDarkMode method to avoid duplicate operations: ```typescript public toggleDarkMode(isDark: boolean): void { if (isDark) { this.customizations = { ...this.customizations, algorithm: antdThemeImport.darkAlgorithm, }; } else { const { algorithm, ...rest } = this.customizations; this.customizations = Object.keys(rest).length > 0 ? rest : {}; } this.theme.setConfig(this.customizations); this.persistTheme(); this.notifyListeners(); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99a0e07f-bbc6-48ae-a085-bbc39746c0b3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99a0e07f-bbc6-48ae-a085-bbc39746c0b3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99a0e07f-bbc6-48ae-a085-bbc39746c0b3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99a0e07f-bbc6-48ae-a085-bbc39746c0b3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/99a0e07f-bbc6-48ae-a085-bbc39746c0b3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e835e7f7-cdce-4447-ae20-88a8e97234cc --> [](e835e7f7-cdce-4447-ae20-88a8e97234cc) ########## superset-frontend/packages/superset-ui-core/src/theme/ThemeController.tsx: ########## @@ -0,0 +1,188 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { theme as antdThemeImport } from 'antd-v5'; +import { Theme } from './Theme'; +import { AnyThemeConfig } from './types'; + +export interface ThemeStorage { + getItem(key: string): string | null; + setItem(key: string, value: string): void; + removeItem(key: string): void; +} + +export class LocalStorageAdapter implements ThemeStorage { + getItem(key: string): string | null { + return localStorage.getItem(key); + } + + setItem(key: string, value: string): void { + localStorage.setItem(key, value); + } + + removeItem(key: string): void { + localStorage.removeItem(key); + } +} + +export interface ThemeControllerOptions { + storage?: ThemeStorage; + storageKey?: string; + defaultTheme?: AnyThemeConfig; + onChange?: (theme: Theme) => void; +} + +export class ThemeController { + private static instance: ThemeController | null = null; + + private theme: Theme; + + private storage: ThemeStorage; + + private storageKey: string; + + private defaultTheme: AnyThemeConfig; + + private customizations: AnyThemeConfig = {}; + + private onChangeCallbacks: Set<(theme: Theme) => void> = new Set(); + + private constructor(options: ThemeControllerOptions = {}) { + this.storage = options.storage || new LocalStorageAdapter(); + this.storageKey = options.storageKey || 'superset-theme'; + this.defaultTheme = options.defaultTheme || {}; + + const savedThemeJson = this.storage.getItem(this.storageKey); + let initialCustomizations: AnyThemeConfig = {}; + + if (savedThemeJson) { + try { + initialCustomizations = JSON.parse(savedThemeJson); + this.customizations = initialCustomizations; + } catch (e) { + console.error('Failed to parse saved theme:', e); + initialCustomizations = {}; + } + } + + this.theme = Theme.fromConfig(initialCustomizations); + + if (options.onChange) { + this.onChangeCallbacks.add(options.onChange); + } + } + + /** + * Get the singleton instance of ThemeController + */ + public static getInstance(options?: ThemeControllerOptions): ThemeController { + if (!ThemeController.instance) { + ThemeController.instance = new ThemeController(options); + } else if (options) { + if (options.storage) { + ThemeController.instance.storage = options.storage; + } + if (options.storageKey) { + ThemeController.instance.storageKey = options.storageKey; + } + if (options.defaultTheme) { + ThemeController.instance.defaultTheme = options.defaultTheme; + } + if (options.onChange) { + ThemeController.instance.onChangeCallbacks.add(options.onChange); + } + } + return ThemeController.instance; + } + + /** + * Get the current theme + */ + public getTheme(): Theme { + return this.theme; + } + + /** + * Set a new theme configuration + */ + public setTheme(config: AnyThemeConfig): void { + this.customizations = config; + this.theme.setConfig(config); + this.persistTheme(); + this.notifyListeners(); + } + + /** + * Toggle dark mode + */ + public toggleDarkMode(isDark: boolean): void { Review Comment: ### Incomplete toggleDarkMode documentation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The docstring for toggleDarkMode lacks explanation of the isDark parameter and its effect on the theme. ###### Why this matters Without clear parameter documentation, developers may misunderstand how the dark mode toggle affects the theme configuration. ###### Suggested change ∙ *Feature Preview* /** * Toggle dark mode and update theme algorithm accordingly * @param isDark - When true, sets dark theme algorithm; when false, reverts to default algorithm */ ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba6cf0a7-0053-41db-8577-149920b28f4c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba6cf0a7-0053-41db-8577-149920b28f4c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba6cf0a7-0053-41db-8577-149920b28f4c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba6cf0a7-0053-41db-8577-149920b28f4c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba6cf0a7-0053-41db-8577-149920b28f4c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:4d84d844-d022-4f08-a8a1-bc171e21bc4d --> [](4d84d844-d022-4f08-a8a1-bc171e21bc4d) ########## superset-frontend/packages/superset-ui-core/src/theme/ThemeController.tsx: ########## @@ -0,0 +1,188 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { theme as antdThemeImport } from 'antd-v5'; +import { Theme } from './Theme'; +import { AnyThemeConfig } from './types'; + +export interface ThemeStorage { + getItem(key: string): string | null; + setItem(key: string, value: string): void; + removeItem(key: string): void; +} + +export class LocalStorageAdapter implements ThemeStorage { + getItem(key: string): string | null { + return localStorage.getItem(key); + } + + setItem(key: string, value: string): void { + localStorage.setItem(key, value); + } + + removeItem(key: string): void { + localStorage.removeItem(key); + } +} + +export interface ThemeControllerOptions { + storage?: ThemeStorage; + storageKey?: string; + defaultTheme?: AnyThemeConfig; + onChange?: (theme: Theme) => void; +} + +export class ThemeController { + private static instance: ThemeController | null = null; + + private theme: Theme; + + private storage: ThemeStorage; + + private storageKey: string; + + private defaultTheme: AnyThemeConfig; + + private customizations: AnyThemeConfig = {}; + + private onChangeCallbacks: Set<(theme: Theme) => void> = new Set(); + + private constructor(options: ThemeControllerOptions = {}) { + this.storage = options.storage || new LocalStorageAdapter(); + this.storageKey = options.storageKey || 'superset-theme'; + this.defaultTheme = options.defaultTheme || {}; + + const savedThemeJson = this.storage.getItem(this.storageKey); + let initialCustomizations: AnyThemeConfig = {}; + + if (savedThemeJson) { + try { + initialCustomizations = JSON.parse(savedThemeJson); + this.customizations = initialCustomizations; + } catch (e) { + console.error('Failed to parse saved theme:', e); + initialCustomizations = {}; + } + } + + this.theme = Theme.fromConfig(initialCustomizations); + + if (options.onChange) { + this.onChangeCallbacks.add(options.onChange); + } + } + + /** + * Get the singleton instance of ThemeController + */ + public static getInstance(options?: ThemeControllerOptions): ThemeController { + if (!ThemeController.instance) { + ThemeController.instance = new ThemeController(options); + } else if (options) { + if (options.storage) { + ThemeController.instance.storage = options.storage; + } + if (options.storageKey) { + ThemeController.instance.storageKey = options.storageKey; + } + if (options.defaultTheme) { + ThemeController.instance.defaultTheme = options.defaultTheme; + } + if (options.onChange) { + ThemeController.instance.onChangeCallbacks.add(options.onChange); + } + } + return ThemeController.instance; + } + + /** + * Get the current theme + */ + public getTheme(): Theme { + return this.theme; + } + + /** + * Set a new theme configuration + */ + public setTheme(config: AnyThemeConfig): void { + this.customizations = config; + this.theme.setConfig(config); + this.persistTheme(); + this.notifyListeners(); + } + + /** + * Toggle dark mode + */ + public toggleDarkMode(isDark: boolean): void { + this.customizations.algorithm = isDark + ? antdThemeImport.darkAlgorithm + : antdThemeImport.defaultAlgorithm; + + if (isDark) { + this.customizations = { + ...this.customizations, + algorithm: 'dark', + }; + } else { Review Comment: ### Conflicting Dark Mode Algorithm Assignment <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code sets the algorithm property twice with different values in the toggleDarkMode method. First with antdThemeImport.darkAlgorithm, then immediately overwrites it with the string 'dark'. ###### Why this matters This inconsistency will lead to incorrect theme application as the actual algorithm object from antd is replaced with a string value that won't work with the theming system. ###### Suggested change ∙ *Feature Preview* Remove the redundant assignment and keep only the antd algorithm: ```typescript public toggleDarkMode(isDark: boolean): void { this.customizations = { ...this.customizations, algorithm: isDark ? antdThemeImport.darkAlgorithm : antdThemeImport.defaultAlgorithm, }; if (!isDark) { const { algorithm, ...rest } = this.customizations; this.customizations = Object.keys(rest).length > 0 ? { ...rest } : {}; } this.theme.setConfig(this.customizations); this.persistTheme(); this.notifyListeners(); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a409e2c8-bfa6-4bd9-bc28-8bd2ed75f7b3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a409e2c8-bfa6-4bd9-bc28-8bd2ed75f7b3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a409e2c8-bfa6-4bd9-bc28-8bd2ed75f7b3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a409e2c8-bfa6-4bd9-bc28-8bd2ed75f7b3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a409e2c8-bfa6-4bd9-bc28-8bd2ed75f7b3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f7c95fd0-059b-418a-9e23-655d2d1c9706 --> [](f7c95fd0-059b-418a-9e23-655d2d1c9706) -- 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]
