sadpandajoe commented on code in PR #37378:
URL: https://github.com/apache/superset/pull/37378#discussion_r2775437091
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -143,8 +143,26 @@ export class ThemeController {
// Setup change callback
if (onChange) this.onChangeCallbacks.add(onChange);
- // Apply initial theme and persist mode
- this.applyTheme(initialTheme);
+ // Apply initial theme with recovery for corrupted stored themes
+ try {
+ this.applyTheme(initialTheme);
+ } catch (error) {
+ // Corrupted dev override or CRUD theme in storage - clear and retry
with defaults
+ console.warn(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ error,
+ );
+ this.devThemeOverride = null;
+ this.crudThemeId = null;
+ this.storage.removeItem(STORAGE_KEYS.DEV_THEME_OVERRIDE);
+ this.storage.removeItem(STORAGE_KEYS.CRUD_THEME_ID);
+ this.storage.removeItem(STORAGE_KEYS.APPLIED_THEME_ID);
+
+ // Retry with clean default theme
+ this.currentMode = ThemeMode.DEFAULT;
+ const safeTheme = this.defaultTheme || {};
+ this.applyTheme(safeTheme);
+ }
this.persistMode();
}
Review Comment:
This is **by design**. The `onChange` callback is intended for changes that
occur *after* construction, not initial state.
Consumers typically:
1. Construct the controller
2. Register callbacks via `onChange()`
3. Callbacks fire on subsequent theme changes
If a callback is passed via constructor options (`{ onChange: fn }`), it's
added to the Set before `applyTheme`, but notifying during construction would
be inconsistent - the theme hasn't "changed", it's being initialized.
If initial state notification is needed, consumers can call
`controller.getTheme()` immediately after construction.
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -796,10 +821,25 @@ export class ThemeController {
this.loadFonts(fontUrls);
} catch (error) {
console.error('Failed to apply theme:', error);
- this.fallbackToDefaultMode();
+ // Re-throw the error so updateTheme can handle fallback logic
+ throw error;
}
}
+ private async applyThemeWithRecovery(theme: AnyThemeConfig): Promise<void> {
+ // Note: This method re-throws errors to the caller instead of calling
+ // fallbackToDefaultMode directly, to avoid infinite recursion since
+ // fallbackToDefaultMode calls this method. The caller's try/catch
+ // handles the fallback flow.
+ const normalizedConfig = normalizeThemeConfig(theme);
+ this.globalTheme.setConfig(normalizedConfig);
+
+ // Load custom fonts if specified, mirroring applyTheme() behavior
+ const fontUrls = (normalizedConfig?.token as Record<string, unknown>)
+ ?.fontUrls as string[] | undefined;
+ this.loadFonts(fontUrls);
Review Comment:
The current code already logs errors at the point of failure.
`applyThemeWithRecovery` re-throws errors, and the caller
(`fallbackToDefaultMode`) catches them. Adding logging in
`applyThemeWithRecovery` would duplicate the error logging that happens in
`applyTheme` (line 832: `console.error('Failed to apply theme:', error)`).
The error flow is: `applyThemeWithRecovery` throws → `fallbackToDefaultMode`
catches → falls through to cached default.
##########
superset-frontend/src/theme/tests/ThemeController.test.ts:
##########
@@ -833,6 +833,191 @@ test('ThemeController handles theme application errors',
() => {
fallbackSpy.mockRestore();
});
+test('ThemeController constructor recovers from corrupted stored theme', () =>
{
+ // Simulate corrupted dev theme override in storage
+ const corruptedTheme = { token: { colorPrimary: '#ff0000' } };
+ mockLocalStorage.getItem.mockImplementation((key: string) => {
+ if (key === 'superset-dev-theme-override') {
+ return JSON.stringify(corruptedTheme);
+ }
+ return null;
+ });
+
+ // Mock Theme.fromConfig to return object with toSerializedConfig
+ mockThemeFromConfig.mockReturnValue({
+ ...mockThemeObject,
+ toSerializedConfig: () => corruptedTheme,
+ });
+
+ // First call throws (corrupted theme), second call succeeds (fallback)
+ let callCount = 0;
+ mockSetConfig.mockImplementation(() => {
+ callCount += 1;
+ if (callCount === 1) {
+ throw new Error('Invalid theme configuration');
+ }
+ });
+
+ const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+ // Should not throw - constructor should recover
+ const controller = createController();
+
+ // Verify recovery happened
+ expect(consoleWarnSpy).toHaveBeenCalledWith(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ expect.any(Error),
+ );
+
+ // Verify invalid overrides were cleared from storage
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-dev-theme-override',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-crud-theme-id',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-applied-theme-id',
+ );
+
+ // Verify controller is in a valid state
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+
+ consoleWarnSpy.mockRestore();
+});
+
+test('recovery flow: fetchSystemDefaultTheme returns theme → applies fetched
theme', async () => {
+ // Test: fallbackToDefaultMode fetches theme from API and applies it
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery
+
+ const controller = createController();
+
+ // Mock fetch to return a system default theme from API
+ const systemTheme = { token: { colorPrimary: '#recovery-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // Track setConfig calls to verify the fetched theme is applied
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode (simulates what happens after applyTheme
fails)
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify API was called to fetch system default theme
+ expect(mockFetch).toHaveBeenCalledWith(
+ expect.stringContaining('/api/v1/theme/'),
+ );
+
+ // Verify the fetched theme was applied via applyThemeWithRecovery
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#recovery-theme' }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
Review Comment:
The tests **do restore** `global.fetch`. See the try/finally pattern at
lines 893-933:
```typescript
const originalFetch = global.fetch;
// ...
try {
global.fetch = mockFetch;
// ... test code ...
} finally {
global.fetch = originalFetch; // ← Restored here
}
```
The `finally` block ensures restoration even if the test throws.
##########
superset-frontend/src/theme/tests/ThemeController.test.ts:
##########
@@ -833,6 +833,191 @@ test('ThemeController handles theme application errors',
() => {
fallbackSpy.mockRestore();
});
+test('ThemeController constructor recovers from corrupted stored theme', () =>
{
+ // Simulate corrupted dev theme override in storage
+ const corruptedTheme = { token: { colorPrimary: '#ff0000' } };
+ mockLocalStorage.getItem.mockImplementation((key: string) => {
+ if (key === 'superset-dev-theme-override') {
+ return JSON.stringify(corruptedTheme);
+ }
+ return null;
+ });
+
+ // Mock Theme.fromConfig to return object with toSerializedConfig
+ mockThemeFromConfig.mockReturnValue({
+ ...mockThemeObject,
+ toSerializedConfig: () => corruptedTheme,
+ });
+
+ // First call throws (corrupted theme), second call succeeds (fallback)
+ let callCount = 0;
+ mockSetConfig.mockImplementation(() => {
+ callCount += 1;
+ if (callCount === 1) {
+ throw new Error('Invalid theme configuration');
+ }
+ });
+
+ const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+ // Should not throw - constructor should recover
+ const controller = createController();
+
+ // Verify recovery happened
+ expect(consoleWarnSpy).toHaveBeenCalledWith(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ expect.any(Error),
+ );
+
+ // Verify invalid overrides were cleared from storage
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-dev-theme-override',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-crud-theme-id',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-applied-theme-id',
+ );
+
+ // Verify controller is in a valid state
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+
+ consoleWarnSpy.mockRestore();
+});
+
+test('recovery flow: fetchSystemDefaultTheme returns theme → applies fetched
theme', async () => {
+ // Test: fallbackToDefaultMode fetches theme from API and applies it
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery
+
+ const controller = createController();
+
+ // Mock fetch to return a system default theme from API
+ const systemTheme = { token: { colorPrimary: '#recovery-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // Track setConfig calls to verify the fetched theme is applied
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode (simulates what happens after applyTheme
fails)
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify API was called to fetch system default theme
+ expect(mockFetch).toHaveBeenCalledWith(
+ expect.stringContaining('/api/v1/theme/'),
+ );
+
+ // Verify the fetched theme was applied via applyThemeWithRecovery
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#recovery-theme' }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+test('recovery flow: both API fetches fail → falls back to cached default
theme', async () => {
+ // Test: When fetchSystemDefaultTheme fails, fallbackToDefaultMode uses
cached theme
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme (fails) →
applyTheme(cached)
+
+ const controller = createController();
+
+ // Mock fetch to fail for both API endpoints
+ const mockFetch = jest.fn().mockRejectedValue(new Error('Network error'));
+ global.fetch = mockFetch;
+
+ // Track setConfig calls
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify fetch was attempted
+ expect(mockFetch).toHaveBeenCalled();
+
+ // Verify fallback to cached default theme was applied via applyTheme
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed', // From DEFAULT_THEME in test setup
+ }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
Review Comment:
Same as above - the test uses try/finally to restore `global.fetch`. See
lines 940-974.
##########
superset-frontend/src/theme/tests/ThemeController.test.ts:
##########
@@ -833,6 +833,191 @@ test('ThemeController handles theme application errors',
() => {
fallbackSpy.mockRestore();
});
+test('ThemeController constructor recovers from corrupted stored theme', () =>
{
+ // Simulate corrupted dev theme override in storage
+ const corruptedTheme = { token: { colorPrimary: '#ff0000' } };
+ mockLocalStorage.getItem.mockImplementation((key: string) => {
+ if (key === 'superset-dev-theme-override') {
+ return JSON.stringify(corruptedTheme);
+ }
+ return null;
+ });
+
+ // Mock Theme.fromConfig to return object with toSerializedConfig
+ mockThemeFromConfig.mockReturnValue({
+ ...mockThemeObject,
+ toSerializedConfig: () => corruptedTheme,
+ });
+
+ // First call throws (corrupted theme), second call succeeds (fallback)
+ let callCount = 0;
+ mockSetConfig.mockImplementation(() => {
+ callCount += 1;
+ if (callCount === 1) {
+ throw new Error('Invalid theme configuration');
+ }
+ });
+
+ const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+ // Should not throw - constructor should recover
+ const controller = createController();
+
+ // Verify recovery happened
+ expect(consoleWarnSpy).toHaveBeenCalledWith(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ expect.any(Error),
+ );
+
+ // Verify invalid overrides were cleared from storage
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-dev-theme-override',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-crud-theme-id',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-applied-theme-id',
+ );
+
+ // Verify controller is in a valid state
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+
+ consoleWarnSpy.mockRestore();
+});
+
+test('recovery flow: fetchSystemDefaultTheme returns theme → applies fetched
theme', async () => {
+ // Test: fallbackToDefaultMode fetches theme from API and applies it
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery
+
+ const controller = createController();
+
+ // Mock fetch to return a system default theme from API
+ const systemTheme = { token: { colorPrimary: '#recovery-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // Track setConfig calls to verify the fetched theme is applied
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode (simulates what happens after applyTheme
fails)
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify API was called to fetch system default theme
+ expect(mockFetch).toHaveBeenCalledWith(
+ expect.stringContaining('/api/v1/theme/'),
+ );
+
+ // Verify the fetched theme was applied via applyThemeWithRecovery
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#recovery-theme' }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+test('recovery flow: both API fetches fail → falls back to cached default
theme', async () => {
+ // Test: When fetchSystemDefaultTheme fails, fallbackToDefaultMode uses
cached theme
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme (fails) →
applyTheme(cached)
+
+ const controller = createController();
+
+ // Mock fetch to fail for both API endpoints
+ const mockFetch = jest.fn().mockRejectedValue(new Error('Network error'));
+ global.fetch = mockFetch;
+
+ // Track setConfig calls
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify fetch was attempted
+ expect(mockFetch).toHaveBeenCalled();
+
+ // Verify fallback to cached default theme was applied via applyTheme
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed', // From DEFAULT_THEME in test setup
+ }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+test('recovery flow: fetched theme fails to apply → falls back to cached
default', async () => {
+ // Test: When applyThemeWithRecovery fails, fallbackToDefaultMode uses
cached theme
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery (fails) → applyTheme(cached)
+
+ const controller = createController();
+
+ // Mock fetch to return a theme
+ const systemTheme = { token: { colorPrimary: '#bad-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // First setConfig call (applyThemeWithRecovery) fails, second (applyTheme)
succeeds
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ if (setConfigCalls.length === 1) {
+ throw new Error('Fetched theme failed to apply');
+ }
+ });
+
+ // Trigger fallbackToDefaultMode
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify fetch was called
+ expect(mockFetch).toHaveBeenCalled();
+
+ // Verify both attempts were made: fetched theme (failed) then cached default
+ expect(setConfigCalls.length).toBe(2);
+
+ // First call was the fetched theme (which failed)
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#bad-theme' }),
+ }),
+ );
+
+ // Second call was the cached default theme
+ expect(setConfigCalls[1]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed', // From DEFAULT_THEME
+ }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
Review Comment:
Same as above - the test uses try/finally to restore `global.fetch`. See
lines 981-1033.
--
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]
