This is an automated email from the ASF dual-hosted git repository. diegopucci pushed a commit to branch geido/fix/local-label in repository https://gitbox.apache.org/repos/asf/superset.git
commit 504c368c17f62922f2385f45ee679ff74a72da2c Author: Diego Pucci <[email protected]> AuthorDate: Wed Oct 15 18:04:54 2025 +0300 fix(Themes): Local label inconsistent behaviors --- .../src/pages/ThemeList/ThemeList.test.tsx | 131 +++++++++----- superset-frontend/src/pages/ThemeList/index.tsx | 201 +++++++++++++-------- superset-frontend/src/theme/ThemeController.ts | 7 +- .../src/theme/tests/ThemeController.test.ts | 171 ++++++++++++++++++ 4 files changed, 392 insertions(+), 118 deletions(-) diff --git a/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx b/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx index d455cf7f78..bb4688d520 100644 --- a/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx +++ b/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx @@ -201,62 +201,109 @@ describe('ThemesList', () => { expect(addButton).toBeInTheDocument(); }); - // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks - describe('Modal.useModal integration', () => { - beforeEach(() => { - jest.clearAllMocks(); - }); + test('uses Modal.useModal hook instead of Modal.confirm', () => { + const useModalSpy = jest.spyOn(Modal, 'useModal'); + renderThemesList(); - test('uses Modal.useModal hook instead of Modal.confirm', () => { - const useModalSpy = jest.spyOn(Modal, 'useModal'); - renderThemesList(); + // Verify that useModal is called when the component mounts + expect(useModalSpy).toHaveBeenCalled(); - // Verify that useModal is called when the component mounts - expect(useModalSpy).toHaveBeenCalled(); + useModalSpy.mockRestore(); + }); - useModalSpy.mockRestore(); - }); + test('renders contextHolder for modal theming', async () => { + const { container } = renderThemesList(); + + // Wait for component to be rendered + await screen.findByText('Themes'); - test('renders contextHolder for modal theming', async () => { - const { container } = renderThemesList(); + // The contextHolder is rendered but invisible, so we check for its presence in the DOM + // Modal.useModal returns elements that get rendered in the component tree + const contextHolderExists = container.querySelector('.ant-modal-root'); + expect(contextHolderExists).toBeDefined(); + }); - // Wait for component to be rendered - await screen.findByText('Themes'); + test('sets up modal context for system theme confirmations', async () => { + fetchMock.post('glob:*/api/v1/theme/*/set_system_default', {}); - // The contextHolder is rendered but invisible, so we check for its presence in the DOM - // Modal.useModal returns elements that get rendered in the component tree - const contextHolderExists = container.querySelector('.ant-modal-root'); - expect(contextHolderExists).toBeDefined(); - }); + renderThemesList(); + + // Wait for list to load + await screen.findByTestId('themes-list-view'); - test('confirms system theme changes using themed modal', async () => { - const mockSetSystemDefault = jest.fn().mockResolvedValue({}); - fetchMock.post( - 'glob:*/api/v1/theme/*/set_system_default', - mockSetSystemDefault, - ); + // Verify the component renders without errors when modal context is available + // The actual modal functionality is tested through the Modal.useModal hook test + expect(screen.getByTestId('themes-list-view')).toBeInTheDocument(); + }); - renderThemesList(); + test('does not use deprecated Modal.confirm directly', () => { + // Create a spy on the static Modal.confirm method + const confirmSpy = jest.spyOn(Modal, 'confirm'); - // Wait for list to load - await screen.findByTestId('themes-list-view'); + renderThemesList(); - // Since the test data doesn't render actual action buttons, we'll verify - // that the modal system is properly set up by checking the hook was called - // This is validated in the "uses Modal.useModal hook" test - expect(true).toBe(true); - }); + // The component should not call Modal.confirm directly + expect(confirmSpy).not.toHaveBeenCalled(); - test('does not use deprecated Modal.confirm directly', () => { - // Create a spy on the static Modal.confirm method - const confirmSpy = jest.spyOn(Modal, 'confirm'); + confirmSpy.mockRestore(); + }); + + test('applying a local theme stores theme ID in localStorage', async () => { + // Clear localStorage before test + localStorage.clear(); - renderThemesList(); + renderThemesList(); + + // Wait for list to load + await screen.findByTestId('themes-list-view'); - // The component should not call Modal.confirm directly - expect(confirmSpy).not.toHaveBeenCalled(); + // Find and click the apply button for the first theme + const applyButtons = await screen.findAllByTestId('apply-action'); + fireEvent.click(applyButtons[0]); - confirmSpy.mockRestore(); + // Check that localStorage was updated + await waitFor(() => { + expect(localStorage.getItem('superset-applied-theme-id')).toBe('1'); }); + + // Cleanup + localStorage.clear(); + }); + + test('component loads successfully with localStorage theme ID set', async () => { + // This test verifies that having a stored theme ID doesn't break the component + localStorage.setItem('superset-applied-theme-id', '1'); + localStorage.setItem( + 'superset-dev-theme-override', + JSON.stringify({ token: { colorPrimary: '#1890ff' } }), + ); + + renderThemesList(); + + // Wait for list to load and verify it renders successfully + const listView = await screen.findByTestId('themes-list-view'); + expect(listView).toBeInTheDocument(); + + // Verify the component can read localStorage without errors + expect(localStorage.getItem('superset-applied-theme-id')).toBe('1'); + + // Cleanup + localStorage.clear(); + }); + + test('component loads successfully and preserves localStorage state', async () => { + // Set initial state to verify localStorage isn't corrupted + localStorage.setItem('superset-applied-theme-id', '1'); + + renderThemesList(); + + // Wait for list to load + await screen.findByTestId('themes-list-view'); + + // Verify localStorage is preserved during component mount + expect(localStorage.getItem('superset-applied-theme-id')).toBe('1'); + + // Cleanup + localStorage.clear(); }); }); diff --git a/superset-frontend/src/pages/ThemeList/index.tsx b/superset-frontend/src/pages/ThemeList/index.tsx index 75755170b9..df6539c7c8 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,7 +110,7 @@ function ThemesList({ refreshData, toggleBulkSelect, } = useListViewResource<ThemeObject>('theme', t('Themes'), addDangerToast); - const { setTemporaryTheme, getCurrentCrudThemeId } = useThemeContext(); + const { setTemporaryTheme, hasDevOverride } = useThemeContext(); const [themeModalOpen, setThemeModalOpen] = useState<boolean>(false); const [currentTheme, setCurrentTheme] = useState<ThemeObject | null>(null); const [preparingExport, setPreparingExport] = useState<boolean>(false); @@ -120,6 +120,30 @@ function ThemesList({ // Use Modal.useModal hook to ensure proper theming const [modal, contextHolder] = Modal.useModal(); + const clearAppliedTheme = useCallback(() => { + setAppliedThemeId(null); + try { + localStorage.removeItem('superset-applied-theme-id'); + } catch (error) { + // Ignore localStorage errors + } + }, []); + + useEffect(() => { + if (hasDevOverride()) { + try { + const storedThemeId = localStorage.getItem('superset-applied-theme-id'); + if (storedThemeId) { + setAppliedThemeId(parseInt(storedThemeId, 10)); + } + } catch (error) { + clearAppliedTheme(); + } + } else { + clearAppliedTheme(); + } + }, [clearAppliedTheme, hasDevOverride]); + const canCreate = hasPerm('can_write'); const canEdit = hasPerm('can_write'); const canDelete = hasPerm('can_write'); @@ -202,8 +226,19 @@ function ThemesList({ if (themeObj.json_data) { try { const themeConfig = JSON.parse(themeObj.json_data); + const themeId = themeObj.id || null; + setTemporaryTheme(themeConfig); - setAppliedThemeId(themeObj.id || null); + setAppliedThemeId(themeId); + + if (themeId) { + localStorage.setItem( + 'superset-applied-theme-id', + themeId.toString(), + ); + } else { + localStorage.removeItem('superset-applied-theme-id'); + } addSuccessToast(t('Local theme set to "%s"', themeObj.theme_name)); } catch (error) { addDangerToast( @@ -219,22 +254,26 @@ function ThemesList({ // Clear any previously applied theme ID when applying from modal // since the modal theme might not have an ID yet (unsaved theme) setAppliedThemeId(null); + localStorage.removeItem('superset-applied-theme-id'); } - 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); @@ -251,60 +290,72 @@ function ThemesList({ }; // Generic confirmation modal utility to reduce code duplication - const showThemeConfirmation = (config: { - title: string; - content: string; - onConfirm: () => Promise<any>; - successMessage: string; - errorMessage: string; - }) => { - modal.confirm({ - title: config.title, - content: config.content, - onOk: () => { - config - .onConfirm() - .then(() => { - refreshData(); - addSuccessToast(config.successMessage); - }) - .catch(err => { - addDangerToast(t(config.errorMessage, err.message)); - }); - }, - }); - }; + const showThemeConfirmation = useCallback( + (config: { + title: string; + content: string; + onConfirm: () => Promise<any>; + successMessage: string; + errorMessage: string; + }) => { + modal.confirm({ + title: config.title, + content: config.content, + onOk: () => { + config + .onConfirm() + .then(() => { + refreshData(); + addSuccessToast(config.successMessage); + }) + .catch(err => { + addDangerToast(t(config.errorMessage, err.message)); + }); + }, + }); + }, + [modal, refreshData, addSuccessToast, addDangerToast], + ); - const handleSetSystemDefault = (theme: ThemeObject) => { - showThemeConfirmation({ - title: t('Set System Default Theme'), - content: t( - 'Are you sure you want to set "%s" as the system default theme? This will apply to all users who haven\'t set a personal preference.', - theme.theme_name, - ), - onConfirm: () => setSystemDefaultTheme(theme.id!), - successMessage: t( - '"%s" is now the system default theme', - theme.theme_name, - ), - errorMessage: 'Failed to set system default theme: %s', - }); - }; + const handleSetSystemDefault = useCallback( + (theme: ThemeObject) => { + showThemeConfirmation({ + title: t('Set System Default Theme'), + content: t( + 'Are you sure you want to set "%s" as the system default theme? This will apply to all users who haven\'t set a personal preference.', + theme.theme_name, + ), + onConfirm: () => setSystemDefaultTheme(theme.id!), + successMessage: t( + '"%s" is now the system default theme', + theme.theme_name, + ), + errorMessage: 'Failed to set system default theme: %s', + }); + }, + [showThemeConfirmation], + ); - const handleSetSystemDark = (theme: ThemeObject) => { - showThemeConfirmation({ - title: t('Set System Dark Theme'), - content: t( - 'Are you sure you want to set "%s" as the system dark theme? This will apply to all users who haven\'t set a personal preference.', - theme.theme_name, - ), - onConfirm: () => setSystemDarkTheme(theme.id!), - successMessage: t('"%s" is now the system dark theme', theme.theme_name), - errorMessage: 'Failed to set system dark theme: %s', - }); - }; + const handleSetSystemDark = useCallback( + (theme: ThemeObject) => { + showThemeConfirmation({ + title: t('Set System Dark Theme'), + content: t( + 'Are you sure you want to set "%s" as the system dark theme? This will apply to all users who haven\'t set a personal preference.', + theme.theme_name, + ), + onConfirm: () => setSystemDarkTheme(theme.id!), + successMessage: t( + '"%s" is now the system dark theme', + theme.theme_name, + ), + errorMessage: 'Failed to set system dark theme: %s', + }); + }, + [showThemeConfirmation], + ); - const handleUnsetSystemDefault = () => { + const handleUnsetSystemDefault = useCallback(() => { showThemeConfirmation({ title: t('Remove System Default Theme'), content: t( @@ -314,9 +365,9 @@ function ThemesList({ successMessage: t('System default theme removed'), errorMessage: 'Failed to remove system default theme: %s', }); - }; + }, [showThemeConfirmation]); - const handleUnsetSystemDark = () => { + const handleUnsetSystemDark = useCallback(() => { showThemeConfirmation({ title: t('Remove System Dark Theme'), content: t( @@ -326,18 +377,17 @@ function ThemesList({ successMessage: t('System dark theme removed'), errorMessage: 'Failed to remove system dark theme: %s', }); - }; + }, [showThemeConfirmation]); const initialSort = [{ id: 'theme_name', desc: true }]; const columns = useMemo( () => [ { Cell: ({ row: { original } }: any) => { - const currentCrudThemeId = getCurrentCrudThemeId(); const isCurrentTheme = - (currentCrudThemeId && - original.id?.toString() === currentCrudThemeId) || - (appliedThemeId && original.id === appliedThemeId); + hasDevOverride() && + appliedThemeId && + original.id === appliedThemeId; return ( <FlexRowContainer> @@ -507,11 +557,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 c4f297916d..6e77e32893 100644 --- a/superset-frontend/src/theme/ThemeController.ts +++ b/superset-frontend/src/theme/ThemeController.ts @@ -303,7 +303,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 diff --git a/superset-frontend/src/theme/tests/ThemeController.test.ts b/superset-frontend/src/theme/tests/ThemeController.test.ts index 855ec10d27..57d82757e0 100644 --- a/superset-frontend/src/theme/tests/ThemeController.test.ts +++ b/superset-frontend/src/theme/tests/ThemeController.test.ts @@ -1137,4 +1137,175 @@ 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 both dev override and crud theme', () => { + 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 both storage keys are removed + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-dev-theme-override', + ); + expect(mockLocalStorage.removeItem).toHaveBeenCalledWith( + 'superset-crud-theme-id', + ); + + // Should reset to default theme + expect(mockSetConfig).toHaveBeenCalled(); + }); });
