Conversation
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
The surface model is appropriately represented as theme data and kept scoped to shell-level consumers. However, the no-flash bootstrap does not handle the rollout state for users who had already selected Ash before this change.
Verdict: REQUEST_CHANGES — Existing Ash users will still receive the flat-to-pane first-paint jump this PR explicitly aims to prevent.
1 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | error-handling | packages/web/index.html |
Migrate existing Ash selections into the bootstrap model cache |
| document.documentElement.setAttribute("data-theme", name); | ||
| // The theme's surface model (SURFACE_MODEL_STORAGE_KEY in | ||
| // lib/theme.ts). Without it a pane theme paints flat, then jumps. | ||
| var surfaceModel = localStorage.getItem("rome-theme-surface-model"); |
There was a problem hiding this comment.
[P2] error-handling — Migrate existing Ash selections into the bootstrap model cache
Users who already have rome-theme-name=ash and the pre-PR CSS cache do not have rome-theme-surface-model. This branch leaves the attribute unset, so Ash first renders as canvas until ThemeProvider later calls applyThemeName() and switches it to pane, producing the exact jump the bootstrap is intended to avoid. Add a versioned/migrated bootstrap representation of the surface model (or a build-generated theme-to-model map) so a missing new key cannot make an existing pane-theme selection paint flat.
Light mode reads two ways. A bare list or table sits on the tinted canvas, where muted ink lands near AA and the page reads dull; a carded page lifts its content onto surface tiles that sit within 1.08:1 of that canvas, so the tiles barely register. Each page picks one, and nothing says where content sits. Ash trials the paper answer: its light canvas moves to `--neutral-25`, the step its cards already sit on, so bare and carded content read alike and a card is its hairline alone. One mapping line changes; Card already carries `border-border`, and `--surface-elevated` stays a step above so popovers still float. Ember and Slate are untouched. The contrast ledger moves with it: Ash's subtle ink on the canvas measures 4.16:1, and its primary on the canvas now clears AA, so that record leaves the table. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
f49e03c to
4189156
Compare
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
On the paper canvas the sidebar and the content are the same sheet with a hairline between them, so nothing says which side is chrome and which is the page. The sidebar and the mobile header now take `bg-surface-muted`, the step that already means "recessed", rather than painting the canvas itself. Ash's recessed step moves with them. It sat at `--neutral-150`, which is 1.05:1 against the new canvas, so a well, a table header, and the sidebar all dissolved into the page; `--neutral-200` gives the sidebar 1.24:1. Its muted ink deepens one step in turn, since `--neutral-550` on the deeper step lands at 4.23:1 and the contrast suite holds that pair to AA. Ember and Slate recess by their own steps, at 1.09:1 and 1.05:1. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
The first pass put the canvas on the top of the ramp and the sidebar two steps below it, and the pair read as a white sheet beside a grey slab. Both ends move in: the canvas settles on `--neutral-50`, so the paper keeps its tint, and the sidebar takes `--neutral-150` instead of 200. The recess goes from 1.24:1 to 1.11:1. `--neutral-150` is retuned for Ash to make that possible. Its light ramp spanned .10 of lightness across five steps and then dropped .12 in one, so nothing could carry a faint fill: a region on 150 vanished against the canvas and one on 200 read as a slab. 150 moves into the gap, which also gives `secondary` and `muted` a fill that registers. The muted ink goes back to `--neutral-550`, since it clears AA on the lighter recess and no longer needs the deeper step. The sidebar's weakest ink now has a measured pairing. Moving the fill did not create that debt — the third text tier reads about the same distance under the bar on all three fills — but nothing measured it against the fill those 12px labels actually render on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR trials a "paper" light mode for the Ash theme: --background and --surface collapse onto a single neutral step (so bare and carded content read alike, a card being its border alone), the sidebar and mobile header recess onto --surface-muted instead of painting the canvas, and Ash's --neutral-150 primitive is darkened to fill a gap in the light ramp. The contrast ledger is updated two-sidedly and the changes are well-reasoned and thoroughly commented.
The implementation itself is internally consistent and the contrast test (which derives from themes.ts) corroborates it: background stays at --neutral-50 and surface moves up to --neutral-50, so both collapse at neutral-50 — proven by primary/surface now reading exactly the same 4.41:1 as the unchanged primary/background. The problem is that the documentation table, which this PR edits specifically to record this case, states the wrong step (--neutral-25) for both --background and --surface in the Ash light column. Since these [mech] docs are treated as the authoritative token reference, a factual contradiction here undermines the stated purpose of the change and will mislead future token work. A secondary, minor concern is that the sidebar's new use of --surface-muted isn't reflected in that token's usage statement.
Verdict: REQUEST_CHANGES — The surfaces doc table — the normative SSOT this PR explicitly amends to document the new "paper case" — records Ash light's collapsed step as --neutral-25, but the shipped code and contrast tests collapse it at --neutral-50.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | documentation | docs/ui/semantic-token/surfaces.md |
Docs table contradicts the implementation for Ash light's collapsed step |
| P3 | documentation | packages/web/src/shell/RomeShellLayout.tsx |
Sidebar's new --surface-muted use not reflected in the token's usage statement |
| | Token | Ember light | Ember dark | Ash light | Slate light | Slate dark | | ||
| |---|---|---|---|---|---| | ||
| | `--background` | `--neutral-50` | `--neutral-950` | `--neutral-50` | `--neutral-50` | `--neutral-950` | | ||
| | `--background` | `--neutral-50` | `--neutral-950` | `--neutral-25` | `--neutral-50` | `--neutral-950` | |
There was a problem hiding this comment.
[P1] documentation — Docs table contradicts the implementation for Ash light's collapsed step
This table is the normative reference this PR amends to document the paper case, but its Ash light values contradict the code. The code sets background: var(--neutral-50) (unchanged) and moves surface from --neutral-25 to var(--neutral-50), so both collapse at neutral-50 — the code comment explicitly argues for step 50 ("the step is 50 rather than the top of the ramp, so the paper keeps its tint"), and the contrast test confirms it (primary/surface now reads the same 4.41:1 as the unchanged primary/background, only possible if surface == background == neutral-50).
Yet this row sets --background Ash light to --neutral-25, and line 35 leaves --surface Ash light at --neutral-25. Both cells should read --neutral-50.
| | `--background` | `--neutral-50` | `--neutral-950` | `--neutral-25` | `--neutral-50` | `--neutral-950` | | |
| | `--background` | `--neutral-50` | `--neutral-950` | `--neutral-50` | `--neutral-50` | `--neutral-950` | | |
| | `--surface` | `--neutral-25` | `--neutral-925` | `--neutral-50` | `--neutral-0` | `--neutral-900` | |
(The PR description's "canvas moves to --neutral-25" narrative appears to describe an earlier iteration; the shipped decision is the neutral-50 collapse.)
| {!hideSidebar ? ( | ||
| <aside | ||
| className={`fixed inset-y-0 left-0 z-40 flex w-64 shrink-0 flex-col border-r border-border bg-background pb-safe pt-safe transition-[transform,width] duration-200 ease-out md:sticky md:top-0 md:h-dvh md:translate-x-0 md:pb-0 md:pt-0 ${ | ||
| className={`fixed inset-y-0 left-0 z-40 flex w-64 shrink-0 flex-col border-r border-border bg-surface-muted pb-safe pt-safe transition-[transform,width] duration-200 ease-out md:sticky md:top-0 md:h-dvh md:translate-x-0 md:pb-0 md:pt-0 ${ |
There was a problem hiding this comment.
[P3] documentation — Sidebar's new --surface-muted use not reflected in the token's usage statement
The sidebar (and the mobile header at line 242) now paint bg-surface-muted. The usage statement for --surface-muted in docs/ui/semantic-token/surfaces.md (line 20) still scopes it to "a region recessed inside a card: a well, a code block, a table header. Not used for a region that floats." Top-level sidebar chrome fits none of those categories. The PR rightly frames the sidebar as taking the recessed role, but the token's documented usage wasn't broadened to cover chrome. Consider adding a line so the doc stays the single source of truth for where this token is used.
What this PR does
Light mode reads two ways. A bare list or table (
/people,/sessions) sits on the tinted canvas, where muted ink lands near AA and the page reads dull. A carded page (/routines,/settings) lifts its content onto surface tiles that measure within 1.08:1 of that canvas, so the tiles barely register. Each page picks one, and nothing says where content sits.Ash trials the paper answer. Its light canvas moves to
--neutral-25, the step its cards already sit on, so bare and carded content read alike and a card is its hairline alone.That leaves the sidebar and the page as the same sheet with a hairline between them, so the second commit recesses the sidebar and the mobile header onto
bg-surface-muted— the step that already means recessed — instead of painting the canvas. Ash's recessed step moves from--neutral-150to--neutral-200to clear the new canvas, and its muted ink deepens one step in turn to hold AA on it.Design & Invariants
--backgroundand--surfaceresolving to one step is now a documented case indocs/ui/semantic-token/surfaces.md: a card reads by its border, whichCardandFormRowsalready carry, and--surface-elevatedstays a step above so popovers still float.--surface-mutedstep, and dark inverts as the token model says it must, so the sidebar sits above the dark canvas rather than below it.themes-contrast.test.tsis two-sided, so it moved with the canvas: Ash's subtle ink on the canvas is 4.16:1 now, and Ash'sprimaryon the canvas clears AA, so that record leaves the table.Test plan
pnpm typecheckrstestsuite forpackages/webbiome checkon the changed files from the devShell/people,/routines,/settings/appearance,/chatwith the apps panel open/routines— resolved tokens read back fromgetComputedStyleNot in this PR
text-subtle-foregroundsites, which sit at 3.36:1 on Ash's recessed step. Raising the third text tier is the open call recorded in the ledger.🤖 Generated with Claude Code