bito-code-review[bot] commented on code in PR #36317:
URL: https://github.com/apache/superset/pull/36317#discussion_r2570513892
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -781,12 +789,41 @@ export class ThemeController {
// The merging with base theme happens in getThemeForMode() and other
methods
// that prepare themes before passing them to applyTheme()
this.globalTheme.setConfig(normalizedConfig);
+
+ // Load custom fonts if specified in theme config
+ const fontUrls = (normalizedConfig?.token as Record<string, unknown>)
+ ?.fontUrls as string[] | undefined;
+ this.loadFonts(fontUrls);
} catch (error) {
console.error('Failed to apply theme:', error);
this.fallbackToDefaultMode();
}
}
+ /**
+ * Loads custom fonts from theme configuration.
+ * Injects CSS @import statements for font URLs that haven't been loaded yet.
+ * @param fontUrls - Array of font URLs to load (e.g., Google Fonts, Adobe
Fonts)
+ */
+ private loadFonts(fontUrls?: string[]): void {
+ if (!fontUrls?.length) return;
+
+ // Filter out already loaded fonts
+ const newUrls = fontUrls.filter(url => !this.loadedFontUrls.has(url));
+ if (newUrls.length === 0) return;
+
+ // Use CSS @import for font loading
+ const css = newUrls.map(url => `@import url("${url}");`).join('\n');
+
+ const style = document.createElement('style');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CSS Injection Risk</b></div>
<div id="fix">
The template literal injection of font URLs into CSS @import statements
could allow injection attacks if URLs contain quotes. It looks like
JSON.stringify would provide safe escaping here.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
// Use CSS @import for font loading
const css = newUrls.map(url => `@import
url(${JSON.stringify(url)});`).join('\n');
const style = document.createElement('style');
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36317#issuecomment-3587953569>#607d71</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/theme/tests/ThemeController.test.ts:
##########
@@ -102,1272 +102,1391 @@ const mockThemeObject = {
theme: DEFAULT_THEME,
} as unknown as Theme;
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('LocalStorageAdapter', () => {
- let adapter: LocalStorageAdapter;
- const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
-
- beforeEach(() => {
- adapter = new LocalStorageAdapter();
- jest.clearAllMocks();
- Object.defineProperty(window, 'localStorage', {
- value: mockLocalStorage,
- writable: true,
- });
+// Helper to create a fresh ThemeController with common setup
+const createController = (
+ options: Partial<ConstructorParameters<typeof ThemeController>[0]> = {},
+) =>
+ new ThemeController({
+ themeObject: mockThemeObject,
+ ...options,
});
- afterAll(() => {
- consoleSpy.mockRestore();
- });
-
- test('should return item from localStorage', () => {
- mockLocalStorage.getItem.mockReturnValue('test-value');
+// Shared console spies
+const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
+const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
- const result = adapter.getItem('test-key');
+beforeEach(() => {
+ jest.clearAllMocks();
- expect(mockLocalStorage.getItem).toHaveBeenCalledTimes(1);
- expect(mockLocalStorage.getItem).toHaveBeenCalledWith('test-key');
- expect(result).toBe('test-value');
+ // Setup DOM environment
+ Object.defineProperty(window, 'localStorage', {
+ value: mockLocalStorage,
+ writable: true,
});
- test('should set item in localStorage', () => {
- adapter.setItem('test-key', 'test-value');
+ Object.defineProperty(window, 'matchMedia', {
+ value: mockMatchMedia,
+ writable: true,
+ });
- expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
- expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
- 'test-key',
- 'test-value',
- );
+ mockMatchMedia.mockReturnValue({
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
});
- test('should handle localStorage errors while setting an item', () => {
- mockLocalStorage.setItem.mockImplementation(() => {
- throw new Error('Storage error');
- });
+ mockSetConfig.mockImplementation(() => {});
+ mockThemeFromConfig.mockReturnValue(mockThemeObject);
- adapter.setItem('test-key', 'test-value');
+ // Mock Theme constructor
+ (Theme as any).fromConfig = mockThemeFromConfig;
- expect(consoleSpy).toHaveBeenCalledTimes(1);
- expect(consoleSpy).toHaveBeenCalledWith(
- 'Failed to write to localStorage:',
- expect.any(Error),
- );
- });
+ // Reset localStorage mocks
+ mockLocalStorage.getItem.mockReturnValue(null);
+ mockLocalStorage.setItem.mockImplementation(() => {});
+ mockLocalStorage.removeItem.mockImplementation(() => {});
- test('should remove item from localStorage', () => {
- adapter.removeItem('test-key');
+ // Default BootstrapData
+ mockGetBootstrapData.mockReturnValue(createMockBootstrapData());
+});
- expect(mockLocalStorage.removeItem).toHaveBeenCalledTimes(1);
- expect(mockLocalStorage.removeItem).toHaveBeenCalledWith('test-key');
- });
+afterAll(() => {
+ consoleSpy.mockRestore();
+ consoleErrorSpy.mockRestore();
+});
- test('should handle localStorage errors while removing an item', () => {
- mockLocalStorage.removeItem.mockImplementation(() => {
- throw new Error('Storage error');
- });
+// LocalStorageAdapter tests
+test('LocalStorageAdapter returns item from localStorage', () => {
+ const adapter = new LocalStorageAdapter();
+ mockLocalStorage.getItem.mockReturnValue('test-value');
- adapter.removeItem('test-key');
+ const result = adapter.getItem('test-key');
- expect(consoleSpy).toHaveBeenCalledTimes(1);
- expect(consoleSpy).toHaveBeenCalledWith(
- 'Failed to remove from localStorage:',
- expect.any(Error),
- );
- });
+ expect(mockLocalStorage.getItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.getItem).toHaveBeenCalledWith('test-key');
+ expect(result).toBe('test-value');
});
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('ThemeController', () => {
- let controller: ThemeController;
- const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
- const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+test('LocalStorageAdapter sets item in localStorage', () => {
+ const adapter = new LocalStorageAdapter();
- beforeEach(() => {
- jest.clearAllMocks();
+ adapter.setItem('test-key', 'test-value');
- // Setup DOM environment
- Object.defineProperty(window, 'localStorage', {
- value: mockLocalStorage,
- writable: true,
- });
+ expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
+ 'test-key',
+ 'test-value',
+ );
+});
- Object.defineProperty(window, 'matchMedia', {
- value: mockMatchMedia,
- writable: true,
- });
+test('LocalStorageAdapter handles localStorage errors while setting an item',
() => {
+ const adapter = new LocalStorageAdapter();
+ mockLocalStorage.setItem.mockImplementation(() => {
+ throw new Error('Storage error');
+ });
- mockMatchMedia.mockReturnValue({
- matches: false,
- addEventListener: jest.fn(),
- removeEventListener: jest.fn(),
- });
+ adapter.setItem('test-key', 'test-value');
- mockSetConfig.mockImplementation(() => {});
- mockThemeFromConfig.mockReturnValue(mockThemeObject);
+ expect(consoleSpy).toHaveBeenCalledTimes(1);
+ expect(consoleSpy).toHaveBeenCalledWith(
+ 'Failed to write to localStorage:',
+ expect.any(Error),
+ );
+});
- // Mock Theme constructor
- (Theme as any).fromConfig = mockThemeFromConfig;
+test('LocalStorageAdapter removes item from localStorage', () => {
+ const adapter = new LocalStorageAdapter();
- // Reset localStorage mocks
- mockLocalStorage.getItem.mockReturnValue(null);
- mockLocalStorage.setItem.mockImplementation(() => {});
- mockLocalStorage.removeItem.mockImplementation(() => {});
+ adapter.removeItem('test-key');
- // Default BootstrapData
- mockGetBootstrapData.mockReturnValue(createMockBootstrapData());
- });
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith('test-key');
+});
- afterAll(() => {
- consoleSpy.mockRestore();
- consoleErrorSpy.mockRestore();
+test('LocalStorageAdapter handles localStorage errors while removing an item',
() => {
+ const adapter = new LocalStorageAdapter();
+ mockLocalStorage.removeItem.mockImplementation(() => {
+ throw new Error('Storage error');
});
- test('should initialize with default options', () => {
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
+ adapter.removeItem('test-key');
- expect(controller.getTheme()).toBe(mockThemeObject);
- });
+ expect(consoleSpy).toHaveBeenCalledTimes(1);
+ expect(consoleSpy).toHaveBeenCalledWith(
+ 'Failed to remove from localStorage:',
+ expect.any(Error),
+ );
+});
- test('should use BootstrapData themes when available', () => {
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
-
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- expect(mockSetConfig).toHaveBeenCalledWith(
- expect.objectContaining({
- token: expect.objectContaining({
- colorBgBase: '#ededed',
- colorPrimary: '#c96f0f',
- }),
- }),
- );
- });
+// ThemeController initialization tests
+test('ThemeController initializes with default options', () => {
+ const controller = createController();
+
+ expect(controller.getTheme()).toBe(mockThemeObject);
+});
+
+test('ThemeController uses BootstrapData themes when available', () => {
+ createController();
- test('should fallback to Superset default theme when BootstrapData themes
are empty', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: {},
- dark: {},
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed',
+ colorPrimary: '#c96f0f',
}),
- );
+ }),
+ );
+});
- const fallbackTheme = {
- token: {
+test('ThemeController fallbacks to Superset default theme when BootstrapData
themes are empty', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: {},
+ }),
+ );
+
+ const fallbackTheme = {
+ token: {
+ colorBgBase: '#ffffff',
+ colorPrimary: '#1890ff',
+ },
+ };
+
+ createController({ defaultTheme: fallbackTheme });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ ...fallbackTheme,
+ algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
+
+test('ThemeController handles system theme preference', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: DARK_THEME,
+ }),
+ );
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM);
+});
+
+test('ThemeController handles only default theme', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: {},
+ }),
+ );
+
+ const controller = createController();
+
+ jest.clearAllMocks();
+
+ expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow(
+ 'Theme mode changes are not allowed when only one theme is available',
+ );
+
+ expect(mockSetConfig).not.toHaveBeenCalled();
+});
+
+test('ThemeController handles only dark theme', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: DARK_THEME,
+ }),
+ );
+
+ const lightThemeFallback = {
+ token: {
+ colorBgBase: '#fff',
+ colorTextBase: '#000',
+ colorPrimary: '#1890ff',
+ },
+ };
+
+ const controller = createController({ defaultTheme: lightThemeFallback });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+
+ const calledWith = mockSetConfig.mock.calls[0][0];
+
+ expect(calledWith.token.colorBgBase).toBe('#fff');
+ expect(calledWith.token.colorTextBase).toBe('#000');
+
+ expect(controller.canSetMode()).toBe(true);
+
+ jest.clearAllMocks();
+ controller.setThemeMode(ThemeMode.DARK);
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+});
+
+test('ThemeController handles completely empty BootstrapData', () => {
+ const fallbackTheme = {
+ token: {
+ colorBgBase: '#ffffff',
+ colorPrimary: '#1890ff',
+ },
+ };
+
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: {},
+ }),
+ );
+
+ createController({ defaultTheme: fallbackTheme });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
colorBgBase: '#ffffff',
colorPrimary: '#1890ff',
- },
- };
-
- controller = new ThemeController({
- themeObject: mockThemeObject,
- defaultTheme: fallbackTheme,
- });
-
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- expect(mockSetConfig).toHaveBeenCalledWith(
- expect.objectContaining({
- ...fallbackTheme,
- algorithm: antdThemeImport.defaultAlgorithm,
}),
- );
+ algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
+
+test('ThemeController handles missing theme object', () => {
+ const fallbackTheme = { token: { colorPrimary: '#fallback' } };
+
+ mockGetBootstrapData.mockReturnValue({
+ common: {
+ application_root: '/',
+ static_assets_prefix: '/static/assets/',
+ conf: {},
+ locale: 'en',
+ feature_flags: {},
+ language_pack: {},
+ extra_categorical_color_schemes: [],
+ extra_sequential_color_schemes: [],
+ menu_data: {},
+ d3_format: {},
+ d3_time_format: {},
+ } as any,
});
- test('should handle system theme preference', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: DEFAULT_THEME,
- dark: DARK_THEME,
+ createController({ defaultTheme: fallbackTheme });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorPrimary: '#fallback',
}),
- );
+ algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
+// Theme Management tests
- expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM);
- });
+test('ThemeController updates theme when allowed', () => {
+ const controller = createController();
- test('should handle only default theme', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: DEFAULT_THEME,
- dark: {},
- }),
- );
+ const newTheme = {
+ token: {
+ colorBgBase: '#000000',
+ colorPrimary: '#ff0000',
+ },
+ };
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
+ controller.setTheme(newTheme);
- // Clear the call from initialization
- jest.clearAllMocks();
+
expect(mockSetConfig).toHaveBeenCalledWith(expect.objectContaining(newTheme));
+});
- // Should throw when trying to change mode with only one theme
- expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow(
- 'Theme mode changes are not allowed when only one theme is available',
- );
+test('ThemeController changes theme mode when allowed', () => {
+ const controller = createController();
- // Config should not have been called since the error was thrown
- expect(mockSetConfig).not.toHaveBeenCalled();
- });
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.DARK);
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DARK);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
+ 'superset-theme-mode',
+ ThemeMode.DARK,
+ );
+});
+
+test('ThemeController handles missing theme gracefully', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: {},
+ }),
+ );
+
+ const controller = createController();
+
+ expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow(
+ 'Theme mode changes are not allowed when only one theme is available',
+ );
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+ expect(consoleSpy).not.toHaveBeenCalled();
+});
+
+test('ThemeController does not change mode if already set', () => {
+ const controller = createController();
- test('should handle only dark theme', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: {},
- dark: DARK_THEME,
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.SYSTEM);
+
+ expect(mockLocalStorage.setItem).not.toHaveBeenCalled();
+});
+
+test('ThemeController resets to default theme', () => {
+ const controller = createController();
+
+ controller.setThemeMode(ThemeMode.DARK);
+ controller.resetTheme();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#141118',
+ colorTextBase: '#fdc7c7',
}),
- );
+ }),
+ );
+});
- // Provide an explicit light theme fallback for this test
- const lightThemeFallback = {
- token: {
- colorBgBase: '#fff',
- colorTextBase: '#000',
- colorPrimary: '#1890ff',
- },
- };
+// System Theme Changes tests
+test('ThemeController listens to system theme changes', () => {
+ const mockMediaQuery = {
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQuery);
+
+ createController();
+
+ expect(mockMediaQuery.addEventListener).toHaveBeenCalledTimes(1);
+ expect(mockMediaQuery.addEventListener).toHaveBeenCalledWith(
+ 'change',
+ expect.any(Function),
+ );
+});
- controller = new ThemeController({
- themeObject: mockThemeObject,
- defaultTheme: lightThemeFallback,
- });
+test('ThemeController updates theme when system preference changes and mode is
SYSTEM', () => {
+ const mockMediaQuery = {
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQuery);
- // When only dark theme is available, controller uses the default fallback
theme initially
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ const controller = createController();
+ controller.setThemeMode(ThemeMode.SYSTEM);
- const calledWith = mockSetConfig.mock.calls[0][0];
+ mockMediaQuery.matches = true;
+ const changeHandler = mockMediaQuery.addEventListener.mock.calls[0][1];
- // Should use the default theme fallback (not dark) for initial load
- expect(calledWith.token.colorBgBase).toBe('#fff');
- expect(calledWith.token.colorTextBase).toBe('#000');
+ changeHandler();
- // Should allow mode changes since dark theme exists
- expect(controller.canSetMode()).toBe(true);
+ expect(mockSetConfig).toHaveBeenCalled();
+});
- // Should be able to switch to dark mode
- jest.clearAllMocks();
- controller.setThemeMode(ThemeMode.DARK);
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- });
+test('ThemeController does not update theme when mode is not SYSTEM', () => {
+ const mockMediaQuery = {
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQuery);
- test('should handle completely empty BootstrapData', () => {
- const fallbackTheme = {
- token: {
- colorBgBase: '#ffffff',
- colorPrimary: '#1890ff',
- },
- };
+ const controller = createController();
+ controller.setThemeMode(ThemeMode.DEFAULT);
+
+ const initialCallCount = mockSetConfig.mock.calls.length;
+
+ mockMediaQuery.matches = true;
+ const changeHandler = mockMediaQuery.addEventListener.mock.calls[0][1];
+
+ changeHandler();
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(initialCallCount);
+});
+
+test('ThemeController switches to dark theme when system is dark and mode is
SYSTEM', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: DARK_THEME,
+ }),
+ );
+
+ const mockMediaQueryDark = {
+ matches: true,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQueryDark);
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM);
+
+ expect(mockSetConfig).toHaveBeenCalled();
+ const lastCall =
+ mockSetConfig.mock.calls[mockSetConfig.mock.calls.length - 1][0];
+ expect(lastCall.token.colorBgBase).toBe(DARK_THEME.token!.colorBgBase);
+ expect(lastCall.token.colorTextBase).toBe(DARK_THEME.token!.colorTextBase);
+});
+
+// Persistence tests
+
+test('ThemeController saves theme mode to localStorage', () => {
+ const controller = createController();
+
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.DARK);
+
+ expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
+ 'superset-theme-mode',
+ ThemeMode.DARK,
+ );
+});
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: {},
- dark: {},
+test('ThemeController loads saved theme mode from localStorage', () => {
+ mockLocalStorage.getItem.mockReturnValue(ThemeMode.DARK);
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DARK);
+});
+
+test('ThemeController handles invalid saved theme mode', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: {},
+ }),
+ );
+
+ mockLocalStorage.getItem.mockReturnValue('invalid-mode' as any);
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+// Theme Structure tests
+
+test('ThemeController handles theme with token structure', () => {
+ const controller = createController();
+
+ const customTheme = {
+ token: {
+ colorBgBase: '#ff0000',
+ colorTextBase: '#ffffff',
+ colorPrimary: '#00ff00',
+ },
+ };
+
+ jest.clearAllMocks();
+
+ controller.setTheme(customTheme);
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ff0000',
+ colorTextBase: '#ffffff',
+ colorPrimary: '#00ff00',
}),
- );
-
- controller = new ThemeController({
- themeObject: mockThemeObject,
- defaultTheme: fallbackTheme,
- });
-
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- expect(mockSetConfig).toHaveBeenCalledWith(
- expect.objectContaining({
- token: expect.objectContaining({
- colorBgBase: '#ffffff',
- colorPrimary: '#1890ff',
- }),
- algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
+
+test('ThemeController preserves algorithm property from dark theme', () => {
+ const controller = createController();
+
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.DARK);
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ algorithm: antdThemeImport.darkAlgorithm,
+ token: expect.objectContaining({
+ colorBgBase: '#141118',
+ colorTextBase: '#fdc7c7',
}),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect test assertion</b></div>
<div id="fix">
The test assertion for 'ThemeController resets to default theme' incorrectly
expects dark theme colors when resetTheme() should apply default theme colors.
This could cause the test to fail or pass incorrectly, masking bugs in the
reset functionality.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
token: expect.objectContaining({
colorBgBase: '#ededed',
colorTextBase: '#120f0f',
}),
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36317#issuecomment-3587953569>#607d71</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
