Add system theme option to ToggleDarkMode

[alecf]

Summary

• ToggleDarkMode now cycles through three states: light → dark → system
• "System" mode honors the OS prefers-color-scheme setting and reacts to changes in real time
• Legacy boolean localStorage values ("true"/"false") are migrated automatically
• New ThemePreference type and systemLabel prop exported for consumers

Test plan

• Click the toggle in the playground header and verify it cycles: Light (Sun) → Dark (Moon) → System (Monitor)
• In System mode, change OS dark mode setting and verify the UI updates immediately
• Refresh the page and verify the selected preference persists
• Clear localStorage and verify it defaults to System mode

🤖 Generated with Claude Code

[github-actions]

Preview deployed! 🚀

View the playground preview at: https://thegpvc.github.io/gp-ui/pr-32/

Last updated: 2026-02-09T15:27:09.076Z

[claude]

Code Review: Add system theme option to ToggleDarkMode

✅ Strengths

1. Great User Experience Enhancement

• The three-state cycle (light → dark → system) is a modern UX pattern that respects user preferences
• Real-time response to OS theme changes in system mode is excellent
• Backward compatibility with legacy boolean values is thoughtful

2. Clean Implementation

• Code is well-structured and easy to follow
• Good use of React patterns (useState, useEffect, useCallback)
• Type safety with ThemePreference export

3. Good Documentation

• JSDoc comments updated appropriately
• Clear prop descriptions for systemLabel
• Updated examples in component documentation

---

🔍 Issues & Recommendations

1. SSR Safety Issue (Medium Priority)

Lines 96-105: The initial state uses window without proper SSR guard in the return statement.

const [theme, setTheme] = useState<ThemePreference>(() => {
  if (typeof window !== 'undefined') {
    // ... 
  }
  return 'system'
})

While the SSR check exists, on line 64 getSystemDark() is called without guards:

function getSystemDark() {
  return window.matchMedia('(prefers-color-scheme: dark)').matches
}

Problem: If this component ever gets server-rendered, getSystemDark() will crash on line 114 when called during SSR.

Fix: Add SSR guard:

function getSystemDark() {
  if (typeof window === 'undefined') return false
  return window.matchMedia('(prefers-color-scheme: dark)').matches
}

2. Memory Leak Risk (Low-Medium Priority)

Lines 108-122: The media query listener cleanup is correct, but could be more explicit.

The current implementation is actually fine, but for clarity and to prevent future bugs, consider extracting the logic:

useEffect(() => {
  const apply = (isDark: boolean) => {
    document.documentElement.classList.toggle('dark', isDark)
  }

  if (theme === 'system') {
    const mq = window.matchMedia('(prefers-color-scheme: dark)')
    apply(mq.matches)  // Use mq.matches instead of calling getSystemDark() again
    const handler = (e: MediaQueryListEvent) => apply(e.matches)
    mq.addEventListener('change', handler)
    return () => mq.removeEventListener('change', handler)
  }

  apply(theme === 'dark')
}, [theme])

This is a minor optimization to avoid calling getSystemDark() twice.

3. Race Condition in State Updates (Low Priority)

Lines 129-135: The toggle callback doesn't include theme in its dependencies.

const toggle = useCallback(() => {
  setTheme((prev) => {
    const next = CYCLE[(CYCLE.indexOf(prev) + 1) % CYCLE.length]
    onChange?.(next)
    return next
  })
}, [onChange])

This is fine as written since setTheme uses the functional updater pattern. However, ESLint might complain about the missing theme dependency in the icon/label/ariaLabel calculations below. Consider memoizing those values:

const { icon, label, ariaLabel } = useMemo(() => {
  const icons = { light: <Sun />, dark: <Moon />, system: <Monitor /> }
  const labels = { light: lightLabel, dark: darkLabel, system: systemLabel }
  const nextTheme = CYCLE[(CYCLE.indexOf(theme) + 1) % CYCLE.length]
  
  return {
    icon: icons[theme],
    label: labels[theme],
    ariaLabel: `Switch to ${nextTheme} mode`
  }
}, [theme, lightLabel, darkLabel, systemLabel])

4. Missing Tests (High Priority)

No test files exist for this component. Given the complexity of:

• Three-state cycling logic
• localStorage persistence and migration
• Media query listener setup/cleanup
• SSR compatibility

Recommendation: Add tests covering:

// Suggested test cases
- Initial state defaults to 'system' when no localStorage
- Legacy migration: 'true' → 'dark', 'false' → 'light'
- Cycling through states: light → dark → system → light
- System mode responds to media query changes
- localStorage updates on theme change
- Cleanup removes media query listener
- SSR doesn't crash (render with 'system' default)
- onChange callback receives correct ThemePreference values

5. Documentation Inconsistency (Low Priority)

DESIGN_GUIDE.md line 180 still shows the old callback signature:

<ToggleDarkMode onChange={(isDark) => {}} />  // With callback

Should be updated to:

<ToggleDarkMode onChange={(theme) => {}} />  // With callback

---

🔒 Security

No security concerns. The component:

• Only reads/writes to localStorage (user's own browser storage)
• No external API calls or data transmission
• No XSS risks (React handles escaping)
• No injection vulnerabilities

---

⚡ Performance

Good:

• Proper use of useCallback for event handlers
• Media query listener only active in system mode
• Minimal re-renders

Could improve:

• Consider memoizing icon/label objects (minor optimization)
• The object literal creation on lines 137-140 happens on every render

---

🧪 Test Coverage

Current: ❌ No tests exist
Needed: Unit tests for state management, cycling logic, localStorage, and SSR safety

---

📋 Summary

This is a solid feature addition with good UX and clean code. The main concerns are:

1. Must fix: SSR safety in getSystemDark()
2. Should fix: Add test coverage for complex state logic
3. Nice to have: Update DESIGN_GUIDE.md example

The implementation is production-ready after addressing the SSR issue. Great work on the backward compatibility and real-time system theme detection!

---

✅ Approval Status

Recommend: Approve with minor changes

The SSR fix is critical for server-rendered apps, but otherwise this is ready to merge.

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com