feat(zustand): implement dark/light theme toggle with zustand - #7
vidyashreebv wants to merge 8 commits into
Conversation
- Create Zustand store for theme state management - Add toggleTheme action to switch between light and dark modes - Add setTheme action for direct theme setting - Implement localStorage persistence for theme preference - Add SSR safety with globalThis.window checks
- Create ThemeToggle atom component with sun/moon icons - Integrate with Zustand theme store using useThemeStore hook - Add comprehensive test suite with 8 test cases - Test theme toggling, initial state, and accessibility features - Include Tailwind dark mode classes for styling
- Add useThemeStore hook to manage theme state - Implement useEffect to apply dark class to document root - Add ThemeToggle component to top-right corner - Add dark mode Tailwind classes to all UI elements - Update background gradients for dark mode support - Style settings panel, inputs, and labels for theme switching
- Add zustand@^5.0.2 to project dependencies - Update package-lock.json with zustand and its dependencies
- Remove incorrect @variant directive syntax - Use class-based dark mode with Tailwind v4 - Dark mode works automatically when 'dark' class is on document root
- Add @theme with color-scheme configuration to index.css - Add colorScheme style property to document root - Ensures proper dark mode activation in Tailwind v4
- Add @variant dark directive to enable dark: prefix - Use selector pattern for class-based dark mode - Remove debug console.log statements - Dark mode now working correctly with theme toggle
Change SummaryThis PR introduces global theme state using Zustand and adds a light/dark theme toggle UI. Note: The PR description mentions dark styling across all components; the diff primarily updates File Changes
|
PR ScorecardScoreScoring MethodologyCommunication ScoreThe overall communication score is a weighted average:
Formula: Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
Each category score is the average of its subcategories. Score Bands: 90-100 Excellent | 75-89 Good | 60-74 Adequate | 40-59 Needs Work | 0-39 Critical PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
|
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| export const useThemeStore = create((set) => ({ | ||
| mode: getInitialTheme(), | ||
| toggleTheme: () => | ||
| set((state) => { | ||
| const newMode = state.mode === 'light' ? 'dark' : 'light' | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', newMode) | ||
| } | ||
| return { mode: newMode } | ||
| }), | ||
| setTheme: (mode) => | ||
| set(() => { | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', mode) | ||
| } | ||
| return { mode } | ||
| }), | ||
| })) |
There was a problem hiding this comment.
Severity: 🟠 Major
Coverage: Add persistence tests for the theme store
There are no tests asserting that theme changes are persisted (setItem calls) and that initial mode is loaded from localStorage.
Why this matters: Persistence is core behavior here; regressions will only show up manually (refresh flows).
Suggested fix:
// src/store/useThemeStore.test.js
import { describe, it, expect, vi, beforeEach } from 'vitest'
beforeEach(() => {
localStorage.clear()
})
describe('useThemeStore persistence', () => {
it('writes to localStorage when toggling', async () => {
vi.resetModules()
const setItemSpy = vi.spyOn(Storage.prototype, 'setItem')
const { useThemeStore } = await import('./useThemeStore')
useThemeStore.setState({ mode: 'light' })
useThemeStore.getState().toggleTheme()
expect(setItemSpy).toHaveBeenCalledWith('theme', 'dark')
})
it('initializes from localStorage when present', async () => {
localStorage.setItem('theme', 'dark')
vi.resetModules()
const { useThemeStore } = await import('./useThemeStore')
expect(useThemeStore.getState().mode).toBe('dark')
})
})
| beforeEach(() => { | ||
| // Reset store to light mode before each test | ||
| useThemeStore.setState({ mode: 'light' }) | ||
| }) |
There was a problem hiding this comment.
Severity: 🟠 Major
Test Design: Reset localStorage between tests (avoid order-dependence)
Tests reset the store, but don’t reset localStorage. Once persistence coverage is added, this will create flaky/order-dependent tests.
Why this matters: Local runs might pass while CI fails depending on test order.
Suggested fix:
beforeEach(() => {
localStorage.clear()
useThemeStore.setState({ mode: 'light' })
})
| <button | ||
| onClick={handleClickToggleTheme} | ||
| aria-label={`Switch to ${isDarkMode ? 'light' : 'dark'} mode`} | ||
| className="p-3 rounded-lg bg-white dark:bg-gray-800 shadow-md hover:shadow-lg transition-all duration-200 border border-gray-200 dark:border-gray-700" |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Robustness: Prevent accidental form submission
If ThemeToggle is ever used inside a <form>, the default button type will submit the form.
Suggested fix:
<button
type="button"
onClick={handleClickToggleTheme}
aria-label={`Switch to ${isDarkMode ? 'light' : 'dark'} mode`}
className="..."
>
| setTheme: (mode) => | ||
| set(() => { | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', mode) | ||
| } | ||
| return { mode } | ||
| }), |
There was a problem hiding this comment.
Severity: 🟠 Major
Correctness: Validate theme mode values
setTheme(mode) will accept any string (and getInitialTheme() can return any string from storage), which can put the app into an unsupported theme state.
Why this matters: Once mode is something unexpected, UI + DOM side effects can drift into a partially-themed state and become hard to reason about.
Suggested fix:
const THEME_STORAGE_KEY = 'theme'
const VALID_MODES = new Set(['light', 'dark'])
const normalizeMode = (value) => (VALID_MODES.has(value) ? value : 'light')
export const useThemeStore = create((set) => ({
mode: normalizeMode(getInitialTheme()),
toggleTheme: () =>
set((state) => {
const newMode = state.mode === 'light' ? 'dark' : 'light'
safeSetItem(THEME_STORAGE_KEY, newMode)
return { mode: newMode }
}),
setTheme: (mode) =>
set(() => {
const normalizedMode = normalizeMode(mode)
safeSetItem(THEME_STORAGE_KEY, normalizedMode)
return { mode: normalizedMode }
}),
}))
|
|
||
| const getInitialTheme = () => { | ||
| if (globalThis.window !== undefined) { | ||
| const savedTheme = globalThis.localStorage.getItem('theme') |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Best Practices: Extract magic storage key
'theme' is repeated across reads/writes.
Why this matters: A single constant prevents drift if the key ever changes.
Suggested fix:
const THEME_STORAGE_KEY = 'theme'
// then use THEME_STORAGE_KEY everywhere
globalThis.localStorage.setItem(THEME_STORAGE_KEY, newMode)
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', newMode) | ||
| } |
There was a problem hiding this comment.
Severity: 🟠 Major
Robustness: Guard for localStorage availability (not just window)
The current environment check only gates on window, but some environments can have a window shim without localStorage (or a throwing localStorage getter).
Why this matters: You can still throw in non-browser runtimes/tests or constrained browser contexts.
Suggested fix:
const getSafeLocalStorage = () => {
try {
return typeof globalThis.window !== 'undefined' && globalThis.window?.localStorage
? globalThis.window.localStorage
: null
} catch {
return null
}
}
const safeGetItem = (key) => {
const storage = getSafeLocalStorage()
if (!storage) return null
try {
return storage.getItem(key)
} catch {
return null
}
}
const safeSetItem = (key, value) => {
const storage = getSafeLocalStorage()
if (!storage) return
try {
storage.setItem(key, value)
} catch {
// ignore
}
}
| // Apply theme to document on mount and when theme changes | ||
| useEffect(() => { | ||
| const root = document.documentElement | ||
| if (mode === 'dark') { | ||
| root.classList.add('dark') | ||
| root.style.colorScheme = 'dark' | ||
| } else { | ||
| root.classList.remove('dark') | ||
| root.style.colorScheme = 'light' | ||
| } | ||
| }, [mode]) |
There was a problem hiding this comment.
Severity: 🟠 Major
Coverage: Add tests for DOM side effects when theme changes
There are no tests verifying document.documentElement.classList / colorScheme changes when the theme updates.
Why this matters: The UI can look correct in component-level tests while the actual DOM-level dark mode toggle silently regresses.
Suggested fix:
// src/App.test.jsx
import { beforeEach, describe, expect, it } from 'vitest'
import { render } from '@testing-library/react'
import App from './App'
import { useThemeStore } from './store/useThemeStore'
beforeEach(() => {
useThemeStore.setState({ mode: 'light' })
document.documentElement.classList.remove('dark')
document.documentElement.style.colorScheme = ''
})
describe('theme DOM side effects', () => {
it('applies .dark class and dark color-scheme when mode becomes dark', () => {
render(<App />)
useThemeStore.setState({ mode: 'dark' })
expect(document.documentElement).toHaveClass('dark')
expect(document.documentElement.style.colorScheme).toBe('dark')
})
})
| const getInitialTheme = () => { | ||
| if (globalThis.window !== undefined) { | ||
| const savedTheme = globalThis.localStorage.getItem('theme') | ||
| return savedTheme || 'light' | ||
| } | ||
| return 'light' | ||
| } |
There was a problem hiding this comment.
Severity: 🔴 Critical
Robustness: localStorage access can throw during import/render
getInitialTheme() reads from localStorage and runs as part of the store initialization (mode: getInitialTheme()), so any localStorage exception (privacy mode, blocked storage, quota) can crash the module import / initial render.
Why this matters: A thrown error here prevents the app from booting at all.
Suggested fix:
const safeGetItem = (key) => {
try {
return globalThis.localStorage?.getItem(key) ?? null
} catch {
return null
}
}
const safeSetItem = (key, value) => {
try {
globalThis.localStorage?.setItem(key, value)
} catch {
// ignore persistence failures; app should still work
}
}
const getInitialTheme = () => safeGetItem(THEME_STORAGE_KEY) || 'light'
| // Apply theme to document on mount and when theme changes | ||
| useEffect(() => { | ||
| const root = document.documentElement | ||
| if (mode === 'dark') { | ||
| root.classList.add('dark') | ||
| root.style.colorScheme = 'dark' | ||
| } else { | ||
| root.classList.remove('dark') | ||
| root.style.colorScheme = 'light' | ||
| } | ||
| }, [mode]) |
There was a problem hiding this comment.
Severity: 🟠 Major
Architecture: Theme DOM side effects are coupled to App
The .dark class and colorScheme mutations live inside App, which makes theme application dependent on App being mounted and harder to reuse (e.g., if you later mount different roots or isolate parts in tests/storybook).
Why this matters: Centralizing the DOM sync reduces hidden coupling and makes theme behavior consistent wherever the store is used.
Recommended approach: extract a dedicated hook (or provider) responsible for syncing store → DOM.
// src/theme/useApplyThemeToDocument.js
import { useEffect } from 'react'
import { useThemeStore } from '../store/useThemeStore'
export function useApplyThemeToDocument() {
const mode = useThemeStore((s) => s.mode)
useEffect(() => {
const root = document.documentElement
const isDark = mode === 'dark'
root.classList.toggle('dark', isDark)
root.style.colorScheme = isDark ? 'dark' : 'light'
}, [mode])
}
// App.jsx
import { useApplyThemeToDocument } from './theme/useApplyThemeToDocument'
function App() {
useApplyThemeToDocument()
// ...
}
| it('toggles theme from light to dark when clicked', async () => { | ||
| render(<ThemeToggle />) | ||
| const user = userEvent.setup() | ||
|
|
||
| const button = screen.getByRole('button') | ||
| await user.click(button) | ||
|
|
||
| expect(useThemeStore.getState().mode).toBe('dark') | ||
| }) |
There was a problem hiding this comment.
Severity: 🟠 Major
Test Design: Prefer user-visible assertions over store internals
Several tests assert useThemeStore.getState().mode after clicks, which couples component tests to Zustand internals rather than verifying what the user sees (aria-label/icon).
Why this matters: These tests become brittle if the state implementation changes while the UI behavior stays correct.
Suggested fix:
it('switches aria-label from dark->light after click', async () => {
render(<ThemeToggle />)
const user = userEvent.setup()
const button = screen.getByRole('button', { name: 'Switch to dark mode' })
await user.click(button)
expect(
screen.getByRole('button', { name: 'Switch to light mode' })
).toBeInTheDocument()
})
| const mode = useThemeStore((state) => state.mode) | ||
| const [initialValue, setInitialValue] = useState(10) | ||
| const [step, setStep] = useState(5) | ||
| const [key, setKey] = useState(0) |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Clarity: Rename key to reflect intent
key is used specifically to force a Counter remount; a more descriptive name will make that purpose obvious.
Suggested fix:
const [counterInstanceKey, setCounterInstanceKey] = useState(0)
const handleClickApply = () => {
setCounterInstanceKey((prev) => prev + 1)
}
<Counter key={counterInstanceKey} initialValue={initialValue} step={step} />
| import { useThemeStore } from '../../../store/useThemeStore' | ||
|
|
||
| export const ThemeToggle = () => { | ||
| const { mode, toggleTheme } = useThemeStore() |
There was a problem hiding this comment.
Severity: 🟠 Major
Best Practices: Avoid subscribing to the entire Zustand store
useThemeStore() without a selector subscribes the component to all store changes, which will cause unnecessary re-renders as the store grows.
Why this matters: It’s easy for small stores to grow over time; this creates avoidable render churn.
Suggested fix:
export const ThemeToggle = () => {
const mode = useThemeStore((state) => state.mode)
const toggleTheme = useThemeStore((state) => state.toggleTheme)
// ...
}
| import { useThemeStore } from '../../../store/useThemeStore' | ||
|
|
||
| export const ThemeToggle = () => { | ||
| const { mode, toggleTheme } = useThemeStore() | ||
|
|
||
| const handleClickToggleTheme = () => { | ||
| toggleTheme() | ||
| } | ||
|
|
||
| const isDarkMode = mode === 'dark' | ||
|
|
||
| return ( | ||
| <button | ||
| onClick={handleClickToggleTheme} | ||
| aria-label={`Switch to ${isDarkMode ? 'light' : 'dark'} mode`} | ||
| className="p-3 rounded-lg bg-white dark:bg-gray-800 shadow-md hover:shadow-lg transition-all duration-200 border border-gray-200 dark:border-gray-700" | ||
| > | ||
| {isDarkMode ? ( | ||
| <svg | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| strokeWidth={2} | ||
| stroke="currentColor" | ||
| className="w-6 h-6 text-yellow-500" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| d="M12 3v2.25m6.364.386l-1.591 1.591M21 12h-2.25m-.386 6.364l-1.591-1.591M12 18.75V21m-4.773-4.227l-1.591 1.591M5.25 12H3m4.227-4.773L5.636 5.636M15.75 12a3.75 3.75 0 11-7.5 0 3.75 3.75 0 017.5 0z" | ||
| /> | ||
| </svg> | ||
| ) : ( | ||
| <svg | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| strokeWidth={2} | ||
| stroke="currentColor" | ||
| className="w-6 h-6 text-gray-700" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| d="M21.752 15.002A9.718 9.718 0 0118 15.75c-5.385 0-9.75-4.365-9.75-9.75 0-1.33.266-2.597.748-3.752A9.753 9.753 0 003 11.25C3 16.635 7.365 21 12.75 21a9.753 9.753 0 009.002-5.998z" | ||
| /> | ||
| </svg> | ||
| )} | ||
| </button> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Consistency: Formatting/indentation differs from the rest of the codebase
This file (and ThemeToggle.test.jsx) uses 4-space indentation while most other files use 2-space indentation.
Why this matters: Inconsistent formatting increases diff noise and makes it harder to scan PRs quickly.
Suggested fix:
export const ThemeToggle = () => {
const mode = useThemeStore((state) => state.mode)
const toggleTheme = useThemeStore((state) => state.toggleTheme)
const handleClickToggleTheme = () => {
toggleTheme()
}
const isDarkMode = mode === 'dark'
return (
<button type="button" onClick={handleClickToggleTheme} aria-label={`Switch to ${isDarkMode ? 'light' : 'dark'} mode`}>
{/* ... */}
</button>
)
}
PR AnalysisFocus Areas for Architect Review
RecommendationAction: REQUEST CHANGES Quick Wins:
Author InsightsPR Type: feat Missing Skills
Strengths
|
2 participants
What does this PR do?
What steps does your reviewer have to take to test this PR manually?
feature/zustand-theme-managementbranch and runnpm installnpm run devto start the development servernpm test -- --runto verify all 35 tests pass (27 existing + 8 new theme tests)Screenshots
Pull Request standards checklist - Please check off
Testing checklist - Please check off
Definition of Done - Please check off