feat: add theme persistence with custom storage key#136
Conversation
- Add storageKey='theme-preference' to ThemeProvider for explicit localStorage key - Add aria-label to theme toggle button showing current theme - Add aria-hidden to sun/moon icons for better accessibility
📝 WalkthroughWalkthrough
ChangesTheme Toggle Accessibility and Storage
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the accessibility of the ThemeToggle component by adding aria-labels and hiding decorative icons from screen readers, and configures a custom storage key for the ThemeProvider. The review feedback suggests enhancing the button's aria-label to describe both the action and the current state, and improving type safety by avoiding unsafe type assertions when resolving the current theme.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const currentTheme = mounted ? resolvedTheme : 'system' | ||
|
|
||
| return ( | ||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button className="relative" size="icon" suppressHydrationWarning variant="outline"> | ||
| <Sun className={`h-[1.2rem] w-[1.2rem] transition-all ${isDark ? 'rotate-90 scale-0' : 'rotate-0 scale-100'}`} /> | ||
| <Button | ||
| aria-label={`Current theme: ${THEME_LABELS[currentTheme as keyof typeof THEME_LABELS] || 'System'}`} |
There was a problem hiding this comment.
Accessibility & Type Safety Improvements
-
Accessibility (Button Action): A button's accessible name should describe its action (what it does), not just its current state. Setting
aria-labelto"Current theme: Light"overrides the inner<span className="sr-only">Toggle theme</span>text, leaving screen reader users with a button labeled only with the state, which is confusing. Combining the action and the state (e.g.,"Toggle theme. Current theme: Light") provides a much better user experience. -
Type Safety: The type assertion
currentTheme as keyof typeof THEME_LABELSis unsafe becauseresolvedThemecan beundefined(e.g., during initialization or SSR), which would makecurrentThemeundefinedat runtime. We can resolve this cleanly and safely by explicitly checking the resolved theme value, which also eliminates the need for any type assertions.
| const currentTheme = mounted ? resolvedTheme : 'system' | |
| return ( | |
| <DropdownMenu> | |
| <DropdownMenuTrigger asChild> | |
| <Button className="relative" size="icon" suppressHydrationWarning variant="outline"> | |
| <Sun className={`h-[1.2rem] w-[1.2rem] transition-all ${isDark ? 'rotate-90 scale-0' : 'rotate-0 scale-100'}`} /> | |
| <Button | |
| aria-label={`Current theme: ${THEME_LABELS[currentTheme as keyof typeof THEME_LABELS] || 'System'}`} | |
| const currentTheme = (mounted && resolvedTheme === 'dark') ? 'dark' : (mounted && resolvedTheme === 'light') ? 'light' : 'system' | |
| return ( | |
| <DropdownMenu> | |
| <DropdownMenuTrigger asChild> | |
| <Button | |
| aria-label={`Toggle theme. Current theme: ${THEME_LABELS[currentTheme]}`} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/src/components/common/ThemeToggle.tsx (1)
26-26: ⚡ Quick winType assertion may be unsafe if
themecan have unexpected values.The
as ThemeModecast assumesthemewill always be one of'light' | 'dark' | 'system'. If next-themes can return other theme values (e.g., custom themes), accessingTHEME_LABELS[currentTheme]on line 32 could fail.Consider defensive coding:
🛡️ Safer implementation with fallback
- const currentTheme = (mounted && theme ? theme : 'system') as ThemeMode + const currentTheme: ThemeMode = (mounted && theme && theme in THEME_LABELS ? theme as ThemeMode : 'system')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/components/common/ThemeToggle.tsx` at line 26, The type assertion `as ThemeMode` on the currentTheme assignment is unsafe because it assumes the theme value from next-themes will always be one of the expected values without validation. If next-themes returns an unexpected theme value, accessing THEME_LABELS[currentTheme] will fail. Remove the unsafe type assertion and instead add validation logic that checks whether the resolved theme value is one of the valid ThemeMode values ('light', 'dark', 'system'), and provide a fallback to 'system' if the theme is not recognized. This ensures the code handles unexpected theme values gracefully without runtime errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ui/src/components/common/ThemeToggle.tsx`:
- Line 26: The type assertion `as ThemeMode` on the currentTheme assignment is
unsafe because it assumes the theme value from next-themes will always be one of
the expected values without validation. If next-themes returns an unexpected
theme value, accessing THEME_LABELS[currentTheme] will fail. Remove the unsafe
type assertion and instead add validation logic that checks whether the resolved
theme value is one of the valid ThemeMode values ('light', 'dark', 'system'),
and provide a fallback to 'system' if the theme is not recognized. This ensures
the code handles unexpected theme values gracefully without runtime errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2aead1b2-6f70-435a-b44b-0acbd1360617
📒 Files selected for processing (2)
ui/src/components/common/ThemeToggle.tsxui/src/providers/providers.tsx
1 participant
Summary
Changes
Summary by CodeRabbit