Improve theme handling particularly for SSR sites#29
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR refactors ThemeProvider to add persistent theme selection and CSS-in-JS stylesheet generation. Theme selection is now persisted to localStorage under 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🤖 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.
Inline comments:
In `@src/components/ThemeProvider/README.md`:
- Around line 125-130: The README for injectThemePreflight is missing the
optional defaults parameter; update the `injectThemePreflight()` docs to include
the `defaults` argument (as implemented in ThemeProvider.store.ts) describing
its shape and purpose (e.g., an object to override the dark/light key names used
when the stored value is "__system"), show the full signature
`injectThemePreflight(defaults?)` and a short example of passing `{ light:
'light-key', dark: 'dark-key' }`, and mention behavior when omitted (falls back
to built-in keys and prefers-color-scheme).
In `@src/components/ThemeProvider/ThemeProvider.hooks.ts`:
- Line 17: Guard direct browser APIs used in the state initializers: change the
useState initializer in useResolvedTheme (function useResolvedTheme) so it does
not call window.matchMedia during SSR (e.g., use a safe default like false or
read window only when typeof window !== 'undefined'), and similarly guard the
localStorage.getItem call in ThemeProvider (component ThemeProvider) or defer
both reads into a useEffect/useLayoutEffect so they run only on the client;
ensure the first client-side useLayoutEffect that sets
document.documentElement.dataset.theme (the existing useLayoutEffect in
ThemeProvider) uses the same initial theme value that injectThemePreflight()
writes to avoid a flash/mismatch on first paint.
In `@src/components/ThemeProvider/ThemeProvider.store.ts`:
- Around line 49-57: getThemeStyleSheet() currently skips updating the DOM when
a <style data-theme-vars> exists and will append a duplicate when SSR injected
styles lack that attribute; change the DOM handling to locate an existing theme
style via document.querySelector('style[data-theme-vars], style[is="inline"]'),
and if found set its data-theme-vars attribute
(style.setAttribute('data-theme-vars', '')) and update existing.textContent =
css; otherwise create and append the new <style> as before—this ensures
re-registered themes update the stylesheet and SSR-inlined styles are tagged and
refreshed instead of duplicated.
- Around line 65-78: Convert the non-interpolated template literal fragments
inside the returned script into single-quoted strings to satisfy
`@stylistic/quotes`, keeping the template literals that include
${THEME_STORAGE_KEY}, ${dark}, and ${light} as-is; for every fragment you
convert (e.g., strings containing 'data-theme', '__system', "['data-theme']",
etc.) escape inner single quotes appropriately so the concatenation still
produces the same script; locate the script assembly around the returned string
in ThemeProvider.store (the helper function a, the localStorage read using
THEME_STORAGE_KEY, the lines referencing ${dark}/${light}, and the
MutationObserver/document.documentElement parts) and replace only the
non-interpolated template literals with single-quoted equivalents while
preserving the concatenation and original behavior.
In `@src/components/ThemeProvider/ThemeProvider.tsx`:
- Around line 40-46: Replace the client-only useLayoutEffect calls with
client-safe effects to avoid SSR warnings: change the two useLayoutEffect usages
around getThemeStyleSheet and the
document.documentElement.setAttribute('data-theme', resolvedKey) to useEffect
(or an isomorphic hook like useIsomorphicLayoutEffect) so DOM mutation only runs
on the client; update references to the existing getThemeStyleSheet invocation
and the effect that depends on resolvedKey (and/or import a shared
useIsomorphicLayoutEffect helper) to ensure behavior remains identical while
eliminating the SSR warning.
- Line 28: The useState initializer currently reads localStorage (const [key,
setKey] = useState(() => localStorage.getItem(THEME_STORAGE_KEY) ??
SYSTEM_THEME_KEY)), which crashes during SSR; change the initializer to default
to SYSTEM_THEME_KEY only, and then add a useEffect that runs on the client to
read localStorage.getItem(THEME_STORAGE_KEY) (guarding with typeof window !==
'undefined' or try/catch) and call setKey with the persisted value to hydrate;
keep THEME_STORAGE_KEY, SYSTEM_THEME_KEY, useState, setKey and the new useEffect
as the referenced symbols.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ee2242ac-7d84-4578-8b8c-63fb810e85bc
📒 Files selected for processing (6)
src/components/ThemeProvider/README.mdsrc/components/ThemeProvider/ThemeProvider.hooks.tssrc/components/ThemeProvider/ThemeProvider.store.tssrc/components/ThemeProvider/ThemeProvider.tsxsrc/components/ThemeProvider/ThemeProvider.types.tssrc/components/ThemeProvider/index.ts
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)
src/components/ThemeProvider/ThemeProvider.hooks.ts (1)
39-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
resolvedKeyaligned with the fallback theme.If
activeKeyis missing from the registry, this branch falls back to the lightthemeobject but still returns the invalid key asresolvedKey.ThemeProvider.tsxLines 46-48 then writes that stale key intodata-theme, while the generated stylesheet only has selectors for registered keys, so a stale persisted value or invalid forced theme can render without any matching CSS variables.🔧 Proposed fix
- return { - theme: registry[activeKey]?.theme ?? registry['light']?.theme ?? light, - resolvedKey: activeKey, - }; + const resolvedKey = registry[activeKey] ? activeKey : 'light'; + return { + theme: registry[resolvedKey]?.theme ?? light, + resolvedKey, + };🤖 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 `@src/components/ThemeProvider/ThemeProvider.hooks.ts` around lines 39 - 41, The returned resolvedKey must match the actual theme source instead of always echoing activeKey; update the return in ThemeProvider.hooks.ts so resolvedKey is set to the key that provided the theme (e.g., use activeKey if registry[activeKey] exists, otherwise use 'light' or the key you fell back to from registry) so ThemeProvider.tsx writes the correct data-theme that corresponds to the generated stylesheet selectors; adjust the logic around registry, activeKey, theme and resolvedKey to compute both values from the same branch.
🤖 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.
Inline comments:
In `@src/components/ThemeProvider/ThemeProvider.store.ts`:
- Around line 66-78: The injected preflight script built in ThemeProvider.store
(the returned string that defines function a) reads
localStorage.getItem('${THEME_STORAGE_KEY}') directly and can throw
SecurityError in sandboxed iframes; wrap the localStorage access in a try/catch
inside the injected function a so that on any error you treat it as if the value
were SYSTEM_THEME_KEY (i.e., fall back to the system branch using
window.matchMedia), and ensure the rest of the logic (setting
document.documentElement dataset and the MutationObserver) remains unchanged;
update the string-building expression where THEME_STORAGE_KEY and
SYSTEM_THEME_KEY are referenced to include this guarded read.
In `@src/components/ThemeProvider/ThemeProvider.tsx`:
- Around line 42-44: The effect that calls getThemeStyleSheet() runs only once
but needs to re-run whenever the theme registry changes; update the
useIsomorphicLayoutEffect in ThemeProvider.tsx to depend on the reactive
registry from useStore(themeStore) (or the specific registry variable returned
by useStore) so getThemeStyleSheet() regenerates whenever registerTheme() adds
themes, ensuring newly registered themes produce the corresponding
:root[data-theme="..."] blocks.
---
Outside diff comments:
In `@src/components/ThemeProvider/ThemeProvider.hooks.ts`:
- Around line 39-41: The returned resolvedKey must match the actual theme source
instead of always echoing activeKey; update the return in ThemeProvider.hooks.ts
so resolvedKey is set to the key that provided the theme (e.g., use activeKey if
registry[activeKey] exists, otherwise use 'light' or the key you fell back to
from registry) so ThemeProvider.tsx writes the correct data-theme that
corresponds to the generated stylesheet selectors; adjust the logic around
registry, activeKey, theme and resolvedKey to compute both values from the same
branch.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a4b4c11e-270b-4a00-adb6-7e9a7d36ff8c
📒 Files selected for processing (6)
src/components/ThemeProvider/README.mdsrc/components/ThemeProvider/ThemeProvider.constants.tssrc/components/ThemeProvider/ThemeProvider.hooks.tssrc/components/ThemeProvider/ThemeProvider.store.tssrc/components/ThemeProvider/ThemeProvider.tsxsrc/components/ThemeProvider/index.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ThemeProvider/ThemeProvider.hooks.ts (1)
48-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the fallback key when the registry lookup misses.
If
activeKeycomes from stale or user-editedlocalStorage, Line 49 falls back thethemeobject to light but Line 50 still returns the invalid key.ThemeProvider.tsxthen writes that unknown value todata-theme, so the generated stylesheet has no matching block while the context says “light”. ResolvethemeandresolvedKeyfrom the same fallback path.🐛 Proposed fix
if (activeKey === SYSTEM_THEME_KEY) { const resolvedKey = isDark ? 'dark' : 'light'; return { theme: registry[resolvedKey]?.theme ?? registry['light']?.theme ?? light, resolvedKey, }; } + const resolvedKey = registry[activeKey] ? activeKey : 'light'; return { - theme: registry[activeKey]?.theme ?? registry['light']?.theme ?? light, - resolvedKey: activeKey, + theme: registry[resolvedKey]?.theme ?? registry['light']?.theme ?? light, + resolvedKey, }; };🤖 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 `@src/components/ThemeProvider/ThemeProvider.hooks.ts` around lines 48 - 50, The return currently picks theme from registry[activeKey] but always returns resolvedKey = activeKey, which can be invalid; change the logic so both theme and resolvedKey come from the same fallback path: compute a resolvedKey variable (e.g., resolvedKey = registry[activeKey] ? activeKey : 'light') and then return theme = registry[resolvedKey]?.theme ?? light and resolvedKey = resolvedKey so ThemeProvider.tsx writes a matching data-theme; update the code in ThemeProvider.hooks.ts where registry, activeKey and resolvedKey are used accordingly.
🤖 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.
Inline comments:
In `@src/components/ThemeProvider/ThemeProvider.hooks.ts`:
- Around line 30-32: Replace the direct useLayoutEffect inside the
useResolvedTheme hook with the SSR-safe isomorphic layout effect used elsewhere:
import and use the same isomorphic helper (the one used by ThemeProvider.tsx,
e.g. useIsomorphicLayoutEffect) instead of React's useLayoutEffect around the
setIsDark(window.matchMedia(...).matches) call so the effect runs client-side
without causing SSR warnings; update the import and call in useResolvedTheme
accordingly, keeping the setIsDark and media query logic unchanged.
In `@src/components/ThemeProvider/ThemeProvider.store.ts`:
- Around line 40-43: The code silently falls back to light when a provided
parentKey is missing; update the themeStore.setState callback (where parentKey,
key, deepmerge, makeEntry and light are used) to detect when parentKey is
non-empty but state[parentKey] is undefined and then surface that (either throw
a clear Error mentioning parentKey and key or at minimum console.warn/log with
the missing parentKey and affected key) instead of silently using light; ensure
the check runs before computing parent and merged so typos/missing parents are
obvious during development.
- Around line 35-43: registerTheme currently accepts arbitrary keys that are
later interpolated into the CSS selector in getThemeStyleSheet, allowing
characters like " or ] or newlines to break the stylesheet; fix by validating
and/or escaping keys: in registerTheme validate the incoming key against a safe
pattern (e.g. allow only alphanumerics, hyphen, underscore) and throw a clear
error for invalid keys, or normalize/sanitize the key before storing it in
themeStore (reference registerTheme, themeStore, makeEntry); additionally, to be
defensive update getThemeStyleSheet to use a CSS-safe escaping function when
serializing the key into :root[data-theme="..."] so any future callers cannot
inject invalid selectors (reference getThemeStyleSheet).
- Around line 103-132: The inline script built by injectThemePreflight embeds
defaults.dark and defaults.light directly into single-quoted JS which can be
broken by quotes, backslashes, newlines or a `</script>` sequence; update
injectThemePreflight to serialize and escape these values before interpolation
(e.g. use a JS string literal serializer like JSON.stringify or a small escape
helper on defaults?.dark and defaults?.light), then also ensure the resulting
string replaces `</script>` with `<\/script>` to avoid terminating the tag
early; keep references to SYSTEM_THEME_KEY and THEME_STORAGE_KEY unchanged and
interpolate the sanitized values instead of raw defaults.dark/defaults.light.
In `@src/components/ThemeProvider/ThemeProvider.tsx`:
- Around line 50-55: The handleSetTheme function currently calls
localStorage.setItem without guarding against exceptions; wrap the setItem call
in a try/catch inside handleSetTheme (while preserving the forcedTheme check) so
that any storage errors (e.g., QuotaExceededError or Safari private mode) are
swallowed/logged but do not prevent calling setKey(newKey); use
THEME_STORAGE_KEY for the key and ensure setKey(newKey) always runs when not
forcedTheme even if setItem throws.
---
Outside diff comments:
In `@src/components/ThemeProvider/ThemeProvider.hooks.ts`:
- Around line 48-50: The return currently picks theme from registry[activeKey]
but always returns resolvedKey = activeKey, which can be invalid; change the
logic so both theme and resolvedKey come from the same fallback path: compute a
resolvedKey variable (e.g., resolvedKey = registry[activeKey] ? activeKey :
'light') and then return theme = registry[resolvedKey]?.theme ?? light and
resolvedKey = resolvedKey so ThemeProvider.tsx writes a matching data-theme;
update the code in ThemeProvider.hooks.ts where registry, activeKey and
resolvedKey are used accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: eef8ffa3-c33a-42d0-a5f2-860485b68504
📒 Files selected for processing (6)
src/components/ThemeProvider/README.mdsrc/components/ThemeProvider/ThemeProvider.constants.tssrc/components/ThemeProvider/ThemeProvider.hooks.tssrc/components/ThemeProvider/ThemeProvider.store.tssrc/components/ThemeProvider/ThemeProvider.tsxsrc/components/ThemeProvider/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Improvements