[codex] add system appearance theme selection#3344
Conversation
Yeraze
left a comment
There was a problem hiding this comment.
Review — system appearance theme selection
Solid, well-structured feature. CI is green and it's cleanly mergeable. The migration logic is careful and the per-slot model is implemented consistently across SettingsContext and SettingsTab. No crashes or blocking bugs found, but two real behavioral issues and a couple of nits are worth addressing before merge.
What it does well
- Clean state model.
appearanceMode: 'system' | 'dark' | 'light'+darkTheme/lightThemeslots, with the legacythemekey retained as the effective theme for backward compat. New keys are correctly added toVALID_SETTINGS_KEYS(the silent-save trap is avoided). - Migration matches the stated behavior and is mirrored in both the localStorage initializer (
getInitialThemePreferences) and the server-load path: new users → system/mocha/latte; mocha users → same; non-mocha users → explicitdarkwith their theme in both slots. - System listener is correct —
matchMedia('(prefers-color-scheme: dark)')with properaddEventListener/cleanup, and a separate effect recomputes the effective theme. No render loop (the apply-effect depends onappearanceMode/darkTheme/lightTheme/systemIsDark, not ontheme, which is whatapplyThememutates). - SettingsTab wiring is complete — local state, the
handleSavedependency array, and reset-to-defaults are all updated; the dark/light selects sharerenderThemeOptions().
Issues
1. Theme gallery (ThemeDocumentation.tsx) silently breaks the new model — main concern.
It still calls the legacy setTheme(id), which this PR redefined to force appearanceMode='dark' and overwrite both slots:
const setTheme = (newTheme) => {
setAppearanceModeState('dark');
setDarkThemeState(newTheme);
setLightThemeState(newTheme); // clobbers the user's light slot
...
}So a user in system mode who clicks any card in the theme gallery is silently kicked into explicit dark mode and loses their separate light theme; picking a light theme there lands it in the dark slot. This contradicts the PR's own design principle for the custom-theme cards ("buttons update only the corresponding slot and do not change appearance mode"). Suggest giving the gallery Apply-as-Dark/Light affordances, or at minimum not flipping the mode — or explicitly scoping it out.
2. onThemeChange is now a dead prop. SettingsTab removed it from destructuring and replaced it with direct context setters, but it's still declared in SettingsTabProps (line 84) and still passed by App.tsx:4976 and GlobalSettingsPage.tsx:132. Harmless, but worth removing to avoid confusion.
Nits
- Migration asymmetry: the localStorage path derives
appearanceMode/lightThemewithout the!hasNewThemePreferencesguard the server-load path uses. Only reachable with partial state (slot keys present butappearanceModemissing), which never happens since they're written together — defensive, not a real bug, but the two paths would ideally share a helper. getInitialThemePreferences()writes tolocalStorageinside auseStateinitializer (a side effect in render). It's idempotent and matches the existing pattern in this file, so acceptable.- Good test coverage on the custom-theme-card slot behavior; nothing covers the gallery/
setThememode-flip in issue #1.
Recommendation
Approve pending #1 (genuine UX regression) and ideally #2 cleanup. Everything else is polish.
🤖 Generated with Claude Code
Summary
Adds a new appearance preference model with system, dark, and light modes. System mode follows prefers-color-scheme and switches automatically when the browser/OS appearance changes.
The theme slots are persisted as darkTheme and lightTheme, while the existing theme setting remains the effective/current theme for backward compatibility.
Behavior
Validation