fix(theme-toggle): use functional setIsDark in toggleTheme (#1247)#1272
Conversation
…#1247) Closes MODSetter#1247. toggleTheme's previous implementation read isDark from the closure via setIsDark(!isDark), which forced isDark into the useCallback dependency array. As a result toggleTheme's reference changed on every click, invalidating any downstream memoization. Switched to the functional updater setIsDark((prev) => !prev) and dropped isDark from the dependency list. The sibling setCrazyLightTheme and setCrazyDarkTheme callbacks already use this pattern (they pass concrete values to setIsDark without listing isDark in deps), so this keeps the three theme callbacks consistent. No observable behavior change — clicking the theme toggle still flips isDark. The callback reference is now stable between clicks, which is also safer under concurrent updates per React's standard guidance.
|
@mvanhorn is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 2b2453e..8cf957b
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (1)
• surfsense_web/components/theme/theme-toggle.tsx
2 participants
Description
Swap
setIsDark(!isDark)→setIsDark((prev) => !prev)intoggleThemeand dropisDarkfrom theuseCallbackdependency list.Motivation and Context
FIX #1247
The previous implementation read
isDarkfrom the closure, which forced it into theuseCallbackdeps. Every theme click therefore produced a newtoggleThemereference, invalidating any downstream memoization.Functional updaters are the React-recommended pattern when a state transition is derived from the previous value (a boolean flip qualifies), and they're also safer under concurrent updates.
The sibling
setCrazyLightTheme/setCrazyDarkThemecallbacks already follow this pattern — they pass concrete values tosetIsDarkand don't listisDarkin deps. This PR bringstoggleThemein line with its two siblings.Screenshots
Not applicable — no visual change. Clicking the theme toggle still flips
isDarkthe same way.API Changes
Change Type
Testing Performed
Checklist
High-level PR Summary
This PR refactors the
toggleThemefunction to use a functional state updater pattern (setIsDark((prev) => !prev)) instead of readingisDarkdirectly from the closure. This removesisDarkfrom theuseCallbackdependency array, preventing unnecessary re-creation of thetoggleThemecallback on every theme change and improving memoization stability. The change alignstoggleThemewith the existing pattern used by its sibling functionssetCrazyLightThemeandsetCrazyDarkTheme.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_web/components/theme/theme-toggle.tsx