Repository navigation
fix(remote): focus the pane a host-delegated split creates - #16513
ASDFGHoney wants to merge 8 commits into
Conversation
Cmd+D on a paired remote pane short-circuits into splitWebRuntimeTerminal before splitManagedPane's setActivePaneId/focus, so the new pane is only materialised later from the mirrored session-tabs snapshot. There the active pane is whatever layout.activeLeafId says, and chooseRemoteTerminalLayout prefers the client's existing activeLeafId over the host's isActive — a deliberate guard against status-echo focus theft (stablyai#5435). With no client-side claim the source pane always won, so remote splits landed unfocused. Carry the created pane's leafId back on RuntimeTerminalSplit (optional, so older hosts stay compatible) and have the split initiator record the same focus intent createWebRuntimeSessionTerminal records for a created tab. Hosts that omit leafId keep today's behaviour rather than claiming the wrong leaf. Fixes stablyai#16510 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (7)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughTerminal split results now include the created pane’s optional 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue [ Full details: Docstring CoverageExplanation Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 7 files. (2 skipped: 1 unsupported, 1 too large.) Full details: Description checkExplanation The description follows the repository template and provides a clear explanation, scope, rationale, linked issue, visual-proof rationale, testing evidence, AI disclosure, review notes, compatibility considerations, and checklist status. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/runtime/web-runtime-session.ts (1)
1484-1500: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFence
terminal.splitwith the captured pairing revision.If the pairing changes before dispatch, the missing revision lets
terminal.splitrun on the replacement host, whilefocusSplitWebRuntimeTerminalPanedrops the handoff because the captured revision is stale. Route this call throughcaptureRuntimeEnvironmentCall(environmentId, intentOwner.pairingRevision)and add a regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f71d99b1-003b-4673-8ded-38cddfc9b57a
📒 Files selected for processing (6)
src/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/renderer/src/runtime/web-runtime-session-terminal-pane-delegation.test.tssrc/renderer/src/runtime/web-runtime-session.tssrc/renderer/src/runtime/web-session-tabs-sync-focus-intent.test.tssrc/shared/runtime-terminal-contracts.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Drives the split through the same helper Cmd+D reaches, against a headed host paired to a real Electron client, and asserts the active pane's PTY is not the one that was split. Without the focus claim the assertion sees the source PTY back, which is the reported bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The split RPC went out unfenced while the focus claim behind it captured a revision, so a same-id re-pair mid-dispatch would split the replacement host and then silently drop the claim — a stray unfocused pane on a host the caller never addressed. Route it through captureRuntimeEnvironmentCall like every other intent-bearing call in this file, so the pairing gate rejects it instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Good catch — fixed in 632b87f. The call really was the odd one out. Every other intent-bearing call in this file already Regression test added: "fences the host split on the revision the focus claim captured". Re-verified after the change: |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
On the one failing pre-merge check — Docstring Coverage 28.57% vs an 80% threshold: Added docstrings in 719b411 to the three functions this PR actually introduces or reshapes I have deliberately not padded the rest to reach 80%, and want to be explicit about why
And
The remaining uncovered functions in the count are pre-existing host methods Happy to write them anyway if a maintainer wants the check green — just say so and it is a Minor: the Merge Risk blurb still lists "follow-up on fencing the split request to the |
|
Two-axis review of Overall: the mechanism is right and the wire-compat reasoning holds up. One reuse violation, and three behavioural questions the PR body doesn't address. StandardsHard —
|
| new spec | existing | |
|---|---|---|
showClient :16-22 |
:80-87 |
byte-for-byte identical |
waitForClientWorktree :24-38 |
:89-105 |
a narrowed copy — the existing one is already the more general form (optional expectedId, returns the id) |
inline call/unwrap :44-58 |
callRuntime :65-78 |
same wrapper, re-done inline |
The standard says "extend or generalize it instead of building a parallel version". Suggest promoting all three into tests/e2e/helpers/paired-electron-client.ts — already the home of createRuntimeDesktopPairingOffer/launchPairedElectronClient — and importing from both specs.
(Not reusing splitActiveTerminalPane (tests/e2e/helpers/terminal.ts:451) is correct and the spec says why at :91-92. No issue there.)
Judgement calls
src/renderer/src/runtime/web-runtime-session.ts:1536-1538— the conditional spread is inert.refreshWebRuntimeSessionTabsSnapshotalready falls back withoptions.expectedEnvironmentPairingRevision ?? getRuntimeEnvironmentRevision(environmentId)(:1031-1032), and all five other call sites (:468, :796, :1181, :1437, :1452) pass it unconditionally. Collapsing to one line matches the file's idiom.web-runtime-session.ts:1548—tab.id === localTabId || tab.id === hostTabId:toWebTerminalSurfaceTabIdalways prefixes (src/shared/terminal-surface-id.ts:4-9), so the raw-id arm can only match a tab that never came from the mirror, and nothing exercises it. Drop it, or add a line saying when a raw host id lands intabsByWorktree.web-runtime-session.ts:1545—resolveWebRuntimeSessionWorktreeIdForHostTabreadstabsByWorktree(terminal tabs only) while placement code in the same file readsunifiedTabsByWorktree(:517-519). The name promises the general answer; consider putting the scope in it.web-runtime-session.ts:1516-1518— the// Why: hosts predating RuntimeTerminalSplit.leafId…comment sits onconst leafId = split?.leafId?.trim()but explains the guard on the next line'sif. It also restatessrc/shared/runtime-terminal-contracts.ts:259-261. Per "Concise/Brief Non-obvious Comments ONLY", a 14-line function carrying a 3-line JSDoc plus two inlineWhys could lose one.src/main/runtime/orca-runtime.ts:30694, 30695, 30709—{ handle: string; leafId: string }spelled inline three times inwaitForNewLeafInTab; one named type reads lighter.
Clean
docs/reference/remote-wire-compatibility.md — compliant on all three rules, and the PR body's analysis is accurate. leafId is optional (Rule 1) and the reader genuinely falls back rather than requiring it (web-runtime-session.ts:1527 returns without claiming); both host paths populate it (orca-runtime.ts:30359-30364, :30554-30559), so absent unambiguously means old host; no new opcode (Rule 2); nothing changes in what the host publishes (Rule 3). ssh-execution-boundary.md is N/A (the split stays delegated; no liveness verdict introduced). No max-lines disable added, no .d.ts, no vague module name.
Spec (#16510)
Entry points — all three covered. The issue's expected behaviour names "shortcut, context menu, or pane header". Shortcut → terminal-keyboard-action-dispatch.ts:207, context menu → use-terminal-pane-split-actions.ts:57, pane header → TerminalPane.tsx:2869 (all under src/renderer/src/components/terminal-pane/); all three call splitTerminalPaneWithInheritedCwd, which forks into splitWebRuntimeTerminal at terminal-pane-split-with-inherited-cwd.ts:19. ✅
#5435 guard intact. chooseRemoteTerminalLayout (web-session-tabs-sync.ts:986-996) still orders requested → existing → parentLayout → isActive, and the file has no source change in the diff. The issue's "the fix belongs on the split initiator, not in the snapshot precedence" is honoured. ✅
Data flow checks out. intent → matcher (web-session-tabs-sync.ts:2686-2698) → focusTarget (:2762-2774) → requestedActiveLeafId (:1075) → activeLeafId (:986) → setActivePane(..., { focus: isActive }) (TerminalPane.tsx:1484). The PTY path publishes the same leafId variable it returns (orca-runtime.ts:30506); the graph path returns leaf.leafId from this.leaves (:30698), which is what the surface records carry. ✅
1. The intent channel carries two effects a local split does not
This is the one I'd most like a second opinion on. The PR argues no expectedCurrentLocalTabId guard is attached because "the local path focuses unconditionally, so adding one would break parity for a split in a non-focused group column". But the intent channel does more than move pane focus, and those extras have no local counterpart:
- With
expectedCurrentLocalTabId === undefined,callerFocusIntentTabalways survives (web-session-tabs-sync.ts:2700-2704), sohonorSnapshotActiveFocusis true (:2713) andintentUnifiedTabIdoverridescurrentVisibleUnifiedTabId(:3081-3087). A remote split in a non-visible tab therefore pulls the unified tab bar to that tab.splitManagedPanenever does. expandedLeafId(:999-1005): with arequestedActiveLeafIdand any existing expanded pane, the new pane inherits zoom. A local split does not zoom.
Both are arguably improvements, but they're parity changes the PR body doesn't claim, and the second one is reachable from any split of a tab that already has a zoomed pane.
2. "Both host split paths populate it" is broader than the code
orca-runtime.ts:30552-30557 returns leafId unconditionally — but publishPtyBackedMobileSessionTerminal sits inside if (createdPty) (:30500-30512). With a falsy createdPty the client records an intent naming a leaf no snapshot will ever carry. It degrades to today's unfocused behaviour rather than misfocusing, so not a regression — just worth not claiming more than holds.
3. waitForNewLeafInTab now decides focus, and it resolves any new leaf
orca-runtime.ts:30694-30700 returns the first new leaf in the tab. That was fine when its only job was minting handle; now it also names the leaf the client will claim, so two splits racing in one tab can claim each other's pane. Narrow in practice — worth a note either way.
4. Minor scope: the pairing fence changes split-failure UX
632b87ffea is a sound fix on its own terms, but routing the split RPC through captureRuntimeEnvironmentCall (web-runtime-session.ts:1489) means a same-id re-pair mid-dispatch now lands in the catch at :1505-1511, which surfaces toast.error(message) with the raw internal message. Previously the split silently went to the replacement host. Better behaviour, but it's a user-visible change outside what #16510 asked for, and the toast text isn't written for users.
Nothing here blocks the core fix — the diagnosis, the wire-compat handling, and the revert-oracle testing are all solid. The e2e helper duplication is the one I'd fix before merge; items 1 and 3 are worth a decision on the record.
Automated two-axis review (Claude Opus 5), findings verified against the source by the author.
Collapse the conditional pairing-revision spread the snapshot refresh already falls back for, scope the resolver's name to the terminal tabs it reads, and name the leaf a split creates instead of respelling its shape three times. Records why the claim rides the create-tab intent channel and why the raw host id arm is live, and states the bound waitForNewLeafInTab now decides focus on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ransport did Fencing the split RPC on the captured pairing revision made a same-id re-pair mid-dispatch reject, and the split's catch put that rejection's raw text -- "Runtime environment pairing changed; refresh and try again" -- straight into a toast. That string is written for logs and names our internals; the browser stream path already classifies it for the same reason. Classify only the fence code, so every other split failure still speaks for itself in the toast, and keep the raw text on the console warning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…es of The split-focus spec re-declared showClient byte-for-byte, narrowed the materialization spec's waitForClientWorktree, and inlined its callRuntime. Both now reveal through the existing revealPairedClientWindow -- which also waits for the renderer's own visibilitychange, the thing the copies were really after -- and share one host-session helper for the RPC and the workspace wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Worked through all ten findings. 8 fixed, 1 rejected, 1 needed no change. Three commits: Fixed
Rejected — B2The // orca-runtime.ts:28361
const operationTabId = request.placement?.tabId ?? deterministicAgentSessionUuid(...)
No change needed — C2The concern does not reach: Decisions recorded in code
Verification
|
ELI5
Split a terminal with
Cmd+Don your own machine and you can start typing in the newpane straight away. Do it on a workspace hosted by a remote Orca runtime and the pane
shows up but your typing still goes to the pane you split from — every time, until you
click the new one.
The reason is that the two cases take completely different routes. Locally, Orca creates
the pane itself and focuses it in the same breath. Remotely, Orca asks the host to do the
split and the pane comes back later as part of the host's session snapshot. When that
snapshot lands, the pane Orca focuses is whichever one the layout says is active — and
the layout deliberately prefers the leaf this client already had active over whatever the
host claims, so an agent's "I'm thinking" status update can't yank your cursor away
(#5435). Nobody ever told it that this particular arrival was the pane you just asked for,
so the old pane won.
This PR gives the split the same thing a remote "new terminal" already has: the client says
"I asked for this one" before the snapshot arrives, and the reconcile hands it focus.
What Changed
RuntimeTerminalSplitcarries the created pane'sleafId. Both host split pathspopulate it — the PTY-backed one already had the id in hand, and the graph-backed one now
resolves the leaf alongside its handle.
splitWebRuntimeTerminalrecords a focus intent naming that leaf, then replays thecurrent snapshot behind it — the same
recordWebSessionFocusIntent+acceptCurrentSnapshotpairingcreateWebRuntimeSessionTerminaluses atweb-runtime-session.ts:465.leafIdis left alone: no intent, today's behaviour. The same holds forany claim the mirror never matches — the layout keeps its existing
activeLeafId, so an unmatchedclaim lands unfocused, never misfocused.
fence's log text.
Why
terminal-pane-split-with-inherited-cwd.ts:19returns before the local split helper runswhenever the pane is remote:
so
splitManagedPane'ssetActivePaneId(newPane.id)(pane-split-close.ts:67) andnewPane.terminal.focus()(:145) never run for a remote split. The pane is materialisedlater by the mirror, and
TerminalPane.tsx:1478-1486focuses whateverrestoredLayout.activeLeafIdnames.That value comes from
chooseRemoteTerminalLayout(web-session-tabs-sync.ts:975-990),whose precedence is
requestedActiveLeafId→ the client's existingactiveLeafId→the host's
surface.isActive. The existing leaf is the pane that was split, so it alwayswon.
requestedActiveLeafIdis fed by a client-recorded focus intent, which the splitnever wrote.
Two alternatives were rejected:
isActiveabove the client's existing leaf. That is the exactordering Fix remote terminal focus snapping during agent runs #5435 installed to stop status-echo snapshots stealing focus. It would trade
this bug for that one.
the graph-backed path returns a leaf handle (
issueHandle) while session surfacespublish PTY handles (
issuePtyHandle), and those two namespaces are not guaranteed toconverge.
leafIdis authoritative on both paths.No
expectedCurrentLocalTabIdguard is attached: the intent names the tab the user justacted on, and the local path focuses unconditionally, so adding one would break parity for
a split in a non-focused group column.
Remote wire compatibility
Rule 1 of
docs/reference/remote-wire-compatibility.md: a new optional JSON field on anexisting frame, and the reader falls back rather than requiring it.
leafIdpresent → new pane focusedleafIdabsent → intent skipped, focus unchanged (today)No opcode, no protocol-version bump, no change to what the host publishes over the session
snapshot.
Linked Issue
Fixes #16510
Visual Proof
N/A — focus is not something a screenshot shows, and the topology needs two paired app
instances. The before/after is captured as an executable artifact instead:
tests/e2e/paired-remote-split-pane-focus.spec.ts, run against a real headed host pairedto a real Electron client.
Cmd+Dremote:e71aab66-…@@term_95147121-…— the pane that was splitThat failing run is the bug, reproduced end-to-end rather than described.
Testing
Six tests, each pinning a different part of the mechanism:
web-session-tabs-sync-focus-intent.test.ts— "activates a host-split leaf only when thesplitting client claimed it". One snapshot, applied twice: unclaimed it keeps the source
leaf active even though the host marks the new surface
isActive(this is the Fix remote terminal focus snapping during agent runs #5435 guard,now pinned so the fix cannot be "solved" by weakening it); claimed it activates the new leaf.
web-runtime-session-terminal-pane-delegation.test.ts— the split records the intent forthe host-returned
leafIdand replays the snapshot behind it, and records nothing whenthe host omits
leafId(the old-host path).orca-runtime.test.ts— the PTY-backed split result names the leaf its spawn actually got,compared against
ORCA_PANE_KEY.tests/e2e/paired-remote-split-pane-focus.spec.ts(new) — the whole journey on a realheaded host paired to a real Electron client: create a host terminal, activate its mirrored
tab in the client, split it through the same helper
Cmd+Dreaches (notmanager.splitPane,which would bypass
splitWebRuntimeTerminalentirely), then assert the active pane's PTY isnot the one that was split.
Revert oracles, both run — a test that passes either way proves nothing:
focusSplitWebRuntimeTerminalPane(unit)Ran green on macOS 26.5.2:
pnpm tcsrc/renderer/src/runtime+orca-runtime.test.ts— 144 files, 2308 passed, 1 skippedsrc/cli,runtime-client-export-parity,claude-agent-teams-service,mobile-subscribe-integration— green in the same sweepelectron-headless:paired-remote-split-pane-focus(17.2s) andpaired-remote-terminal-materialization-reconnect(18.9s). The second one matters because itshelpers moved into the shared module — it is the check that the extraction changed no behaviour.
Its third case stays skipped: that is a pre-existing
test.fixmeonmain, untouched.typecheck:e2eclean for the new spec and the shared helperNot run here: Windows and Linux. Nothing in this change is platform-dependent — no paths, no
shells, no key handling — so the risk is CI-shaped rather than behavioural.
AI Disclosure
Written with Claude Code (Claude Opus 5). Diagnosis, patch and tests were reviewed against
the source by the author before submitting.
Review
Root cause was traced through the whole chain rather than guessed at: the local/remote fork
in
terminal-pane-split-with-inherited-cwd.ts, the host's two split implementations, themirror's layout precedence, and the effect in
TerminalPane.tsxthat turnsactiveLeafIdinto an actual
setActivePane. The sync-layer test was written first against unpatched codeto confirm the precedence really is what strands focus, and the E2E spec was then run with
the fix stubbed out to confirm the same conclusion holds in a real paired topology and not
only in the reasoning.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
splitWebRuntimeTerminaland is untouched. Only paired-runtime panes (remote:ids) change.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (targeted suites + typecheck + lint ran locally; full suite and build left to CI)