feat(web): sidebar drafts, collapsible groups, and 14px card marks

[NoahHendrickson]

Summary

• Unpromoted drafts show as sidebar cards (title from typed prompt when you leave the draft); discard navigates off /draft/$id before clearing so it cannot race into a fresh draft
• Project groups can collapse (persisted in localStorage), keeping the open route thread visible like the snoozed shelf
• Card status/rain slot and title line stay at 14px — drop text-xs on card titles so --text-xs--line-height: 1rem no longer grows the row to 16px; branch/worktree/runtime marks at 14px with runtime flush to the trailing action axis

Test plan

• Create a draft via +, confirm a sidebar card appears; type without sending, switch away, confirm title updates from the prompt
• Discard the open draft — lands on a neighboring thread, does not immediately open a new draft
• Toggle project grouping, collapse/expand a group, reload — collapsed state persists; active thread still shows when its group is collapsed
• Inspect a working card’s rain/status slot in DevTools — height is 14px, not 16px
• Confirm branch/worktree/runtime icons read at 14px and runtime aligns with settle/discard

Made with Cursor

[cursor]

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.

[cursor]

Thermo-nuclear code quality review — do not approve.

Custom projection for draft shells (sidebarV2DraftRows.ts) and collapse helpers are in the right layer and tested; the 14px mark work is contained. The approval bar fails on structure in SidebarV2.tsx (already ~3.2k → ~3.5k):

1. Draft identity is still a Map/boolean probe bolted into navigate, rename, settle, snooze, context menu, render props, and discard — not a row kind / adapter that would delete those branches.
2. Discard reimplements adjacency + draft-aware routing beside planForwardNavigation.
3. Collapse membership is computed twice.
4. Manifest metrics contradict the code (56/79 vs 54/77).

Inline comments ordered by structural priority. Fix the draft model first; the discard planner and trailing-cell branching largely fall out of that.

  

_{Sent by Cursor Automation: Thermo-nuclear PR review}

[NoahHendrickson]

Review — aggressive pass

Read the full diff against custom@96a5aa3 and traced each change into the surrounding code. The typography retune is careful and the draft projection is a genuinely good shape (pure helper + thin seam, tested next to the module). But three things here are load-bearing regressions, and one of them silently defeated the fork guard that exists to prevent exactly it. Ranked by what I'd fix before merge.

I could not run the suite (no node_modules in this environment), so everything below is read from source, not from a failing test.

---

1. overflow-hidden on the 14px title row clips the 24px action cell — blocking

SidebarV2.tsx:1118

<div className="flex h-[14px] min-h-[14px] min-w-0 items-center gap-2.5 overflow-hidden">

SidebarV2.tsx:1172 puts a grid h-6 (24px) cell inside it. A flex item with an explicit height under items-center doesn't shrink — it overflows 5px above and below and the new overflow-hidden clips all of it. The comment directly above that cell still asserts the opposite:

The cell is centred in the title line, so it overhangs into the card's py-2 above and its gap-2 below — neither of which carries anything to collide with.

That overhang is now clipped away. Three concrete losses, all owned by the sidebar-v2-row-action-hit-area entry:

• Hit target 24px → 14px. Clipping applies to hit testing, not just paint.
• Coarse-pointer target 44px → 14px. SIDEBAR_V2_ICON_BUTTON_CLASS carries pointer-coarse:after:min-h-11 pointer-coarse:after:min-w-11 — the whole touch-target expansion is inside the clip. This lands on tablets, touch laptops, and app.t3.codes on a phone.
• Focus ring gone. The wrapper's focus-within:overflow-visible exists solely because the clip "cuts the focus ring: it is a box-shadow 4px outside a button the wrapper hugs exactly, so a keyboard user got no indicator at all on the one control the ring was added for." An ancestor overflow-hidden re-imposes the clip the escape hatch was written to defeat. The escape hatch is now dead code.

The worst part is that sidebarV2RowActionHitArea.test.ts still passes — it greps for the literal grid h-6 shrink-0 grid-cols-1 items-center justify-items-end, which is still present and now inert. The guard went vacuous in the same commit that broke the thing it guards, and its comment was edited to describe the overhang as still working.

The clip also buys nothing. The stated justification (sidebarV2CardRows.test.ts: "overflow-hidden is load-bearing — the native grid is taller") was true when the SVG rendered at its native 18.05px. It isn't anymore: this PR gives the SVG h-[14px] plus its own overflow-hidden, and the status span is a third size-[14px] overflow-hidden clip around it. The rain cannot exceed 14px before it ever reaches the row.

Drop overflow-hidden from line 1118. Then either restore the hit-area guard to something that can actually fail (assert no clipping ancestor between the h-6 cell and the card), or delete the focus-within:overflow-visible machinery — keeping both a broken guard and a dead escape hatch is worse than either.

2. useLocalStorage gets a fresh [] every render — blocking

sidebarV2ProjectGrouping.ts:56

const [keys, setKeys] = useLocalStorage(
  SIDEBAR_V2_COLLAPSED_PROJECTS_STORAGE_KEY,
  [] as readonly string[],   // ← new identity every render
  CollapsedProjectKeys,
);

initialValue is in the deps of useLocalStorage's storedValue memo (useLocalStorage.ts:148). A literal [] inline means that memo never hits, so keys is a new array on every render → collapsed is a new Set on every render → and then, in order:

visibleActiveSections (SidebarV2.tsx:1953) → orderedActiveThreads → orderedThreads → orderedThreadKeys → threadByKey

all rebuild on every render of SidebarV2, including every nowMinute tick and every shell stream update. That's several O(n) array walks plus a Map build over the whole thread list, per render, forever — even for the overwhelmingly common user who has never collapsed a group.

The file two hooks away already warns about this exact failure mode: "a fresh array identity per shell update would defeat every row's memoization." useSidebarV2GroupByProject sidesteps it by passing a primitive.

Hoist a module-level const NO_COLLAPSED_KEYS: readonly string[] = []. While you're there, visibleActiveSections should return activeSections unchanged when collapsedProjectKeys.size === 0 — otherwise every navigation still re-spreads every section object and invalidates the ordering chain for a filter that removed nothing.

3. Bulk delete on a selection containing a draft breaks the whole batch — blocking

Draft shells now land in activeThreads, therefore in orderedThreads, therefore in threadByKey. handleMultiSelectContextMenu builds its actionable set from threadByKeyRef.current.has(threadKey) — which is now true for drafts — and never got the guard that attemptSettle and attemptSnooze did.

SidebarV2.tsx:2525:

const result = await deleteThread(scopeThreadRef(thread.environmentId, thread.id), { deletedThreadKeys });
if (result._tag === "Failure") { ...toast...; return; }

Cmd-click a draft card into a multi-selection, right-click a real thread, Delete (3). The draft's reserved thread id doesn't exist server-side, the call fails, and the loop returns — aborting deletion of every thread after it in the batch, with a generic "Failed to delete threads" toast and no indication which row poisoned it. The single-row context menu is correctly intercepted; the bulk one isn't, and it's reachable in one click.

Same set, softer: Settle (N) / Snooze (N) / Mark unread (N) all count drafts they will silently no-op on (settle/snooze) or apply meaninglessly to (unread on a thread with no turns). Filter drafts out of threadKeys at the top of handleMultiSelectContextMenu and the counts fix themselves.

---

4. The manifest contradicts the code and itself

.fork/customizations.yaml:346 — "contain-intrinsic-size is 56 / 79, the drawn card exactly" — and :366 — "the hints equal the drawn heights of 56 and 79."

The code, the guard, and the card comment all say 54 / 77, and 54/77 is the correct arithmetic: 8 + 14 + 8 + 16 + 8 = 54, + 8 + 15 = 77. A third paragraph in the same manifest entry also says 54 / 77. 56/79 looks like a leftover from an intermediate gap tuning.

In a fork whose entire discipline is "the manifest is the derivation," a manifest that disagrees with its own guard in two places is the thing most likely to send the next sync in the wrong direction.

5. leading-[14px] on 14px text leaves zero leading

threadCardTitleClassName returns text-[0.875rem] leading-[14px] — and the same span carries truncate, i.e. overflow: hidden. Geist's content area at 14px is ~17.5px, so glyphs overflow the line box ~1.7px each way and get shaved by the span's own clip (and again by the row clip from §1). Descenders (g p y q j) and capital diacritics are the visible casualties, on every card title.

This may well be fine in practice — but there's no way to tell from here, and the PR has no before/after images. CLAUDE.md and CONTRIBUTING.md both require them for UI changes, and this is a PR whose entire third bullet is a 2px typographic retune. Please attach them; a pixel-level claim like "height is 14px, not 16px" belongs in a screenshot, not in a test-plan checkbox.

6. Discard is one click, unconfirmed, and unrecoverable

The X destroys typed-but-unsent composer text with no confirm and no undo — and this PR specifically made that text the card's own title, so the row is now advertising exactly what the adjacent button silently deletes. Delete-thread confirms. Snooze has unsnooze; settle has unsettle; the fork's own "reverse states" rule says a one-way door is a bug.

At minimum, confirm when the composer is non-empty. sidebarDraftTitleFromPrompt already tells you whether there's anything to lose.

7. The prompt-snapshot rationale doesn't hold

// Prompt titles are read from getState() on route/shell changes — not
// subscribed per keystroke ...
// routeThreadKey: snapshot composer prompts when the open thread changes.

draftRows also depends on serverThreadKeys, which is a new Set whenever threads changes — i.e. on any shell update anywhere. So getState() re-reads every draft's prompt at effectively arbitrary moments, not "when you leave the draft." User-visible consequence: you're typing, some unrelated thread streams a token, and the sidebar title pops from "New thread" to your half-typed sentence mid-word. Non-deterministic, and worse than either honest option (subscribe with a debounce, or genuinely snapshot on route change only).

Separately: reading an unsubscribed external store inside useMemo is unsound under React's rules — a memo may be discarded and recomputed at any time, so this is the same class of bug even without the dep-array issue.

8. Persisted drafts never expire, and skip the environment filter

listSidebarDraftRows walks the whole persisted draftThreadsByThreadKey. Two consequences worth an explicit decision:

• Every project where someone hit + and never sent now keeps a permanent card, surviving reload, removable only by finding it and discarding it. Is there any pruning story?
• threads is environment-scoped by the shell atoms; drafts are filtered only by scopedProjectKeys. A draft belonging to an environment that is no longer connected still paints a card and still routes to /draft/$id. Given how much this repo cares about multi-environment and relay/tunnel modes, that's worth confirming rather than inheriting.

9. Smaller

• The collapse button swaps folder→chevron on group-hover/collapse: only. A keyboard user tabbing to an expanded group's collapse control sees an unchanged folder and no focus-driven affordance — add group-focus-within/collapse:.
• A collapsed group shows no count of what it's hiding. The snoozed shelf you're modelling this on does tell you.
• collapsedProjectKeys is never pruned when a project is deleted, so the key list grows monotonically in localStorage.
• Worth noting in a comment: composerDraftStore.draftThreadsByThreadKey is keyed by DraftId, not by a thread key. Upstream's misnomer, and the code here handles it correctly — but draftsById receiving something named ...ByThreadKey is exactly the kind of thing a future reader "fixes."

10. Scope

Three independent concerns in one PR: draft rows (a new feature crossing routing, selection, context menus, and bulk actions), collapsible groups (a new persisted preference), and a 14px typographic retune (a pure presentation pass). CLAUDE.md: "One concern per PR. If the description says 'also', split it." The title enumerates three.

That isn't pedantry here — §1 and §3 are both cases where the retune and the feature quietly reached into machinery the other one owned, and a reviewer looking at any single concern in isolation would have caught them faster. If splitting is too expensive now, at least land the 14px retune separately so the hit-area regression is reviewable on its own.

---

Summary: §1, §2, §3 should block. §4 is a two-line manifest fix. §5 needs screenshots before I'd sign off on the type change at all. The rest is judgement.

---

Generated by Claude Code

[NoahHendrickson]

Follow-up review — d58d3f8 → 5880bf6

Three commits landed after my last pass. Reviewed the delta; fixes get the same scrutiny as the code they fix.

✅ 5880bf6 — clip removed, correctly

overflow-hidden is off the title row and the rationale in the new comment is right: the rain clips itself twice below (size-[14px] overflow-hidden on the status span, overflow-hidden on the SVG at h-[14px]), so the row-level clip was pure cost. §1 is resolved — hit target, pointer-coarse 44px expansion, and the focus-within:overflow-visible ring escape all come back.

The guard was also upgraded from a passive toContain to a not.toMatch, so it can fail now. But it's narrow: it pins the absence of overflow-hidden in one exact position of one exact class string. The failure mode I actually described was a clipping ancestor — overflow-clip, a reordered class list, or the same clip added one level up on the card div all sail past it. If this is worth a guard, guard the property, not the spelling.

✅ 4d1db81 — findLast is clean

lib: ["ES2023", ...] in apps/web/tsconfig.json, and there are seven existing .findLast( call sites (ChatView.logic.ts, session-logic.ts, packages/client-runtime/state/projects.ts, …). Semantically identical, one less array copy. No notes.

🔴 a0e6ac3 fixed one assertion in that guard and missed that the one below it had already gone vacuous

This is new, and it's the same failure class as §1 — a guard that passes while its subject is unprotected.

sidebarV2ProjectGrouping.test.ts:

const start  = sidebar.indexOf("const items: ReactNode[] = visibleActiveSections.flatMap(");
const render = sidebar.slice(start, sidebar.indexOf("});", start));

Adding collapsed / onToggleCollapsed pushed that call past the print width, so prettier reformatted it and the callback now closes with },\n); instead of });. I checked the file at 5880bf6:

start index: 137511 (line 3113)
indexOf("});", start) = -1        ← no match anywhere after it
slice length: 342 lines           ← runs to EOF, not the ~40-line flatMap body

indexOf returns -1, so slice(start, -1) yields everything from line 3113 to the end of the file — the snoozed shelf, the settled shelf, all of it. Every expect(render).toContain(...) below is now satisfied by the string existing anywhere later in the component.

That defeats the test's stated purpose, in its own words:

the header from the same section whose threads follow it. Dropping this hunk is the likeliest outcome of a merge that rewrites upstream's list body, and it would leave a flat-looking sidebar over a grouped ordered list.

It's a regression from this PR — at custom@96a5aa3 the block did end in }); and the slice was correct. a0e6ac3 repaired the multiline spelling of the assertion four lines above this one, which is why it's worth calling out: the reformat that broke that assertion loudly is the same reformat that broke this one silently.

Fix: slice with a brace-matched end, or bound it with a sentinel that survives formatting (sidebar.indexOf("/* fork:end sidebar-v2-project-grouping */", start) is right there). Then confirm it fails when you delete the header hunk — a containment guard that has never been mutation-tested is decoration.

Still open from the last pass

Verified unchanged at 5880bf6:

• §2 — [] as readonly string[] at sidebarV2ProjectGrouping.ts:56. Untouched; the whole ordering chain still rebuilds every render. This is the cheapest fix on the list.
• §3 — bulk delete. handleMultiSelectContextMenu still admits drafts at SidebarV2.tsx:2393; a draft in a multi-selection still aborts the rest of the batch.
• §4 — manifest still says 56 / 79 at .fork/customizations.yaml:346 and :366, against 54 / 77 in the code, the guard, and its own third paragraph.
• §5 — not resolved by 5880bf6. Removing the row clip does not un-clip the title: threadCardTitleClassName is still truncate text-[0.875rem] leading-[14px] (sidebarV2RowPolicy.ts:107), and truncate is overflow: hidden on that span, whose height is the 14px line box. Descenders are still shaved by the span's own clip, one level in from the one that was just removed. Still needs eyes on a real render, and the PR still has no before/after images.
• §6–§10 (unconfirmed discard, prompt-snapshot rationale, draft pruning / environment scope, keyboard affordance, PR scope) unchanged.

Net: one real blocker fixed, one new guard-erosion found, §2/§3/§4 still blocking.

---

Generated by Claude Code

[cursor]

Code review — f61186d (HEAD)

Reviewed the full PR against custom, then re-verified every still-open finding at HEAD after the thermo-nuclear follow-up. Do not merge: Check and Test are both red on this commit, and two prior behavioral blockers are still present.

---

Blocking — new in f61186d

1. TDZ: showSnoozeButton used before declaration — Check fails

SidebarV2.tsx:805 computes hasHoverActions from showSnoozeButton, but showSnoozeButton is declared at :834. CI typecheck:

error TS2448: Block-scoped variable 'showSnoozeButton' used before its declaration.
error TS2454: Variable 'showSnoozeButton' is used before being assigned.

Move the hasHoverActions line below the snooze derivation (or hoist showSnoozeButton above it). This was introduced by “compute hover actions once.”

2. replace: opts?.replace fails under exact optional props — Check fails

navigateToThread passes replace: opts?.replace (boolean | undefined) into both draft and thread navigates (:2076, :2084). CI:

Argument of type '{ to: "..."; params: ...; replace: boolean | undefined }'
is not assignable to parameter of type 'NavigateOptions<...>'.

Omit the key when unset (...(opts?.replace ? { replace: true } : {})), or require replace: boolean at the call sites that need it.

3. Draft-rows guard pins Prettier-unstable spelling — Test fails

sidebarV2DraftRows.test.ts:73 expects:

"const hasHoverActions =\n    props.settlementSupported || showSnoozeButton || showDiscardDraft;"

Source is one line. Same class of failure as the earlier flatMap needle. Assert presence of hasHoverActions / its three uses, not a specific wrap.

---

Blocking — still open from earlier passes

4. Grouping render-slice guard is vacuous

sidebarV2ProjectGrouping.test.ts:69:

const render = sidebar.slice(start, sidebar.indexOf("});", start));

At HEAD, indexOf("});", start) === -1 (callback closes as },\n);). slice(start, -1) runs to EOF, so every later toContain can pass from snoozed/settled code. Bound with /* fork:end sidebar-v2-project-grouping */ (or brace-match), then mutation-test by deleting the header hunk.

5. Fresh [] identity into useLocalStorage — render thrash

sidebarV2ProjectGrouping.ts:56 still passes [] as readonly string[] inline. That value is in useLocalStorage’s storedValue deps, so keys → collapsed → visibleActiveSections → the whole ordering/threadByKey chain rebuild every SidebarV2 render (including nowMinute ticks), even with nothing collapsed. Hoist a module-level empty array; ideally short-circuit visibleActiveSections to activeSections when the set is empty.

6. Bulk delete still poisoned by drafts

handleMultiSelectContextMenu still builds threadKeys from threadByKey (:2365) with no draft filter. Drafts are in that map. Cmd-click a draft into a multi-selection → Delete: deleteThread fails on the reserved id, the loop returns, and later real threads are left alone under a generic toast. Single-row menu is correct; bulk is not. Filter drafts out of the actionable set (and out of Settle/Snooze/Mark-unread counts).

---

Should fix before merge (not CI-red, still real)

7. Prompt title snapshot is not route-scoped

Comment claims titles refresh on routeThreadKey only, but draftRows also depends on serverThreadKeys (new Set whenever threads changes). Unrelated shell updates re-run getState() and can retitle the open draft mid-keystroke. Either subscribe with debounce, or snapshot only on route change (drop serverThreadKeys from the prompt path).

8. truncate + leading-[14px] on 14px titles

threadCardTitleClassName is still truncate text-[0.875rem] leading-[14px]. truncate ⇒ overflow: hidden on a 14px line box; Geist’s content area is taller, so descenders/diacritics get clipped. Needs a real render check — and this PR still has no before/after screenshots for a 2px typographic claim.

9. Collapse keyboard affordance

Folder→chevron swap is group-hover/collapse: only. Focused collapse control still shows the folder. Add group-focus-within/collapse:.

---

Verified fixed / improved at HEAD

• Title-row overflow-hidden removed; rain self-clips at 14px; 24px action overhang comment matches.
• Manifest contain-intrinsic / drawn heights agree at 54 / 77.
• collapsed carried on visible sections (no second membership probe at the header).
• Discard uses pickDiscardNeighborKey + navigateToThread({ replace: true }) before clearDraftThread.
• sidebarDraftRowCapabilities at the list boundary is the right seam for settle/snooze/rename/discard.
• Pure helpers in sidebarV2DraftRows.ts are well-tested next to the module.

---

Scope note

Three concerns in one PR (draft rows, collapsible groups, 14px retune). The latest “fix” commit both addressed structure and introduced the two Check failures — another argument for landing the type retune separately once CI is green.

Merge bar: fix §1–§3 (CI), §4–§6 (prior blockers), then screenshots for the type change. I did not run the suite locally beyond reading CI logs and the source; findings above are confirmed against f61186d on disk.

Review by Cursor Cloud agent (run)