Repository navigation
feat(ui): implement dynamic theme engine with custom tweakcn presets - #20
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a theming system: typed ThemeContext, ThemeProvider (with persistence and DOM sync), theme registry, useTheme hook, ThemeSwitcher UI, ThemeProvider wired into app bootstrap, and updates to global CSS tokens and layout styling to use semantic theme variables. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (main.tsx)
participant TP as ThemeProvider
participant Store as localStorage
participant Registry as Themes Registry
participant Root as DocumentRoot
participant Switcher as ThemeSwitcher
participant User as User
App->>TP: mount (defaultTheme)
TP->>Store: read(storageKey)
alt stored theme found
Store-->>TP: storedTheme
else
TP-->>TP: use defaultTheme
end
TP->>Registry: lookup cssVars for theme
Registry-->>TP: cssVars (light/dark)
TP->>Root: apply CSS variables
TP->>Root: observe class mutations (MutationObserver)
TP-->>App: provide context {theme, setTheme, availableThemes}
App->>Switcher: render (useTheme -> context)
Switcher->>Switcher: display availableThemes
User->>Switcher: select newTheme
Switcher->>TP: setTheme(newTheme)
TP->>Registry: fetch cssVars for newTheme
Registry-->>TP: return cssVars
TP->>Root: update CSS variables
TP->>Store: persist(newTheme)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/theme/ThemeContext.ts`:
- Around line 10-14: initialState defines setTheme as a stub that returns null
while ThemeProviderState expects () => void; change the stub to be a
void-returning function (e.g., () => { } or () => undefined) so its return type
matches the declared signature; update the initialState object (symbol:
initialState) and ensure setTheme's implementation aligns with the
ThemeProviderState type to remove the inconsistency.
In `@apps/web/src/lib/theme/ThemeProvider.tsx`:
- Around line 21-37: The theme resolution and CSS variable application
duplicated in ThemeProvider's two useEffect hooks should be extracted into a
helper: create a function (e.g., resolveAndApplyThemeCssVars or
applyThemeCssVars) that accepts (theme, defaultTheme, storageKey,
root=document.documentElement) and performs the logic currently repeated —
resolve currentTheme via themes[theme] || themes[defaultTheme] ||
Object.values(themes)[0], determine isDark by checking
root.classList.contains("dark"), select cssVars (currentTheme.cssVars.dark or
.light), apply Object.entries(cssVars).forEach(...) to set CSS properties on
root, and persist localStorage.setItem(storageKey, theme); then call this helper
from both useEffect hooks in place of the duplicated blocks.
- Around line 17-19: The useState initializer in ThemeProvider that calls
localStorage.getItem(storageKey) and the code that calls localStorage.setItem
when updating theme should be wrapped in defensive try-catch logic: in the
initializer for theme (the useState callback referencing storageKey and
defaultTheme) catch any exception from localStorage.getItem and return
defaultTheme on error, and in the theme update path (where setTheme and
localStorage.setItem are used) guard the localStorage.setItem call with
try-catch (or a feature check) so failures are swallowed/logged and do not break
state updates; reference the ThemeProvider component, the
storageKey/defaultTheme symbols, and the setTheme/localStorage.setItem usage to
locate where to apply these changes.
In `@apps/web/src/lib/theme/themes.ts`:
- Around line 10-551: The dark variants across themes are missing the "--radius"
token, causing inconsistent corner rounding (e.g., "pastel-dreams" light sets
"--radius": "0.75rem" but its dark map lacks it). For each theme in the exported
themes object (look for cssVars -> dark in "modern-minimal", "clean-slate",
"bold-tech", "cappuccino", "violet-bloom", "darkmatter", "midnight-bloom",
"northern-lights", "twitter", "pastel-dreams", "cosmic-night", etc.), add a
"--radius" entry in the dark map matching the intended radius (or unify by
moving "--radius" into a shared place applied to both light and dark) so both
light and dark maps define the same radius token.
In `@apps/web/src/lib/theme/useTheme.ts`:
- Around line 4-10: The hook useTheme currently checks for undefined but
ThemeProviderContext is created with initialState so the check never fires;
update the context/provider pattern: change ThemeProviderContext's default to
undefined and its type to possibly undefined in ThemeContext.ts (so
useContext(ThemeProviderContext) can return undefined), then keep the existing
undefined check in useTheme (throwing when context is undefined), or
alternatively leave the default as initialState and modify useTheme to detect a
sentinel value from initialState (e.g., a specific flag like isInitialized) and
throw when that sentinel indicates no provider; reference ThemeProviderContext,
initialState, useTheme, and ThemeContext.ts when making the change.
Comment on lines
+10
to
+14
| export const initialState: ThemeProviderState = { | ||
| theme: "bold-tech", | ||
| setTheme: () => null, | ||
| availableThemes: [], | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: setTheme returns null but type declares void.
The setTheme stub returns null, but the type signature declares () => void. While this works at runtime, it's slightly inconsistent.
Suggested fix
export const initialState: ThemeProviderState = {
theme: "bold-tech",
- setTheme: () => null,
+ setTheme: () => {},
availableThemes: [],
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const initialState: ThemeProviderState = { | |
| theme: "bold-tech", | |
| setTheme: () => null, | |
| availableThemes: [], | |
| }; | |
| export const initialState: ThemeProviderState = { | |
| theme: "bold-tech", | |
| setTheme: () => {}, | |
| availableThemes: [], | |
| }; |
🤖 Prompt for AI Agents
In `@apps/web/src/lib/theme/ThemeContext.ts` around lines 10 - 14, initialState
defines setTheme as a stub that returns null while ThemeProviderState expects ()
=> void; change the stub to be a void-returning function (e.g., () => { } or ()
=> undefined) so its return type matches the declared signature; update the
initialState object (symbol: initialState) and ensure setTheme's implementation
aligns with the ThemeProviderState type to remove the inconsistency.
Comment on lines
+4
to
+10
| export const useTheme = () => { | ||
| const context = useContext(ThemeProviderContext); | ||
|
|
||
| if (context === undefined) | ||
| throw new Error("useTheme must be used within a ThemeProvider"); | ||
|
|
||
| return context; |
Contributor
There was a problem hiding this comment.
The undefined check will never trigger due to context default value.
Since ThemeProviderContext is created with initialState as the default value (in ThemeContext.ts), context will never be undefined. If useTheme is called outside a ThemeProvider, it will silently return the non-functional initialState instead of throwing.
To properly detect missing provider, either:
- Use
undefinedas the context default and update the type, or - Check for a sentinel value in the initial state.
Option 1: Use undefined as the default (recommended)
In ThemeContext.ts:
-export const ThemeProviderContext = createContext<ThemeProviderState>(initialState);
+export const ThemeProviderContext = createContext<ThemeProviderState | undefined>(undefined);Then update the hook check accordingly (current check would work).
🤖 Prompt for AI Agents
In `@apps/web/src/lib/theme/useTheme.ts` around lines 4 - 10, The hook useTheme
currently checks for undefined but ThemeProviderContext is created with
initialState so the check never fires; update the context/provider pattern:
change ThemeProviderContext's default to undefined and its type to possibly
undefined in ThemeContext.ts (so useContext(ThemeProviderContext) can return
undefined), then keep the existing undefined check in useTheme (throwing when
context is undefined), or alternatively leave the default as initialState and
modify useTheme to detect a sentinel value from initialState (e.g., a specific
flag like isInitialized) and throw when that sentinel indicates no provider;
reference ThemeProviderContext, initialState, useTheme, and ThemeContext.ts when
making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/layouts/AdminLayout.spec.tsx`:
- Around line 54-58: The tests render AdminLayout with ThemeProvider using an
invalid defaultTheme value "light"; update the ThemeProvider defaultTheme prop
to a real production theme key (for example "violet-bloom") in both places where
AdminLayout is rendered so state/localStorage uses a valid theme key (check the
ThemeProvider usage in AdminLayout.spec.tsx and replace defaultTheme="light"
with defaultTheme="violet-bloom").
In `@apps/web/src/lib/theme/ThemeProvider.tsx`:
- Around line 17-38: The current logic lets an unknown theme key persist in the
theme state causing UI desync; normalize and clamp theme keys when reading from
storage and when setting theme: in the useState initializer that reads
localStorage (the theme state), validate the retrieved key against the themes
map and default to defaultTheme if missing, and also ensure any setter path
(e.g., functions that call setTheme or the applyTheme call path) validates or
maps the incoming currentThemeKey to a known key before updating state or
calling applyTheme; use the same lookup logic used in applyTheme
(themes[currentThemeKey] || themes[defaultTheme] || Object.values(themes)[0]) to
compute a canonical key and store that canonical key via setTheme so theme state
always holds a valid theme key.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/layouts/AdminLayout.spec.tsx (1)
70-70: Inconsistent ThemeProvider wrapping across tests.Tests at lines 70 and 86 render
AdminLayoutwithout theThemeProviderwrapper, while other tests include it. IfAdminLayoutor any descendant callsuseTheme, these tests will fail with a context error.For consistency and to prevent potential failures if component dependencies change, consider wrapping all test renders:
Suggested fix
- render(<AdminLayout />); + render( + <ThemeProvider defaultTheme="violet-bloom" storageKey="test-theme"> + <AdminLayout /> + </ThemeProvider> + );Apply the same pattern to the test at line 86.
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/theme/ThemeProvider.tsx`:
- Around line 17-23: getValidTheme currently trusts defaultTheme and can return
an invalid key causing themes[validKey] to be undefined; update the useCallback
implementation (getValidTheme) to: 1) check if the provided key exists in themes
and return it; 2) if not, verify that defaultTheme is a valid key in themes and
return it; 3) as a final fallback return the first available theme key (e.g.
Object.keys(themes)[0]). Also ensure the useCallback dependency array includes
themes (i.e., [defaultTheme, themes]) so the validator updates when theme map
changes; this protects the subsequent use of themes[validKey] in the
ThemeProvider render path.
Comment on lines
+17
to
+23
| // Helper to get a valid theme key | ||
| const getValidTheme = useCallback((key: string | undefined | null) => { | ||
| if (key && themes[key]) { | ||
| return key; | ||
| } | ||
| return defaultTheme; | ||
| }, [defaultTheme]); |
Contributor
There was a problem hiding this comment.
getValidTheme doesn't validate defaultTheme itself.
If defaultTheme is not a valid key in themes, getValidTheme returns it anyway, and themes[validKey] on line 40 will be undefined. Consider falling back to the first available theme as a final safety net:
Suggested fix
const getValidTheme = useCallback((key: string | undefined | null) => {
if (key && themes[key]) {
return key;
}
- return defaultTheme;
+ if (themes[defaultTheme]) {
+ return defaultTheme;
+ }
+ // Ultimate fallback to first available theme
+ return Object.keys(themes)[0] ?? defaultTheme;
}, [defaultTheme]);🤖 Prompt for AI Agents
In `@apps/web/src/lib/theme/ThemeProvider.tsx` around lines 17 - 23, getValidTheme
currently trusts defaultTheme and can return an invalid key causing
themes[validKey] to be undefined; update the useCallback implementation
(getValidTheme) to: 1) check if the provided key exists in themes and return it;
2) if not, verify that defaultTheme is a valid key in themes and return it; 3)
as a final fallback return the first available theme key (e.g.
Object.keys(themes)[0]). Also ensure the useCallback dependency array includes
themes (i.e., [defaultTheme, themes]) so the validator updates when theme map
changes; this protects the subsequent use of themes[validKey] in the
ThemeProvider render path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/layouts/AdminLayout.spec.tsx (1)
40-44: Consider clearing localStorage inbeforeEachfor test isolation.
ThemeProviderreads from localStorage on mount. WhilestorageKey="test-theme"isolates from production, clearing localStorage between tests ensures no state leakage across test runs.Suggested addition
beforeEach(() => { vi.clearAllMocks(); + localStorage.clear(); (useNavigate as unknown as Mock).mockReturnValue(mockNavigate); (useLocation as unknown as Mock).mockReturnValue({ pathname: '/admin' }); });
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/theme/ThemeProvider.tsx`:
- Around line 90-100: The context `value` object (used in
ThemeProviderContext.Provider) is recreated each render causing needless
consumer re-renders; wrap the creation of `value` in a useMemo that depends only
on `theme` (since `setTheme` is stable and `availableThemes` can be derived from
the module-level `themes`) and pass the memoized object to
ThemeProviderContext.Provider to avoid unnecessary re-renders.
♻️ Duplicate comments (1)
apps/web/src/lib/theme/ThemeProvider.tsx (1)
28-37: Catch block bypasses theme validation.Line 34 returns
defaultThemedirectly without validation, but line 32 properly validates viagetValidTheme(). If localStorage throws anddefaultThemeis an invalid key, the state will hold an invalid theme.Suggested fix
const [theme, setThemeState] = useState<string>( () => { try { const stored = localStorage.getItem(storageKey); return getValidTheme(stored); } catch { - return defaultTheme; + return getValidTheme(undefined); } } );
This was referenced Jan 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI Updates
Tests
✏️ Tip: You can customize this high-level summary in your review settings.