fix(theme-provider): callback deps, inline script, and onThemeChange#760
fix(theme-provider): callback deps, inline script, and onThemeChange#760paanSinghCoder wants to merge 1 commit intomainfrom
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.
|
📝 WalkthroughWalkthroughThis change adds an 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
| useEffect(() => { | ||
| if (!themeChangeMounted.current) { | ||
| themeChangeMounted.current = true; | ||
| return; | ||
| } | ||
| if (theme) { | ||
| const resolved = | ||
| theme === 'system' && resolvedTheme ? resolvedTheme : theme; | ||
| onThemeChangeRef.current?.(theme, resolved); | ||
| } | ||
| }, [theme, resolvedTheme]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the theme.tsx file to understand the full context
cat -n packages/raystack/components/theme-provider/theme.tsx | head -200Repository: raystack/apsara
Length of output: 6726
🏁 Script executed:
# Let's also check if there are any tests for this component
find . -type f -name "*.test.*" -o -name "*.spec.*" | grep -i theme | head -20Repository: raystack/apsara
Length of output: 139
🏁 Script executed:
# Search for onThemeChange usage and callback invocation patterns
rg "onThemeChange" -B 3 -A 3Repository: raystack/apsara
Length of output: 2857
🏁 Script executed:
# Let's examine the test file to understand existing test coverage
cat -n packages/raystack/components/theme-provider/__tests__/theme-provider.test.tsx | head -300Repository: raystack/apsara
Length of output: 10401
🏁 Script executed:
# Let's understand the flow better by checking getTheme and getSystemTheme functions
rg "function getTheme|const getTheme|export.*getTheme" -A 10Repository: raystack/apsara
Length of output: 886
🏁 Script executed:
# Check how resolvedTheme gets initialized and when it changes
rg "setResolvedTheme|resolvedTheme" -B 2 -A 2 | head -80Repository: raystack/apsara
Length of output: 6010
🏁 Script executed:
# Search for tests related to onThemeChange callback
rg "onThemeChange" packages/raystack/components/theme-provider/__tests__/ -B 5 -A 5Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Let's also look at the getSystemTheme function to understand when resolvedTheme gets populated
rg "function getSystemTheme|const getSystemTheme|export.*getSystemTheme" -A 15Repository: raystack/apsara
Length of output: 596
🏁 Script executed:
# Check the full test file to see if callback testing exists
wc -l packages/raystack/components/theme-provider/__tests__/theme-provider.test.tsxRepository: raystack/apsara
Length of output: 141
🏁 Script executed:
# Read the full test file to see all tests, especially any for onThemeChange
cat -n packages/raystack/components/theme-provider/__tests__/theme-provider.test.tsx | tail -100Repository: raystack/apsara
Length of output: 3389
🏁 Script executed:
# Let's trace through the execution by checking the exact initialization logic
cat -n packages/raystack/components/theme-provider/theme.tsx | sed -n '54-59p; 145-152p; 183-193p'Repository: raystack/apsara
Length of output: 111
🏁 Script executed:
# Let's trace through the execution by checking the exact initialization logic
cat -n packages/raystack/components/theme-provider/theme.tsx | sed -n '54,59p; 145,152p; 183,193p'Repository: raystack/apsara
Length of output: 980
🏁 Script executed:
# Now let's create a test scenario to understand the race condition
# Let's trace the exact sequence: what are the initial values and when do they change?
python3 << 'EOF'
print("=== Execution Trace ===")
print("\n1. Initial mount:")
print(" - theme: getTheme(storageKey, defaultTheme)")
print(" * storageKey='theme', defaultTheme='system'")
print(" * localStorage.getItem('theme') returns null (empty storage)")
print(" * So theme = 'system' (fallback)")
print(" - resolvedTheme: getTheme(storageKey)")
print(" * getTheme(storageKey) without fallback")
print(" * localStorage.getItem('theme') returns null")
print(" * So resolvedTheme = undefined (no fallback)")
print()
print("2. During initial render:")
print(" - useEffect at lines 145-152 runs (Listen to System preference)")
print(" * Calls handleMediaQuery(media) immediately")
print(" * handleMediaQuery calls getSystemTheme()")
print(" * getSystemTheme() returns 'dark' or 'light' based on matchMedia")
print(" * Calls setResolvedTheme(resolved)")
print(" * This triggers resolvedTheme state update")
print()
print("3. After resolvedTheme updates:")
print(" - useEffect at lines 183-193 runs again (onThemeChange callback)")
print(" * Dependencies: [theme, resolvedTheme]")
print(" * theme = 'system' (unchanged)")
print(" * resolvedTheme = 'dark' or 'light' (changed!)")
print(" * Check: themeChangeMounted.current = false (first run skips)")
print(" * WAIT: On second run when resolvedTheme changes:")
print(" - themeChangeMounted.current = true (was set in previous effect run)")
print(" - theme = 'system' (truthy)")
print(" - resolved = resolvedTheme (just populated)")
print(" - onThemeChangeRef.current?.('system', 'dark' or 'light')")
print(" - CALLBACK FIRES! Not supposed to fire on initial mount!")
print()
print("=== The Bug ===")
print("When theme='system' and storage is empty:")
print("1. First effect run: skip and set themeChangeMounted=true ✓")
print("2. handleMediaQuery fires and updates resolvedTheme")
print("3. Second effect run (triggered by resolvedTheme change):")
print(" - themeChangeMounted is already true")
print(" - condition skips: NO, because theme changed? NO, theme='system' unchanged")
print(" - But resolvedTheme did change, triggering this effect run!")
print(" - So the callback fires with (theme='system', resolved='dark/light')")
print(" - This is still during initial mount phase, VIOLATION OF CONTRACT!")
EOFRepository: raystack/apsara
Length of output: 2034
🏁 Script executed:
# Let's verify there's no test coverage for onThemeChange callback behavior
grep -n "onThemeChange" packages/raystack/components/theme-provider/__tests__/theme-provider.test.tsxRepository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Double-check the contract in types
cat -n packages/raystack/components/theme-provider/types.ts | grep -A 1 -B 1 "onThemeChange"Repository: raystack/apsara
Length of output: 316
onThemeChange fires during initial mount when theme === 'system' and storage is empty.
The callback effect (lines 183-193) runs twice during initial mount: once to skip (setting the flag), then again when resolvedTheme populates via the media query listener. On the second run, the flag is already true, so the callback executes with the resolved system theme, violating the documented contract "Not fired on initial mount."
The fix is to check whether resolved has a value before marking the mount as complete—ensuring the callback only fires after both theme and resolvedTheme are stable.
Proposed fix
const themeChangeMounted = useRef(false);
useEffect(() => {
- if (!themeChangeMounted.current) {
- themeChangeMounted.current = true;
- return;
- }
- if (theme) {
- const resolved =
- theme === 'system' && resolvedTheme ? resolvedTheme : theme;
- onThemeChangeRef.current?.(theme, resolved);
- }
+ const resolved = theme === 'system' ? resolvedTheme : theme;
+ if (!theme || !resolved) return;
+
+ if (!themeChangeMounted.current) {
+ themeChangeMounted.current = true;
+ return;
+ }
+
+ onThemeChangeRef.current?.(theme, resolved);
}, [theme, resolvedTheme]);Add a test: render with defaultTheme="system", empty storage, and an onThemeChange callback. Verify the callback does not fire on mount, then fires on user-initiated theme changes.
📝 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.
| useEffect(() => { | |
| if (!themeChangeMounted.current) { | |
| themeChangeMounted.current = true; | |
| return; | |
| } | |
| if (theme) { | |
| const resolved = | |
| theme === 'system' && resolvedTheme ? resolvedTheme : theme; | |
| onThemeChangeRef.current?.(theme, resolved); | |
| } | |
| }, [theme, resolvedTheme]); | |
| useEffect(() => { | |
| const resolved = theme === 'system' ? resolvedTheme : theme; | |
| if (!theme || !resolved) return; | |
| if (!themeChangeMounted.current) { | |
| themeChangeMounted.current = true; | |
| return; | |
| } | |
| onThemeChangeRef.current?.(theme, resolved); | |
| }, [theme, resolvedTheme]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/theme-provider/theme.tsx` around lines 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.
1 participant
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