Skip to content

fix(theme-provider): callback deps, inline script, and onThemeChange #760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paanSinghCoder wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −8
Open

fix(theme-provider): callback deps, inline script, and onThemeChange #760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions packages/raystack/components/theme-provider/theme.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
useContext,
useEffect,
useMemo,
useRef,
useState
} from 'react';

Expand Down Expand Up @@ -47,7 +48,8 @@ const Theme = ({
nonce,
style = 'modern',
accentColor = 'indigo',
grayColor = 'gray'
grayColor = 'gray',
onThemeChange
}: ThemeProviderProps) => {
const [theme, setThemeState] = useState(() =>
getTheme(storageKey, defaultTheme)
Expand Down Expand Up @@ -124,7 +126,7 @@ const Theme = ({
// Unsupported
}
},
[forcedTheme]
[storageKey]
);

const handleMediaQuery = useCallback(
Expand All @@ -136,7 +138,7 @@ const Theme = ({
applyTheme('system');
}
},
[theme, forcedTheme]
[theme, forcedTheme, enableSystem, applyTheme]
);

// Always listen to System preference
Expand Down Expand Up @@ -171,6 +173,25 @@ const Theme = ({
applyTheme(forcedTheme ?? theme);
}, [forcedTheme, theme]);

// Fire onThemeChange on actual changes, skipping the initial mount.
// Ref keeps the latest callback without re-firing when consumers pass an inline function.
const onThemeChangeRef = useRef(onThemeChange);
Copy link
Copy Markdown
Contributor

@rohanchkrabrty rohanchkrabrty May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?, on each render the ref is getting updated

useEffect(() => {
onThemeChangeRef.current = onThemeChange;
});
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);
}
}, [theme, resolvedTheme]);
Comment on lines +183 to +193
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -200

Repository: 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 -20

Repository: raystack/apsara

Length of output: 139

🏁 Script executed:

# Search for onThemeChange usage and callback invocation patterns
rg "onThemeChange" -B 3 -A 3

Repository: 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 -300

Repository: 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 10

Repository: 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 -80

Repository: 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 5

Repository: 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 15

Repository: 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.tsx

Repository: 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 -100

Repository: 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!")
EOF

Repository: 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.tsx

Repository: 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.

Suggested change
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.


Comment on lines +183 to +194
Copy link
Copy Markdown
Contributor

@rohanchkrabrty rohanchkrabrty May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can call the onThemeChange in applyTheme or in applyTheme useEffect.
Right now there are two many useEffects

const providerValue = useMemo(
() => ({
theme,
Expand Down Expand Up @@ -277,7 +298,7 @@ const ThemeScript = memo(
setColorScheme = true
) => {
const resolvedName = value ? value[name] : name;
const val = literal ? name + `|| ''` : `'${resolvedName}'`;
const val = literal ? name : `'${resolvedName}'`;
let text = '';

// MUCH faster to set colorScheme alongside HTML attribute/class
Expand All @@ -293,13 +314,17 @@ const ThemeScript = memo(
}

if (attribute === 'class') {
if (literal || resolvedName) {
if (literal) {
text += `if(${val})c.add(${val})`;
Copy link
Copy Markdown
Contributor

@rohanchkrabrty rohanchkrabrty May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the if block needed in the generated script string?

} else if (resolvedName) {
text += `c.add(${val})`;
} else {
text += `null`;
}
} else {
if (resolvedName) {
if (literal) {
text += `if(${val})d[s](n,${val})`;
} else if (resolvedName) {
text += `d[s](n,${val})`;
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/raystack/components/theme-provider/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export interface UseThemeProps {
/** If `enableSystem` is true and the active theme is "system", this returns whether the system preference resolved to "dark" or "light". Otherwise, identical to `theme` */
resolvedTheme?: string;
/** If enableSystem is true, returns the System theme preference ("dark" or "light"), regardless what the active theme is */
systemTheme?: "dark" | "light";
systemTheme?: 'dark' | 'light';
}

export interface ThemeProviderProps {
Expand All @@ -33,7 +33,7 @@ export interface ThemeProviderProps {
/** Default theme name (for v0.0.12 and lower the default was light). If `enableSystem` is false, the default theme is light */
defaultTheme?: string;
/** HTML attribute modified based on the active theme. Accepts `class` and `data-*` (meaning any data attribute, `data-mode`, `data-color`, etc.) */
attribute?: string | "class";
attribute?: string | 'class';
/** Mapping of theme name to HTML attribute value. Object where key is the theme name and value is the attribute value */
value?: ValueObject;
/** Nonce string to pass to the inline script for CSP headers */
Expand All @@ -46,4 +46,6 @@ export interface ThemeProviderProps {
accentColor?: 'indigo' | 'orange' | 'mint';
/** Gray color variant for the theme, options are 'gray', 'mauve', or 'slate' */
grayColor?: 'gray' | 'mauve' | 'slate';
/** Called when the active theme changes. `resolvedTheme` is the actual applied theme (`'light'`/`'dark'` when `theme` is `'system'`). Not fired on initial mount. */
onThemeChange?: (theme: string, resolvedTheme: string) => void;
}
Loading