feat(fork): group sidebar v2 threads by project

[NoahHendrickson]

Adds an optional Group by project mode to Sidebar V2, and collapses thread cards to the design's two-line form when there is nothing to put on the third line.

Group by project

A switch at the top of the fork's project scope row. Upstream's flat stream stays the default; grouping answers the question scoping cannot — "everything, arranged by repo" rather than "only this repo" — which is why the two live in the same menu.

• Only the active cards group. The snoozed and settled shelves are time-ordered tails whose value is being short; slicing a tail by project turns one header into a dozen sub-headers over one or two rows each.
• Skipped while a scope is set — the list is already one project, so the header would repeat the scope row's own label one line lower.
• Group order is projectGroups' own order, already sorted by the user's project sort order and already folded per the environment-grouping mode. Thread order inside a group is untouched, so grouping introduces no second notion of sorting.
• orderedThreads is rebuilt from the grouped order. It backs the ⌘-jump labels, arrow navigation and shift-range selection — a flat ordered list under a grouped DOM misaddresses all three, and nothing about the render looks wrong when it does.
• A thread whose project resolves to no group (just-deleted project, environment still loading) keeps a trailing "Other" header rather than vanishing or trailing headerless under the previous project.
• Grouped cards drop their project name, since the header two rows up carries it and the branch is what actually tells two threads on one project apart.
• The preference is device-local (localStorage), not a packages/contracts field — no fork-shaped key in a schema upstream owns.

Two-line cards

The third card line exists to carry the PR badge and the diff counts. With neither, it is a blank 15px strip, so the model and runtime move up beside the branch and the card closes at the design's two-line 64px (Figma). Nothing else varies the height — an absent branch or model leaves its half of a row empty rather than closing the row up. The contain-intrinsic-size hint is chosen by the same predicate the component renders from, so it cannot claim a height the row does not have.

Worth knowing: latestTurnDiff() is still an upstream stub returning null, so until shells carry checkpoint summaries the three-line form only appears on threads with a PR.

Fork placement

New Tier-1 files under apps/web/src/custom/; SidebarV2.tsx carries four small fenced hunks under a new sidebar-v2-project-grouping manifest id with its own guard test. Intents are invariants-only per the convention that landed in #21, with the narrative in .fork/notes/FORK-CUSTOMIZATION-DECISIONS.md.

Verification

• vp test run apps/web/src/__fork_guards__ apps/web/src/custom — 21 files, 157 tests passing, including a new render test for the card's two-line collapse and unit tests for the bucketing (order, folded member projects, empty groups, unresolved projects, key collisions).
• vp fmt, vp lint, node .fork/lint-owned.mjs (46 fork-owned files, no warnings), web typecheck all clean.
• Browser pass in a seeded environment (3 projects, 5 active + 2 settled threads) confirmed grouped render from a cold mount, flat render with the switch off, the switch surviving reload, scoping suppressing headers, the settled shelf staying flat across projects, and jump hints reading ⌘1–⌘7 down the grouped list. The final two-line/no-project-name revision was verified by tests only — the preview browser's MCP token expired before it could be re-checked visually.

🤖 Generated with Claude Code

[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 — REQUEST_CHANGES

Behavior is fine. Structure is not. The approval bar fails on a visible code-judo miss and spaghetti growth inside an already oversized SidebarV2.tsx.

1. Null dual-mode is the wrong model — collapse to one list shape

activeThreadProjectGroups === null means “flat.” That sentinel is then re-checked in three places that must stay synchronized:

• ordered-thread memo (orderedActiveThreads)
• projectTitle suppression (!== null && section === "active")
• active-items render (.map vs .flatMap + headers)

That is mode-flag spaghetti, not a preference feature.

Code judo: in custom/sidebarV2ProjectGrouping.ts, return one section list always:

type ActiveSection<T> = {
  header: { projectKey: string; displayName: string } | null;
  threads: readonly T[];
};
// flat / scoped → [{ header: null, threads }]
// grouped     → groups.map(g => ({ header: {...}, threads: g.threads }))

Then SidebarV2 becomes one path:

• orderedActiveThreads = sections.flatMap(s => s.threads)
• render: sections.flatMap with optional header
• hideProjectTitle = section.header !== null

Delete the null sentinel, the second ternary memo, the dual render branch, and the projectTitle special case. Fork-friendly: shrinks fenced hunks by pushing the model into custom/ (same move as ChromeRows / ThreadCardMeta).

2. More dual-mode control flow in a 2655-line file

SidebarV2.tsx was already far past 1k. Fenced hunks are allowed, but this PR still grows the busy render IIFE with a second control-flow universe. The fix is not “rewrite upstream ownership” — it is extract the list model so the fence is a call site, not three coordinated branches.

3. Guard tests duplicate unit behavior and ossify the dual-mode shape

__fork_guards__/sidebarV2ProjectGrouping.test.ts re-tests orphan retention and project/thread order already covered by custom/sidebarV2ProjectGrouping.test.ts, and it pins the exact string activeThreadProjectGroups !== null && section === "active". Guards should only pin the seam (ordered list wiring, menu toggle, active-only grouping). Behavior stays once in the unit file; stop locking the null-sentinel design into the guard.

What is fine

• Pure groupThreadsByProject + structural interfaces + localStorage preference (correct fork boundary)
• SidebarV2ProjectGroupHeader as a small presentational extract
• threadCardShowsMetaRow + two-line card collapse (clean, shared with contain-intrinsic-size)

Ship after the list-model collapse. Do not approve the dual-mode as-is.

  

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

[cursor]

this null-as-mode dual model is the structural problem. null means flat, and that sentinel has to stay synchronized with orderedActiveThreads, the projectTitle branch, and the active-items ternary below — three coordinated special cases in an already 2.6k-line file.

code-judo: extract a single section model in custom/sidebarV2ProjectGrouping.ts that always returns ReadonlyArray<{ header: { projectKey; displayName } | null; threads }> — flat/scoped is one section with header: null; grouped is one section per project with a header. Then orderedActiveThreads = sections.flatMap(s => s.threads) and the render path stops branching on null. that deletes this second memo's ternary entirely and shrinks the fenced hunk to a call site (same extraction pattern as ChromeRows / ThreadCardMeta).

[cursor]

this activeThreadProjectGroups !== null && section === "active" branch is the same dual-mode leaking into row props. under a section model, hide-project-title is just section.header !== null at the call site that already knows which section it is painting — no null-sentinel check inside renderThreadRow, and no section === "active" coupling to grouping state.

also: the fork guard currently pins this exact expression, which will fight the refactor. drop that string assert when you collapse the model.

[cursor]

dual render path is the symptom of the null mode above. once sections always exist, this is one flatMap: optional <SidebarV2ProjectGroupHeader /> when header !== null, then the thread rows. no .map vs .flatMap fork, and no need to keep activeThreads and activeThreadProjectGroups both live in this IIFE.

minor while you are here: flatMap((group) => [...group.threads]) above copies for nothing — group.threads is already a readonly array.

[cursor]

these two cases re-test groupThreadsByProject orphan retention and project/thread order — already covered (better) in custom/sidebarV2ProjectGrouping.test.ts. the guard's job is the SidebarV2 seam (ordered list wiring, menu toggle, active-only / no-scope gating), not a second copy of the pure function's behavior.

delete the duplicated behavior tests from the guard. keeping them here also encourages pinning implementation strings like the null-sentinel projectTitle check, which locks in the dual-mode design we should be deleting.

[NoahHendrickson]

Review

Read the whole diff plus the surrounding upstream wiring in SidebarV2.tsx, Sidebar.logic.ts, sidebarProjectGrouping.ts and useLocalStorage.ts. The grouping core (groupThreadsByProject) is genuinely good: total, order-preserving, no second sort, NUL-joined ref key that matches what sortLogicalProjectsForSidebar already does upstream, and memberProjectRefs rather than memberProjects is the right member list to bucket on (it survives the duplicate-member dedup that memberProjects applies). The scope-suppression argument and the "only active cards group" argument both hold up.

The problems are all at the seams — one real regression in the card change, and a guard that does not guard the thing it was written for.

---

1. The two-line collapse makes card height depend on an async query — every PR card grows 15px after mount

SidebarV2.tsx:831-848 decides the card's row count from prBadge !== null, and prBadge comes from pr / prStatus at SidebarV2.tsx:506-511, which are derived from gitStatus.data — a useEnvironmentQuery. On first paint gitStatus.data is undefined, so pr is null, so every card renders two lines at 64px. When the VCS status resolves, cards with a PR re-render at three lines / 86px and push everything below them down.

Before this change the card was a fixed three rows, so the query resolving just filled the badge into a slot that was already reserved. Now it reflows the list. With N PR-carrying threads you get up to N separate 15px shifts on every sidebar mount and every environment reconnect, staggered as each per-row query lands. Browser scroll anchoring will absorb some of it, but not the in-viewport reflow, and rows can move under the pointer between mousedown and click.

This is also structurally worse than it looks today. latestTurnDiff() is stubbed to null (SidebarV2.tsx:986), so right now the PR is the only thing that can open the third row — the feature's entire variability is async-driven. When checkpoint summaries land and latestTurnDiff() starts returning data, that will be async too, and the shift will apply to nearly every card instead of just PR ones. The problem compounds rather than resolving.

The predicate needs a third state: PR unknown. A thread that has a branch and a pending git-status query should reserve the row rather than collapse and then grow. Something like passing prPending (thread.branch != null && gitStatus.isPending) into threadCardShowsMetaRow and treating it as truthy keeps the height stable through resolution, and still gives you the two-line card for the branchless / no-query threads immediately and permanently. Threads where the query is never issued at all (no branch, no worktree — SidebarV2.tsx:492-497) collapse straight away with no shift, which is the common case the design is actually about.

Worth adding a guard for once fixed, because "collapses only when we know there is no PR" is exactly the kind of invariant a later refactor flattens back into hasPr.

2. The ordering guard cannot fail on the divergence it exists to catch

__fork_guards__/sidebarV2ProjectGrouping.test.ts opens by naming its purpose precisely: the ordered thread list "backs the jump labels, arrow navigation and range selection, and nothing about a grouped list looks wrong when that list still holds the flat order." Correct diagnosis. But the guard is expect(sidebar).toContain(...) against source text, and no assertion in the file mentions SidebarV2ProjectGroupHeader or the grouped items construction at all.

Concretely: delete only the fenced items hunk at SidebarV2.tsx:2363-2377 — the single most likely outcome of an upstream merge that rewrites the list body, since that hunk sits in the middle of upstream's own render and the other three hunks don't. Every assertion still passes:

• "[...orderedActiveThreads, ...visibleSnoozedThreads" — still there (:1530)
• definition index < use index — still true
• "groupByProject && projectScopeKey === null" — still there (:1513)
• 'activeThreadProjectGroups !== null && section === "active"' — still there, it's in the projectTitle hunk (:2336)
• both for (const thread of …) shelf assertions — untouched
• the two pure unit tests — they test groupThreadsByProject in isolation and never see the component

Green suite, and the sidebar now paints flat while orderedThreads holds grouped order and cards render with projectTitle={null} under no header at all. That is a worse state than either mode, reached silently, by the exact mechanism the file's docblock describes.

The fix I'd argue for is structural rather than another toContain: build the ordered sequence once and render from it. Right now items (:2363) and orderedActiveThreads (:1520) are two independent flatMaps over activeThreadProjectGroups that must agree by convention. If a fork-owned helper returned the interleaved sequence — headers and threads in paint order — then items maps it to nodes and orderedActiveThreads filters it to threads, and divergence stops being representable. That helper is pure, lives in custom/, and is testable the same way groupThreadsByProject already is. Grep guards are the right tool for "this literal must survive"; they are the wrong tool for "these two derivations must agree."

Two related corrections to the claim itself, which matters because the manifest intent is what a future merge reasons from:

(a) Jump labels cannot misaddress. jumpLabelByKey maps thread key → label (:1588-1597) and the handler navigates to orderedThreadKeys[jumpIndex] (:2179) — both indexed off the same array, so ⌘3 always opens the row that displays ⌘3, in any DOM order. A mismatch makes the badges appear out of sequence down the screen (⌘1, ⌘5, ⌘2…), which is confusing, not wrong. The two that genuinely misaddress are resolveAdjacentThreadId({threadIds: orderedThreadKeys, …}) (:2168) and rangeSelectTo(threadKey, orderedThreadKeysRef.current) (:1668), both positional. The PR body and .fork/customizations.yaml both assert all three misaddress; the manifest should say what's actually true so the next agent doesn't over- or under-weight the constraint.

(b) There is a fourth consumer, and it silently changes behavior. planForwardNavigation (:1690-1700) reads orderedThreadKeysRef.current positionally to pick where you land after settling or snoozing the thread you're viewing. Under grouping, "forward" now means next card in the grouped order, so settling the last active card in project A drops you into project B's first card instead of the next most-recently-active thread. That's arguably the right answer — forward should mean the next visible row — but it is a real change to a navigation behavior, it isn't in the PR body, and it isn't in the manifest intent's list of what orderedThreads backs. Please add it, and confirm it's the intended landing.

3. groupThreadsByProject rebuilds its ref→key map far more often than it needs to

sidebarV2ProjectGrouping.ts:80-86 builds projectKeyByRef from every group's every member ref on each call, and the memo at SidebarV2.tsx:1511-1516 depends on activeThreads. activeThreads (:1362) recomputes on nowMinute, snoozeWakeTick, serverConfigs, and changeRequestStateByKey — and every row reports its PR state up through onChangeRequestState on mount (:513-516), so a cold sidebar with N PR threads produces a burst of N activeThreads identities, each rebuilding the full project map. Splitting the map into its own useMemo keyed on projectGroups alone, or accepting a prebuilt map, drops that to once per project-list change.

While you're there: sortLogicalProjectsForSidebar (Sidebar.logic.ts:765-786) already builds the identical \0-joined ref→projectKey map from the same memberProjectRefs, immediately upstream of this call, and throws it away. Not a blocker — the fork file deliberately keeps a narrow structural interface and I wouldn't widen custom/ to import a components/ internal — but it's worth a line in the docblock noting the parallel exists, so the two can't drift apart on what a ref key is.

4. The group headers are invisible to assistive tech

SidebarV2ProjectGroupHeader.tsx:28-42 renders an inert <li> holding an aria-hidden icon and a bare <span>. Inside upstream's thread <ul> this reads to a screen reader as a flat list of N+G items where G of them are orphan text with no relationship to the cards under them — the grouping is purely visual. Upstream's two shelf headers get away with this because they're buttons: focusable, announced, with expanded state. A static header has no such fallback.

Cheapest honest fix is role="presentation" on the <li> plus a real heading on the label (role="heading" aria-level={3}, or an <h3> styled to match). Better, if the list markup tolerates it, is one <ul aria-label={group.displayName}> per group, which gives the cards actual containment instead of a sighted-only adjacency. Since grouped cards now drop their project name entirely (SidebarV2.tsx:2336), a non-visual user loses the project association completely in grouped mode — it isn't merely un-nice, it's strictly less information than flat mode carries.

Also: data-thread-selection-safe on the header is inert in V2. THREAD_SELECTION_SAFE_SELECTOR is only consumed by shouldClearThreadSelectionOnMouseDown, which only Sidebar.tsx (V1) calls. Copying it from upstream's V2 shelf headers is defensible for consistency, but it isn't doing anything, and if V2 ever wires up click-to-deselect, an inert header that swallows the deselect gesture is the wrong default for a non-interactive element.

5. The switch is silently inert while a scope is set

Suppressing headers under a scope is right. But the switch stays enabled and toggleable in that state (SidebarV2ChromeRows.tsx:170-184), one row above the scope radio group that's causing the suppression — so a user flips it, watches nothing happen, and has no way to learn why. The comment acknowledges this ("the sidebar simply has nothing to group until the scope is cleared") but the UI never says it.

disabled while projectScopeKey !== null, with a tooltip naming the reason, costs a line. The alternative reading — that picking a scope should clear grouping, or that enabling grouping should clear the scope — is defensible too given they answer adjacent questions, but silently-inert is the one option that teaches the user nothing.

6. The single-project case repeats the PR's own anti-pattern

The stated reason to skip grouping under a scope is that "the header would repeat the scope row's own label one line lower." With exactly one project group, grouping renders one header naming the only project the user has, above every card, and strips the project name off all of them — the same redundancy, from the same cause. projectGroups.length <= 1 deserves the same suppression as projectScopeKey !== null. (Upstream already special-cases projectGroups.length <= 1 at :2216, so the notion of "there's only one project, don't bother" is established in this component.)

---

Nits

• contain-intrinsic-size is 4px short on both branches. The comment's arithmetic (24 + 48 + 2×7 = 86, and 24 + 33 + 7 = 64) describes the card, but the hint sits on the <li>, which adds py-0.5 — 4px. So the li is 90px / 68px. The 86 was already wrong before this PR and I wouldn't touch it in isolation, but the change pins a second number using the same off-by-py-0.5, and the guard now asserts both. Given the justification offered is scrollbar honesty over skipped rows, 4px × a few hundred rows is the same argument at a scale that matters. Fix both or note the discrepancy in the guard comment.
• [...group.threads] (:1525) copies each group's array for no reason — flatMap accepts a ReadonlyArray return, so flatMap((group) => group.threads) types fine and allocates once instead of twice per group.
• "Other" is user-facing copy embedded in a bucketing function (sidebarV2ProjectGrouping.ts:113). The function is otherwise pure data and independently testable; the label belongs at the render site, with UNGROUPED_PROJECT_KEY as the signal. It also reads ambiguously beside real project names — "Unknown project" or similar says more.
• SidebarV2ChromeRows.tsx is now listed under two manifest ids (.fork/customizations.yaml:336 and :437). Probably fine, but a reader can't tell which entry owns which part of the file, and a merge reasoning from intent will read both.
• Tier 4 is the right classification, and I'd note in the entry that the four fences are debt with the specific interest rate that finding 2 describes: three of them are cheap to re-apply, the items one is the one that gets resolved away.

What I verified and liked

• groupThreadsByProject is total — the ungrouped bucket means no thread can be dropped, and the guard test covers it directly rather than by proxy.
• Bucketing on memberProjectRefs rather than memberProjects is correct and non-obvious: memberProjects is deduped by shouldReplaceDuplicateMember, so a thread on a deduped-out ref would have fallen to "Other". It doesn't. That's also why grouped mode names a project that projectDisplayNameByKey would leave null in flat mode — grouping is strictly better here.
• useLocalStorage(key, false, Schema.Boolean) is safe with this hook — the primitive initial value and module-singleton schema keep the memo and callback deps stable, and a corrupt stored value falls back rather than throwing.
• Keeping the preference out of packages/contracts is the right call per AGENTS.md, and the reasoning given is the right reasoning.
• The threadCardShowsMetaRow extraction so the hint and the render read one predicate is exactly right — see finding 1 for the state it's missing, not the shape.

---

Generated by Claude Code

[NoahHendrickson]

Both reviews addressed in 7ffa914. The two independent findings converged on the same fix, so that is where most of the change is.

The dual-mode model is gone

Cursor's finding 1 and Noah's finding 2 are the same defect seen from two sides: groups | null had to stay synchronized across the ordered-thread memo, the projectTitle prop and the render, and the guard could not fail when they diverged — deleting only the items hunk left every assertion green while the sidebar painted flat over a grouped ordered list.

buildActiveThreadSections now returns sections in both modes, flat being the single headerless one. The render maps that sequence and orderedActiveThreads flattens the same one, so divergence is unrepresentable rather than merely asserted against. projectTitle suppression became section.header !== null at the call site that already knows which section it is painting, and the guard drops the pinned activeThreadProjectGroups !== null && section === "active" string that would have fought the refactor.

The guard also stops re-testing what the unit file covers (orphan retention, project/thread order) and covers the seam instead — including the render hunk, with an assertion that the header comes from the same section whose threads follow it.

Card height no longer waits on a query

Finding 1 was a real regression and it was worse than described. prBadge !== null is false both for "no PR" and for "the query has not answered", so every PR card would have painted at two lines and grown mid-scroll.

The predicate takes an explicit unknown state — but unknown is gitStatus.data === null && gitStatus.isPending, not isPending alone. isPending re-enters on every 30s refresh, so reading it directly would have flipped the height on a loop rather than once. I found that in the browser: the seeded fixture repos have no remotes, their status refresh fails every 30s, and cards stayed three-line forever under the first version of the fix. Threads where no query is issued at all collapse immediately and permanently, which is the case the design is about.

The rest

• 4 (a11y). The header is role="heading" aria-level={3} inside role="presentation" li, and grouped cards keep the project name as sr-only rather than dropping it — you were right that grouped mode was strictly worse than flat mode for a screen reader. Verified in the a11y tree: headers announce as level-3 headings, and card accessible names still read … alpha-service main gpt-5.4. The inert data-thread-selection-safe is gone.
• 5. The switch is disabled when grouping can draw no header, with the reason on both the accessible name and the native tooltip.
• 6. projectGroups.length <= 1 gets the same suppression as a set scope.
• 3. The ref index is memoized on projectGroups alone. The docblock notes the parallel map in sortLogicalProjectsForSidebar so the two cannot drift on what a ref key is; I left the import boundary as-is for the reason you gave.
• Nits. contain-intrinsic-size is 90/68 — it measures the li, so the drawn card plus py-0.5; the [...group.threads] copy is gone; "Other" became "Unknown project" at the render site with UNGROUPED_PROJECT_KEY as the signal; the manifest says which entry owns which part of SidebarV2ChromeRows; the intent records the four fences and which one is the expensive one.

Corrections you asked for

(a) Fixed in the manifest and above: jump labels are keyed by thread and cannot misaddress — a stale order only numbers them out of sequence down the screen. The three positional consumers are resolveAdjacentThreadId, rangeSelectTo and planForwardNavigation.

(b) Confirmed intended, and now written into both the manifest intent and the decisions note: under grouping, settling the thread you are viewing lands on the next card in grouped order, which may be the next project's first. Forward should mean the next visible row.

Verification

163 tests across 21 files (vp test run apps/web/src/__fork_guards__ apps/web/src/custom), vp fmt, vp lint, node .fork/lint-owned.mjs (46 files, no warnings), web typecheck. Browser pass on the seeded environment confirmed: grouped sections with level-3 headings, cards at 64px with hint 68 once the query settles, the project name present in the a11y tree but 1×1px on screen, the switch disabled with its reason while scoped, headers suppressed under scope, and the settled shelf flat across projects.