fix(app): wire settings theme switcher with resilient persistence#2167
fix(app): wire settings theme switcher with resilient persistence#2167AchuAshwath wants to merge 2 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.
| 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], |
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?