Skip to content

fix(theme): remove stale dark root class in light mode #1960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
elibosley merged 1 commit into from
Mar 24, 2026
Merged

fix(theme): remove stale dark root class in light mode #1960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions web/__test__/store/theme.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,5 +286,17 @@ describe('Theme Store', () => {

vi.restoreAllMocks();
});

it('should remove stale dark classes when the DOM theme is light', () => {
document.documentElement.style.setProperty('--theme-name', 'white');
originalDocumentElementAddClass.call(document.documentElement.classList, 'dark');
originalAddClassFn.call(document.body.classList, 'dark');

const store = createStore();

expect(document.documentElement.classList.remove).toHaveBeenCalledWith('dark');
expect(document.body.classList.remove).toHaveBeenCalledWith('dark');
expect(store.darkMode).toBe(false);
});
Comment on lines +290 to +300
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Assert the final DOM state instead of the mocked remove() calls.

In this file, classList.remove is stubbed to vi.fn() in beforeEach, so these expectations can pass while dark is still present on <html> and <body>. Restoring the real remove methods for this case (or not stubbing them) and asserting contains('dark') === false will make this a real regression test instead of an implementation-detail check.

💡 Suggested change 
   it('should remove stale dark classes when the DOM theme is light', () => {
+    document.documentElement.classList.remove = originalDocumentElementRemoveClass;
+    document.body.classList.remove = originalRemoveClassFn;
     document.documentElement.style.setProperty('--theme-name', 'white');
     originalDocumentElementAddClass.call(document.documentElement.classList, 'dark');
     originalAddClassFn.call(document.body.classList, 'dark');
 
     const store = createStore();
 
-    expect(document.documentElement.classList.remove).toHaveBeenCalledWith('dark');
-    expect(document.body.classList.remove).toHaveBeenCalledWith('dark');
+    expect(document.documentElement.classList.contains('dark')).toBe(false);
+    expect(document.body.classList.contains('dark')).toBe(false);
     expect(store.darkMode).toBe(false);
   });

As per coding guidelines Test what the code does, not implementation details like exact error message wording.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@web/__test__/store/theme.test.ts` around lines 290 - 300, The test "should
remove stale dark classes when the DOM theme is light" is asserting mocked
classList.remove calls (which are stubbed in beforeEach) instead of verifying
the actual DOM state; update the test to restore or avoid stubbing
document.documentElement.classList.remove and document.body.classList.remove for
this case (or temporarily replace them with the real methods) before calling
createStore(), then assert that
document.documentElement.classList.contains('dark') and
document.body.classList.contains('dark') are false and that store.darkMode is
false; reference the existing test block and the createStore() invocation to
locate where to adjust the stubbing/restore.

});
});
22 changes: 11 additions & 11 deletions web/src/store/theme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ const getCssVar = (name: string): string => {

const readDomThemeName = () => getCssVar('--theme-name');

const isDarkThemeName = (themeName: string) =>
DARK_UI_THEMES.includes(themeName as (typeof DARK_UI_THEMES)[number]);

const syncDarkClass = (method: 'add' | 'remove') => {
if (!isDomAvailable()) return;
document.documentElement.classList[method]('dark');
Expand Down Expand Up @@ -98,12 +101,6 @@ export const useThemeStore = defineStore('theme', () => {
const devOverride = ref(false);
const darkMode = ref<boolean>(false);

// Initialize dark mode from CSS variable set by PHP or any pre-applied .dark class
if (isDomAvailable()) {
darkMode.value = isDarkModeActive();
bootstrapDarkClass(darkMode);
}

// Lazy query - only executes when explicitly called
const { load, onResult, onError } = useLazyQuery<GetThemeQuery>(GET_THEME_QUERY, null, {
fetchPolicy: 'cache-and-network',
Expand Down Expand Up @@ -208,16 +205,19 @@ export const useThemeStore = defineStore('theme', () => {
watch(
() => theme.value.name,
(themeName) => {
const isDark = DARK_UI_THEMES.includes(themeName as (typeof DARK_UI_THEMES)[number]);
applyDarkClass(isDark, darkMode);
applyDarkClass(isDarkThemeName(themeName), darkMode);
},
{ immediate: false }
);

// Initialize theme from DOM on store creation
const domThemeName = themeName.value;
if (domThemeName && domThemeName !== DEFAULT_THEME.name) {
theme.value.name = domThemeName;
const domThemeName = readDomThemeName();
if (domThemeName) {
setTheme({ name: domThemeName });
applyDarkClass(isDarkThemeName(domThemeName), darkMode);
} else if (isDomAvailable()) {
darkMode.value = isDarkModeActive();
bootstrapDarkClass(darkMode);
}

return {
Expand Down
Loading