feat: RFC theme improvements - #880
Conversation
Proposes a rewrite of the Theme component and a restructuring of the token stylesheets. Moves token mounting off <html> onto a theme element so root, scoped, and portalled themes share one mechanism, and adds the customisation surface tracked in #578 — appearance, accent, gray, radius, scaling, panel background, reduced motion and font families — plus per-component radius overrides. Retires the `style` prop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak6Gi4GPEXvYyGZ3mrPMBW
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe RFC proposes a breaking rewrite of Apsara’s theme system. It defines element-mounted tokens, SSR and hydration behavior, controlled and uncontrolled settings, persistence, appearance resolution, scaling, radius, surfaces, motion, fonts, and palettes. It also specifies portal theme reinjection, component radius overrides, API removals, migration impacts, implementation phases, testing requirements, alternatives, and rejected approaches. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
docs/rfcs/004-theme-improvements.md (1)
391-409: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd downstream
ThemeSwitcherand portal-container tests.Existing tests at
packages/raystack/components/theme-provider/__tests__/theme.test.tsx:788-901cover icon state, persistence, accessible names, and keyboard activation. Add migrated tests for the newvalue/resolvedAPI, live system appearance, same-document updates, portalcontainerplacement, and component-radius precedence.🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 391 - 409, Extend the existing ThemeSwitcher tests around the value/resolved API to cover live system appearance and same-document updates, and add portal-container tests verifying placement in the supplied container. Also add component-radius precedence coverage, preserving the existing icon, persistence, accessibility, and keyboard behavior tests.
🤖 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 `@docs/rfcs/004-theme-improvements.md`:
- Around line 381-383: Revise the Phase 1 description to remove the claims that
it is invisible and has no behavioral delta. Document the consumer impact of
moving spacing, effects, and z-index tokens and the cascade change from wrapping
declarations in :where(), or specify temporary fallback tokens and atomic phase
delivery to preserve compatibility.
- Around line 10-12: Clarify the retired API in the RFC by distinguishing the
Theme component’s removed style prop from supported token customization
mechanisms. Update the migration guidance and examples around the Theme
style-prop retirement and the `--rs-*` usage to explicitly identify whether
customization uses a rendered DOM style attribute, a `.rs-theme` CSS rule, or
another supported mechanism, and keep the documented mechanism consistent
throughout.
- Around line 187-203: Update the inline script emission rules and the “The
Inline Script” description so they are based on server/client nondeterminism
rather than persistence alone. Include every uncontrolled setting whose client
value may differ from the server-rendered value, including persisted enumerated
settings and appearance values resolved from system preference, or explicitly
document the chosen fallback behavior and keep the shared configuration
consistent for both the script and React reader.
- Around line 189-197: Update the RFC’s inline-script design to require enum
validation for fontFamily, fontFamilyTitle, fontFamilyMono, and persist
setting-key arrays before use. Require context-safe serialization of all
embedded values, including escaping quotes and the </script> sequence, and add
tests covering hostile persisted values.
- Around line 143-147: Update the RFC’s storage documentation around the
storageKey setting to define the default namespace and exact per-setting key
format, using the existing theme storage convention as the reference. Add
explicit migration behavior for existing theme data, including whether migration
is automatic, opt-in, or results in a reset when adopting the new format.
- Around line 183-185: Update the setting write path described in the
useSyncExternalStore section to notify internal subscribers before persisting
the new value, since same-page writes do not emit storage events. Retain the
existing storage-event listener to preserve cross-tab synchronization, and
ensure subscribers receive the updated primitive value.
- Around line 225-251: Update the theme token definitions described in the
Radius and scaling sections so every rs-theme scope recomputes scaled spacing
and radius tokens from raw base values using that scope’s own --rs-scaling and
radius factor, rather than inheriting already-resolved ancestor values. Define
scaling as a unitless multiplier such as 0.9 wherever it participates in calc(),
and document the updated value format consistently.
---
Nitpick comments:
In `@docs/rfcs/004-theme-improvements.md`:
- Around line 391-409: Extend the existing ThemeSwitcher tests around the
value/resolved API to cover live system appearance and same-document updates,
and add portal-container tests verifying placement in the supplied container.
Also add component-radius precedence coverage, preserving the existing icon,
persistence, accessibility, and keyboard behavior tests.
🪄 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 Plus
Run ID: 7fea38a7-4292-4188-a2ba-9102fc78647f
📒 Files selected for processing (1)
docs/rfcs/004-theme-improvements.md
| | `defaultValue` | Partial settings. Seeds uncontrolled keys. Persisted and settable at runtime. | | ||
| | `value` | Partial settings. Controlled keys — always win, never persisted, never written by the script. | | ||
| | `onValueChange` | Fires with the full next settings object and the changed subset. | | ||
| | `persist` | `true`, `false`, or an array of setting keys. Default `true`. | | ||
| | `storageKey` | Storage key prefix. One key per setting, suffixed by setting name. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)docs/rfcs/004-theme-improvements\.md$|(^|/)theme-provider|theme-provider.*test|theme.*test|storageKey|persist' || true
echo
echo "== docs excerpt =="
if [ -f docs/rfcs/004-theme-improvements.md ]; then
nl -ba docs/rfcs/004-theme-improvements.md | sed -n '120,215p'
fi
echo
echo "== search storageKey/default storage =="
rg -n "storageKey|persist|localStorage|getItem|setItem|removeItem|theme\"" . -S --glob '!node_modules' --glob '!dist' --glob '!build' | head -200Repository: raystack/apsara
Length of output: 549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs excerpt =="
awk '{printf "%d\t%s\n", NR, $0}' docs/rfcs/004-theme-improvements.md | sed -n '120,215p'
echo
echo "== implementation relevant searches =="
for f in packages/raystack/components/theme-provider/*.tsx packages/raystack/components/theme-provider/*.ts; do
echo "--- $f"
grep -n -E "storageKey|persist|localStorage|getItem|setItem|removeItem|theme" "$f" || true
done
echo
echo "== test relevant lines around theme storage =="
awk '{printf "%d\t%s\n", NR, $0}' packages/raystack/components/theme-provider/__tests__/theme.test.tsx | sed -n '730,930p'
echo
echo "== full source snippets for theme/provider maybe =="
wc -l packages/raystack/components/theme-provider/*.tsx packages/raystack/components/theme-provider/*.tsRepository: raystack/apsara
Length of output: 22281
Define the default storage namespace, per-setting key format, and migration behavior.
storageKey is described as a prefix for one key per setting, but the RFC does not state the default namespace or exact generated keys. Existing code and tests use the theme key for the single-theme model, so the default prefix/key format should be explicit and the RFC should document how existing theme storage migrates, whether consumers can opt in, or that using the new format causes a reset.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 143 - 147, Update the RFC’s
storage documentation around the storageKey setting to define the default
namespace and exact per-setting key format, using the existing theme storage
convention as the reference. Add explicit migration behavior for existing theme
data, including whether migration is automatic, opt-in, or results in a reset
when adopting the new format.
| React state uses `useSyncExternalStore`, because it is the only primitive that reads storage during the first render under CSR without poisoning hydration under SSR. Its server snapshot returns the seed, so the hydration render matches the server; its client snapshot reads storage. Under CSR there is no hydration, so the first render — and therefore the first paint — is already correct with no script involved. Under SSR the script has already corrected the DOM, and the post-hydration snapshot returns the same value, so nothing moves. Its subscribe function listens to the `storage` event, which delivers cross-tab sync for free and replaces the two hand-rolled listeners in the current implementation. | ||
|
|
||
| Each setting is stored under its own key holding a primitive, rather than one JSON blob, so snapshots compare by value and do not loop. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files of interest:"
git ls-files | rg '(^|/)(004-theme-improvements|theme|settings|storage|store)' || true
echo
echo "RFC excerpt:"
sed -n '150,205p' docs/rfcs/004-theme-improvements.md || true
echo
echo "Search useSyncExternalStore / storage event / setValue:"
rg -n "useSyncExternalStore|storage event|localStorage\.setItem|setItem\(|sync|setValue|subscribe\(" . -g '!node_modules' -g '!dist' -g '!build' | head -200Repository: raystack/apsara
Length of output: 30948
🌐 Web query:
MDN window.onstorage storage event same tab localStorage setItem documentation
💡 Result:
According to MDN documentation, the storage event does not fire in the same tab (or window) where the localStorage.setItem method was called [1][2]. The storage event is specifically designed as a mechanism for different documents sharing the same origin—such as other tabs or windows—to synchronize their state when a change is made to localStorage [1][2]. When a document updates its localStorage, the browser notifies other browsing contexts of the same origin by firing a storage event on their respective Window objects [1][3]. Key points regarding this behavior: - Scope: The event is intentionally excluded from the window that initiated the storage change [1]. - Use Case: It is intended for cross-tab or cross-window communication within the same origin [2]. - Workaround: If you need to detect changes within the same tab, you must manually trigger the logic or use a wrapper around your localStorage calls, as the native storage event will not trigger in that environment [4]. Some developers choose to manually dispatch a custom event or a simulated StorageEvent in the same tab to achieve this behavior [4].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Window/storage_event
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API/Using_the_Web_Storage_API
- 3: https://html.spec.whatwg.org/multipage/webstorage.html
- 4: https://stackoverflow.com/questions/35865481/storage-event-not-firing
Notify internal subscribers after setting values.
A storage event is only emitted in other browsing contexts; writes from the current page do not dispatch it. Update the write path to also notify subscribers before persisting, and keep the storage listener for cross-tab sync.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 183 - 185, Update the
setting write path described in the useSyncExternalStore section to notify
internal subscribers before persisting the new value, since same-page writes do
not emit storage events. Retain the existing storage-event listener to preserve
cross-tab synchronization, and ensure subscribers receive the updated primitive
value.
| | Scales with `--rs-scaling` | Does not scale | | ||
| |---|---| | ||
| | `--rs-space-1` … `--rs-space-17` | Border and divider widths | | ||
| | `--rs-radius-1` … `--rs-radius-6` | Font weights | | ||
| | `--rs-font-size-*`, including title and mono steps | Letter spacing — already `em`-based, so it scales implicitly | | ||
| | `--rs-line-height-*` | | | ||
| | Intrinsic component dimensions with no matching token | | | ||
|
|
||
| Roughly 113 of the 357 raw pixel literals in component CSS are hairline borders and dividers, which must not scale. The remaining fixed dimensions are a tail of around forty and get triaged individually: an intrinsic control height scales, a truncation guard's `max-width` does not. | ||
|
|
||
| `--rs-scaling` is set absolutely per value rather than multiplied, so a 90% scope nested inside a 90% scope remains 90%. | ||
|
|
||
| Spacing, effects, and z-index move off `:root` onto the theme selector as part of this, which is what makes them scopable. | ||
|
|
||
| #### Radius | ||
|
|
||
| Radius becomes a factor applied to a fixed base scale, replacing the two hardcoded scales currently keyed off `data-style`. Two variables drive it: a numeric factor, and a separate full-radius length that pill-shaped elements select with `max()`. | ||
|
|
||
| | `radius` | Factor | Full | | ||
| |---|---|---| | ||
| | `none` | 0 | 0 | | ||
| | `small` | 0.75 | 0 | | ||
| | `medium` | 1 | 0 | | ||
| | `large` | 1.5 | 0 | | ||
| | `full` | 1.5 | pill | | ||
|
|
||
| Each `--rs-radius-N` derives from its base step multiplied by `--rs-scaling` and the factor. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate RFC =="
fd -a '004-theme-improvements\.md$' . || true
echo
echo "== relevant RFC excerpt =="
if [ -f docs/rfcs/004-theme-improvements.md ]; then
sed -n '1,320p' docs/rfcs/004-theme-improvements.md | nl -ba | sed -n '200,270p'
fi
echo
echo "== search for scaling/radius token declarations/usages =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"\-\-(rs.scaling|rs-(space|radius|font-size|line-height|[A-Z0-9_-]+))|rs\.scaling|rs-radius|data-style|--rs-scaling|--rs-radius" . | head -n 300Repository: raystack/apsara
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RFC relevant excerpt =="
if [ -f docs/rfcs/004-theme-improvements.md ]; then
sed -n '200,270p' docs/rfcs/004-theme-improvements.md | cat -n
fi
echo
echo "== search for scaling/radius declarations/usages =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"\-\-(rs.scaling|rs-(space|radius|font-size|line-height|[A-Z0-9_-]+))|rs\.scaling|rs-radius|data-style|--rs-scaling|--rs-radius" . | head -n 500
echo
echo "== repository files around RFC and CSS/token =="
git ls-files | rg '(^|/)(styles?|themes?|.*theme.*|.*\.css$|package\.json)$' | head -n 200Repository: raystack/apsara
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== theme provider relevant sections =="
if [ -f packages/raystack/components/theme-provider/theme.tsx ]; then
sed -n '1,290p' packages/raystack/components/theme-provider/theme.tsx | cat -n
sed -n '480,570p' packages/raystack/components/theme-provider/theme.tsx | cat -n
fi
echo
echo "== occurrences of --rs-scaling/current css variable declarations =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"\-\-[rs_.-]*(scale|radius|spacing|space|font|line-height|color|surface|effect|z-index).*[:=]|scaling|--rs-scaling|rs-scaling|--rs-radius-factor|--rs-radius-full" . | head -n 300
echo
echo "== node CSS variable inheritance probe =="
node - <<'JS'
const css = `
.root { --base: 8px; --scaled: var(--base); --base: calc(var(--base) * 0.9); }
.nested { --base: calc(var(--base) * 0.9); }
`;
const re = /--(\w[-\w]*)\s*:\s*([^;{}]+)/g;
const decls = [...css.matchAll(re)].map(m => ({ var: m[1], value: m[2].trim() }));
console.log(JSON.stringify({
parentDeclaredScaledBeforeParentScaled = { --base: 8px, --scaled: var(--base) },
parentReDeclaresBaseAfterScaled: { --base: calc(var(--base) * 0.9) },
nestedReDeclaresBase: { --base: calc(var(--base) * 0.9) },
inherited --scaled: var(--base) // resolves to parent's current --base: 8px, not nested
}, null, 2));
JSRepository: raystack/apsara
Length of output: 48992
🌐 Web query:
CSS specification custom property used value order of operations inherited properties nested declaration updates inherited values
💡 Result:
In CSS, custom properties (variables) are processed according to the standard cascade and inheritance rules [1][2][3]. Their processing order and interaction with inherited values are summarized below: Cascade and Resolution Custom properties are ordinary CSS properties [1][3]. When a custom property is declared, it is subject to the normal cascade process (origin, importance, specificity, and order of appearance) [4][5]. The computed value of a custom property is the specified value with any var functions substituted [1][3]. Inheritance By default, custom properties defined with the --* syntax are inherited [1][2]. When an element does not have a cascaded value for a custom property, it inherits the computed value from its parent [6]. For the root element, the inherited value is the initial value, which is the guaranteed-invalid value [1][6][3]. If a custom property is registered using @property, the author can explicitly control inheritance using the inherits descriptor (which defaults to true unless otherwise specified) [7][2]. Used Value Calculation The used value of a property that consumes a custom property is determined after variable substitution occurs [1][3]. Because custom properties are resolved based on the computed value of the property on the same element, they do not inherit the "raw" variable reference, but rather the result of the substitution at the parent level [5][8]. This means that if a custom property is used to define a complex value (like a calculation), the calculation is performed on the parent, and only the resulting computed value is inherited by the child [8]. Nested Declaration Updates CSS Nesting allows style rules to be nested, which maintains the same selector context and cascade behavior as non-nested rules [9][10]. When declarations (including custom property definitions) are nested, they are treated as part of the style rule's declaration block [9]. In the CSS Object Model (CSSOM), these are represented by the CSSNestedDeclarations interface, which ensures that nested rules are parsed and evaluated in the order they are declared [11]. Consequently, updating a custom property value within a nested declaration block follows standard order-of-appearance rules; later definitions within the same or nested scope will override earlier ones if they have equal specificity [4][12]. Top_results: [1][6][4][3][9][11]
Citations:
- 1: https://drafts.csswg.org/css-variables-1/
- 2: https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Cascading_variables/Using_custom_properties
- 3: https://drafts.csswg.org/css-variables/
- 4: https://www.w3.org/TR/css-cascade-6/
- 5: https://stackoverflow.com/questions/35375560/how-do-css-custom-properties-cascade
- 6: https://www.w3.org/TR/css-cascade/
- 7: https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Properties_and_values_API/Registering_properties
- 8: https://www.smashingmagazine.com/2019/07/css-custom-properties-cascade/
- 9: https://www.w3.org/TR/css-nesting-1/
- 10: https://drafts.csswg.org/css-nesting-1/
- 11: https://developer.mozilla.org/en-US/docs/Web/API/CSSNestedDeclarations
- 12: https://github.com/web-platform-tests/wpt/blob/master/css/css-nesting/nested-declarations-matching.html
Recompute scaled tokens from raw base values on each theme scope.
CSS custom properties inherit resolved values, so deriving --rs-space-* and radius tokens from ancestor scalers/factors does not update when only --rs-scaling or the radius factor changes on a scoped wrapper. Re-declare scaled tokens from raw base values on every rs-theme element, or use an equivalent per-element calculation, and document scaling as a unitless multiplier such as 0.9 instead of percent if it is multiplied in token calc().
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 225 - 251, Update the theme
token definitions described in the Radius and scaling sections so every rs-theme
scope recomputes scaled spacing and radius tokens from raw base values using
that scope’s own --rs-scaling and radius factor, rather than inheriting
already-resolved ancestor values. Define scaling as a unitless multiplier such
as 0.9 wherever it participates in calc(), and document the updated value format
consistently.
| **1 — Token foundation.** Wrap every `--rs-*` declaration in `:where()`. Move spacing, effects, and z-index off `:root` onto the theme selector. Introduce `--rs-scaling`, the radius factor and full-radius variables, and the surface token family. Give each accent its own selector. Add `sage` to the gray union. | ||
|
|
||
| This phase is invisible to consumers. The current provider writes `data-theme` to `<html>`, and a `:where([data-theme])` rule matches it exactly as `:root` did — so the widest-reaching, highest-risk change in the RFC lands with no behavioural delta and can be verified on its own. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Correct the Phase 1 compatibility claim.
Moving spacing, effects, and z-index tokens from :root removes them from consumers outside Theme before Phase 2 ships. Wrapping declarations in :where() also changes cascade behavior. This contradicts the “invisible” and “no behavioural delta” claims. Ship the phases atomically, preserve temporary fallback tokens, or document the Phase 1 migration impact.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 381 - 383, Revise the Phase
1 description to remove the claims that it is invisible and has no behavioral
delta. Document the consumer impact of moving spacing, effects, and z-index
tokens and the cascade change from wrapping declarations in :where(), or specify
temporary fallback tokens and atomic phase delivery to preserve compatibility.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak6Gi4GPEXvYyGZ3mrPMBW
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/rfcs/004-theme-improvements.md (2)
122-123: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSpecify the scaling unit and raw token bases.
The public values are
90%through110%, but radius formulas multiply by--rs-scaling. Define the mapping to unitless values such as0.9, and list the raw spacing and radius base values used by every scope.Without this contract,
calc()validity and nested-scope behavior are undefined.Also applies to: 222-226, 311-312
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 122 - 123, Update the scaling documentation around the token table and the referenced scope sections to define each public percentage value’s unitless mapping (for example, 90% to 0.9) for use with --rs-scaling. Document the raw spacing and radius base token values used by every scope, including nested scopes, so calc() behavior and scope inheritance are explicitly specified.
337-348: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDistinguish the removed
valuemap from the newvalueprop.Line 139 introduces
valueas the controlled settings object. Line 347 saysvalueis removed. This can cause migration readers to remove the new controlled API.-| `themes`, `attribute`, `value` as a name-to-attribute map | None | +| `themes`, `attribute`, legacy name-to-attribute `value` map | None |🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 337 - 348, Clarify the migration table’s `value` entry to distinguish the removed name-to-attribute theme map from the new controlled settings object introduced earlier. Update the row near the `ThemeProvider` replacement so it explicitly states that only the legacy map form is removed, while the controlled `value` prop remains supported.
♻️ Duplicate comments (1)
docs/rfcs/004-theme-improvements.md (1)
138-142: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine the per-setting storage wire format and migration.
storageKeyis only described as a prefix. The RFC does not define the default prefix, exact generated keys, canonical encoding forreducedMotion, invalid-value handling, or migration from the existingthemekey used bypackages/raystack/components/theme-provider/__tests__/theme.test.tsx.Add a storage table and state whether legacy data is migrated, opt-in, or reset.
Also applies to: 176-190
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 138 - 142, Expand the RFC’s storage specification around the `storageKey` and persistence descriptions with a table defining the default prefix, generated per-setting keys, canonical encoding for `reducedMotion`, and behavior for invalid stored values. Explicitly document how the existing `theme` key is handled—migration, opt-in migration, or reset—and keep the format consistent with the `persist` and controlled-value semantics.
🤖 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 `@docs/rfcs/004-theme-improvements.md`:
- Around line 362-364: Update the Phase 1 description to remove the claims that
it is invisible to consumers and has no behavioural delta. Document the cascade
and token-availability compatibility impact introduced by wrapping declarations
in :where() and moving non-theme tokens off :root, or revise the plan to provide
fallback tokens and ship both phases atomically.
- Around line 172-176: Update the shared theme store’s setValue implementation
to write the new primitive value, refresh the shared snapshot, and notify all
subscribers in the current document. Keep the existing storage-event listener in
the useSyncExternalStore subscription path for cross-tab synchronization.
- Around line 370-389: Add browser-level acceptance coverage for the theme CSS
contract, including custom-property arithmetic, :has() color-scheme behavior,
portal reinjection in the real DOM, and non-compounding radius. Alternatively,
define an explicit supported-browser release gate covering these behaviors
before completing the implementation phases, rather than relying only on unit
tests, jsdom, or manual docs-playground review.
- Around line 182-188: Update the inline theme script generation and persistence
configuration so every persisted uncontrolled setting whose value is unavailable
to the server is patched before first paint, including accent, radius, scaling,
panel background, and reduced motion. Ensure script emission is based on the
complete set of uncontrolled persisted keys rather than appearance alone, while
preserving omission when all persistable settings are controlled or excluded
from persist.
- Around line 161-164: The RFC’s description of root replacing useTheme({
storageKey }) incorrectly claims equivalent arbitrary scope targeting. Update
the section describing root and the removed scopes registry to either specify a
typed mechanism that still targets persistent scopes, or explicitly document the
loss of non-root scope targeting as a breaking capability change.
- Around line 180-184: Update the shared theme configuration and inline-script
generation to validate persisted setting names and values against the supported
schema before using them. Safely context-serialize all embedded
localStorage-derived values and neutralize any </script> sequence in generated
script content, then add hostile-storage tests covering invalid keys, arbitrary
values, and script-breakout payloads.
- Around line 95-109: The RFC’s root render contract does not ensure that a
caller-supplied element provides the layout required by root hasBackground.
Update the “Root Colour Scheme” and “The hasBackground Prop” design to restrict
valid root render targets or explicitly require the necessary display,
positioning, sizing, stacking-context, and overflow behavior; preserve an
internal wrapper whenever the supplied render target cannot satisfy those
requirements.
- Around line 101-103: Document browser support for the CSS :has() selector as a
package requirement, alongside the existing Node engine metadata, or implement
an imperative fallback that updates the root color scheme when theme attributes
change. Ensure supported browsers without :has() still receive the active
theme’s color scheme.
- Around line 126-130: Update the RFC’s fontFamily precedence documentation to
make the intended .rs-theme override behavior consistent with inline --rs-font-*
properties, or revise the implementation description to emit those font
variables through stylesheet tokens so .rs-theme can override them. Ensure the
stated precedence and override mechanisms agree.
---
Outside diff comments:
In `@docs/rfcs/004-theme-improvements.md`:
- Around line 122-123: Update the scaling documentation around the token table
and the referenced scope sections to define each public percentage value’s
unitless mapping (for example, 90% to 0.9) for use with --rs-scaling. Document
the raw spacing and radius base token values used by every scope, including
nested scopes, so calc() behavior and scope inheritance are explicitly
specified.
- Around line 337-348: Clarify the migration table’s `value` entry to
distinguish the removed name-to-attribute theme map from the new controlled
settings object introduced earlier. Update the row near the `ThemeProvider`
replacement so it explicitly states that only the legacy map form is removed,
while the controlled `value` prop remains supported.
---
Duplicate comments:
In `@docs/rfcs/004-theme-improvements.md`:
- Around line 138-142: Expand the RFC’s storage specification around the
`storageKey` and persistence descriptions with a table defining the default
prefix, generated per-setting keys, canonical encoding for `reducedMotion`, and
behavior for invalid stored values. Explicitly document how the existing `theme`
key is handled—migration, opt-in migration, or reset—and keep the format
consistent with the `persist` and controlled-value semantics.
🪄 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 Plus
Run ID: 6dcb5b7a-e177-4c48-ad6a-c0b6982e48bc
📒 Files selected for processing (1)
docs/rfcs/004-theme-improvements.md
| Because all token declarations are attribute-based, the mount element can be anything. A `render` prop provides the `asChild`-style escape hatch for merging the attributes onto a caller-supplied element instead of adding a wrapper node. | ||
|
|
||
| The trade-off accepted here is that consumer CSS and hand-rolled portals living outside the provider no longer see `--rs-*`. That is documented, and the `container` props added to the portalling components give consumers a supported way to place portalled content inside the theme. | ||
|
|
||
| #### Root Colour Scheme | ||
|
|
||
| Four things the browser paints itself live outside the React tree and cannot be styled from an element inside it: the overscroll area, the document scrollbar, the region below short content, and native widget defaults such as autofill and date pickers. All four follow the root element's `color-scheme`. | ||
|
|
||
| The root theme element MUST carry a marker attribute, `data-rs-root`, that nested themes and portal re-injections do not, so the stylesheet can tell the theme that owns the page from a theme somewhere in the tree. `<html>` then derives its colour scheme from that element with `:has()`, in the shape Radix uses: a `:root:where(:has(...))` rule matching the marker plus the appearance attribute. This needs no JavaScript, writes nothing to `<html>`, and re-evaluates as soon as the pre-hydration script patches the attribute. `enableColorScheme` and the imperative `d.style.colorScheme` write are both deleted. The overscroll area follows `color-scheme` on its own, so it lands close to the theme background without a second mechanism. | ||
|
|
||
| #### The hasBackground Prop | ||
|
|
||
| The component cannot infer whether it should paint, because re-tinting the accent and flipping a panel to dark use the same component but want opposite behaviour. A `hasBackground` prop decides. | ||
|
|
||
| When true, the element paints `--rs-color-background-base-primary`, and the root additionally establishes a stacking context and covers the viewport so short pages do not leave an unthemed strip below the content. The default follows Radix's heuristic: true at the root, true for a nested theme that sets an explicit `light` or `dark` appearance, and false for a nested theme that only changes accent, gray, radius or scaling. Consumers that paint their own page background SHOULD pass `hasBackground={false}`; `apps/www` will, since it paints its body in `layout.module.css`. Foreground colour applies regardless of `hasBackground`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Define the root render contract.
The RFC says the mount element can be anything. Root hasBackground must also establish a stacking context and cover the viewport. An arbitrary caller element does not guarantee those properties.
Restrict root render targets, or define the required display, positioning, sizing, and overflow behavior. Preserve a wrapper when the target cannot satisfy those requirements.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 95 - 109, The RFC’s root
render contract does not ensure that a caller-supplied element provides the
layout required by root hasBackground. Update the “Root Colour Scheme” and “The
hasBackground Prop” design to restrict valid root render targets or explicitly
require the necessary display, positioning, sizing, stacking-context, and
overflow behavior; preserve an internal wrapper whenever the supplied render
target cannot satisfy those requirements.
| Four things the browser paints itself live outside the React tree and cannot be styled from an element inside it: the overscroll area, the document scrollbar, the region below short content, and native widget defaults such as autofill and date pickers. All four follow the root element's `color-scheme`. | ||
|
|
||
| The root theme element MUST carry a marker attribute, `data-rs-root`, that nested themes and portal re-injections do not, so the stylesheet can tell the theme that owns the page from a theme somewhere in the tree. `<html>` then derives its colour scheme from that element with `:has()`, in the shape Radix uses: a `:root:where(:has(...))` rule matching the marker plus the appearance attribute. This needs no JavaScript, writes nothing to `<html>`, and re-evaluates as soon as the pre-hydration script patches the attribute. `enableColorScheme` and the imperative `d.style.colorScheme` write are both deleted. The overscroll area follows `color-scheme` on its own, so it lands close to the theme background without a second mechanism. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'package.json' . -x rg -n -C2 'browserslist|engines|target' {}
fd -a -e css . -x rg -n -C2 ':has\(' {}Repository: raystack/apsara
Length of output: 4915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git tracked docs/rfcs/004-theme-improvements.md =="
git ls-files docs/rfcs/004-theme-improvements.md
wc -l docs/rfcs/004-theme-improvements.md
sed -n '1,180p' docs/rfcs/004-theme-improvements.md
echo
echo "== package manifests and common build/browser config files =="
git ls-files | rg '(^|/)(package.json|browserslist|package-lock.json|pnpm-lock.yaml|yarn.lock|tsconfig.*json|vite\.config\.(js|ts|mjs|cjs)|rollup\.config\.(js|ts|mjs|cjs)|postcss\.config\.(js|cjs|ts))$'
for f in $(git ls-files | rg '(^|/)(package.json|browserslist|package-lock.json|pnpm-lock.yaml|yarn.lock|tsconfig.*json|vite\.config\.(js|ts|mjs|cjs)|rollup\.config\.(js|ts|mjs|cjs)|postcss\.config\.(js|cjs|ts))$'); do
echo "--- $f"
rg -n -C2 'browserslist|target|preset-env|core-js|`@babel`|`@vite`|css|sass|less|styl|postcss|autoprefixer|legacy|modern' "$f" || true
doneRepository: raystack/apsara
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package files relevant to builds/config =="
for f in $(git ls-files | rg '(^|/)(package.json|browserslist|postcss\.config\.(js|cjs|ts)|vite\.config\.(js|ts|mjs|cjs)|.*\.config\.(mjs|js|cjs)$|$)'); do
case "$f" in
package.json|browserslist|postcss.config.*|vite.config.*|*config.mjs|*config.js|*config.cjs)
echo "--- $f"
rg -n 'browserslist|target|preset-env|core-js|`@babel`|`@vite`|css|sass|less|styl|lightningcss|autoprefixer|legacy|modern' "$f" || true
;;
esac
done
echo
echo "== RFC text around breaking changes and open items =="
sed -n '360,440p' docs/rfcs/004-theme-improvements.md
sed -n '180,230p' docs/rfcs/004-theme-improvements.mdRepository: raystack/apsara
Length of output: 15193
Add a browser target or fallback for :has().
The RFC accepts :has() for root color-scheme, but the package metadata only declares Node engines, not supported browsers. A supported browser without :has() will still patch theme attributes while native controls, scrollbars, autofill and date pickers keep the old scheme. Document :has() as a requirement or add an imperative fallback.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 101 - 103, Document browser
support for the CSS :has() selector as a package requirement, alongside the
existing Node engine metadata, or implement an imperative fallback that updates
the root color scheme when theme attributes change. Ensure supported browsers
without :has() still receive the active theme’s color scheme.
| | `fontFamily` | `{ body, title, mono }`, each any font stack | Inter, Inter, JetBrains Mono | inline `--rs-font-body`, `--rs-font-title`, `--rs-font-mono` | | ||
|
|
||
| The seven enumerated settings become data attributes. `fontFamily` is a single setting holding three free-form stacks that cannot be enumerated, so its members are written as inline custom properties on the theme element. That makes the prop and the CSS variable the same mechanism, which is what lets fonts be overridden either way, and it gives a clear precedence chain: stylesheet default, then the prop as an inline style, then any consumer rule on a deeper element. | ||
|
|
||
| `fontFamily` members merge independently in `defaultValue`, in `value` and in `setValue`, exactly as top-level keys do, so passing `{ title: '...' }` leaves body and mono on their defaults rather than clearing them. Only the members actually supplied emit a custom property; the rest fall through to the stylesheet. The member is named `title` rather than issue #578's `heading` so it matches the existing `--rs-font-title` token. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)004-theme-improvements\.md$|theme|fontFamily|--rs-font' || true
echo
echo "RFC excerpts:"
if [ -f docs/rfcs/004-theme-improvements.md ]; then
nl -ba docs/rfcs/004-theme-improvements.md | sed -n '110,140p;190,215p'
fi
echo
echo "Search fontFamily/font variables in tracked files:"
rg -n "fontFamily|--rs-font|rs-theme|defaultTheme|defaultValue|setValue|value" --glob '!node_modules' --glob '!dist' --glob '!build' .
echo
echo "Repository top-level outline-like file list:"
git ls-files | sed 's#/.*##' | sort -u | head -200Repository: raystack/apsara
Length of output: 1610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "RFC excerpts:"
awk 'NR>=110 && NR<=140 { printf "%5d %s\n", NR, $0 }' docs/rfcs/004-theme-improvements.md
printf "\n"
awk 'NR>=190 && NR<=215 { printf "%5d %s\n", NR, $0 }' docs/rfcs/004-theme-improvements.md
echo
echo "Search fontFamily/font variables/theme override docs:"
rg -n "fontFamily|--rs-font|rs-theme|defaultTheme|defaultValue|setValue|value|override" \
docs/rfcs/004-theme-improvements.md \
apps/www/src/content/docs/theme \
packages/raystack/components/theme-provider \
apps/www/src/components/theme.tsx
echo
echo "Read theme override docs relevant sections:"
for f in apps/www/src/content/docs/theme/overview/index.mdx apps/www/src/content/docs/theme/overview/props.ts apps/www/src/content/docs/theme/radius/index.mdx apps/www/src/components/theme.tsx; do
echo "===== $f ====="
awk '{ printf "%5d %s\n", NR, $0 }' "$f" | sed -n '1,220p'
doneRepository: raystack/apsara
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "theme.tsx relevant sections:"
awk '{ printf "%5d %s\n", NR, $0 }' apps/www/src/components/theme.tsx | sed -n '1,140p'
echo
echo "CSS variable declarations and theme rule snippets:"
rg -n ":where\\(|\\.\\s*[, {]rs-theme|data-theme|--rs-font-body|--rs-font-title|--rs-font-mono|--rs-radius-|--rs-space-|--rs-accent|data-reduced-motion|data-panel" \
styles packages apps/www/src/components/theme.tsx apps/www/src/content/docs/theme -S --glob '*.{css,tsx,mdx,ts}' | head -n 200Repository: raystack/apsara
Length of output: 23860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
paths = [
Path("docs/rfcs/004-theme-improvements.md"),
Path("apps/www/src/components/theme.tsx"),
]
for path in paths:
text = path.read_text()
print(f"===== {path} =====")
print("fontFamily mentions:", len(re.findall(r'fontFamily|--rs-font-', text)), "rs-theme mentions:", len(re.findall(r'(?:^|[^\w])rs-theme\b', text)))
print("inline style/style attr mentions:", len(re.findall(r'\bstyle\b|\bstyle=', text)))
for i, line in enumerate(text.splitlines(), 1):
if re.search(r'fontFamily|--rs-font-|\.rs-theme|data-theme', line):
print(f"{i}: {line.strip()}")
PYRepository: raystack/apsara
Length of output: 7207
Align the font variable precedence promise with the RFC override target.
fontFamily writes --rs-font-* as inline custom properties. A .rs-theme stylesheet rule on the same theme element cannot override those values, but the RFC says .rs-theme reliably beats every built-in token declaration and that .rs-theme plus CSS variables are enough for overrides. Document .rs-theme as authoritative for fontFamily, or emit font variables through stylesheet tokens so .rs-theme overrides work.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 126 - 130, Update the RFC’s
fontFamily precedence documentation to make the intended .rs-theme override
behavior consistent with inline --rs-font-* properties, or revise the
implementation description to emit those font variables through stylesheet
tokens so .rs-theme can override them. Ensure the stated precedence and override
mechanisms agree.
| | `root` | The same shape bound to the root provider, for flipping the page theme from inside a scope. | | ||
| | `systemAppearance` | What the OS reports, whatever the current setting is. | | ||
|
|
||
| `root` replaces the `useTheme({ storageKey })` scope-targeting API and the internal `scopes` registry, with the same capability and no magic string keys. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not claim that root preserves arbitrary scope targeting.
The existing packages/raystack/components/theme-provider/theme.tsx implementation registers persistent scopes in scopes, and useTheme({ storageKey }) can address those scopes beyond the nearest provider. useTheme().root only targets the root provider.
Either retain a typed scope-targeting mechanism or document this as a breaking capability removal instead of claiming equivalent behavior.
Also applies to: 351-351
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 161 - 164, The RFC’s
description of root replacing useTheme({ storageKey }) incorrectly claims
equivalent arbitrary scope targeting. Update the section describing root and the
removed scopes registry to either specify a typed mechanism that still targets
persistent scopes, or explicitly document the loss of non-root scope targeting
as a breaking capability change.
| Storage is read in two places, for two different jobs, and both are needed. The inline script reads it to patch the DOM before first paint, and exists only because server-rendered HTML cannot know a client-side value. React state reads it so the component knows the value, for `useTheme` consumers, re-renders and cross-tab sync. | ||
|
|
||
| React state uses `useSyncExternalStore`, the only primitive that reads storage on the first render under CSR without breaking hydration under SSR. Its server snapshot returns the seed, so the hydration render matches the server, and its client snapshot reads storage. Under CSR there is no hydration, so the first render, and therefore the first paint, is already correct with or without the script. Under SSR the script has already corrected the DOM and the post-hydration snapshot returns the same value, so nothing moves. Its subscribe function listens to the `storage` event, which covers cross-tab sync and replaces the two hand-rolled listeners. | ||
|
|
||
| Each setting MUST be stored under its own key holding a primitive rather than as one JSON blob, so snapshots compare by value and do not loop. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Notify local subscribers after setValue.
The useSyncExternalStore subscription is described as a storage listener. That does not define a notification path for writes made by the current document.
Update the shared snapshot and notify local subscribers in setValue. Retain the storage listener for cross-tab synchronization.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 172 - 176, Update the
shared theme store’s setValue implementation to write the new primitive value,
refresh the shared snapshot, and notify all subscribers in the current document.
Keep the existing storage-event listener in the useSyncExternalStore
subscription path for cross-tab synchronization.
| The script renders as the first child of the theme element and patches its own parent, located through `document.currentScript.parentElement` with a query-selector fallback. The parent's opening tag has already been parsed at that point, so the attribute is corrected before any child content is parsed. Nothing is written to `<html>`. | ||
|
|
||
| It is far smaller than the current one, because accent, gray, radius, scaling and panel background are ordinary props that React server-renders correctly on the first byte. The script only patches persisted, uncontrolled settings, which in the default configuration means appearance alone, and is omitted entirely when every persistable setting is either controlled or excluded from `persist`. A consumer reading appearance from a cookie therefore ships no inline script. | ||
|
|
||
| The theme element MUST carry `suppressHydrationWarning`, which suppresses attribute diffs one level deep, exactly the scope required. The script and the React reader MUST be generated from one shared configuration of key names, defaults and the `system` resolution rule; the current implementation writes this logic twice and the two copies have already diverged over the `value` and `attribute` mapping. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Validate and safely serialize values used by the inline script.
Excluding fontFamily limits the schema, but localStorage values remain arbitrary strings. Validate setting names and values before use. Context-safely serialize embedded values and neutralize the </script> sequence. Add hostile-storage tests.
Also applies to: 188-190
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 180 - 184, Update the
shared theme configuration and inline-script generation to validate persisted
setting names and values against the supported schema before using them. Safely
context-serialize all embedded localStorage-derived values and neutralize any
</script> sequence in generated script content, then add hostile-storage tests
covering invalid keys, arbitrary values, and script-breakout payloads.
| It is far smaller than the current one, because accent, gray, radius, scaling and panel background are ordinary props that React server-renders correctly on the first byte. The script only patches persisted, uncontrolled settings, which in the default configuration means appearance alone, and is omitted entirely when every persistable setting is either controlled or excluded from `persist`. A consumer reading appearance from a cookie therefore ships no inline script. | ||
|
|
||
| The theme element MUST carry `suppressHydrationWarning`, which suppresses attribute diffs one level deep, exactly the scope required. The script and the React reader MUST be generated from one shared configuration of key names, defaults and the `system` resolution rule; the current implementation writes this logic twice and the two copies have already diverged over the `value` and `attribute` mapping. | ||
|
|
||
| #### What Persists | ||
|
|
||
| `persist` defaults to `true`, covering all enumerated settings, and consumers opt out wholesale or per key. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Patch every persisted uncontrolled key before first paint.
persist defaults to all seven enumerated settings, but the inline script is described as patching appearance alone. Persisted accent, radius, scaling, panel-background, or reduced-motion values can differ from server defaults and appear only after hydration.
Base script emission on every uncontrolled persisted value that the server cannot know, or change the persistence defaults.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 182 - 188, Update the
inline theme script generation and persistence configuration so every persisted
uncontrolled setting whose value is unavailable to the server is patched before
first paint, including accent, radius, scaling, panel background, and reduced
motion. Ensure script emission is based on the complete set of uncontrolled
persisted keys rather than appearance alone, while preserving omission when all
persistable settings are controlled or excluded from persist.
| Phase 1 lays the token foundation. Wrap every `--rs-*` declaration in `:where()`, move spacing, effects and z-index off `:root` onto the theme selector, add `--rs-scaling`, the radius factor and full-radius variables and the surface tokens, give each accent its own selector, and add `sage` to the gray union. This phase is invisible to consumers, because the current provider writes `data-theme` to `<html>` and a `:where([data-theme])` rule matches it exactly as `:root` did. The widest-reaching and highest-risk change in the RFC therefore lands with no behavioural delta and can be verified on its own. | ||
|
|
||
| Phase 2 is the new `Theme`: the element-mounted architecture, `defaultValue` and `value`, `useTheme`, `useSyncExternalStore` persistence, the inline script, the `:has()` colour-scheme rule, `hasBackground` and `render`. It removes `style`, the next-themes leftovers and the `Root` and `Scoped` split. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Remove the Phase 1 “invisible” and “no behavioural delta” claims.
Phase 1 wraps declarations in :where() and moves non-theme tokens from :root. Both changes affect consumer cascade and token availability. Document the compatibility impact, or ship fallback tokens and the phases atomically.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 362 - 364, Update the Phase
1 description to remove the claims that it is invisible to consumers and has no
behavioural delta. Document the cascade and token-availability compatibility
impact introduced by wrapping declarations in :where() and moving non-theme
tokens off :root, or revise the plan to provide fallback tokens and ship both
phases atomically.
| ## Testing | ||
|
|
||
| Unit tests only, extending the existing suite. No browser-based testing is introduced; the repository has none today and standing it up is a separate project. | ||
|
|
||
| Coverage to add: | ||
|
|
||
| - Attribute output for every setting, at the root and in nested scopes | ||
| - Controlled versus uncontrolled precedence, per key, including that a controlled key ignores stored values and is never written | ||
| - `persist` in all three forms, and that `fontFamily` is not persistable in any of them | ||
| - `fontFamily` members merging independently, so a partial object leaves the other two members on their defaults and emits no custom property for them | ||
| - Storage reads under CSR, where the first render already carries the correct value | ||
| - Storage reads under SSR, where the server snapshot matches the hydration render and the script renders inside the theme element | ||
| - Cross-tab synchronisation through the `storage` event | ||
| - `system` and `auto` resolution, and `resolved` versus `value` | ||
| - The portal re-injector emitting inherited attributes and font custom properties | ||
| - `useTheme().root` reaching the root provider from inside a scope | ||
| - `useTheme` throwing outside a provider | ||
| - Script omission when every persistable setting is controlled or excluded | ||
|
|
||
| One known limitation: jsdom does not resolve custom properties from stylesheets, so token arithmetic cannot be asserted at this layer. Whether `scaling: '90%'` produces the right spacing, and whether a per-component `radius` leaks to descendants, are verified by review against the docs playground, which renders every component. Closing that gap is Future Work. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add browser-level acceptance coverage for the CSS contract.
Unit tests and jsdom cannot validate custom-property arithmetic, :has() color-scheme behavior, portal reinjection in the actual DOM, or non-compounding radius. The RFC currently delegates these checks to manual review against the docs playground.
Add browser-based coverage or define an explicit supported-browser release gate before completing the implementation phases.
🤖 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 `@docs/rfcs/004-theme-improvements.md` around lines 370 - 389, Add
browser-level acceptance coverage for the theme CSS contract, including
custom-property arithmetic, :has() color-scheme behavior, portal reinjection in
the real DOM, and non-compounding radius. Alternatively, define an explicit
supported-browser release gate covering these behaviors before completing the
implementation phases, rather than relying only on unit tests, jsdom, or manual
docs-playground review.
|
One thought on If we later want a real font switcher (persisted, changeable at runtime), that's worth designing properly then, with font loading and metrics correction in scope. For now, keeping it as plain CSS vars seems like less to build and maintain. |
|
Question on multiple root themes: |
|
With multiple root Themes now supported, I think there's a collision risk in persistence: storage is per-setting-key under a prefix ( |
2 participants
Preview RFC
Summary
Themeto mount design tokens on a theme element instead of writing attributes to<html>, so the root theme, a scoped subtree, and a portalled popover all become the same mechanism — collapsing today'sRoot/Scopedsplit, making the root server-renderable, and fixing scoped theming for the 13 portalling components where it is currently broken.appearance,accentColor,grayColor(withautopairing),radius,scaling,panelBackground,reducedMotion, and font families — behind adefaultValue/valuecontrolled-per-key API with opt-out persistence.radiusoverrides with element-only, non-compounding semantics (a component prop affects only that component; tree-level changes stay withTheme), via a shared cva variant and CSS module.:where()on every--rs-*declaration so consumer overrides win regardless of load order, a stable.rs-themeoverride target, spacing/effects/z-index moved off:rootso they become scopable, a factor-based radius scale, and a new surface token family behindpanelBackground.styleprop, audits all 10 items in [Theme] Enhance customization capabilities #578 with verdicts, and records the rejected options — component-level defaults, cursor tokens, atokensdeep-override prop, and keeping tokens on<html>— with rationale. Breaking, with no shim; a migration guide accompanies implementation.