fix(sidebar): attribute each split pane's runtime title to its own leaf (STA-3264) - #14702
brennanb2025 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughRuntime pane titles now sort live slots before parked slots. Leaf resolution supports single-leaf layouts, parked slots, dense live IDs, duplicate titles, and sparse live IDs. The implementation uses runtime layout mapping and live-slot indexes instead of positional mapping across all title entries. Tests cover parked slots, live-slot precedence, nested splits, sparse IDs, and closed panes. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review statusState: What the review loop established. Three before/after captures were produced by driving Scope: 3 distinct root causes behind 4 tickets, 1 fixed here. STA-3264/#12228 is fixed. STA-2811/#11069 (rows share one name that flips on click — Supersedes nothing, and the claim holds: no other PR targets STA-3264 (a Known limit — reasoned from source, not reproducedStep 3 (the identity-based creation-order resolution) is deliberately gated on id density: The consequence is that the in-session-close case is not fixed by this PR. A sparse id set falls to step 5 — positional index within the live slots only — which is today's behaviour, preserved on purpose. The PR's own test 4 (closed split pane) passes both with and without the fix and is stated as a guard on that preserved path, not as evidence of a fix. When resolution fails outright the title is dropped ( The underlying resolver is still positional: Validation — what was and was not done. Done: 4 new tests in a Not done — be explicit about this:
Merge-collision check, with a caveat. |
3992533 to
95cdff9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/components/sidebar/worktree-title-derived-agent-rows.ts (1)
283-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the slot-mapping comments.
The two docblocks repeat the same ID-space context and describe implementation details. Keep one short comment per helper that states its rule.
As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab1f7bdf-b914-47e5-8dd7-df02a66f515b
📒 Files selected for processing (2)
src/renderer/src/components/sidebar/worktree-title-derived-agent-rows.test.tssrc/renderer/src/components/sidebar/worktree-title-derived-agent-rows.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 4 remain after this review.
| ? Object.entries(paneTitles).sort(compareRuntimePaneTitleSlots) | ||
| : [] | ||
|
|
||
| if (paneTitleEntries.length > 0) { | ||
| // Why: hoisted per tab — the leaf lists are layout-derived, not pane-derived. | ||
| const leafIds = collectLeafIds(layout?.root ?? null) | ||
| const liveSlotIds = paneTitleEntries | ||
| .map(([paneId]) => Number(paneId)) | ||
| .filter((paneId) => paneId >= FIRST_PANE_ID) | ||
| // Why: pane ids only encode creation order while they are the dense sequence a | ||
| // fresh mount or replay allocates; an in-session pane close leaves them sparse. | ||
| const liveSlotsAreDense = | ||
| liveSlotIds.length === leafIds.length && | ||
| liveSlotIds.every((paneId, index) => paneId === FIRST_PANE_ID + index) | ||
| for (const [paneId, title] of paneTitleEntries) { | ||
| const leafId = resolveLeafIdForTitleFallback({ | ||
| layout, | ||
| paneTitleEntries, | ||
| leafIds, | ||
| liveSlotIds, | ||
| liveSlotsAreDense, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Claim the leaf when a live slot resolves.
Sorting only changes processing order. If a live title produces no row, Lines 107-111 do not mark its pane key as seen. A stale parked title for the same leaf can then create an incorrect agent row.
Track resolved live leaf IDs before title classification. Skip parked slots for those leaf IDs. Add a regression case with a plain live title and a stale parked working title.
Proposed fix
+ const liveLeafIds = new Set<string>()
for (const [paneId, title] of paneTitleEntries) {
+ const numericPaneId = Number(paneId)
const leafId = resolveLeafIdForTitleFallback({
layout,
leafIds,
liveSlotIds,
liveSlotsAreDense,
- paneId: Number(paneId),
+ paneId: numericPaneId,
title
})
if (!leafId) {
continue
}
+ const isLiveSlot = numericPaneId >= FIRST_PANE_ID
+ if (!isLiveSlot && liveLeafIds.has(leafId)) {
+ continue
+ }
+ if (isLiveSlot) {
+ liveLeafIds.add(leafId)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ? Object.entries(paneTitles).sort(compareRuntimePaneTitleSlots) | |
| : [] | |
| if (paneTitleEntries.length > 0) { | |
| // Why: hoisted per tab — the leaf lists are layout-derived, not pane-derived. | |
| const leafIds = collectLeafIds(layout?.root ?? null) | |
| const liveSlotIds = paneTitleEntries | |
| .map(([paneId]) => Number(paneId)) | |
| .filter((paneId) => paneId >= FIRST_PANE_ID) | |
| // Why: pane ids only encode creation order while they are the dense sequence a | |
| // fresh mount or replay allocates; an in-session pane close leaves them sparse. | |
| const liveSlotsAreDense = | |
| liveSlotIds.length === leafIds.length && | |
| liveSlotIds.every((paneId, index) => paneId === FIRST_PANE_ID + index) | |
| for (const [paneId, title] of paneTitleEntries) { | |
| const leafId = resolveLeafIdForTitleFallback({ | |
| layout, | |
| paneTitleEntries, | |
| leafIds, | |
| liveSlotIds, | |
| liveSlotsAreDense, | |
| ? Object.entries(paneTitles).sort(compareRuntimePaneTitleSlots) | |
| : [] | |
| if (paneTitleEntries.length > 0) { | |
| // Why: hoisted per tab — the leaf lists are layout-derived, not pane-derived. | |
| const leafIds = collectLeafIds(layout?.root ?? null) | |
| const liveSlotIds = paneTitleEntries | |
| .map(([paneId]) => Number(paneId)) | |
| .filter((paneId) => paneId >= FIRST_PANE_ID) | |
| // Why: pane ids only encode creation order while they are the dense sequence a | |
| // fresh mount or replay allocates; an in-session pane close leaves them sparse. | |
| const liveSlotsAreDense = | |
| liveSlotIds.length === leafIds.length && | |
| liveSlotIds.every((paneId, index) => paneId === FIRST_PANE_ID + index) | |
| const liveLeafIds = new Set<string>() | |
| for (const [paneId, title] of paneTitleEntries) { | |
| const numericPaneId = Number(paneId) | |
| const leafId = resolveLeafIdForTitleFallback({ | |
| layout, | |
| leafIds, | |
| liveSlotIds, | |
| liveSlotsAreDense, | |
| paneId: numericPaneId, | |
| title | |
| }) | |
| if (!leafId) { | |
| continue | |
| } | |
| const isLiveSlot = numericPaneId >= FIRST_PANE_ID | |
| if (!isLiveSlot && liveLeafIds.has(leafId)) { | |
| continue | |
| } | |
| if (isLiveSlot) { | |
| liveLeafIds.add(leafId) | |
| } |
Live validation on the packaged-equivalent dev app (closes the visual-proof gap)Rebased onto current RebaseBranch was 146 commits behind. Rebased onto Checked for the split/hoist trap before trusting anything: Red/green oracle re-run after the rebase: reverting only the production file gives 3 failed | 18 passed; restored gives 21/21. Tests 1–3 fail, test 4 passes both ways, exactly as the PR states. RigIsolated dev instance (own Panes run a real process that repaints its OSC 0 title from a control file — the same byte sequence a hookless TUI agent emits, with no hook posts. That is precisely the affected population: agents whose rows are title-derived. Before/after is a true one-line diff on one live instance — the fix is reverted in place, Vite HMR reloads it, and the same panes/state are re-measured. Each measurement re-fetches the served source and asserts which version is running ( 1. Local: three panes, nested split — the reachable divergent caseSplit once, then split the first pane. Traversal order
Siblings inherit each other's agent identity and state pre-fix; correct post-fix. User-visible consequence. With focus parked on PANE A, clicking the sidebar row that shows the Codex spinner:
2. SSH — the population this PR names as most exposedRan for real, on a throwaway
Same defect, same fix, over a real remote host.
3. The known limit is real, and it is a live mis-attribution — not just an untested pathReproduced the in-session-close case live: split to three panes, close pane 2, split again. Live ids stay sparse (
So the limit is stated correctly in substance — the sparse case never reaches the creation-order resolver and keeps today's positional index — but it is worth stating more plainly in the PR: this is not merely "no regression", it is a surviving mis-attribution of the same class the PR fixes, reachable by an ordinary split → close → split. In this configuration the failure mode is a swap, not a dropped row; a row would only vanish where the live-slot count exceeds the leaf count. What is still NOT proven
|
This branch has not been deployed
1 participant
ELI5
Two agents in one split terminal tab each get their own sidebar row. Orca works out which row a pane's status belongs to by looking at the pane's terminal title — but it matched titles to panes by position in a list, and that list is not in pane order. So one pane's status could land on its sibling's row: you finish one agent and the sidebar keeps both rows spinning "Running", while the pane that actually finished is the one still showing work.
This makes each pane title resolve to its own pane by identity instead of by position.
What Changed
buildTitleDerivedAgentRowsresolves each runtime pane title to the layout leaf that actually owns it.runtimePaneTitlesByTabId[tabId]is keyed by runtime pane id, and it holds two disjoint id spaces:>= 1PaneManager(this.nextPaneId++)-(leafIndex + 1)fallbackParkedPaneCandidatesfor parked (unmounted) tabsNeither space is ordered like the layout tree's in-order leaf traversal, and the two can be present at the same time. The old resolver sorted every slot numerically and used the slot's index into
collectLeafIds(root), which crosses both spaces.The new resolution, per slot:
< 1) →leafIds[-paneId - 1], the exact inverse of howfallbackParkedPaneCandidatesmints it.FIRST_PANE_ID→resolveRuntimePaneTitleLeafIdFromRoot, the repo's existing creation-order resolver already used byworktree-status.ts,smart-attention.ts,worktree-agent-rows.ts, andai-vault-original-pane.ts. This PR does not add a resolver; it stops the sidebar from being the one place that hand-rolls its own.titlesByLeafIdmatch → unchanged, but demoted below id-based resolution. It compares a live OSC title against a user-assigned pane name, so it must not take a leaf away from the pane whose id names it.Slots are also ordered live-before-parked, so when a revealed tab carries both, the live slot claims the leaf and
seenPaneKeysdrops the stale parked duplicate rather than the other way round.Deliberately gated on density (step 3). Pane ids only encode creation order while they are the dense sequence a fresh mount or replay allocates. An in-session pane close leaves survivors sparse (
{2, 3}), where creation order would answer confidently and wrongly. Sparse falls to step 5, which is exactly today's behavior — so no in-session-close case regresses.Why
The literal string
Runningreaches a sidebar agent row through exactly one line in the renderer —worktree-title-derived-agent-rows.ts— so STA-3264's "both rows Running" is specifically about title-derived rows, the path hookless agents (Pi, Codex over SSH, Cursor Agent) depend on. Everything upstream of it is already pane-scoped:agentStatusByPaneKey,selectLiveAgentStatusEntriesForWorktree, and the explicit-entry loop inbuildWorktreeAgentRowsall key ontabId:leafId. The tab-scoped error is in the last step — deciding which leaf a pane title belongs to.I drove
buildWorktreeAgentRowsdirectly to confirm the mechanism rather than reason about it. Real output, same builder the sidebar renders from, before the fix:Parked split tab. Leaf 1 finished (idle title in slot
-1), leaf 2 still working (slot-2). Sorting ascending gives[-2, -1], which is the reverse of the leaf list the slots were numbered against:Revealed parked tab — this is the reported symptom.
disposeParkedTabWatcherstears down watchers without clearing their title slots, so mounting adds live slots{1, 2}alongside the parked{-1, -2}. Four slots, two leaves: indices 0 and 1 are the two stale parked spinner titles, and the live idle title of the pane that finished is pushed to index 2 and dropped entirely.Three panes, left pane split again. Tree traversal is
[1, 3, 2]; creation order is[1, 2, 3]. Panes 2 and 3 take each other's title, so sibling panes inherit each other's agent identity and state:That last case is why the fix is identity-based rather than another positional tweak: no ordering of a positional index can be right, because pane ids and tree position are independent facts.
Which of the four cluster tickets this is — and is not
Three distinct root causes sit behind the four tickets. They are not one bug.
getAgentRowConversationNamereadstab.title, which carries only the focused pane's titletab.titlefallback and re-synthesizes a row from the tab's stale spinner titleNo PR is superseded. There is no open or closed PR against STA-3264 (searched
12228 in:bodyandSTA-3264 in:body, both empty), and #11372 is an issue, not a PR.#11070 (@pythonstrup) is complementary, not competing, and is not superseded. It suppresses the tab's live title as a conversation name source for split rows; this PR fixes which pane a title's lifecycle state is attributed to. STA-3264 puts the name half explicitly out of scope ("conversation names and lifecycle states are separate projections and should not be coupled"), and STA-2811 is listed under STA-3264's Out of Scope. The two changes touch disjoint files — #11070 edits
agent-row-conversation-name/use-agent-row-conversation-name/build-dashboard-snapshot; this editsworktree-title-derived-agent-rows— and land independently. Worth noting they compose in the right direction: #11070's fallback for a split row is the pane's own prompt keyed bypaneKey, which is only correct once thatpaneKeyis the right pane, which is what this PR establishes.Not the subagent-clobbers-parent class (STA-2112/2035, STA-2243). Nothing here reads or writes
orchestration.parentPaneKey, and no row's preview or completion state is derived from another row. The change is confined to choosing a leaf id for one pane's own title.Prior art this builds on
launchAgentis tab-scoped, soresolveTitleDerivedPaneOwnerreturns an owner only for a single-leaf layout — a split pane cannot brand its sibling. That guard is untouched and still holds: this PR changes which leaf a title maps to, never whether a split pane may claim tab-scoped ownership.worktree-card-compact-agent-row.tsxis not touched.Linked Issue
Fixes #12228 (STA-3264)
Does not fix #11069 (STA-2811) or #11372 (STA-2926) — see the table above.
Visual Proof
Captured on a live app. See the full validation comment for the rig and the SSH leg.
Three panes in one tab (split once, then split the first pane, so traversal order
[1,3,2]diverges from creation order[1,2,3]), each running a hookless process emitting a real OSC 0 agent title. Before/after is a one-line diff on a single live instance: the production hunk is reverted in place, Vite HMR reloads it, and the same panes are re-measured — each sample asserts which source version is actually served.59d0919d2522ba8a2d238208With focus on PANE A, clicking the row showing the Codex spinner lands on PANE C (the idle Gemini pane) pre-fix, and on PANE B (the pane actually running) post-fix.
Read the images with this caveat. A runtime title carries the agent name and its lifecycle state together, so mis-attribution moves both — the rendered multiset of (name, state) pairs is identical either way, and what changes is which pane each row is bound to. The shots therefore differ in row order and per-row icon, not in the name text. Also, all three rows render the same name there because of STA-2811 / #11069, which this PR does not fix; per-row identity is correct in the accessible summary (
Antigravity idle; Gemini idle; Codex working).Not captured: the parked/revealed path. I parked a split tab for real (pane manager gone, parked watchers owning it) but never observed a negative title slot —
resolveParkedTerminalPaneCandidatespreferscapturedPanesByTabId, so a tab that has mounted once keeps reusing its positive ids while parked. The{-1,-2,1,2}state behind the "both rows Running" block under Why remains unverified on a live app (that block is still constructed-state output).Testing
New tests —
src/renderer/src/components/sidebar/worktree-title-derived-agent-rows.test.ts, newsplit-pane runtime title attributionblock. They assert externally visible projection only (paneKey, agentType, lifecycle state, row count), per STA-3264's testing decisions:Red/green oracle. Reverting only
worktree-title-derived-agent-rows.tsand keeping the tests: 3 failed | 18 passed. Tests 1–3 fail; test 4 passes both ways and is stated as a guard on the sparse-id path this PR deliberately preserves, not as evidence of a fix. Restored: 21/21.Regression scope.
src/renderer/src/components/sidebar+src/renderer/src/components/dashboard: the two suites that own the changed behavior (worktree-title-derived-agent-rows,useWorktreeAgentRows,WorktreeCardAgents) pass 79/79. Running the full pair of directories surfaces failures in heavyWorktreeCard.*render tests, all timeouts at the 20s/60s marks and all pre-existing:WorktreeCard.pr-display.test.tsxfails 2 tests on the pre-fix source and 1 with the change applied, i.e. it is a timing flake in both directions and not caused here. Nothing in the agent-row path fails.paired-reconnect-sidebar-agent-count,runtime-pane-title-leaf-id,worktree-status,smart-attention: 84 passed.check:code-quality:changed: 0 new findings across 2 changed files, on all three gates (code quality, type-aware, React Doctor). Nomax-linessuppression added.pnpm typecheckdeliberately not run (OOM risk on this host). The type-aware gate above covers the changed files. Types touched are narrow: one internal function's argument object and two imports.Rebase. Rebased onto
origin/main(c4e188a25f) from 146 commits behind; no conflicts,merge-tree --write-treeclean, red/green oracle re-run after the rebase (3 failed reverted → 21/21 restored). Confirmedworktree-title-derived-agent-rows.tsis byte-identical between the branch base and current main and thatWorktreeCardAgents.tsx → useWorktreeAgentRows → buildWorktreeAgentRows → buildTitleDerivedAgentRowsis still the live render chain.SSH. Exercised for real against a throwaway
ubuntu:22.04SSH target with a real relay (remotePlatform: linux): three hookless agents in one remote split tab reproduce the same swap pre-fix and are correct post-fix.Known limit, verified live and worth restating. The in-session-close case is not merely untested, it is a surviving mis-attribution of the same class: split → close pane 2 → split again leaves live ids sparse (
{1,3,4}),liveSlotsAreDenseis false, and with this PR applied PANE D (OpenCode, working) still reportsgemini/idlewhile PANE C (Gemini, idle) reportsopencode/working.worktree-status.tscallsresolveRuntimePaneTitleLeafIdFromRootdirectly with no density gate and is likewise untouched.Merge-collision check.
git merge-tree --write-treeagainst the head SHA of each in-flight PR that touches this area — #14486 (the 119-file worktree-list reorg), #14682, #14654 — all three merge with no conflict. I did not stop at the flag: I materialized the #14486 merge into a scratch worktree and ran the agent-row suites on the merged tree — 79 passed, so the merged result is green, not merely mergeable. (GitHub'smergeablefield was independently verified stale during this sweep; I did not rely on it.)Platforms. Written and run on macOS. Nothing platform-dependent is introduced: no shortcuts, modifier keys, shortcut labels, path construction, shell invocation, or Electron platform APIs. The change is integer/string id resolution over an in-memory layout tree. I did not exercise Windows, Linux, or a live SSH host.
Review
process.platformornavigator.userAgent; nothing OS-specific to diverge.orca agent hooks statusstill reportscodex: installed#8711, OpenCode [Bug] opencode is displayed as Claude Code #8940) surface only decorated titles, so their rows are title-derived and were the most exposed to mis-attribution. No probing, no Git commands, no local-only assumptions — identical for local, remote-runtime, and SSH panes, and for folder workspaces as well as git worktrees (nothing reads git state).collectLeafIds(O(n)) and anObject.entries(titlesByLeafId)scan per slot — O(k·n). The new code hoists the leaf list to once per tab and resolves each slot in O(n) worst case, with thetitlesByLeafIdscan now only reached when id-based resolution fails, which for a normally-mounted tab is never. Net: strictly fewer scans than today, one added O(k) pass to computeliveSlotIds, and no change to row count — so nothing downstream multiplies either.AI Disclosure
Claude Opus 5 (Claude Code), on macOS.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) — scoped equivalents run locally and green; full typecheck left to CI per the OOM constraint aboveAuthor