fix(app): theme switcher implementation #2167
fix(app): theme switcher implementation #2167AchuAshwath wants to merge 4 commits intokriasoft:mainfrom
Conversation
Signed-off-by: AchuAshwath <achuashwath88@gmail.com> Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Wires the settings dark-mode toggle to actual theme state via a new ThemeProvider context, with localStorage persistence, system-preference fallback, and cross-tab sync via storage events.
Changes:
- New
ThemeProvidercontext (apps/app/lib/theme.tsx) managing theme state, DOM class sync, guarded localStorage persistence, and cross-tab synchronization. - Settings page switch connected to theme context; app entry point wrapped with
ThemeProvider. - Comprehensive test suite covering persisted theme, system fallback, DOM/storage sync, and failure modes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
apps/app/lib/theme.tsx |
New theme context provider with localStorage persistence, system preference fallback, and cross-tab sync |
apps/app/index.tsx |
Wraps RouterProvider with ThemeProvider at the app root |
apps/app/routes/(app)/settings.tsx |
Connects the dark-mode Switch to the theme context |
apps/app/lib/theme.test.tsx |
Tests for theme initialization, toggling, storage events, and failure handling |
You can also share your feedback on Copilot code review. Take the survey.
apps/app/lib/theme.tsx
Outdated
| setTheme, | ||
| toggleTheme, | ||
| }), | ||
| [theme], |
There was a problem hiding this comment.
The useMemo dependency array [theme] omits toggleTheme. While toggleTheme is currently stable (due to useCallback([], [])), this will break silently if toggleTheme ever gains dependencies—and it will trigger an react-hooks/exhaustive-deps lint warning. Include toggleTheme in the dependency array for correctness and future safety. (setTheme from useState is guaranteed stable by React, so it doesn't need to be listed.)
| [theme], | |
| [theme, toggleTheme], |
|
Theme switching is much closer now, but I still see a few issues worth fixing before merge. 1. First paint still flashes the light theme
That is especially visible here because the app background/text colors come from CSS variables on Suggested fix:
2. The "system fallback" is immediately persisted as an explicit choiceRight now
After that first mount, the app is no longer "following system" at all; it has silently converted the OS-derived value into a fixed user preference. So even if the user never touched the toggle:
I think the state needs to distinguish:
Then only persist the preference, and derive the resolved theme from 3. No listener for live OS theme changesRelated to the above: there is no If the intended behavior is "follow system unless the user explicitly overrides it", this needs a media-query change listener in addition to storing a 4.
|
…nd accessibility - Refactor theme state to support light/dark/system preference model - Add pre-paint bootstrap script in index.html to prevent first-load flash - Persist preference (not resolved theme) to avoid overwriting system choice - Add live OS theme change listener for system-follow behavior - Sync meta theme-color on theme changes - Suppress transitions during theme apply to reduce visual flash - Replace custom radio with accessible RadioGroup + RadioGroupItem - Expand tests for preference persistence and theme-color updates - Add sync comments between bootstrap and runtime logic
|
@koistya Added a few more things to this PR:
Quick manual check
Tests are updated and all pass. Let me know if anything needs tweaking! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
| const value = useMemo( | ||
| () => ({ | ||
| theme, | ||
| preference, | ||
| setPreference, | ||
| }), | ||
| [theme, preference], | ||
| ); |
Summary
ThemeProviderand wire the settings dark-mode switch to shared theme state.Why
The dark-mode switch in Settings was previously not connected to application theme behavior. This change makes theme switching functional, persistent, and robust across browser/storage conditions.
Changes
ThemeProviderinapps/app/index.tsx.apps/app/lib/theme.tsx:<html>withuseLayoutEffectlocalStorageread/writestorageevent handling for multi-tab syncSwitchinapps/app/routes/(app)/settings.tsxto theme state.apps/app/lib/theme.test.tsxwith behavior-first coverage for:In
ThemeProvider, the context value is memoized with:Since
toggleThemeis currently stabilized withuseCallback([]), is[theme]sufficient here, or do we prefer[theme, toggleTheme]for explicitness/future-proofing iftoggleThemedependencies change later?