diff --git a/superset-frontend/packages/superset-ui-core/src/theme/types.ts b/superset-frontend/packages/superset-ui-core/src/theme/types.ts index f4ae4f64df2..7943b712a42 100644 --- a/superset-frontend/packages/superset-ui-core/src/theme/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/theme/types.ts @@ -402,7 +402,7 @@ export interface ThemeContextType { setTheme: (config: AnyThemeConfig) => void; setThemeMode: (newMode: ThemeMode) => void; resetTheme: () => void; - setTemporaryTheme: (config: AnyThemeConfig) => void; + setTemporaryTheme: (config: AnyThemeConfig, themeId?: number | null) => void; clearLocalOverrides: () => void; getCurrentCrudThemeId: () => string | null; hasDevOverride: () => boolean; @@ -410,6 +410,7 @@ export interface ThemeContextType { canSetTheme: () => boolean; canDetectOSPreference: () => boolean; createDashboardThemeProvider: (themeId: string) => Promise; + getAppliedThemeId: () => number | null; } /** diff --git a/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx b/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx index eb526467750..69151fcc462 100644 --- a/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx +++ b/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx @@ -62,6 +62,7 @@ jest.mock('src/views/CRUD/hooks', () => ({ // Mock the useThemeContext hook const mockSetTemporaryTheme = jest.fn(); +const mockGetAppliedThemeId = jest.fn(); jest.mock('src/theme/ThemeProvider', () => ({ ...jest.requireActual('src/theme/ThemeProvider'), useThemeContext: jest.fn(), @@ -141,10 +142,13 @@ beforeEach(() => { }); // Mock useThemeContext + mockGetAppliedThemeId.mockReturnValue(null); (useThemeContext as jest.Mock).mockReturnValue({ getCurrentCrudThemeId: jest.fn().mockReturnValue('1'), appliedTheme: { theme_name: 'Light Theme', id: 1 }, setTemporaryTheme: mockSetTemporaryTheme, + hasDevOverride: jest.fn().mockReturnValue(false), + getAppliedThemeId: mockGetAppliedThemeId, }); fetchMock.reset(); @@ -460,7 +464,7 @@ test('shows create theme button when user has permissions', async () => { expect(addButton).toBeInTheDocument(); }); -test('clicking apply button calls setTemporaryTheme with parsed theme data', async () => { +test('clicking apply button calls setTemporaryTheme with parsed theme data and ID', async () => { render( { - expect(mockSetTemporaryTheme).toHaveBeenCalledWith({ - colors: { primary: '#ffffff' }, - }); + expect(mockSetTemporaryTheme).toHaveBeenCalledWith( + { + colors: { primary: '#ffffff' }, + }, + 1, // theme ID + ); }); }); + +test('applying a local theme calls setTemporaryTheme with theme ID', async () => { + render( + , + { + useRedux: true, + useRouter: true, + useQueryParams: true, + useTheme: true, + }, + ); + + await screen.findByText('Custom Theme'); + + // Find and click the apply button for the first theme + const applyButtons = await screen.findAllByTestId('apply-action'); + await userEvent.click(applyButtons[0]); + + // Check that setTemporaryTheme was called with both theme config and ID + await waitFor(() => { + expect(mockSetTemporaryTheme).toHaveBeenCalledWith( + { colors: { primary: '#ffffff' } }, + 1, // theme ID + ); + }); +}); + +test('component loads successfully with applied theme ID set', async () => { + // This test verifies that having a stored theme ID doesn't break the component + // Mock hasDevOverride to return true since we have a dev override set + mockGetAppliedThemeId.mockReturnValue(1); + (useThemeContext as jest.Mock).mockReturnValue({ + getCurrentCrudThemeId: jest.fn().mockReturnValue('1'), + appliedTheme: { theme_name: 'Light Theme', id: 1 }, + setTemporaryTheme: mockSetTemporaryTheme, + hasDevOverride: jest.fn().mockReturnValue(true), + getAppliedThemeId: mockGetAppliedThemeId, + }); + + render( + , + { + useRedux: true, + useRouter: true, + useQueryParams: true, + useTheme: true, + }, + ); + + // Wait for list to load and verify it renders successfully + await screen.findByText('Custom Theme'); + + // Verify the component called getAppliedThemeId + expect(mockGetAppliedThemeId).toHaveBeenCalled(); +}); + +test('component loads successfully and preserves applied theme state', async () => { + // Mock hasDevOverride to return true and getAppliedThemeId to return a theme + mockGetAppliedThemeId.mockReturnValue(1); + (useThemeContext as jest.Mock).mockReturnValue({ + getCurrentCrudThemeId: jest.fn().mockReturnValue('1'), + appliedTheme: { theme_name: 'Light Theme', id: 1 }, + setTemporaryTheme: mockSetTemporaryTheme, + hasDevOverride: jest.fn().mockReturnValue(true), + getAppliedThemeId: mockGetAppliedThemeId, + }); + + render( + , + { + useRedux: true, + useRouter: true, + useQueryParams: true, + useTheme: true, + }, + ); + + // Wait for list to load + await screen.findByText('Custom Theme'); + + // Verify getAppliedThemeId is called during component mount + expect(mockGetAppliedThemeId).toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/pages/ThemeList/index.tsx b/superset-frontend/src/pages/ThemeList/index.tsx index adef9153a36..9e708fb2ff0 100644 --- a/superset-frontend/src/pages/ThemeList/index.tsx +++ b/superset-frontend/src/pages/ThemeList/index.tsx @@ -17,7 +17,7 @@ * under the License. */ -import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; import { t, SupersetClient, styled } from '@superset-ui/core'; import { Tag, @@ -110,15 +110,27 @@ function ThemesList({ refreshData, toggleBulkSelect, } = useListViewResource('theme', t('Themes'), addDangerToast); - const { setTemporaryTheme, getCurrentCrudThemeId } = useThemeContext(); + const { setTemporaryTheme, hasDevOverride, getAppliedThemeId } = + useThemeContext(); const [themeModalOpen, setThemeModalOpen] = useState(false); const [currentTheme, setCurrentTheme] = useState(null); const [preparingExport, setPreparingExport] = useState(false); const [importingTheme, showImportModal] = useState(false); - const [appliedThemeId, setAppliedThemeId] = useState(null); + const [appliedThemeId, setLocalAppliedThemeId] = useState( + null, + ); const { showConfirm, ConfirmModal } = useConfirmModal(); + useEffect(() => { + if (hasDevOverride()) { + const storedThemeId = getAppliedThemeId(); + setLocalAppliedThemeId(storedThemeId); + } else { + setLocalAppliedThemeId(null); + } + }, [hasDevOverride, getAppliedThemeId]); + const canCreate = hasPerm('can_write'); const canEdit = hasPerm('can_write'); const canDelete = hasPerm('can_write'); @@ -201,8 +213,11 @@ function ThemesList({ if (themeObj.json_data) { try { const themeConfig = JSON.parse(themeObj.json_data); - setTemporaryTheme(themeConfig); - setAppliedThemeId(themeObj.id || null); + const themeId = themeObj.id || null; + + setTemporaryTheme(themeConfig, themeId); + setLocalAppliedThemeId(themeId); + addSuccessToast(t('Local theme set to "%s"', themeObj.theme_name)); } catch (error) { addDangerToast( @@ -217,23 +232,26 @@ function ThemesList({ function handleThemeModalApply() { // Clear any previously applied theme ID when applying from modal // since the modal theme might not have an ID yet (unsaved theme) - setAppliedThemeId(null); + setLocalAppliedThemeId(null); } - const handleBulkThemeExport = async (themesToExport: ThemeObject[]) => { - const ids = themesToExport - .map(({ id }) => id) - .filter((id): id is number => id !== undefined); - setPreparingExport(true); - try { - await handleResourceExport('theme', ids, () => { + const handleBulkThemeExport = useCallback( + async (themesToExport: ThemeObject[]) => { + const ids = themesToExport + .map(({ id }) => id) + .filter((id): id is number => id !== undefined); + setPreparingExport(true); + try { + await handleResourceExport('theme', ids, () => { + setPreparingExport(false); + }); + } catch (error) { setPreparingExport(false); - }); - } catch (error) { - setPreparingExport(false); - addDangerToast(t('There was an issue exporting the selected themes')); - } - }; + addDangerToast(t('There was an issue exporting the selected themes')); + } + }, + [addDangerToast], + ); const openThemeImportModal = () => { showImportModal(true); @@ -346,11 +364,10 @@ function ThemesList({ () => [ { Cell: ({ row: { original } }: any) => { - const currentCrudThemeId = getCurrentCrudThemeId(); const isCurrentTheme = - (currentCrudThemeId && - original.id?.toString() === currentCrudThemeId) || - (appliedThemeId && original.id === appliedThemeId); + hasDevOverride() && + appliedThemeId && + original.id === appliedThemeId; return ( @@ -520,11 +537,12 @@ function ThemesList({ canDelete, canApply, canExport, - getCurrentCrudThemeId, + hasDevOverride, appliedThemeId, canSetSystemThemes, addDangerToast, handleThemeApply, + handleBulkThemeExport, handleSetSystemDefault, handleUnsetSystemDefault, handleSetSystemDark, diff --git a/superset-frontend/src/theme/ThemeController.ts b/superset-frontend/src/theme/ThemeController.ts index c4f297916d8..3cd4a1523bb 100644 --- a/superset-frontend/src/theme/ThemeController.ts +++ b/superset-frontend/src/theme/ThemeController.ts @@ -37,6 +37,7 @@ const STORAGE_KEYS = { THEME_MODE: 'superset-theme-mode', CRUD_THEME_ID: 'superset-crud-theme-id', DEV_THEME_OVERRIDE: 'superset-dev-theme-override', + APPLIED_THEME_ID: 'superset-applied-theme-id', } as const; const MEDIA_QUERY_DARK_SCHEME = '(prefers-color-scheme: dark)'; @@ -303,7 +304,12 @@ export class ThemeController { public setThemeMode(mode: ThemeMode): void { this.validateModeUpdatePermission(mode); - if (this.currentMode === mode) return; + if ( + this.currentMode === mode && + !this.devThemeOverride && + !this.crudThemeId + ) + return; // Clear any local overrides when explicitly selecting a theme mode // This ensures the selected mode takes effect and provides clear UX @@ -367,8 +373,12 @@ export class ThemeController { * Sets a temporary theme override for development purposes. * This does not persist the theme but allows live preview. * @param theme - The theme configuration to apply temporarily + * @param themeId - Optional theme ID to track which theme was applied (for UI display) */ - public setTemporaryTheme(theme: AnyThemeConfig): void { + public setTemporaryTheme( + theme: AnyThemeConfig, + themeId?: number | null, + ): void { this.validateThemeUpdatePermission(); this.devThemeOverride = theme; @@ -377,6 +387,11 @@ export class ThemeController { JSON.stringify(theme), ); + // Store the theme ID if provided + if (themeId !== undefined) { + this.setAppliedThemeId(themeId); + } + const mergedTheme = this.getThemeForMode(this.currentMode); if (mergedTheme) this.updateTheme(mergedTheme); } @@ -392,6 +407,7 @@ export class ThemeController { this.storage.removeItem(STORAGE_KEYS.DEV_THEME_OVERRIDE); this.storage.removeItem(STORAGE_KEYS.CRUD_THEME_ID); + this.storage.removeItem(STORAGE_KEYS.APPLIED_THEME_ID); // Clear dashboard themes cache this.dashboardThemes.clear(); @@ -413,6 +429,34 @@ export class ThemeController { return this.devThemeOverride !== null; } + /** + * Gets the applied theme ID (for UI display purposes). + */ + public getAppliedThemeId(): number | null { + try { + const storedId = this.storage.getItem(STORAGE_KEYS.APPLIED_THEME_ID); + return storedId ? parseInt(storedId, 10) : null; + } catch (error) { + console.warn('Failed to get applied theme ID:', error); + return null; + } + } + + /** + * Sets the applied theme ID (for UI display purposes). + */ + public setAppliedThemeId(themeId: number | null): void { + try { + if (themeId !== null) { + this.storage.setItem(STORAGE_KEYS.APPLIED_THEME_ID, themeId.toString()); + } else { + this.storage.removeItem(STORAGE_KEYS.APPLIED_THEME_ID); + } + } catch (error) { + console.warn('Failed to set applied theme ID:', error); + } + } + /** * Checks if OS preference detection is allowed. * Allowed when dark theme is available (including base dark theme) diff --git a/superset-frontend/src/theme/ThemeProvider.tsx b/superset-frontend/src/theme/ThemeProvider.tsx index 2ec902889be..0f3b8dd3a4f 100644 --- a/superset-frontend/src/theme/ThemeProvider.tsx +++ b/superset-frontend/src/theme/ThemeProvider.tsx @@ -117,6 +117,11 @@ export function SupersetThemeProvider({ [themeController], ); + const getAppliedThemeId = useCallback( + () => themeController.getAppliedThemeId(), + [themeController], + ); + const contextValue = useMemo( () => ({ theme: currentTheme, @@ -132,6 +137,7 @@ export function SupersetThemeProvider({ canSetTheme, canDetectOSPreference, createDashboardThemeProvider, + getAppliedThemeId, }), [ currentTheme, @@ -147,6 +153,7 @@ export function SupersetThemeProvider({ canSetTheme, canDetectOSPreference, createDashboardThemeProvider, + getAppliedThemeId, ], ); diff --git a/superset-frontend/src/theme/tests/ThemeController.test.ts b/superset-frontend/src/theme/tests/ThemeController.test.ts index 855ec10d27f..0b3a5a25b8b 100644 --- a/superset-frontend/src/theme/tests/ThemeController.test.ts +++ b/superset-frontend/src/theme/tests/ThemeController.test.ts @@ -1137,4 +1137,236 @@ describe('ThemeController', () => { ); }); }); + + test('setThemeMode clears dev override and crud theme from storage', () => { + mockGetBootstrapData.mockReturnValue( + createMockBootstrapData({ + default: DEFAULT_THEME, + dark: DARK_THEME, + }), + ); + + mockLocalStorage.getItem.mockReturnValue(null); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + // Simulate having overrides after initialization using Reflect to access private properties + Reflect.set(controller, 'devThemeOverride', { + token: { colorPrimary: '#ff0000' }, + }); + Reflect.set(controller, 'crudThemeId', '123'); + + jest.clearAllMocks(); + + // Change theme mode - should clear the overrides + controller.setThemeMode(ThemeMode.DARK); + + // Verify both storage keys were removed + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-dev-theme-override', + ); + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-crud-theme-id', + ); + }); + + test('setThemeMode can be called with same mode when overrides exist', () => { + mockGetBootstrapData.mockReturnValue( + createMockBootstrapData({ + default: DEFAULT_THEME, + dark: DARK_THEME, + }), + ); + + mockLocalStorage.getItem.mockReturnValue(null); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + jest.clearAllMocks(); + + // Simulate having dev override after initialization using Reflect + Reflect.set(controller, 'devThemeOverride', { + token: { colorPrimary: '#ff0000' }, + }); + + // Call setThemeMode with DEFAULT mode - should clear override + controller.setThemeMode(ThemeMode.DEFAULT); + + // Verify override was removed even though mode is the same + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-dev-theme-override', + ); + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-crud-theme-id', + ); + + // Theme should still be updated to clear the override + expect(mockSetConfig).toHaveBeenCalled(); + }); + + test('setThemeMode with no override and same mode does not trigger update', () => { + mockGetBootstrapData.mockReturnValue( + createMockBootstrapData({ + default: DEFAULT_THEME, + dark: DARK_THEME, + }), + ); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + // Set mode to DEFAULT + controller.setThemeMode(ThemeMode.DEFAULT); + + jest.clearAllMocks(); + + // Call again with same mode and no override - should skip + controller.setThemeMode(ThemeMode.DEFAULT); + + // Should not trigger any updates + expect(mockSetConfig).not.toHaveBeenCalled(); + expect(mockLocalStorage.removeItem).not.toHaveBeenCalled(); + }); + + test('hasDevOverride returns true when dev override is set', () => { + mockGetBootstrapData.mockReturnValue( + createMockBootstrapData({ + default: DEFAULT_THEME, + dark: DARK_THEME, + }), + ); + + mockLocalStorage.getItem.mockReturnValue(null); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + // Simulate dev override after initialization using Reflect + Reflect.set(controller, 'devThemeOverride', { + token: { colorPrimary: '#ff0000' }, + }); + + expect(controller.hasDevOverride()).toBe(true); + }); + + test('hasDevOverride returns false when no dev override in storage', () => { + mockGetBootstrapData.mockReturnValue( + createMockBootstrapData({ + default: DEFAULT_THEME, + dark: DARK_THEME, + }), + ); + + mockLocalStorage.getItem.mockReturnValue(null); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + expect(controller.hasDevOverride()).toBe(false); + }); + + test('clearLocalOverrides removes dev override, crud theme, and applied theme ID', () => { + mockGetBootstrapData.mockReturnValue( + createMockBootstrapData({ + default: DEFAULT_THEME, + dark: DARK_THEME, + }), + ); + + mockLocalStorage.getItem.mockReturnValue(null); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + jest.clearAllMocks(); + + // Clear overrides + controller.clearLocalOverrides(); + + // Verify all storage keys are removed + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-dev-theme-override', + ); + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-crud-theme-id', + ); + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-applied-theme-id', + ); + + // Should reset to default theme + expect(mockSetConfig).toHaveBeenCalled(); + }); + + test('getAppliedThemeId returns stored theme ID', () => { + mockLocalStorage.getItem.mockImplementation((key: string) => { + if (key === 'superset-applied-theme-id') { + return '42'; + } + return null; + }); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + expect(controller.getAppliedThemeId()).toBe(42); + }); + + test('getAppliedThemeId returns null when no theme ID is stored', () => { + mockLocalStorage.getItem.mockReturnValue(null); + + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + expect(controller.getAppliedThemeId()).toBeNull(); + }); + + test('setAppliedThemeId stores theme ID in storage', () => { + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + jest.clearAllMocks(); + + controller.setAppliedThemeId(123); + + expect(mockLocalStorage.setItem).toHaveBeenCalledWith( + 'superset-applied-theme-id', + '123', + ); + }); + + test('setAppliedThemeId removes theme ID when null is passed', () => { + const controller = new ThemeController({ + storage: mockLocalStorage, + themeObject: mockThemeObject, + }); + + jest.clearAllMocks(); + + controller.setAppliedThemeId(null); + + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-applied-theme-id', + ); + }); });