feat(fork): redesign the sidebar, surfaces and workspace header - #14
Conversation
Implements the Figma redesign (t3-fork, Frame 51 / node 53-1690) across five areas. Dark mode only; light keeps upstream's zinc palette untouched. Surfaces. The dark shell moves off pure black: the workspace stage is #212121 and the sidebar v2 panel #1e1e1e, so the panel sits below the stage rather than lifting off it. Values are flat opaque sRGB, not alphas, because they are only correct against #1e1e1e and alpha compounds through hover and selection. Two selectors are required, not one: upstream re-declares --background, --card, --border and the --sidebar-* family on the panel element itself, so a rule written only against :root is inert on exactly the surface this is about. Upstream's 0.035 surface grain is dropped in dark. Measured, it carried the panel from #1e1e1e to ~#212121 -- the stage's colour -- erasing the separation the whole palette is built on, because the chat content paints over the workspace copy of the grain but nothing paints over the sidebar's. Thread rows. Two lines become three, at 8px radius: title, repo, and a meta line whose halves sit in fixed corners (PR + diff left, model + runtime right). Surface now encodes interaction only -- a row is filled on hover or selection, never because of its status. Working and Idle titles recede at rest and return on hover, matching the component set: the rule is whether there is anything here to act on. Idle gains a hollow ring so the trailing slot is never empty; it used to fall back to a relative-time string, which left nothing below it able to align. Diff counts bind to semantic tokens and render once upstream's latestTurnDiff() stub carries data. Sidebar chrome. The collapse toggle moves inline beside the traffic lights and the brand moves to the trailing edge. The toggle leaves with the panel, so the floating control still renders while the sidebar is shut -- the case COLLAPSED_SIDEBAR_TITLEBAR_INSET_CLASS already reserves space for -- and the keybinding stays mounted above both. The header always carries artwork now: green for a release build, orange for dev, both painted from the designer's PNGs. Upstream's other two art surfaces keep gating on the build channel, so the Dev cue survives outside the sidebar. Nightly is untouched. Workspace header. 56px, project and title swap weight, favicon dropped, and the three action controls become fully-rounded split pills. They were already built from the right primitives, so the only edit in those files is a data-fork-pill attribute; the styling lives in the fork stylesheet. Selected as `button` rather than [data-slot="button"] -- Base UI's render prop overwrites the slot, so a slot-based selector matches nothing and fails silently. Identity. The fork is "no3y Code" everywhere, renamed from "N3 Code", including the rendered app: branding.ts's bridge-less fallback pointed at upstream's "T3 Code", so dev and hosted sessions were branded as upstream while the packaged app was not. Safe by construction -- the bundle id and ~/.t3-fork are both held stable across renames. An installed N3 Code.app is not replaced by a build installing as no3y Code.app. Four new manifest entries with a guard test each, and three upstream tests carried through where they pinned behaviour this deliberately changes.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thermo-nuclear code quality — changes requested
The redesign's fork-owned visual system (custom/*, theme.custom.css, pill attribute + CSS) is the right direction, and SidebarV2ThreadCardMeta / idle mark / header backdrop are clean seams. This still does not meet the maintainability bar: Tier-4 surgery keeps landing in SidebarV2.tsx (2741 lines) and other upstream hot files with zero overrides/ (shadows: [] on every new manifest entry), and most of the SidebarV2 presentation rewrite sits outside fences.
APPROVE: no.
Findings (priority order)
-
Structural — no
overrides/while doing Tier-4 surgery in hot upstream modules. Fork order iscustom/→overrides/→ theme → last-resort fenced inline. New art/meta modules correctly live incustom/, then the PR rewritesSidebarChrome,AppSidebarLayout's toggle lifetime, and a largeSidebarV2region with fences + watch lists only. ShadowSidebarChrome(135-line leaf) and extract chrome/card shells so sync cost matches the placement rules. -
Missed code-judo — ~150-line fenced chrome rewrite still inside
SidebarV2. Search/project rows are pure presentation. Extract tocustom/so the fence collapses to a call site — the same judo already used for meta, not applied here. -
Missed code-judo — card-row extraction stopped halfway. Surface policy, title mute, and the trailing status/actions grid remain in
SidebarV2Row. Finish a fork-owned card shell sosidebar-v2-card-rowswatches a call site, not an unfenced presentation rewrite in a megacomponent. -
Spaghetti — two parallel "recede" policies.
shouldRecedeand the title-mute predicate (working || topStatus === null) encode overlapping but different rules. Collapse to one policy helper. -
File-size / decomposition.
SidebarV2.tsxstays at 2741 lines with in-place rewrite. Does not newly cross 1k, but refuses an available shrink that would materially improve maintainability.
Keep the pill-via-data-fork-pill pattern — that is the right Tier-4 minimalism. Do more of that shape for chrome and card presentation.
Sent by Cursor Automation: Thermo-nuclear PR review
| <SidebarContent className="gap-0"> | ||
| <SidebarGroup className="px-2 pb-2 pt-3"> | ||
| <div className="flex items-center gap-1"> | ||
| {/* fork:begin fork-sidebar-chrome — see .fork/customizations.yaml#fork-sidebar-chrome |
There was a problem hiding this comment.
Structural / missed code-judo (blocker). This ~150-line fenced chrome rewrite is the clearest missed simplification. Search/project rows are pure presentation (heights, icons, classNames) — extract them to custom/ (one call site) or own the region via a small override.
Leaving class/icon essays inside a 2741-line hot file is a structural regression even with guards. The PR already proved this judo with SidebarV2ThreadCardMeta; apply it here too.
| ) : null} | ||
| </span> | ||
| </div> | ||
| <SidebarV2ThreadCardMeta |
There was a problem hiding this comment.
Missed decomposition (blocker). Meta extraction is good and incomplete. Surface policy, title mute, and the trailing status/actions grid above still live largely unfenced in SidebarV2Row.
Finish the card shell extraction (fork-owned title + trailing slot, or a full card presentational component) so sidebar-v2-card-rows watches a call site — not a presentation rewrite inside a high-churn megacomponent.
| // hold it bright for the same reason. | ||
| "truncate", | ||
| status === "ready" && shouldRecede ? "text-muted-foreground" : "text-foreground", | ||
| (status === "working" || topStatus === null) && !props.isActive && !isSelected |
There was a problem hiding this comment.
Spaghetti branching (blocker). This is a second "recede" predicate beside shouldRecede (~L447). Idle (topStatus === null) and working get special title muting with different unread/woke rules than the surface policy.
Collapse to one pure helper (e.g. resolveCardTitleTone(...)) used by surface + title, and delete the duplicate boolean algebra. Also drop the stale 65%/45% "two-specimen" lore still attached to shouldRecede — SidebarV2ThreadCardMeta already collapsed that model.
| (composer send button, auth screen) still gate on the variant and are | ||
| untouched, so they keep signalling Dev. */} | ||
| <ForkSidebarHeaderBackdrop stageLabel={stageLabel} /> | ||
| {/* fork:begin fork-sidebar-chrome — see .fork/customizations.yaml#fork-sidebar-chrome |
There was a problem hiding this comment.
Wrong placement tier (blocker). This is an ideal overrides/components/sidebar/SidebarChrome.tsx candidate: 135-line leaf, header mostly rewritten.
Prefer shadowing this file over Tier-4 fencing + watching it. Fork rules put overrides/ ahead of last-resort inline edits; shadows: [] here is the structural smell.
| return () => window.removeEventListener("keydown", onKeyDown, true); | ||
| }, [keybindings, toggleSidebar]); | ||
|
|
||
| if (isSidebarVisible) return null; |
There was a problem hiding this comment.
Acceptable fenced seam — do not treat as license for more megacomponent surgery. The early-return so sidebar.toggle stays mounted is correctness-critical and small.
Keep this as the rare justified Tier-4 behavior change if chrome ownership moves out of inline SidebarV2 / SidebarChrome edits (via custom/ extract or override).
| - apps/web/src/custom/assets/sidebar-stage-dither-dev.png | ||
| - apps/web/src/custom/assets/sidebar-stage-dither.png | ||
| - apps/web/src/theme.custom.css | ||
| shadows: [] |
There was a problem hiding this comment.
Architecture boundary (blocker). shadows: [] on fork-sidebar-chrome / sidebar-v2-card-rows / fork-workspace-header while watching large upstream modules (SidebarV2.tsx, SidebarChrome.tsx, 2k-line pill hosts) inverts the fork placement ladder.
Prefer non-empty shadows for SidebarChrome (and extracted leaves) so sync cost matches .fork/AGENTS.md. Guards that string-match sources are not a substitute for ownership boundaries.
Review: sidebar / surfaces / workspace header redesignReviewed at 1.
|
Addresses review findings 2, 3, 4 and 5 by pushing presentation up the placement ladder (README §3: "push every customization as far up this list as it will go"), so the Tier-4 surface in upstream's file collapses to call sites. The search and project-scope rows become custom/SidebarV2ChromeRows.tsx. They were ~150 lines of pure presentation fenced in place, which meant SidebarV2 carried the whole rewrite while the manifest could only watch the file it sat in. The scope row is generic over the project type so callbacks hand back upstream's own richer object while the fork declares only the four fields it reads. Row presentation policy becomes custom/sidebarV2RowPolicy.ts: the surface model, the card title colour, and the predicate behind it. These are pure decisions about how a row should look given what it is, with no React and no upstream state. That predicate and upstream's `shouldRecede` are now named and documented rather than sitting inline as two anonymous overlapping tests. They are deliberately not merged: `shouldRecede` describes a slim row in the settled shelf, dims the whole row including its surface, and folds in read/woke state; the card rule touches the title only, never the surface, and ignores read state. They agree on read-ready threads and diverge everywhere else, so collapsing them into one predicate would silently change both. SidebarV2.tsx drops from 2741 to 2597 lines. Three new guards pin the extraction: the chrome rows must stay fork-owned, the policy must stay out of the megacomponent, and the recede rule is asserted as behaviour across the component set's matrix rather than as a class string. No behaviour change -- verified live: surfaces, row heights, title colours at rest and on hover, and the project menu all render identically.
|
Addressed findings 2, 3, 4 and 5. Pushed in What changed#2 — chrome rows extracted. The search and project-scope rows move to #3 — card policy extracted. Surface model, title colour and the predicate behind it move to #4 — the two recede rules are now named and documented. They are deliberately not merged, and I'd push back on collapsing them:
They agree on read-ready threads and diverge everywhere else, so one predicate would silently change both. The duplication was that they were two anonymous inline tests; naming them is the fix. #5 — Three new guards pin it: the chrome rows must stay fork-owned, the policy must stay out of the megacomponent, and the recede rule is asserted as behaviour across the component set's matrix (Working and Idle recede; Approval/Input/Done/Failed don't; active or selected never does) rather than as a class string. On #1 —
|
NoahHendrickson
left a comment
NoahHendrickson
left a comment
There was a problem hiding this comment.
Review: aggressive pass, verified against 342e3f2
Verdict: approve with minor cleanups. Read the full diff, then verified the riskiest claims against the actual PR head rather than taking the description's word for it. No correctness bugs that would block merging; a handful of small, real issues below. Most of the traps I went hunting for turned out to be already handled.
Adversarial checks that came back clean
- The
:has(.stage-dither)hard-edge override can't leak. SinceDevBlueprintArtnow emits dither, I checked whether the composer send button or auth screen would pick up the forcedheight/ mask removal. They can't:AuthSurfaceShell.tsx:22-25andComposerPrimaryActions.tsx:221-224both render the art in their own wrappers without thesidebar-stage-backdropclass. - Electron drag region. The toggle now sits inside the
drag-regionheader on desktop — butindex.css:1067-1072givesbutton/ainside drag regionsno-drag, so the toggle and the brand link stay clickable. - Mobile isn't stranded.
if (isSidebarVisible) return nullgates the floating control onuseSidebarVisibility(), which is mobile-aware (sidebar.tsx:89-92returnsopenMobileon mobile), so a closed mobile sheet still gets the floating trigger. The keybinding effect runs before the early return — correct hook order, and the guard test pins it. StageBackdropArtis a real upstream export (SidebarStageBackdrop.tsx:50), so the fork's import is sound.- The 86px arithmetic checks out (24 padding + 18 + 15 + 15 rows + 2×7 gap), and it's applied only to the card
li; the slim shelf keeps its own 34px intrinsic size. threadTimeLabelis not dead — still used by slim rows (SidebarV2.tsx:790); only the card's idle slot dropped it.COLS = COLUMNS.lengthis a good hardening — a hand-kept count was a real silent-crop hazard.- Rename consistency — release workflow, build script, protocol handler names, and every touched test agree on "no3y Code"; the bridge-injecting
branding.test.tscase correctly still expects "T3 Code".
Findings (ranked)
-
Five dead icon imports in
SidebarV2.tsx(lines 21, 25, 26, 30, 32).ChevronsUpDownIcon,FolderIcon,FolderOpenIcon,PlusCircleIcon, andSearchIconeach appear exactly once at PR head — the import line. Leftovers from extracting the chrome rows intoSidebarV2ChromeRows.tsx, which imports its own copies. Harmless at runtime, but it's exactly the kind of drift the fence discipline exists to avoid — and it shows the repo's lint doesn't catch unused imports. -
Inert
truncateon the meta line's right half (SidebarV2ThreadCardMeta.tsx:83). The model-label span sits inside ashrink-0flex item, so it can never be squeezed and itstruncatenever fires. A long model label will crowd out the PR/diff half (which can shrink) and can overflow the card. Latent today since real labels are short — either dropshrink-0and addmin-w-0, or cap the label with amax-w. -
Stale focus-ring styling on the inline toggle (
SidebarChrome.tsx:65).focus-visible:ring-offset-blue-700was tuned for the blue blueprint art this PR deletes; the header is now green or orange dither on every build, so keyboard focus draws a blue halo on orange artwork. The comment in the same block fixed thestroke-*→text-*half but carried this one over. -
One
role="status"live region per idle row (SidebarV2.tsx:892,915). Every idle card mounts its own polite live region saying "Idle" — dozens of live regions on a long list, and each thread transitioning to idle can trigger an announcement. If announcement-on-settle is intended, fine; if the goal was just an accessible label for the aria-hidden ring, a plainsr-onlyspan (norole) does that without the announcement machinery. -
Two guard-test checks are weaker than they read.
forkWorkspaceHeader.test.ts("styles the pills only under the fork marker") checks that some marker occurrence exists earlier in the file than each[data-fork-pill]line — any rule below the first marker block passes, scoped or not.forkSurfacePalette.test.ts("leaves light mode on upstream's palette") only matches blocks whose selector is exactly the bare marker; a fork hex leaked into:root[marker] .some-descendant { }outside.darkwouldn't be caught.
Both are fine as tripwires; just don't treat them as proofs.
-
Nits. The
SquareHalf/SquareHalfBottomentries inlucide-phosphor.tsxbroke the import list's alphabetical order (they landed betweenSidebarandSidebarSimple). And upstream'sSidebarStageBackdropwrapper now has zero production call sites — only its test renders it; consistent with keeping upstream code intact, but worth a one-line comment so a future sync doesn't "clean it up".
Known-risk items already flagged in the description
Agreed with the calls on the low-res green raster, the v1-sidebar-on-new-stage mismatch, and the dual-bundle upgrade caveat. The 1.93:1 brand-text contrast on orange is the only user-facing one I'd prioritize — that's genuinely hard to read at 12px, not just non-compliant.
Security
Nothing concerning: no new inputs, no injection surface, assets are bundled PNGs, and the rename deliberately keeps the bundle id and ~/.t3-fork stable so no state is orphaned or adopted across identities.
Net: items 1–3 are worth fixing before merge (all five-minute changes); 4–6 are judgment calls.
🤖 Generated with Claude Code
Review follow-ups. All six findings verified against the working tree before
being acted on; every one was accurate.
Five dead icon imports in SidebarV2.tsx -- ChevronsUpDownIcon, FolderIcon,
FolderOpenIcon, PlusCircleIcon and SearchIcon each appeared exactly once, on
their own import line. Left behind when the chrome rows moved to
SidebarV2ChromeRows.tsx, which imports its own. Worth noting the repo's lint
does not flag unused imports, so nothing else would have caught them.
The meta line's right half could never truncate: the model label sat inside a
shrink-0 flex item, so a long name would have pushed the PR and diff counts --
the half that can shrink -- off the row rather than clipping itself. Now
min-w-0 with a max-w-[50%] cap so neither side can starve the other.
The inline sidebar toggle still carried focus-visible:ring-offset-blue-700,
tuned for the blue blueprint art this branch deletes. On green or orange dither
that drew a blue halo on keyboard focus.
Idle rows no longer each mount a polite live region. Idle is a resting state
rather than an event, so a long list mounted dozens of role="status" nodes and
announced on every settle; a plain sr-only span still names the aria-hidden
ring, which is all that was wanted.
Two guard assertions read stronger than they were, and both are now real:
- The pill-scoping check asked whether the fork marker appeared anywhere
earlier in the file, so any rule below the first marker block passed
whether scoped or not. It now reads each rule's own selector.
- The light-mode leak check only matched blocks whose selector was exactly
the bare marker, missing a hex leaked into a marker-rooted descendant.
It now checks every marker-rooted block outside .dark.
Both go through a new __fork_guards__/cssRules.ts, because a regex over
"selector { body }" mis-parses this stylesheet specifically: it holds
@Keyframes, whose nested braces swallow the wrong span, and comments
containing braces. Verified by injecting each violation and confirming the
guard fails naming the offending selector, then reverting.
Nits: the Phosphor import list is alphabetical again (SquareHalf and
SquareHalfBottom had landed between Sidebar and SidebarSimple), and upstream's
SidebarStageBackdrop wrapper -- now with no production call site, since the
header renders the fork's own -- carries a note so a future sync does not
delete it as dead code.
|
Thanks — all six were accurate. I verified each against the working tree before touching it rather than taking the review's word for it, and every one reproduced. Pushed in
#1 — worth flagging that the repo's lint doesn't catch unused imports, so nothing else would have found these. They were exactly the extraction drift you'd expect. #2 — you were right that it's latent rather than broken today. The failure mode is worse than the label clipping: #4 — the label was only ever meant to name the #5 — the important one. You said "fine as tripwires; just don't treat them as proofs", and that was fair, so I made them proofs and then checked:
Both go through a new I then injected each violation and confirmed the guard fails naming the offender: Reverted after; On the contrast item — agreed it's the one user-facing issue, and I'd rather not guess. 1.93:1 on the orange base at 12px; Full repo suite green (188 web test files, 105 fork guards), lint, typecheck and format check clean. |
|
Closing out the contrast item: won't fix. The 1.93:1 case is the orange artwork, which only renders on a dev build — so the audience is one person who already knows. The release build uses the green, which stays as drawn in the mockup. No code change; recording it here so it does not read as an unaddressed accessibility issue on a later pass. |
1 participant
Implements the Figma redesign (Frame 51) across five areas. Dark mode only — light keeps upstream's zinc palette untouched.
Surfaces
The dark shell moves off pure black: workspace stage
#212121, sidebar v2 panel#1e1e1e. The panel sits below the stage rather than lifting off it — the panel is chrome, the stage is the work.Values are flat opaque sRGB rather than alphas: they're only correct against
#1e1e1e, and alpha compounds through hover and selection.Two selectors are required, not one. Upstream re-declares
--background/--card/--borderand the whole--sidebar-*family on the panel element itself, so a rule written only against:rootis inert on exactly the surface this is about.Upstream's
--surface-grainis dropped in dark. Measured, its 0.035 white noise carried the panel from#1e1e1eto ~#212121— the stage's colour — erasing the three-level separation the palette is built on. The workspace kept its value only because chat content paints over its copy opaquely; nothing paints over the sidebar's. The opacity that would hold spec is ~0.008, at which point there's no texture left to preserve.Thread rows
Two lines become three at 8px radius: title, repo, and a meta line whose halves sit in fixed corners (PR + diff left, model + runtime right).
--success-foreground/--destructive-foregroundand light up once upstream'slatestTurnDiff()stub carries data.Sidebar chrome
Collapse toggle moves inline beside the traffic lights; brand moves to the trailing edge.
The toggle now leaves with the panel, so the floating control still renders while the sidebar is shut — exactly the case
COLLAPSED_SIDEBAR_TITLEBAR_INSET_CLASSalready reserves space for — and thesidebar.togglekeybinding stays mounted above both, or a collapsed panel would have no way back.The header always carries artwork now — green for release, orange for dev, from the designer's PNGs. Upstream treats header art as a channel indicator; here it's brand chrome. The Dev cue survives: the composer send button and auth screen still gate on the build channel and needed no fork code. Nightly keeps its night sky.
Workspace header
56px, project and title swap weight, favicon dropped, and the three action controls become fully-rounded split pills.
They were already built from the right primitives (
Group+ outlineButton size="xs"+ separator + caret), so the only edit in those three files is adata-fork-pillattribute; all styling lives in the fork stylesheet.Selected as
buttonrather than[data-slot="button"]— Base UI'srenderprop overwrites the slot withtooltip-trigger/menu-trigger, so a slot-based selector matches nothing and fails silently.Identity
The fork is "no3y Code" everywhere, renamed from "N3 Code" — including the rendered app.
branding.ts's bridge-less fallback pointed at upstream's"T3 Code", so dev and hosted sessions were branded as upstream while the packaged app wasn't.Safe by construction: the bundle id and
~/.t3-forkare both deliberately held stable across renames, so nothing is orphaned.N3 Code.appis not replaced by a build installing asno3y Code.app— an upgrader keeps both bundles until the old one is deleted by hand.Fork bookkeeping
Four new manifest entries (
fork-surface-palette,fork-sidebar-chrome,fork-workspace-header, plus a rewrittensidebar-v2-card-rows), each with a guard test. Three upstream tests were carried through where they pinned behaviour this deliberately changes:branding.test.ts— the bridge-less display name. One of its three cases injects a bridge and exercises a path the fork doesn't touch, so it correctly still expects "T3 Code".SidebarStageBackdrop.test.tsx— asserted the Dev art emits unique SVG<defs>ids; a raster has none. Only the "is this exercising anything" precondition narrows to Nightly, so reverting Dev to SVG art re-arms it.Verification
#212121/#1e1e1e/#2d2e2e/#2a2a2a; hover and selection confirmed; v1 verified still on upstream's#000; light mode verified unchanged; collapsed sidebar verified still reachable.detect-drift.mjsresolves every entry against its watch list.Known follow-ups
#000panel against the new#212121stage. The stage is global chrome with no sidebar-version attribute to key off.SidebarV2Row). "Tabs" here meansRightPanelTabs.🤖 Generated with Claude Code