fix(sidebar): retire the unpublished-title fallback that minted an agent row and both dots (STA-2926) - #14708
brennanb2025 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 3 remain after this review. 📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/sidebar/worktree-title-derived-agent-rows.ts (1)
74-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the inline comment detail.
Keep only the pane-provenance reason near the production logic. Remove the test-history explanation because the
describeand test names state the scenario.
src/renderer/src/components/sidebar/worktree-title-derived-agent-rows.ts#L74-L81: replace the multi-line history with one concise comment about using only pane-scoped runtime titles.src/renderer/src/components/sidebar/worktree-title-derived-agent-rows.test.ts#L387-L391: remove or reduce the scenario-history comment to one line.As per coding guidelines: “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aeaaef83-cf48-4a6e-a0e4-b3ebf4428a34
📒 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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/lib/worktree-status.ts (1)
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the added rationale comments.
Keep one concise comment only when the test name or code cannot state the reason.
src/renderer/src/lib/worktree-status.ts#L66-L71: replace the six-line rationale with one sentence that states the live-pane provenance rule.src/renderer/src/lib/worktree-status-spinner-launch-agent.test.ts#L48-L50: remove or reduce the comment to one sentence.src/renderer/src/lib/worktree-status-spinner-launch-agent.test.ts#L61-L63: remove the explanation that repeats the test name.src/renderer/src/lib/worktree-status-spinner-launch-agent.test.ts#L149-L152: reduce the dot-row agreement explanation to one sentence.src/renderer/src/lib/worktree-status.test.ts#L146-L148: reduce the stale-title explanation to one sentence.As per coding guidelines: “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0be415fc-bdab-404f-bd0a-f2e7d090a4a3
📒 Files selected for processing (4)
src/renderer/src/components/sidebar/WorktreeCard.test.tssrc/renderer/src/lib/worktree-status-spinner-launch-agent.test.tssrc/renderer/src/lib/worktree-status.test.tssrc/renderer/src/lib/worktree-status.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 1 remains after this review.
…nt row (STA-2926)
…STA-2926) The row half of this PR stopped `tab.title` from synthesizing an agent row once every pane's runtime title slot is cleared. The dot half still read that same title, so a closed agent pane left the worktree dot spinning "working" over zero sidebar rows -- the exact "0 agents" symptom #9040 was filed to fix. worktree-status.ts:106-109 states its spinner fallback exists to mirror the row builder's fallback "so the dot and the sidebar row agree", so removing one side and keeping the other breaks the agreement that comment describes. Apply the same rule to the dot: with no pane publishing a title, there is nothing to attribute. #9040's real feature is unaffected -- spinner attribution runs through the pane-title branch, which is untouched.
…STA-2926) The tab-bar dot resolves through resolveWorktreeStatus, so the dot fix reaches it too: a `tab.title` no pane is publishing no longer feeds the heuristic. Five cases fed their titles through `tab.title` alone and went red or vacuous; each keeps its original assertion and now publishes the title on a pane, which is where a live title actually lives in production. Adds the negative contract directly -- an agent-shaped tab title with every pane slot cleared resolves to 'active', for both a spinner and a named provider. Restoring the old `tab.title` fallback turns that case red.
334b66d to
37b4635
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Review statusState: What the review loop established. Two title channels exist, and only one knows about panes. The loop also found why the intended reset doesn't help: Scope expanded past STA-2926 — read this before mergingThis PR also changes the worktree-card dot and the tab-bar dot, neither of which was in STA-2926's scope. The tab-bar dot comes along because The expansion is deliberate and was forced by CI, not chosen for tidiness. The row-only change failed #9040's actual feature survives: spinner attribution runs through the pane-title branch, which is untouched. Only the tab-title variant is removed, reachable only once a pane has stopped publishing. A second intended consequence, stated so it is not discovered later: the change also stops minting rows from named-provider tab titles ( Behaviour removed, plainly: a tab with a live PTY, an agent-shaped Validation — what was and was not done. Done: a red/green oracle (reverting only the row builder gives 2 failed / 19 passed; tests 3 and 4 pass both ways and are stated as guards, not evidence). Three independent mutations run against the rebased tree — restoring the old The 12 rewritten tests each keep their original assertion with the status source moved from Not done:
Conflict with #14702 — re-verified live for this comment. I ran Supersedes nothing — #11372 is an issue, not a PR, and searches for open PRs against STA-2926 return none. Does not fix #11069 / STA-2811 (split rows sharing a conversation name), which is Wire / compat: no wire change, nothing persisted, no migration. Row count can only shrink. |
What this actually is — read this first
This is not a live user-facing fix, and the original framing of it was wrong. Live validation established that the symptom STA-2926 describes — closing a split pane and having its agent row re-point at the survivor — no longer reproduces on current
main, in either arm.resolveTabTitleAfterPaneClose(terminal-pane-close-identity.ts:12, from #8569) already resets a tab to its stable fallback on pane close, with a comment describing exactly this case. App restart doesn't preserve it either.What this PR does is retire a branch that is wrong whenever it is entered, and stop the dot from outliving the row. The removed fallback minted an agent row and both dots from a
tab.titlethat no pane was publishing — verified live:tab.titleonly,runtimePaneTitles = {}activeWhy the dot half is in here too, and why it is not optional. Removing only the row fallback would have left the dot still reading stale
tab.title— producing a dot spinning "working" forever over zero agent rows, which is #9040. CI caught that, and the pre-fix half of that state was observed directly during validation. The two halves are one change; shipping the row half alone reintroduces a fixed bug.Verified not to under-report: a plain shell with no agent, including a surviving split pane, gives rows 0 / both dots
activein both arms. A live agent in a split or unsplit pane still gets its row and dots — the pane branch is untouched.One path is untested:
web-session-tabs-syncpublishes host-synced tab titles and could still deliver an agent-shapedtab.titlewith empty runtime pane titles. That is the one place the precondition might still arise, and it was not exercised.Original framing, kept for the record
The
before:table further down comes from executing the pre-fix row builder against constructed store states, not from a running app. It shows what the branch does when entered — which is real — but should not be read as a captured live repro of the ticket.What Changed
One branch removed from
buildTitleDerivedAgentRows: thetab.titlefallback that ran when a tab had no runtime pane titles at all.The matching branch is removed from the worktree dot in
worktree-status.ts— which the tab-bar dot also resolves through — see Scope expanded beyond the ticket below for why that is in this PR.A title-derived row is now minted only from
runtimePaneTitlesByTabId[tabId]— the pane-scoped title channel. Everything else in the file is untouched: identity resolution, the Claude token guard, the owner fallback, the Cursor/claude agentscarve-outs, the leaf resolver.Scope expanded beyond the ticket
This PR now also changes the worktree card DOT, not just the sidebar row. That was not in STA-2926's scope.
And the tab-bar dot with it.
resolveTerminalTabActivityStatus(terminal-tab-activity-status.ts:151) delegates straight toresolveWorktreeStatus— "Reuse the WorktreeCard status vocabulary and resolver so the tab's live states resolve identically to the sidebar." One resolver, so the dot rule lands on both surfaces at once. That is the correct outcome (the tab-bar dot had the same "spinning over zero rows" hole), but it is a second surface changed, so it is stated here rather than left to the diff. It cost three more rewritten tests and one new one, listed below.It was done because the alternative was shipping a regression, and CI caught it.
worktree-status-spinner-launch-agent.test.ts> #9040 spinner attribution matches named-provider dot/row agreement failed on the row-only change — and it is not a stale expectation. #9040 and STA-2926 feed structurally identical inputs (single-leaf layout,runtimePaneTitles = {}, one live PTY, an agent-identifyingtab.title) and demand opposite outputs. Every candidate discriminator was checked —launchAgentpresent/absent, spinner vs. named identity, PTY count, leaf count,layout.titlesByLeafId— and both suites cover both sides of each. No narrowing guard can separate them.Why the two halves cannot be allowed to disagree is written into the source.
worktree-status.ts:106-109says its spinner fallback exists specifically to mirror "the row builder's spinner fallback (#9647) so the dot and the sidebar row agree" — the exact fallback the row half removes. Removing one side and keeping the other is not minimal scope; it is a half-applied change that breaks the agreement that comment describes, leaving the dot spinningworkingover zero sidebar rows. That is precisely the "0 agents" symptom #9040 was filed to fix.So the dot gets the same rule: with no pane publishing a title, there is nothing to attribute.
#9040's real feature survives — this fixes it rather than reverting it. Spinner attribution runs through the pane-title branch, which is untouched: spins for a spinner-only pane title when the tab was launched as claude passes unchanged before and after. What is removed is only the tab-title variant, reachable only once a pane has stopped publishing.
Named-provider tab titles: intended, not collateral
The row half also stops minting rows from named-provider tab titles (
claude [working]), not only stale spinners. This is intended, and the guard is deliberately not narrowed.The defect is attribution, not title content.
tab.titlecarries no pane provenance at all; the removed branch assigned it tolayout.activeLeafId ?? firstLeaf— a guess at which leaf owned it. That guess is exactly as unfounded forclaude [working]as for⠋ implementing the feature: the title's specificity changes what the row would say, not which pane it belongs to. The PR's own test shows the named case is a live bug —✳ Claude Coderecycled onto both a promoted sibling and a replacement session.Named providers lose nothing while they are alive.
onTitleChangewritessetRuntimePaneTitlefor every pane (pane 0 of an unsplit tab included) and only then mirrors the focused pane intotab.title, so a live agent always occupies a pane slot and is rowed by the pane branch. Empty slots plus an agent-shapedtab.titleis, by construction, the cleared/stale state. The rewritten control assertion pins this:claude [working]published on a pane still produces both a spinning dot and exactly one row.The 12 rewritten tests
Each keeps its original assertion; the status source moves from
tab.titleto the pane-title channel that actually feeds the heuristic in production. None was weakened to pass.worktree-status.test.tspermissionbeatsworking+ browser presence); the two titles are now published on panes.workingassertion, via the pane channel.runtimePaneTitlesByTabId.runtimePaneTitlesByTabId.WorktreeCard.test.tspermissionandworkingeach beat browser presence); titles published on panes.worktree-status-spinner-launch-agent.test.ts(#9040)Two further tests in that file — stays active for a spinner title with no launch identity and does not manufacture activity from a non-spinner title with a launch identity — still passed after the change, but only because everything without pane titles now returns
active. They had gone vacuous, asserting nothing about the #9647 gate they exist to pin, so they were moved to the pane channel as well. They are not counted among the failures.terminal-tab-spinner-launch-agent.test.ts(#9040, tab-bar dot)workingassertion, via the pane channel.stays out of working for a spinner pane title with no launch identity was also moved to the pane channel for the same vacuity reason as the two sidebar cases above.
terminal-tab-activity-status.test.tsworkingbeats a >30m-staledonehook); the title is now published on a pane, which is what "live" means after this change.active); still load-bearing, since without therestoredUnconfirmedsuppression the published pane title would classify asworking.zshpublished on a pane so it reaches the classifier instead of stopping at the new no-pane-titles guard.Nothing was deleted. Every case above kept its original assertion; none was weakened into vacuity, so no test lost its reason to exist.
Mutation-proved, not assumed. Three independent mutations, each run against the rebased tree:
tab.titlefallback inworktree-status.tstitleStatusIsAgentAttributable→false)The second mutation is the direct evidence for #9040's feature surviving on both dots: those cases are only green because the spinner→launch-identity attribution still runs, through the pane-title branch this PR does not touch. Option B fixes #9040 rather than reverting it.
Why
Two title channels, only one of which knows about panes.
runtimePaneTitlesByTabId[tabId][paneId]tab.titleonTitleChange)clearRuntimePaneTitleon PTY exit (pty-connection.ts), on pane close (use-terminal-pane-lifecycle.ts), and on parked-pane exit (shouldDeferParkedPtyExitTabClose)So the removed branch ran precisely when every pane's live title had been cleared, and the string it read was, by construction, produced by a pane that is no longer publishing. It then attributed that string to
layout.activeLeafId— the pane that survived the close. That is the recycle.The intended reset already exists and silently no-ops.
onPaneCloseddoes try to fix this at the source:but
applyTerminalTabTitleUpdatesresolves the empty string throughgetFallbackTabTitle, whose last resort istab.titleitself:A tab with no custom title, no quick-command label and no
defaultTitle(defaultTitleis only ever stamped from aTerminal Ntitle, so a tab that never wore one has none) gets its stale title handed straight back,currentTab.title === nextTitle, no update. The reset is a no-op exactly when it is needed.tab.titleis unfixable in place. There is no signal in this function that separates "this tab title belongs to the survivor" from "it belongs to the pane that just closed" — the tab title carries no pane provenance at all. So the only correct action is to stop minting a pane row from it.Symmetry with the sweep's other two identity fixes
tab.titleis the inverse case: positive evidence of nothing about the pane it would be attributed to. Same treatment — do not synthesize the row. fix(sidebar): keep owned agent panes visible when their title carries no agent frame (#14464) #14650 lands the owner (launchAgent) fallback insidebuildTitleDerivedAgentRow, which is still reached from the pane-scoped branch a live pane always uses, so the two compose: this PR removes a row source that could only ever be wrong, and fix(sidebar): keep owned agent panes visible when their title carries no agent frame (#14464) #14650 keeps a live owned pane visible on the branch that is right.shouldClearLaunchAgentForClosedPane— "Closing that PTY must not transfer its bootstrap identity to a surviving shell sibling." A closed pane's title must not transfer either.Relationship to #14702 (same file)
#14702 fixed the other branch of this function — which leaf a live pane title resolves to (STA-3264) — and explicitly left this one open:
This is that branch. The two are independent and land in either order — see the merge note under Testing. Nothing in #14702's leaf-resolution work is re-done here.
Not #11069 / STA-2811 (split rows sharing one conversation name) — that is
getAgentRowConversationName, a different file, addressed by #11070.No PR is superseded. #11372 is an issue, not a PR;
11372 in:bodyandSTA-2926 in:bodyboth return nothing open.Linked Issue
Fixes #11372 (STA-2926)
Visual Proof
Now captured live. An isolated Orca dev instance running this branch (own
ORCA_DEV_USER_DATA_PATH, own CDP port,getIdentity()verified per arm, never the instance the user is running), with a realclaudesession in a real split pane. Arms differ only bygit checkout origin/main --on the two production files, with a full relaunch per arm —electron-vite devdoes not rebuild the main process, so HMR would have measured the same build twice.Full log: sta2926-live-validation.txt ·
All three changed surfaces were read from the live renderer store on every sample —
buildWorktreeAgentRows(rows),resolveWorktreeStatus(worktree-card dot),resolveTerminalTabActivityStatus(tab-bar dot).The removed branch, when reached. Same tab, same title,
launchAgent: 'claude', one live PTY, single-leaf layout; the only difference is whether a pane is publishing:tab.titleonly,runtimePaneTitles = {}activeSo the branch really did mint a row and both dots from a title no pane was publishing, and after the fix all three go silent together. That is the dot/row agreement the scope expansion exists to preserve: no state was found in which a dot spins
workingover zero rows.Live scenarios, all run in both arms:
launchAgent: 'claude'— rows=1working, both dotsworking. Identical in both arms; the untouched pane branch is unaffected.active. Removing the fallback does not under-report a pane that has no agent.Reachability — stated plainly, because it cuts against the ticket's story
I could not reach the removed branch through ordinary user actions on this tree:
runtimePaneTitles → {},tab.title → "Terminal N", rows=0, both dotsactive. No recycled row in either arm. The cause is already onorigin/main:resolveTabTitleAfterPaneClose(terminal-pane-close-identity.ts:12, from fix(native-chat): target native chat to the active split leaf #8569) resets the tab to its stable fallback rather than "leaving the closed pane's agent title attached to an untitled survivor."tab.titlereverts to itsdefaultTitle, and the panes republish. Sampled every 2s for 40s across the boot window in both arms: rows and both dots agreed at every sample.So on current
mainthe branch this PR deletes looks dead for the paths the ticket describes. That makes the change safe — a dead-branch removal that also restores dot/row symmetry — but it means the recycle itself was not live-reproducible here, and thebefore:outputs below come from executing the pre-fix builder against constructed states rather than from a running app.Not exhausted: the remote/SSH path.
web-session-tabs-syncpublishes host-synced tab titles, which could still deliver an agent-shapedtab.titlewith empty runtime pane titles; that was not tested.The constructed-state outputs previously quoted here remain accurate for what the pre-fix builder does when the branch is reached — real assertion output from the pre-fix source, and now corroborated by the live pre-fix arm above:
Testing
claudesession, both arms, all three surfaces; see Visual Proof. Constructed store states were used additionally, to reach the branch that ordinary actions no longer reach.New tests —
worktree-title-derived-agent-rows.test.ts, newclosed split paneblock, asserting externally visible projection only (paneKey,agentType,state):⠋ Codexwhiletab.titlestill says✳ Claude Code; the row must be Codex on the survivor's leaf.Red/green oracle. Reverting only
worktree-title-derived-agent-rows.tsand keeping the tests: 2 failed | 19 passed. Tests 1 and 2 fail; 3 and 4 pass both ways and are stated as guards on the pane-scoped path this PR preserves, not as evidence of the fix. Restored: 21/21. Each of the three titles in test 2 was additionally verified to recycle pre-fix individually (a loop stops at its first failure), which is where thebefore:table above comes from.Is the removed branch load-bearing? Checked before writing anything: with the fallback neutralized outright,
src/renderer/src/components/sidebar+src/renderer/src/components/dashboardran 295 files / 2640 tests, all passing — no existing test depended on it. Both consumers ofbuildWorktreeAgentRows(useWorktreeAgentRows,build-dashboard-snapshot) are inside that scope.Regression scope (re-run on the rebased tree).
sidebar+lib/worktree-status*+tab-bar+dashboard: 358 files / 3068 tests passed.oxlintand React Doctor across all 8 changed files: clean. Proved the gate was live rather than silently dead by appending adebugger/loose-equality probe toworktree-status.ts— it reportedeslint(no-debugger)— then reverting. Nomax-linessuppression added.oxfmt --checkclean on all 8.pnpm typecheckdeliberately not run (OOM risk on this host, per the sweep constraint). The only type surface touched is the deletion of one call site plus test-fixture fields.CI red → green, cause by cause
tests node 24/26 · 2/16—terminal-tab-spinner-launch-agent.test.ts, 2 failuresresolveWorktreeStatus, so the dot fix reached it and twotab.title-fed cases went red.tests node 24/26 · 8/16—terminal-tab-activity-status.test.ts, 1 failuretab.title.verifyTEST: failure, everything elsesuccess.test vs non-test LoC.github/workflows/pr-test-loc.ymlfetches.github/scripts/pr-test-loc-{table,summary}.mjsfrompull/14708/head; the branch predated those scripts, sogh apireturned404andbase64 --decodefailed. The job has no threshold at all —pr-test-loc-summary.mjsonly counts and PATCHes this body.main(82 commits), which brings the scripts onto the head ref. No code change could have fixed it.Rebase safety.
git merge-tree --write-tree origin/main HEADwas clean before rebasing, andgit log HEAD..origin/main -- <the 8 files>was empty —mainhad not touched any file in this PR, so the rebase is mechanical. All three commits replayed without conflict; the full scoped suite above was re-run afterwards, and all three mutation proofs were re-run on the rebased tree.Merge-collision check — run locally with
git merge-tree --write-tree, not read off GitHub'smergeableflag (independently verified stale during this sweep). Against each in-flight PR's head SHA:I did not stop at the flags:
leafIds/liveSlotIds/liveSlotsAreDenseand itsresolveLeafIdForTitleFallbacksignature, drop theif (paneTitleEntries.length > 0) { … }wrapper and thetab.titleblock below it, keep both test blocks and delete fix(sidebar): attribute each split pane's runtime title to its own leaf (STA-3264) #14702's now-duplicate innerLEAF_ID_3. The merged tree runs 25/25 in this file and 295 files / 2648 tests across sidebar + dashboard. Whichever of the two lands second takes that resolution; neither PR needs the other, and this one is based onmain, not stacked.Platforms. Written and run on macOS. Nothing platform-dependent is introduced or removed: no shortcuts, modifier keys, shortcut labels, path construction, shell invocation or Electron platform APIs. Windows, Linux and a live SSH host were not exercised.
AI Disclosure
Claude Opus 5 (Claude Code), on macOS.
Review
process.platform/navigator.userAgentbranching added or removed; 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. Their live titles arrive throughsetRuntimePaneTitleexactly like local panes, so the surviving branch covers them unchanged; what is removed is a source that could only ever fire once a pane had stopped publishing. No probing, no Git commands, no local-only assumptions. Identical for folder workspaces and git worktrees — nothing reads git state.tab.title, and zero runtime pane titles no longer gets a row. Beyond the closed-pane case that is only reachable in the window between a pane's spawn and its first OSC title frame, where the correct display is "no pane evidence yet" rather than a persisted string. If that window ever wants a row, fix(sidebar): keep owned agent panes visible when their title carries no agent frame (#14464) #14650's owner fallback is the right mechanism for it — durable, pane-scoped for a single-leaf tab, and gated on a live PTY — not a title with no provenance.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