fix(Themes): Local label inconsistent behaviors (#35663)

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<Theme | null>;
+  getAppliedThemeId: () => number | null;
 }
 
 /**

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(
     <ThemesList
       user={mockUser}
@@ -483,8 +487,106 @@ test('clicking apply button calls setTemporaryTheme with parsed theme data', asy
   await userEvent.click(applyButtons[0]);
 
   await waitFor(() => {
-    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(
+    <ThemesList
+      user={mockUser}
+      addDangerToast={jest.fn()}
+      addSuccessToast={jest.fn()}
+    />,
+    {
+      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(
+    <ThemesList
+      user={mockUser}
+      addDangerToast={jest.fn()}
+      addSuccessToast={jest.fn()}
+    />,
+    {
+      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(
+    <ThemesList
+      user={mockUser}
+      addDangerToast={jest.fn()}
+      addSuccessToast={jest.fn()}
+    />,
+    {
+      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();
+});

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<ThemeObject>('theme', t('Themes'), addDangerToast);
-  const { setTemporaryTheme, getCurrentCrudThemeId } = useThemeContext();
+  const { setTemporaryTheme, hasDevOverride, getAppliedThemeId } =
+    useThemeContext();
   const [themeModalOpen, setThemeModalOpen] = useState<boolean>(false);
   const [currentTheme, setCurrentTheme] = useState<ThemeObject | null>(null);
   const [preparingExport, setPreparingExport] = useState<boolean>(false);
   const [importingTheme, showImportModal] = useState<boolean>(false);
-  const [appliedThemeId, setAppliedThemeId] = useState<number | null>(null);
+  const [appliedThemeId, setLocalAppliedThemeId] = useState<number | null>(
+    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 (
             <FlexRowContainer>
@@ -520,11 +537,12 @@ function ThemesList({
       canDelete,
       canApply,
       canExport,
-      getCurrentCrudThemeId,
+      hasDevOverride,
       appliedThemeId,
       canSetSystemThemes,
       addDangerToast,
       handleThemeApply,
+      handleBulkThemeExport,
       handleSetSystemDefault,
       handleUnsetSystemDefault,
       handleSetSystemDark,

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)

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,
     ],
   );
 

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',
+    );
+  });
 });