Conversation
The dashboard had no rule for where a page's content sits. A bare list or table lay on the tinted canvas, where muted ink reads dull; a carded page lifted its rows onto surface tiles a shade off that canvas, so the tiles barely registered. Each page picked one, and the two looked like different products. The shell now raises the content column onto `--surface`: inset from the canvas on desktop, full-bleed on a phone. The canvas is what remains around it, the sidebar and the gutters. Where the free canvas splits the column, each region becomes a pane of its own and the gutter runs between them, which is the same rule at one more level and the one the split screen needed. Layers that float above a pane — the trace drawer, the app drag preview — take `--surface-elevated`. Pages, layouts and kit primitives are untouched and never learn whether they sit in a pane. A card inside one is an outlined region of the same step, which is what its hairline already drew, so `Card`, `FormRows`, `List` and `Table` needed no change. What did change is every region that painted `--background` while sitting inside the column: it now reads as a block of canvas cut into the pane, so those sites take `--surface`. The pane clips with `overflow-clip` rather than `overflow-hidden`. A page whose own column paints to the edge would otherwise square off the corners, but `hidden` would make the column a scroll container and the `sticky bottom-0` composers inside would stop tracking the viewport. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR raises routed-page content onto a --surface "pane" at the shell (RomeShellLayout <main>), so pages paint bg-surface and the shell keeps --background for the sidebar and gutters. It uses a has-[[data-slot=tool-workspace]] selector to hand off the paint to per-region panes in the split (free-canvas) view, insets the trace drawer into the gutter, and updates docs plus the TraceDrawer placement tests to match. The overflow-clip vs overflow-hidden reasoning (keeping the sticky composer out of a new scroll container) is sound, and the surface-token migrations across AppFullPage, MemoryPage, SessionsPage, ToolWorkspace, etc. are consistent with the documented depth model.
Overall the design is clean and the scope is disciplined — deferred kit/app conversions are explicitly listed. The main issue is a small width-accounting drift in ToolWorkspace: the resize separator grew from 1px to 12px but the SEPARATOR_WIDTH constant that drives the pane-width clamp was not updated, so the right pane can be squeezed slightly below its minimum. Verified that no unit test asserts the exact separator width or the old shell background, so the change set is self-consistent.
Verdict: COMMENT — The change is coherent, well-documented, and test-backed; only a minor layout-math inconsistency and a low-priority affordance note are worth addressing, neither blocking.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | correctness | packages/web/src/pages/free/ToolWorkspace.tsx |
Separator widened to 12px but SEPARATOR_WIDTH constant still 1 |
| P3 | design | packages/web/src/pages/free/ToolWorkspace.tsx |
Resize gutter has no resting affordance |
| // The hairline between the two regions is a canvas gutter now, so the | ||
| // handle widens to it and drops the painted line; hover and drag tint | ||
| // the gutter instead of thickening a rule. | ||
| className="relative z-40 w-3 shrink-0 cursor-col-resize touch-none rounded-full transition-colors before:absolute before:inset-y-0 before:-inset-x-1 hover:bg-surface-hover focus-visible:outline-ring/50 data-[resizing]:bg-surface-hover" |
There was a problem hiding this comment.
[P2] correctness — Separator widened to 12px but SEPARATOR_WIDTH constant still 1
The resize handle changed from w-px (1px) to w-3 (12px), but SEPARATOR_WIDTH (line 26) is still 1. That constant feeds available = width - SEPARATOR_WIDTH (line 124), the chatWidth clamp (available - MIN_PANE_WIDTH), and COMPACT_WIDTH. Because the real gutter now consumes 11px more than the math assumes, at the extreme of the range the tabs pane can be squeezed ~11px below MIN_PANE_WIDTH (and the non-compact threshold is off by the same amount). Update the constant so the layout math matches the rendered gutter:
const SEPARATOR_WIDTH = 12; // matches the w-3 resize gutter| // The hairline between the two regions is a canvas gutter now, so the | ||
| // handle widens to it and drops the painted line; hover and drag tint | ||
| // the gutter instead of thickening a rule. | ||
| className="relative z-40 w-3 shrink-0 cursor-col-resize touch-none rounded-full transition-colors before:absolute before:inset-y-0 before:-inset-x-1 hover:bg-surface-hover focus-visible:outline-ring/50 data-[resizing]:bg-surface-hover" |
There was a problem hiding this comment.
[P3] design — Resize gutter has no resting affordance
Dropping the painted after:...bg-border-subtle line means the split handle is now visually indistinguishable from the plain 12px gutter at rest — it only tints on hover/drag. This is intentional per the comment and the two panes' own borders imply the boundary, but discoverability of the drag affordance drops (no cursor/line hint until pointer-over). Optional: consider a very subtle resting indicator (e.g. a faint centered pill) so keyboard/mouse users can find it, or confirm this is the desired behavior.
What this PR does
The dashboard had no rule for where a page's content sits. A bare list or table (
/people,/sessions) lay on the tinted canvas, where muted ink reads dull. A carded page (/routines,/settings) lifted its rows onto surface tiles a shade off that canvas, so the tiles barely registered. Each page picked one, and the two looked like different products.The shell now raises the content column onto
--surface: inset from the canvas on desktop, full-bleed on a phone. The canvas is what remains around it — the sidebar and the gutters. Where the free canvas splits the column, each region becomes a pane of its own with the gutter between them, which is the same rule one level down and the part the split screen needed.Design & Invariants
Page,Section,ListCollection,FormRows,CardandTableare untouched.border-borderhairline already drew, which is why the card components needed no change.--backgroundis now the shell's. Every region that painted it while sitting inside the column now reads as a block of canvas cut into the pane, so those sites take--surface. Layers that float above a pane take--surface-elevated: the trace drawer, the app drag preview.overflow-clip, notoverflow-hidden. A page whose own column paints to the edge squares off the pane's corners, which/memoryand/projectsdo.hiddenfixes that but makes the column a scroll container, and thesticky bottom-0composers inside would stop tracking the viewport.clipclips without joining the scroll chain.surfaceModelfield. This is a migration rather than a trial, and a per-theme switch would leave two layouts to reason about forever.Test plan
pnpm typecheckrstestsuite forpackages/web— 1487 passingbiome checkon the changed files from the devShell/routines,/people,/sessions,/apps,/settings,/memory,/projects, and/chatwith the apps panel open/memoryand/projects, which is whatoverflow-clipis forpnpm test:layout(Playwright) — not run locally; the bundled browser will not launch on this host, so CI's pinned container is the first runNot in this PR
bg-backgroundas their lightest fill:Button's outline variant, theSwitchthumb,Calendar, andSegmentedControl. Inside a pane they paint the canvas, about 1.08:1 against it in Ember. The fix belongs to@rome-os/uiand needs its ownfix(ui)type to bump the package, so it follows separately.rome_apps/CLAUDE.mdgains the contract — an app root sits on--surfaceand never paints--background— but the apps that do (briefing,showcases, and four others, 29 sites) are not converted here, andshowcasesis the sample app authors copy.FileView's recessed viewer now reads as a large--surface-mutedblock on/projects, where it used to blend into the canvas. That is the depth model working rather than a token error, but it is worth a design look.--surfacesits within 1.11:1 of--backgroundeverywhere.🤖 Generated with Claude Code