fix(theme-provider): callback deps, inline script, and onThemeChange#760
Conversation
…646) - Correct setTheme deps to track storageKey instead of unused forcedTheme. - Add missing enableSystem and applyTheme to handleMediaQuery deps. - Fix inline-script generation in updateDOM: drop the `name + "|| ''"` concat that produced classList.add('') (throws) or setAttribute(n, '') (junk attribute) when a value-map lookup missed; guard the literal path with if(${val}) so the DOM call only runs when the lookup is truthy. - Add onThemeChange prop. Fires on actual theme/resolvedTheme changes, skipping the initial mount. Uses a ref so the callback identity is read at fire time, decoupling effect cadence from consumer render churn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an optional onThemeChange callback to the ThemeProvider and invokes it (only after the initial mount) when theme or resolvedTheme changes, passing both the raw theme and the resolved light/dark theme. Updates useCallback dependency arrays for setTheme and handleMediaQuery to include storageKey and relevant flags/functions. Adjusts ThemeScript generation for literal values by removing the empty-string fallback and wrapping literal-mode DOM updates with conditional guards. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ThemeProvider
participant Storage as localStorage
participant MediaQuery as SystemPref
participant DOM as ThemeScript/DOM
participant Consumer as onThemeChange
User->>ThemeProvider: set theme / toggle
ThemeProvider->>Storage: persist theme (storageKey)
ThemeProvider->>MediaQuery: query/observe system preference (if enabled)
MediaQuery-->>ThemeProvider: resolved (light/dark)
ThemeProvider->>DOM: apply theme (attribute/class or data-theme)
ThemeProvider->>Consumer: (after mount) onThemeChange(theme, resolved)
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/raystack/components/theme-provider/theme.tsx`:
- Around line 183-193: The effect handling theme changes currently flips
themeChangeMounted.current on the first run even when theme === 'system' and
resolvedTheme is still undefined, causing onThemeChangeRef.current to fire when
resolvedTheme later populates; update the useEffect (the block referencing
themeChangeMounted, onThemeChangeRef, theme, resolvedTheme) to only treat the
initial run as "mounted" when a stable resolved value exists (i.e., if theme ===
'system' require resolvedTheme to be non-null before setting
themeChangeMounted.current = true), so the callback is not invoked on initial
mount, and add a unit test that renders with defaultTheme="system" and empty
storage asserting onThemeChange is not called on mount but is called on
subsequent user-initiated theme changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edf4cd10-e5af-479a-ae27-6f4572d173b6
📒 Files selected for processing (2)
packages/raystack/components/theme-provider/theme.tsxpackages/raystack/components/theme-provider/types.ts
rohanchkrabrty
left a comment
rohanchkrabrty
left a comment
There was a problem hiding this comment.
Also let's update the docs in theme/overview with the new props
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/www/src/content/docs/theme/overview/props.ts (1)
71-75: 💤 Low valueLGTM — type signature and JSDoc are accurate.
The
onThemeChangeprop is correctly typed and the JSDoc faithfully reflects the PR-stated contract (fires after mount, passes both raw and resolved themes).One minor doc nit: the JSDoc only calls out the
"system"→ resolved mapping but doesn't mention theforcedThemecase, whereresolvedThemewould equal the forced value regardless of user preference. Worth a one-liner addition for completeness, but not blocking.📝 Optional JSDoc enhancement
/** * Called when the active theme changes. `resolvedTheme` is the actual applied theme * (`"light"`/`"dark"` when `theme` is `"system"`). Not fired on initial mount. + * When `forcedTheme` is set, `resolvedTheme` reflects the forced value. */ onThemeChange?: (theme: string, resolvedTheme: string) => void;🤖 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 `@apps/www/src/content/docs/theme/overview/props.ts` around lines 71 - 75, Update the JSDoc for onThemeChange to mention the forcedTheme case: note that when a forcedTheme is set, resolvedTheme will equal that forced value regardless of user preference; keep the existing note about "system" mapping and retain the type signature onThemeChange?: (theme: string, resolvedTheme: string) => void. Refer to the onThemeChange declaration and include a one-line addition in its comment clarifying forcedTheme behavior.
🤖 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 `@apps/www/src/content/docs/theme/overview/props.ts`:
- Around line 71-75: Update the JSDoc for onThemeChange to mention the
forcedTheme case: note that when a forcedTheme is set, resolvedTheme will equal
that forced value regardless of user preference; keep the existing note about
"system" mapping and retain the type signature onThemeChange?: (theme: string,
resolvedTheme: string) => void. Refer to the onThemeChange declaration and
include a one-line addition in its comment clarifying forcedTheme behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83ab8049-1b50-4c45-bd45-810ba2e21cfd
📒 Files selected for processing (1)
apps/www/src/content/docs/theme/overview/props.ts
…tation for resolvedTheme
2 participants
Description
Addresses items 2, 3, and 5 from #646.
setThemenow tracksstorageKey(wasforcedTheme, unused).handleMediaQuerynow includesenableSystemandapplyTheme. Both fix latent stale-closure bugs that surface only when those props change at runtime.updateDOMno longer emitsc.add(x[e]|| ''). The literal path is gated withif(${val})so a missingvalue-map entry doesn't reachclassList.add('')(throws) orsetAttribute(n, '')(junk attr).onThemeChangecallback — fires on actualtheme/resolvedThemechanges, skips initial mount. Held via ref so consumer render churn doesn't over-fire and the callback never goes stale.Type of Change
How Has This Been Tested?
Manual repro on
mainvs branch for each fix. Tests TODO — leaving as draft untiltheme-provider.test.tsxis extended.Related Issues
Refs #646 (items 2, 3, 5).
🤖 Generated with Claude Code