Fix ModeToggle rotation/restart and in-place centering (fixes #73)#74
Conversation
WalkthroughCSS animations for theme-toggle functionality added to globals.css, paired with a refactored ModeToggle component that manages animation state via refs and timeouts to control rotation and opacity transitions while respecting reduced motion preferences. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/mode-toggle.tsx (1)
67-70:⚠️ Potential issue | 🟡 MinorHardcoded strings should be externalized for internationalization.
The
aria-labeland screen-reader text are user-visible and should be externalized to resource files per project coding guidelines.As per coding guidelines: "User-visible strings should be externalized to resource files (i18n)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/mode-toggle.tsx` around lines 67 - 70, Externalize the user-visible strings used in the ModeToggle JSX: replace the hardcoded aria-label value (`aria-label={`Toggle color scheme (current: ${resolvedTheme ?? "light"})`}`) and the screen-reader text inside the <span className="sr-only">Toggle theme</span> with lookups to your i18n resource (e.g., use the project's translation function such as useTranslation/t or a localized strings module), passing resolvedTheme (falling back to the localized "light" key) into the interpolated aria-label; update the component to import and use the translation key names (e.g., "modeToggle.ariaLabel" and "modeToggle.label") instead of hardcoded English strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/mode-toggle.tsx`:
- Around line 44-47: The 20ms window.setTimeout call that calls
setIsSwitching(true) can fire after the component unmounts; store the timeout id
in a ref (e.g., switchingTimeoutRef) when you call window.setTimeout inside the
component (where setIsSwitching is used) and clear it in a useEffect cleanup (or
the componentWillUnmount equivalent) using
clearTimeout(switchingTimeoutRef.current) to avoid calling setIsSwitching on an
unmounted component; ensure you also reset the ref after clearing so repeated
toggles are handled correctly.
- Around line 15-18: The component sets a timeout via timeoutRef but lacks a
cleanup on unmount, so the pending timeout may call setIsSwitching(false) after
the component unmounts; add a useEffect cleanup that checks timeoutRef.current
and calls clearTimeout(timeoutRef.current) (and sets timeoutRef.current = null)
to prevent the callback from running on an unmounted component; reference the
existing timeoutRef, isSwitching/setIsSwitching and the useEffect hook to add
the cleanup return function.
---
Outside diff comments:
In `@components/mode-toggle.tsx`:
- Around line 67-70: Externalize the user-visible strings used in the ModeToggle
JSX: replace the hardcoded aria-label value (`aria-label={`Toggle color scheme
(current: ${resolvedTheme ?? "light"})`}`) and the screen-reader text inside the
<span className="sr-only">Toggle theme</span> with lookups to your i18n resource
(e.g., use the project's translation function such as useTranslation/t or a
localized strings module), passing resolvedTheme (falling back to the localized
"light" key) into the interpolated aria-label; update the component to import
and use the translation key names (e.g., "modeToggle.ariaLabel" and
"modeToggle.label") instead of hardcoded English strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c08ee497-44e9-48d5-b72d-65acceac3ee1
📒 Files selected for processing (2)
app/globals.csscomponents/mode-toggle.tsx
| const [isSwitching, setIsSwitching] = useState(false); | ||
| const timeoutRef = useRef<number | null>(null); | ||
|
|
||
| useEffect(() => setMounted(true), []); |
There was a problem hiding this comment.
Missing cleanup effect for timeout — potential memory leak.
If the component unmounts while the animation is running, the timeout callback will attempt to call setIsSwitching(false) on an unmounted component. Add a cleanup effect to clear the timeout.
🛠️ Proposed fix: Add cleanup effect
const timeoutRef = useRef<number | null>(null);
- useEffect(() => setMounted(true), []);
+ useEffect(() => {
+ setMounted(true);
+ return () => {
+ if (timeoutRef.current) {
+ window.clearTimeout(timeoutRef.current);
+ }
+ };
+ }, []);📝 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.
| const [isSwitching, setIsSwitching] = useState(false); | |
| const timeoutRef = useRef<number | null>(null); | |
| useEffect(() => setMounted(true), []); | |
| const [isSwitching, setIsSwitching] = useState(false); | |
| const timeoutRef = useRef<number | null>(null); | |
| useEffect(() => { | |
| setMounted(true); | |
| return () => { | |
| if (timeoutRef.current) { | |
| window.clearTimeout(timeoutRef.current); | |
| } | |
| }; | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/mode-toggle.tsx` around lines 15 - 18, The component sets a
timeout via timeoutRef but lacks a cleanup on unmount, so the pending timeout
may call setIsSwitching(false) after the component unmounts; add a useEffect
cleanup that checks timeoutRef.current and calls
clearTimeout(timeoutRef.current) (and sets timeoutRef.current = null) to prevent
the callback from running on an unmounted component; reference the existing
timeoutRef, isSwitching/setIsSwitching and the useEffect hook to add the cleanup
return function.
| // small tick to allow CSS animation to restart | ||
| window.setTimeout(() => { | ||
| setIsSwitching(true); | ||
| }, 20); |
There was a problem hiding this comment.
Untracked 20ms timeout could cause setState on unmounted component.
The restart timeout isn't stored in a ref, so it can't be cancelled on unmount. While the window is brief, consider tracking it for completeness.
🛠️ Proposed fix
- // small tick to allow CSS animation to restart
- window.setTimeout(() => {
- setIsSwitching(true);
- }, 20);
+ // small tick to allow CSS animation to restart
+ timeoutRef.current = window.setTimeout(() => {
+ setIsSwitching(true);
+ // Then schedule the end-of-animation timeout
+ timeoutRef.current = window.setTimeout(() => {
+ setIsSwitching(false);
+ timeoutRef.current = null;
+ }, 1800);
+ }, 20);
+ return; // Skip the timeout scheduling belowAlternatively, use a single ref or restructure the logic to chain timeouts properly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/mode-toggle.tsx` around lines 44 - 47, The 20ms window.setTimeout
call that calls setIsSwitching(true) can fire after the component unmounts;
store the timeout id in a ref (e.g., switchingTimeoutRef) when you call
window.setTimeout inside the component (where setIsSwitching is used) and clear
it in a useEffect cleanup (or the componentWillUnmount equivalent) using
clearTimeout(switchingTimeoutRef.current) to avoid calling setIsSwitching on an
unmounted component; ensure you also reset the ref after clearing so repeated
toggles are handled correctly.
| export function ModeToggle() { | ||
| const { setTheme, resolvedTheme } = useTheme(); | ||
| const [mounted, setMounted] = useState(false); | ||
| const [isSwitching, setIsSwitching] = useState(false); |
There was a problem hiding this comment.
Can you also improVe the dashboard button bg-colors during theme change? interchange the bg colors between themes.
2 participants
Summary
Recording.2026-04-01.162301.mp4
Testing