The theme toggle should show the monitor icon when the system theme is selected
#45
Conversation
WalkthroughThe ModeToggle component is refactored to introduce an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
🚀 Preview DeploymentStatus: ✅ Ready! Preview URL: Open Preview Commit: Built and deployed successfully |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/mode-toggle.tsx`:
- Around line 12-14: Remove the unused getTheme prop from ThemeToggleProps and
the component's props list: delete the getTheme?: () => UserTheme declaration
and any references to getTheme in the mode-toggle component so the component
relies solely on the internal/user-selection state; update the ThemeToggleProps
type and the component signature (e.g., ThemeToggleProps,
ThemeToggle/ModeToggle) to reflect the removed prop and run type checks to
ensure no callers pass getTheme anymore.
🧹 Nitpick comments (1)
src/components/mode-toggle.tsx (1)
45-55: Consider harmonizing icon sizing/spacing in the radio items.The trigger uses a fixed icon size, while the menu items currently render icons without sizing or spacing classes. Consistent sizing avoids visual jumps.
♻️ Suggested tweak
- <DropdownMenuRadioItem value="light"> - <SunIcon /> + <DropdownMenuRadioItem value="light"> + <SunIcon className="mr-2 h-4 w-4" /> Light </DropdownMenuRadioItem> - <DropdownMenuRadioItem value="dark"> - <MoonIcon /> + <DropdownMenuRadioItem value="dark"> + <MoonIcon className="mr-2 h-4 w-4" /> Dark </DropdownMenuRadioItem> - <DropdownMenuRadioItem value="system"> - <MonitorIcon /> + <DropdownMenuRadioItem value="system"> + <MonitorIcon className="mr-2 h-4 w-4" /> System </DropdownMenuRadioItem>
| type ThemeToggleProps = { | ||
| getTheme?: () => UserTheme; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the mode-toggle.tsx file to understand the context
cd src/components && wc -l mode-toggle.tsxRepository: fulopkovacs/trytanstackdb.com
Length of output: 91
🏁 Script executed:
# Read the mode-toggle.tsx file to understand the implementation
cat -n src/components/mode-toggle.tsxRepository: fulopkovacs/trytanstackdb.com
Length of output: 2259
🏁 Script executed:
# Find all ModeToggle usages
rg -n -C3 '<ModeToggle'Repository: fulopkovacs/trytanstackdb.com
Length of output: 450
🏁 Script executed:
# Find getTheme prop assignments
rg -n -C3 'getTheme\s*='Repository: fulopkovacs/trytanstackdb.com
Length of output: 55
🏁 Script executed:
# Find UserTheme type definition in theme-provider
rg -n "type UserTheme|interface UserTheme" src/components/Repository: fulopkovacs/trytanstackdb.com
Length of output: 261
🏁 Script executed:
# Get more context around UserTheme definition
cat -n src/components/theme-provider.tsx | head -50Repository: fulopkovacs/trytanstackdb.com
Length of output: 2144
Document or remove the unused getTheme prop, and clarify its contract.
The optional getTheme parameter is never used in the codebase but could easily be misused if provided. If a caller passes a function returning a resolved theme (light/dark) instead of the user's selection (which could be "system"), the icon and radio group will display incorrect state. Either add inline documentation specifying that getTheme() must return the user-selected theme including "system", or remove the parameter if it's not needed.
🤖 Prompt for AI Agents
In `@src/components/mode-toggle.tsx` around lines 12 - 14, Remove the unused
getTheme prop from ThemeToggleProps and the component's props list: delete the
getTheme?: () => UserTheme declaration and any references to getTheme in the
mode-toggle component so the component relies solely on the
internal/user-selection state; update the ThemeToggleProps type and the
component signature (e.g., ThemeToggleProps, ThemeToggle/ModeToggle) to reflect
the removed prop and run type checks to ensure no callers pass getTheme anymore.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.