fix(theme): remove stale dark root class in light mode#1960
Conversation
- Purpose: ensure light themes do not inherit a stale `dark` class on initial page load.\n- Before: theme bootstrap trusted any existing root/body `dark` class before reconciling against the authoritative DOM theme name.\n- Problem: a stale client-side class could survive into a light-theme render and keep Tailwind dark styles active.\n- Change: make `--theme-name` authoritative during store initialization and only fall back to class-based dark-mode bootstrap when no theme name is present.\n- Coverage: add a regression test that starts with a light DOM theme plus stale `dark` classes and verifies they are removed.
WalkthroughThis pull request refactors theme store initialization to read DOM-based theme configuration via CSS variables instead of computing dark mode status directly. A new helper function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1960 +/- ##
==========================================
+ Coverage 51.74% 51.95% +0.21%
==========================================
Files 1028 1030 +2
Lines 70792 71122 +330
Branches 7881 7933 +52
==========================================
+ Hits 36630 36951 +321
- Misses 34039 34048 +9
Partials 123 123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/store/theme.ts (1)
51-53: Normalize the theme name here and drop the cast.
mapPublicTheme()already lowercases server values, but this helper still compares raw strings and needs a type assertion to compile. If--theme-nameever arrives asBlack/Gray, the first-load bootstrap will miss the dark path. Normalizing once here lets you keep the check typed without the cast.♻️ Suggested change
-const isDarkThemeName = (themeName: string) => - DARK_UI_THEMES.includes(themeName as (typeof DARK_UI_THEMES)[number]); +const isDarkThemeName = (themeName: string) => { + const normalizedThemeName = themeName.trim().toLowerCase(); + return DARK_UI_THEMES.some((darkThemeName) => darkThemeName === normalizedThemeName); +};As per coding guidelines
Avoid using casting whenever possible, prefer proper typing from the start.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/store/theme.ts` around lines 51 - 53, The isDarkThemeName helper compares the raw themeName and uses a cast to satisfy types, which breaks when theme names vary in case; normalize themeName (e.g., toLowerCase) inside isDarkThemeName before checking DARK_UI_THEMES so the comparison is case-insensitive and you can remove the type assertion; ensure this matches the normalization done by mapPublicTheme and handles values coming from --theme-name like "Black"/"Gray" so first-load bootstrapping correctly detects dark themes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/__test__/store/theme.test.ts`:
- Around line 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.
---
Nitpick comments:
In `@web/src/store/theme.ts`:
- Around line 51-53: The isDarkThemeName helper compares the raw themeName and
uses a cast to satisfy types, which breaks when theme names vary in case;
normalize themeName (e.g., toLowerCase) inside isDarkThemeName before checking
DARK_UI_THEMES so the comparison is case-insensitive and you can remove the type
assertion; ensure this matches the normalization done by mapPublicTheme and
handles values coming from --theme-name like "Black"/"Gray" so first-load
bootstrapping correctly detects dark themes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 39e9ac9f-168d-4c16-9f72-2c88cde32290
📒 Files selected for processing (2)
web/__test__/store/theme.test.tsweb/src/store/theme.ts
| 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); | ||
| }); |
There was a problem hiding this comment.
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.
1 participant
Summary
darkclasses when the active theme is lightRoot cause
The frontend bootstrap path could trust a pre-existing
.darkclass before reconciling against the authoritative DOM theme name. If that class lingered on<html>from an earlier client-side mutation, a light theme could still boot into Tailwind dark mode.Validation
pnpm test __test__/store/theme.test.tspnpm test __test__/components/ThemeSwitcher.test.tsReference
Summary by CodeRabbit
Tests
Refactor