feat(theme): revamped Theme - #893
Conversation
…nel tokens (RFC 004) Adds `ThemePreview`, an element-mounted theme alongside the existing `Theme`, implementing RFC 004. Tokens move off `<html>` onto a real element, so the root theme, a nested scope and a portal re-injection are the same component rendering the same attributes. Token layer: - every `--rs-*` declaration wrapped in `:where()`, so a consumer rule on the stable `.rs-theme` class wins without `!important` or load-order control - spacing, effects and z-index move off `:root` onto the theme selector, which is what makes them scopable - `--rs-scaling` multiplies spacing, radius, font sizes and line heights - radius becomes a factor over a fixed base scale plus a pill length, replacing the two hardcoded `data-style` scales - panel, overlay and backdrop-filter tokens; `data-panel-background` selects solid or translucent, defaulting to solid - each accent gets its own selector, so `indigo` resets inside an orange scope - `sage` joins the gray union Component: - seven independently seedable, controllable and persistable settings - `useSyncExternalStore` persistence, gated on `persistKey`, with merge-on-write so themes may share a namespace - a small pre-hydration script that patches its own parent, emitted only for a namespace's uncontrolled settings - `:has()` colour-scheme rule on `<html>`, no JavaScript and no writes to it - `hasBackground`, `isRoot`, `render`, `useThemePreview().root` Portals and per-component radius: - theme re-injection and a `container` prop across the thirteen portalling components, fixing scoped themes inside portals - a shared `radius` cva variant and CSS module; `Image` and `Avatar` migrate onto the five-value scale Stylesheets: - `style-no-fonts.css` published alongside `style.css` - `--rs-font-mono` reordered so JetBrains Mono precedes Menlo Docs: a theme playground panel and a preview page with a migration guide. The existing `Theme` ships unchanged, so this is additive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vcxAxsBpKAvtUfDi2wFKr
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedToo many files! This PR contains 113 files, which is 13 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (113)
You can disable this status message by setting the 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: |
rohanchkrabrty
changed the title
|
|
||
| const entry: StoredEntry = { v: STORAGE_VERSION, settings }; | ||
| try { | ||
| window.localStorage.setItem(persistKey, JSON.stringify(entry)); |
There was a problem hiding this comment.
Theme changes get dropped when localStorage is unavailable.
For persisted keys, storage is the source of truth. readRaw (L41) and this setItem both catch-and-return, so the write is lost, notifyThemeStorage() on L115 still fires, readers re-read the previous value, and the setting doesn't move.
Reproduced: with persistKey set and a throwing localStorage, appearance stays light on click; without persistKey it toggles fine. That covers Safari private mode, sandboxed iframes, and browsers with site data blocked — and the more correct config (with persistKey) is the affected one.
Could writeStoredSettings return whether it succeeded, so the caller can fall back to in-memory state?
| if (isSettingsEmpty(patch)) return; | ||
|
|
||
| const persisted = persistedKeysRef.current; | ||
| if (persistKey && persisted.length > 0) { |
There was a problem hiding this comment.
Companion to the store.ts:111 comment — this is where the fallback would live. When the write fails there's no path to setLocal, since the loop below skips anything in persisted.
Something like if (!writeStoredSettings(...)) { /* mirror the patch into local */ } would keep the switcher working when storage is unavailable.
| `if(o&&typeof o==="object"&&typeof o.v==="number"&&o.v<=${STORAGE_VERSION}` + | ||
| `&&o.settings&&typeof o.settings==="object")s=o.settings}}catch(t){}` + | ||
| `for(var i=0;i<m.length;i++){` + | ||
| `var k=m[i][0],a=m[i][1],v=s[k];` + |
There was a problem hiding this comment.
v is sourced only from storage, so an unstored key is skipped at L72 and the server's data-theme="light" survives to first paint. Since DEFAULT_SETTINGS.appearance is system, that's the first visit for most users.
Verified: renderToStaticMarkup(<ThemePreview persistKey="app">) with OS dark and empty storage emits data-theme="light", script present but with nothing to act on.
Passing the effective settings into createThemeScript and falling back — var v = (k in s) ? s[k] : d[k] — would let system resolve through matchMedia on a first visit. Worth also emitting the script when appearance is uncontrolled system even without a persistKey.
| } | ||
| :where([data-radius="full"]) { | ||
| --rs-radius-factor: 1.5; | ||
| --rs-radius-pill: 9999px; |
There was a problem hiding this comment.
--rs-radius-pill: 9999px feeds 89 max(var(--rs-radius-pill, 0px), …) sites, which includes every large panel.
Measured in Chromium against the built style.css: a dialog inside data-radius="full" computes border-radius: 9999px. Browsers clamp radius to half the box, so it renders as a stadium — same for dropdowns, the command palette, code blocks, data tables, sidebars and calendars.
Capping panel surfaces (min(var(--rs-radius-pill), 16px), or a separate --rs-radius-panel-max) while keeping the raw pill for buttons, badges, chips and inputs would fix it.
| defaultVariants: { | ||
| size: 3, | ||
| radius: 'small', | ||
| radius: 'medium', |
There was a problem hiding this comment.
This default means Avatar no longer follows the theme radius. .radius-medium.radius-medium is (0,2,0) and always applied, so .avatar-size-N at (0,1,0) can never take effect.
Measured on a size-8 avatar:
| theme radius | with the default class | without it |
|---|---|---|
full |
8px | 9999px |
large |
8px | 12px |
So "renders exactly as the old default did" holds only at factor 1. Every other component (Button, IconButton, Badge, Callout, Chip, Input, TextArea) sets no radius default and does respond to the theme.
Dropping radius from defaultVariants fixes both — one line. It also makes the 13 max(var(--rs-radius-pill, …)) declarations in avatar.module.css reachable again.
| try { | ||
| parsed = JSON.parse(raw); | ||
| } catch { | ||
| // Includes the legacy `"dark"` bare theme name, which is not an object. |
There was a problem hiding this comment.
This comment is the only place the legacy format is acknowledged, and it's handled safely — but the user's saved appearance is silently lost. The old provider stored a bare string (localStorage.setItem('theme', 'dark'), default key theme), JSON.parse throws, and we fall back to defaults.
Either read a bare light / dark / system value once as appearance, or note in the migration guide that a fresh persistKey is recommended.
| padding: var(--rs-space-3); | ||
| background-color: var(--rs-color-background-base-primary); | ||
| background-color: var(--rs-color-panel); | ||
| backdrop-filter: var(--rs-panel-backdrop-filter); |
There was a problem hiding this comment.
Separate from the -webkit- point: Drawer.Content's new radius prop has no base to work from — this file declares neither border-radius nor --rs-radius-step.
Measured: radius="medium" gives a flat 4px on all four corners of a full-height sheet, including the two flush with the viewport; bare gives 0px. Drawers also don't pick up the theme radius at all.
Per-side radii would solve it, or the prop could be left off Drawer.
| rendered on macOS. */ | ||
| --rs-font-mono: | ||
| var(--rs-font-menlo), var(--rs-font-jetbrains-mono), monospace; | ||
| var(--rs-font-jetbrains-mono), var(--rs-font-menlo), monospace; |
There was a problem hiding this comment.
This reorder is the right fix, but it changes all monospace text on macOS for existing Theme consumers too. It's covered on the new ThemePreview page — worth a line in the release notes as well, since Theme users won't have a reason to read that page.
| assignSetting(changed, key, settings[key]); | ||
| } | ||
| } | ||
| if (!isSettingsEmpty(changed)) |
There was a problem hiding this comment.
onValueChange fires without a user action on the hydration path.
The useSyncExternalStore correction from the server snapshot to the stored value looks identical to a real change here, so consumers get a phantom event on every SSR page load where storage differs from the seed. Anything with a side effect — analytics, a server write, a toast — will fire spuriously.
Verified: CSR gives 0 calls; hydration gives 1, {"accentColor":"mint"}.
Gating the first post-hydration reconciliation (e.g. skip while previousSettings.current is still the server-derived value) would keep the callback meaning "the user changed something".
| ); | ||
| expect(readStoredSettings('app')).toEqual({ radius: 'large' }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The version guard discards a newer-schema entry instead of leaving it alone, so a write from an older build destroys settings a newer build owns.
Verified: with {v: 99, settings: {appearance: 'dark', radius: 'full'}} stored, writeStoredSettings('key', ['accentColor'], {accentColor: 'mint'}) leaves {v: 1, settings: {accentColor: 'mint'}}.
Not reachable while STORAGE_VERSION is 1, so no rush — but the branch is here to handle exactly this case and currently handles it destructively. Bailing out of the write when the stored v is higher would be safer.
2 participants
Summary
ThemePreview, the element-mounted theme from RFC 004: tokens move off<html>onto a real element, so the root theme, a nested scope and a portal re-injection are the same component rendering the same attributes — which makes the theme server-renderable, allows more than one provider per page, and fixes scoped themes inside portals.--rs-*declaration is wrapped in:where()so a consumer rule on the stable.rs-themeclass wins without!important; spacing, effects and z-index move onto the theme selector so a scope can change them; adds--rs-scaling, a radius factor over a fixed base scale replacing the two hardcodeddata-stylescales, and panel/overlay/backdrop-filter tokens.useSyncExternalStorepersistence gated onpersistKey, plus a small pre-hydration script that patches its own parent and a:has()colour-scheme rule that writes nothing to<html>.containerprop across the thirteen portalling components, and a sharedradiuscva variant and CSS module;ImageandAvatarmigrate onto the five-value scale.style-no-fonts.cssalongsidestyle.css, reorders--rs-font-monoso JetBrains Mono precedes Menlo, and adds a theme playground panel plus a docs page with a migration guide.Themeships unchanged, so this is additive — no consumer migration is forced by this PR.