feat: dark mode — CSS tokens, theme toggle, no-flash init - #2
Conversation
- Introduce [data-theme="dark"] CSS token overrides for all colour variables - Add new tokens (--img-bg, --bg-blue-tint, --bg-green-tint, --bg-error, --border-error) and replace hard-coded hex colours in .device-card img, .device-head img, .fallback, .release-notes, and .error-box - Add .theme-btn styles for the header toggle button - Add no-flash inline script in <head> that sets data-theme before first paint - Add #theme-toggle button to header nav (System ⬤ / Dark 🌙 / Light ☀) - New js/theme.js: 3-way cycle (system → dark → light), localStorage persistence, and live media-query listener for OS preference changes - Wire initThemeToggle() into js/app.js
WalkthroughAdds a light/dark/system theme toggle: a new ChangesTheme Toggle Feature
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant IndexHTML as index.html (boot script)
participant AppJS as app.js
participant ThemeJS as theme.js
participant DOM as document.documentElement
Browser->>IndexHTML: Load page
IndexHTML->>DOM: Read localStorage.theme / prefers-color-scheme
IndexHTML->>DOM: Set data-theme="dark" (if applicable)
Browser->>AppJS: Execute app startup
AppJS->>ThemeJS: initThemeToggle()
ThemeJS->>DOM: Find `#theme-toggle` button
ThemeJS->>ThemeJS: Load saved preference
ThemeJS->>DOM: Apply theme (set/remove data-theme)
ThemeJS->>ThemeJS: Register matchMedia change listener
Browser->>ThemeJS: Click `#theme-toggle`
ThemeJS->>ThemeJS: Cycle to next theme
ThemeJS->>DOM: Persist and re-apply theme
ThemeJS->>DOM: Update button icon/aria-label/title
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
js/theme.js (2)
35-35: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
matchMediais unguarded intheme.jswhile the boot script wraps it in try/catch.If
matchMediathrows (e.g., in a restricted environment),initThemeTogglewill abort at line 35 before the button is made visible, leaving the toggle permanently hidden. The boot script already handles this gracefully. Consider wrapping thematchMediacall or providing a fallback.Also applies to: 26-26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/theme.js` at line 35, `initThemeToggle` in `theme.js` calls `matchMedia('(prefers-color-scheme: dark)')` without protection, so mirror the boot script’s defensive handling here. Wrap the `matchMedia` lookup in try/catch or add a safe fallback so `initThemeToggle` can continue and still reveal the toggle button even when `matchMedia` is unavailable or throws. Apply the same fix anywhere else this unguarded `matchMedia` usage appears in the theme setup flow.
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoot script and
applyPrefduplicate dark-detection logic.The inline script in
index.html(lines 9–18) andapplyPrefhere independently implement the same dark-mode decision tree. While the inline script can't import from this module (it runs before module evaluation), the two copies can silently drift if one is updated without the other. Consider adding a brief comment in both locations noting the contract they must keep in sync.Also applies to: 9-18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/theme.js` around lines 22 - 29, The dark-mode detection logic is duplicated between the inline boot script and applyPref, so add a short sync-contract comment in both places to keep their behavior aligned. Update the applyPref function and the corresponding inline script block to clearly note that they must use the same pref === 'dark' / system matchMedia decision tree, so future changes are made in both locations together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@js/theme.js`:
- Line 35: `initThemeToggle` in `theme.js` calls
`matchMedia('(prefers-color-scheme: dark)')` without protection, so mirror the
boot script’s defensive handling here. Wrap the `matchMedia` lookup in try/catch
or add a safe fallback so `initThemeToggle` can continue and still reveal the
toggle button even when `matchMedia` is unavailable or throws. Apply the same
fix anywhere else this unguarded `matchMedia` usage appears in the theme setup
flow.
- Around line 22-29: The dark-mode detection logic is duplicated between the
inline boot script and applyPref, so add a short sync-contract comment in both
places to keep their behavior aligned. Update the applyPref function and the
corresponding inline script block to clearly note that they must use the same
pref === 'dark' / system matchMedia decision tree, so future changes are made in
both locations together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c854ff3f-1648-4ed5-ae3c-af3f90735196
📒 Files selected for processing (4)
css/style.cssindex.htmljs/app.jsjs/theme.js
bharvey88
left a comment
bharvey88
left a comment
There was a problem hiding this comment.
Really nice work on this, thanks @jesserockz. I pulled it down and went through it end to end, and from my side it's good to merge.
What I checked:
- Playwright suite passes 10/10 against your branch. theme.js loads fine and the token refactor didn't disturb any selector the tests key off.
- Deploy path: our Pages workflow stages files with
cp -r ... js ..., so the whole js/ dir (theme.js included) ships to production automatically. No workflow change needed. - Brand palette: light :root keeps --blue #417AAB and --green #9ABD32, and the dark overrides read as intentional lightened variants. No regression in light mode.
- No-flash init: the inline script is dependency-free and matches applyPref()'s logic, so no flash and no extra request or build step. Nice touch keeping the toggle hidden until the module wires it up.
Two tiny, non-blocking notes (take or leave):
- The toggle is a 3-way cycle (system -> dark -> light). We have an a11y pass queued in #6 that adds aria-pressed to the filter pills and toggles. Worth remembering this one is a cycle rather than a boolean, so the aria-label you already update is the right call here, not aria-pressed.
- The placeholder moon glyph in the button markup never actually renders (the button stays hidden until updateBtn sets the real icon), so it's harmless. Just flagging in case a future reader wonders why it's there.
Thanks again for kicking off the dark theme.
Adds full dark mode support: system-preference auto-detection, an explicit 3-way toggle (System / Dark / Light), and localStorage persistence. Light mode remains the default.
CSS (
css/style.css)[data-theme="dark"]block overriding the full token set with dark-appropriate values.device-card img,.device-head img,.fallback,.release-notes,.error-box) into new tokens (--img-bg,--bg-blue-tint,--bg-green-tint,--bg-error,--border-error) so they switch automaticallyNo-flash init (
index.html<head>)Inline script runs before the stylesheet is applied — reads
localStorageand setsdata-themeon<html>before first paint:Theme toggle (
js/theme.js,index.html,js/app.js)#theme-togglebutton in the header nav; hidden via CSS until JS initialises it (avoids a ghost-button flash)⬤→ Dark🌙→ Light☀on each click;aria-label/titlealways describe the current modelocalStoragekey and re-evaluatesprefers-color-scheme; aMediaQueryListchangelistener keeps the page in sync if the OS preference changes while loadedSummary by CodeRabbit
New Features
Style