From bfcbcfd59c0ed9d01fae131366519ee35e0ce57d Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 6 Oct 2026 23:51:42 -0400 Subject: [PATCH] refactor(terminal): move a dragged-out pane in main before the window opens its tab (#25380) * refactor(terminal): move the topology revision and leaf-lookup helpers into terminal-topology advanceTerminalTopologyRevision, findTerminalTabIdForLeaf and hasHostAuthoritativeTerminalMembership move verbatim from the renderer-save membership rebase into persistence/terminal-topology, the home of the commit boundary. Importers are repointed; no behavior change. * test(terminal): test-only guard for topology writes outside the commit boundary The three session sinks now publish through one commitWorkspaceSessionPartition helper, which hands the prior and published partition to the topology write guard. Production never arms the guard, so each sink pays one global lookup. The unit suite arms it in report mode: a sink write that changes class-(a) topology (membership, root, bindings, titles, incarnations, sleeping records, remote session ids, tombstones, default-applied, revision) outside a commit scope is attributed to its writer's file by stack, and fails the test unless the writer is on the unrouted-writers allowlist that later routing PRs shrink. Renderer saves and test seeding are exempt. Deep-freeze is available but stays off suite-wide until the in-place writers return new sessions. * refactor(terminal): add the topology commit module with bindLeaf, closeLeaf and closeTab terminal-topology-commit.ts is the boundary for class-(a) terminal topology. bindLeaf forwards to persistPtyBinding, whose write now runs in a commit scope; the spawn commits and the relay reattach bind through it. closeLeaf and closeTab wrap the existing close mutation in a commit scope and one persistence.terminal-topology span (kind, outcome, refusal reason; no ids), and the runtime close goes through them. Session output is unchanged: tests compare it byte for byte with the old writers for local, ssh: and folder workspaces. * chore(terminal): drop an unused lint suppression from the topology write guard * fix(terminal): attribute topology guard writers relative to the repo root The guard read a frame's file through its last /src/ segment, so a test under tests/ (folder-upgrade-identity-persistence.unit.test.ts) had no source frame and failed as an unknown writer. Frames are now taken relative to the repo root the setup passes in, tests/ counts as test seeding, Windows backslashes are normalized before the node_modules skip, and nested src/ paths keep their full path. The two runtime funnel files skip only their funnel function, so another writer in them still shows. The R9 allowlist key names the file whose frame actually writes. The class-(a) diff and the attribution move into their own files; the stack limit is restored in a finally. * refactor(terminal): drop bindLeaf until binding reaches a sink; add the boundary ratchet bindLeaf and the commit scope inside persistPtyBinding changed nothing: the binding write never reaches a session sink, and a scope inside the Store method would have admitted every direct caller once it did. Both return in B1-4; the spawn commits and the relay reattach call persistPtyBinding directly again. The runtime close now calls one closeLeafOrTab entry, so its callbacks keep their contextual types. A census test is the primary enforcement: only persistence/terminal-topology and the callers it lists may call persistPtyBinding, the session setters or the three sinks, and every unrouted-writer allowlist entry must name an existing file. * fix(test): resolve the topology guard's repo root without the global URL Under happy-dom the global URL is not Node's, so fileURLToPath(new URL(...)) threw in the setup file and failed every happy-dom test file. * feat(terminal): moveLeaf commits a pane detach in main before its new tab mounts Ported from fix/terminal-topology-stage-a (1bafc87, 3941bd4, 2ea2070, 3e79f31, a9d428e minus the bridge comment) and exposed as the commit module's moveLeaf, which runs inside the topology commit scope and span and publishes every rewritten partition through the session sink, so the test-only write guard sees it. Store.moveTerminalLeafToNewTab only hands it the store's state. Detach-to-new-tab now asks main first: the leaf and its binding move into the new tab in every owner partition (local and ssh:), with the incarnation, sleeping session, remote session id, UI marks and SSH lease, and the topology revision bumps. pty:moveLeafToNewTab then aliases the agent-status pane key and re-keys orchestration worker resources; the renderer's repeat transfer is skipped. A refused or failed move toasts and the pane stays; a move the renderer cannot apply is undone (STA-9259). Stage A review-2 fixes: - SF1: an undo whose source tab has closed retires the moved tab instead of refusing, so no ghost tab comes back on restart. - SF2: an undo restores the split direction, ratio and position, the source tab's PTY id and its SSH remote session id, from origins main kept for the move; it never adds a second copy of a leaf the source holds again. - N1: layouts of tabs that no longer exist do not refuse a move. - N2: an undo into a tab that gained a pane is refused as target_changed, not a silent not_held. - N3: a repeat drop of a pane whose move is still committing is a no-op. Note: the move bumps the topology revision, so a revision-0 repo enters rebased membership on its first detach. * fix(terminal): tighten moveLeaf undo and move across disagreeing SSH copies - An undo ignores a target layout whose tab row is gone, as the move does. - A retired undo leaves UI marks and SSH leases on the moved pane key instead of re-keying them to the closed source tab. - When an SSH respawn bound one partition before the other caught up, the move follows the renderer's live PTY id and drops the stale copy's incarnation instead of refusing with a toast. - A test pins the late-spawn graft the renderer's in-flight guard exists for. * fix(terminal): a pane move the renderer cannot finish never leaves main holding it - A pane whose PTY spawn is still in flight is not dragged out (no toast): the late spawn result would bind it to the source tab and graft the leaf back beside its moved copy. The IPC transport reports a pending connect. - A thrown or timed-out commit (10 s bound) sends an undo, since the write may have landed; undo answers not_held when nothing moved. The in-flight entry clears with it, so a stalled main cannot wedge the pane's drag. - A throw while opening the new tab after the pane left its source undoes the move too. - No "stays where it was" toast when the undo found the source tab closed or the source pane is gone; a failed undo says the pane may open in a new tab after a restart instead. * fix(terminal): drop a half-opened move tab so the window agrees with main's undo If opening the moved pane's tab throws after createTab ran, the renderer now closes that tab without killing its PTY or telling main (main never had it), and restores the source layout, while main undoes the move. * refactor(terminal): drop the runtime topology write guard; the boundary ratchet enforces The AST boundary ratchet is the enforcement for B1. The stack-attributed runtime guard, its class-(a) diff, the unrouted-writer allowlist, the vitest setup and the freeze option are removed; it saw two writers in the whole unit suite and its real value starts only once binding reaches a sink (B1-4). Also: one closeLeafOrTab wraps the close in the span (no per-kind copies or narrowed types), the span has one finish like persistence.pty-binding, the sink helper is publishWorkspaceSessionPartition (it publishes; the commit boundary is the module), the ratchet drops the private publishSession row and checks that every listed caller file exists, and the close comparison keeps its two meaningful cases with span cases chosen by name. * refactor(terminal): roll a pane move forward instead of undoing it Once main commits a pane move, the renderer only rolls forward: - A repeat of a committed move answers moved and writes nothing, so a thrown commit (outcome unknown) is retried, up to three attempts; the IPC skips the agent-status transfer it already made. - The renderer applies the move by leaf id. If the user closed the pane or its tab meanwhile, it closes main's new tab through the ordinary session:close-terminal-surface path; if a sibling closed and the pane is the source's last, the source tab closes without killing the PTY. - One toast, for a move main refused or never answered. Deleted: the undo planner, origin ledger, undo flag, retired and target_changed results, the commit timeout, the half-opened-tab cleanup and the second toast. Also: one request validation (distinct tabs, stable leaf id, tab ids a pane key can carry, built through makePaneKey); a two-line PTY check where the live PTY id wins; the move and close share one span wrapper; the moved tab's layout always takes the live PTY id (a stale saved id was kept before); fallbackPtyId is now livePtyId; tests are organized by behavior. * refactor(terminal): trim the B1-1 commit module and ratchet to what they enforce - Point the acknowledged-tab-retirement audit fixture at the moved advanceTerminalTopologyRevision; its old import no longer resolved. - Drop publishWorkspaceSessionPartition: it was the removed guard's interception point, so the three session sinks return to origin/main. - One traced(kind, mutate) wrapper in the commit file replaces the span factory; closeLeafOrTab is one call. - The ratchet walks src/main with the shared scanSourceTree, drops the loading-store-internal rows and the redundant file-exists test; exact-set equality already fails on a missing file. - The commit test is a pure unit test of the span outcomes: no Store harness, electron mock or self-comparing close. * fix(terminal): keep a moved last pane alive and trim moveLeaf to one attempt A sibling closed while main committed the move persists the source layout down to the moved leaf, so the renderer took that for "pane gone" and closed main's new tab while the pane was live: after a restart it was held nowhere. The renderer now reads the store layout: when the moved leaf is the only one left it opens the new tab with that layout, syncs PTY ownership, and closes the source tab without killing the PTY. Main's new tab is closed only when main moved the pane and it is gone here. Also: - One commit attempt; the repeat answer, the retry loop and the IPC alias guard never ran in production (a write with an unknown outcome faults the writer, so every retry rethrows before it reaches the move). - traced takes a typed refusalOf; one write-and-restore helper and a shared partition assign replace the duplicated local/remote and rollback checks. - The planner's holders carry their source tab and layout, so the partition move cannot fail; pane-keyed UI marks are re-keyed in one loop; the request validator calls makePaneKey directly. - applyMove looks the pane up once and takes one PTY id (main's when it moved the pane); the move-commit file is inlined; duplicated renderer tests are removed and the commit-path tests live together. * refactor(terminal): census the runtime session controller's write and drop stage ids from comments The controller's setter was named set, which the boundary census could not list without matching every Map.set, so a new OrcaRuntime mixin could write sessions through it unseen. Rename it setForWorktree and census it. Comments now describe state instead of citing plan stage ids. * refactor(terminal): census writer references and trace refusals by callback - traced() takes refusalOf instead of assuming an Error refusal, and only mutate() sits in the try, so a span outcome of threw means the write threw. - The boundary ratchet counts references, not just direct calls: non-null calls, bracket keys, aliases, destructures, .call/.bind and parenthesized callees all count; declared names and type positions do not. - Census terminalSurfaceCloseMutation (boundary-only) and the partition sinks setLocalWorkspaceSession / setHostWorkspaceSession. * test(terminal): count writer uses in extends clauses and instantiations, skip type-only imports and local declarations The census skipped ExpressionWithTypeArguments as a type, which also holds `extends f(x)` and `x` value expressions. Type-only import/export specifiers and declared names (variables, parameters, accessors, enum members) no longer count as uses. The audit fixture is listed in the table instead of a separate exemption. * test(terminal): count quoted and assignment-pattern destructures of layout writers * test(terminal): count every mention of a layout writer except its definition Telling definitions from uses per syntax kind kept missing nested and for-of destructures. Exempt only the writer's own function or class-member definition; any other mention (including object-literal keys) counts, so the census errs toward a loud false alarm rather than a silent miss. Quoted names count only in member-name position. * test(terminal): count every string literal naming a layout writer Member-name positions missed wrapped keys like store[('name')] and store['name' as const]. Counting every string literal outside types is shorter and errs toward a loud false alarm. * test(terminal): exempt only class members and functions as writer definitions Object-literal methods and accessors were exempt while equivalent arrow properties counted; all object-literal keys now count alike. * test(terminal): parse files with unicode escapes in the writer census A name spelled with a \u escape never appears verbatim, so the text prefilter skipped it. * test(terminal): parse any file with an escape in the writer census \x, identity and line-continuation escapes also decode to a writer name without it appearing verbatim. * test(agent-hooks): stub isPaneAuthorityTransferredTo on the hook server fake * refactor(terminal): one move path for every client and an idempotent status transfer The pane manager decides whether the dragged pane is the last one, and a refused detach closes main's new tab like a vanished pane. Paired web clients answer the move as not held, so every client takes the same moved / not_held / refused branch. Repeating an agent-status transfer that is already in place is now a no-op where it happens, replacing the IPC special case. * fix(lint): bring structured-agent-session-host back under max-lines (cherry picked from commit 1a5f6872441304ca0f553d76b9d2b5c5137c2b08) --- .../agent-hooks/server-pane-authority.test.ts | 17 + .../server/server-authority-aliases.ts | 8 + src/main/ipc/pty/ipc/leaf-move.test.ts | 70 ++++ src/main/ipc/pty/ipc/leaf-move.ts | 58 +++ src/main/ipc/pty/register-handlers.ts | 3 + .../loading-store/pty-binding-persistence.ts | 21 + ...erminal-leaf-move-concurrent-close.test.ts | 120 ++++++ .../terminal-leaf-move-fixture.ts | 86 +++++ .../terminal-leaf-move.test.ts | 363 ++++++++++++++++++ .../terminal-topology/terminal-leaf-move.ts | 236 ++++++++++++ .../terminal-topology-commit.ts | 100 ++++- .../terminal-topology-membership.ts | 32 ++ .../worker-terminal-resource-store.ts | 18 + src/preload/api/pty-api.ts | 5 + src/preload/api/pty-bridge-session-control.ts | 6 + .../pty-transport-connect-spawn.test.ts | 22 ++ .../terminal-pane/pty-transport-types.ts | 2 + .../components/terminal-pane/pty-transport.ts | 8 + .../terminal-pane-tab-detach-commit.test.ts | 272 +++++++++++++ .../terminal-pane-tab-detach-fixture.ts | 122 ++++++ .../terminal-pane-tab-detach.test.ts | 189 +++------ .../terminal-pane/terminal-pane-tab-detach.ts | 179 ++++++--- .../use-terminal-pane-close-actions.ts | 31 +- src/renderer/src/i18n/locales/en.json | 3 + .../src/web/preload-api/web-terminal-api.ts | 2 + src/shared/terminal-leaf-move.test.ts | 35 ++ src/shared/terminal-leaf-move.ts | 59 +++ 27 files changed, 1853 insertions(+), 214 deletions(-) create mode 100644 src/main/ipc/pty/ipc/leaf-move.test.ts create mode 100644 src/main/ipc/pty/ipc/leaf-move.ts create mode 100644 src/main/persistence/terminal-topology/terminal-leaf-move-concurrent-close.test.ts create mode 100644 src/main/persistence/terminal-topology/terminal-leaf-move-fixture.ts create mode 100644 src/main/persistence/terminal-topology/terminal-leaf-move.test.ts create mode 100644 src/main/persistence/terminal-topology/terminal-leaf-move.ts create mode 100644 src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-commit.test.ts create mode 100644 src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-fixture.ts create mode 100644 src/shared/terminal-leaf-move.test.ts create mode 100644 src/shared/terminal-leaf-move.ts diff --git a/src/main/agent-hooks/server-pane-authority.test.ts b/src/main/agent-hooks/server-pane-authority.test.ts index 7d622b5a8c9..57c3f8f8718 100644 --- a/src/main/agent-hooks/server-pane-authority.test.ts +++ b/src/main/agent-hooks/server-pane-authority.test.ts @@ -53,6 +53,23 @@ describe('AgentHookServer pane authority', () => { ]) }) + // A pane move is announced by main and again by the renderer; the repeat must change nothing. + it('ignores a transfer that is already in place', () => { + const server = new AgentHookServer() + const listener = vi.fn() + server.setPaneKeyAliasPersistenceListener(listener) + + server.transferPaneAuthority(SOURCE, TARGET, 'pty-1', 10, { authorityVerified: true }) + server.transferPaneAuthority(SOURCE, TARGET, 'pty-1', 20) + server.transferPaneAuthority(SOURCE, TARGET, undefined, 30) + + expect(listener).toHaveBeenCalledOnce() + server.transferPaneAuthority(SOURCE, TARGET, 'pty-2', 40) + expect(listener).toHaveBeenLastCalledWith([ + { legacyPaneKey: SOURCE, stablePaneKey: TARGET, ptyId: 'pty-2', updatedAt: 40 } + ]) + }) + it('persists one physical alias while chained transfers advance its owner', () => { const server = new AgentHookServer() const listener = vi.fn() diff --git a/src/main/agent-hooks/server/server-authority-aliases.ts b/src/main/agent-hooks/server/server-authority-aliases.ts index fb4890e10a2..d5d3b0666d1 100644 --- a/src/main/agent-hooks/server/server-authority-aliases.ts +++ b/src/main/agent-hooks/server/server-authority-aliases.ts @@ -144,6 +144,14 @@ export abstract class AgentHookServerAuthorityAliases extends AgentHookServerAut } const previousOwnerPaneKey = this.resolvePaneKeyAlias(fromPaneKey) const physicalPaneKey = this.getPhysicalPaneKeyForAuthority(fromPaneKey, ptyId) + const requestedPtyId = ptyId?.trim() + // Why: a repeat would clear the owner's own polls; a pane move announces from main and renderer. + if ( + previousOwnerPaneKey === toPaneKey && + (!requestedPtyId || this.legacyPaneKeyAliases.get(physicalPaneKey)?.ptyId === requestedPtyId) + ) { + return + } for (const key of [fromPaneKey, previousOwnerPaneKey, physicalPaneKey, toPaneKey]) { this.takeRetiredPaneRestartId(key) } diff --git a/src/main/ipc/pty/ipc/leaf-move.test.ts b/src/main/ipc/pty/ipc/leaf-move.test.ts new file mode 100644 index 00000000000..724a706a56b --- /dev/null +++ b/src/main/ipc/pty/ipc/leaf-move.test.ts @@ -0,0 +1,70 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { agentHookServer } from '../../../agent-hooks/server' +import type { Store } from '../../../persistence' +import type { OrcaRuntimeService } from '../../../runtime/orca-runtime' +import type { TerminalLeafMoveResult } from '../../../../shared/terminal-leaf-move' +import { commitLeafMoveAndRekey } from './leaf-move' + +const LEAF = '22222222-2222-4222-8222-222222222222' +const request = { + worktreeId: 'repo-1::/tmp/wt', + sourceTabId: 'tab-source', + targetTabId: 'tab-target', + leafId: LEAF, + ptyId: 'pty-agent' +} + +function deps(result: TerminalLeafMoveResult) { + const rekeyWorkerTerminalResourcePaneKey = vi.fn(() => 1) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the move reads only this Store method. + const store = { moveTerminalLeafToNewTab: vi.fn(async () => result) } as unknown as Store + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the move reads only the orchestration DB accessor. + const runtime = { + getExistingOrchestrationDb: () => ({ rekeyWorkerTerminalResourcePaneKey }) + } as unknown as OrcaRuntimeService + return { store, runtime, rekeyWorkerTerminalResourcePaneKey } +} + +afterEach(() => { + vi.restoreAllMocks() +}) + +describe('pty:moveLeafToNewTab', () => { + it('aliases agent status and re-keys worker resources after a committed move', async () => { + const transfer = vi.spyOn(agentHookServer, 'transferPaneAuthority').mockImplementation(() => {}) + const { store, runtime, rekeyWorkerTerminalResourcePaneKey } = deps({ + status: 'moved', + ptyId: 'pty-agent' + }) + + await expect(commitLeafMoveAndRekey({ store, runtime }, request)).resolves.toEqual({ + status: 'moved', + ptyId: 'pty-agent' + }) + + expect(transfer).toHaveBeenCalledWith( + `tab-source:${LEAF}`, + `tab-target:${LEAF}`, + 'pty-agent', + expect.any(Number), + { authorityVerified: true } + ) + expect(rekeyWorkerTerminalResourcePaneKey).toHaveBeenCalledWith({ + fromPaneKey: `tab-source:${LEAF}`, + toPaneKey: `tab-target:${LEAF}` + }) + }) + + it.each([ + { status: 'not_held' }, + { status: 'refused', reason: 'pty_mismatch' } + ])('leaves every pane key alone when the move did not commit (%o)', async (result) => { + const transfer = vi.spyOn(agentHookServer, 'transferPaneAuthority').mockImplementation(() => {}) + const { store, runtime, rekeyWorkerTerminalResourcePaneKey } = deps(result) + + await expect(commitLeafMoveAndRekey({ store, runtime }, request)).resolves.toEqual(result) + + expect(transfer).not.toHaveBeenCalled() + expect(rekeyWorkerTerminalResourcePaneKey).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ipc/pty/ipc/leaf-move.ts b/src/main/ipc/pty/ipc/leaf-move.ts new file mode 100644 index 00000000000..c8b0cb9f07a --- /dev/null +++ b/src/main/ipc/pty/ipc/leaf-move.ts @@ -0,0 +1,58 @@ +import { getPtyIpc } from '../../pty-host-bindings' +import type { Store } from '../../../persistence' +import type { OrcaRuntimeService } from '../../../runtime/orca-runtime' +import { agentHookServer } from '../../../agent-hooks/server' +import { + isTerminalLeafMoveRequest, + terminalLeafMovePaneKeys, + type TerminalLeafMoveRequest, + type TerminalLeafMoveResult +} from '../../../../shared/terminal-leaf-move' + +/** Durable move first; agent-status and orchestration keys follow only a committed move. */ +export async function commitLeafMoveAndRekey( + deps: { store?: Store; runtime?: OrcaRuntimeService }, + request: TerminalLeafMoveRequest +): Promise { + if (!deps.store) { + return { status: 'not_held' } + } + const result = await deps.store.moveTerminalLeafToNewTab(request) + if (result.status !== 'moved') { + return result + } + const { from: fromPaneKey, to: toPaneKey } = terminalLeafMovePaneKeys(request) + try { + // The process keeps the pane key baked into its env, so status must alias old to new. + agentHookServer.transferPaneAuthority( + fromPaneKey, + toPaneKey, + result.ptyId ?? undefined, + Date.now(), + { authorityVerified: true } + ) + } catch (error) { + console.warn('[pty] moved pane kept its old agent-status key:', error) + } + try { + deps.runtime?.getExistingOrchestrationDb()?.rekeyWorkerTerminalResourcePaneKey({ + fromPaneKey, + toPaneKey + }) + } catch (error) { + console.warn('[pty] moved pane kept its old orchestration resource key:', error) + } + return result +} + +export function installPtyLeafMoveIpcHandler(deps: { + store?: Store + runtime?: OrcaRuntimeService +}): void { + getPtyIpc().handle('pty:moveLeafToNewTab', async (_event, args: unknown) => { + if (!isTerminalLeafMoveRequest(args)) { + return { status: 'refused', reason: 'invalid_request' } satisfies TerminalLeafMoveResult + } + return commitLeafMoveAndRekey(deps, args) + }) +} diff --git a/src/main/ipc/pty/register-handlers.ts b/src/main/ipc/pty/register-handlers.ts index 9ce1d4082a4..7f29e895e0b 100644 --- a/src/main/ipc/pty/register-handlers.ts +++ b/src/main/ipc/pty/register-handlers.ts @@ -20,6 +20,7 @@ import { } from './ipc/renderer-kill' import { installPtyWriteIpcHandlers } from './ipc/write' import { installPtySpawnIpcHandler } from './ipc/spawn' +import { installPtyLeafMoveIpcHandler } from './ipc/leaf-move' import { installPtyRuntimeController } from './runtime/controller' import { installPtySnapshotIpcHandlers } from './ipc/snapshot' import { @@ -112,6 +113,7 @@ export function registerPtyHandlers( // Remove prior handlers so re-registration (e.g. macOS re-activate creating a new window) doesn't double-register. ipcMain.removeHandler('pty:spawn') + ipcMain.removeHandler('pty:moveLeafToNewTab') ipcMain.removeHandler('pty:kill') ipcMain.removeHandler('pty:listSessions') ipcMain.removeHandler('pty:hasPty') @@ -255,6 +257,7 @@ export function registerPtyHandlers( rememberSyntheticKillExit: session.rememberSyntheticKillExit, sendPtyExitToRenderer: session.sendPtyExitToRenderer } + installPtyLeafMoveIpcHandler({ store, runtime }) installPtySpawnIpcHandler({ runtime, store, diff --git a/src/main/persistence/loading-store/pty-binding-persistence.ts b/src/main/persistence/loading-store/pty-binding-persistence.ts index 5ad0acab94a..9e6041adb04 100644 --- a/src/main/persistence/loading-store/pty-binding-persistence.ts +++ b/src/main/persistence/loading-store/pty-binding-persistence.ts @@ -16,6 +16,11 @@ import { evaluatePtyBindingFastLane } from './pty-binding-fast-lane' import { ptyBindingIsRefused } from './pty-binding-refusals' import { startPtyBindingSpan, type PtyBindingOrigin, type PtyBindingSpan } from './pty-binding-span' import { applyPtyBinding } from './pty-binding-session-update' +import type { + TerminalLeafMoveRequest, + TerminalLeafMoveResult +} from '../../../shared/terminal-leaf-move' +import { moveLeaf } from '../terminal-topology/terminal-topology-commit' import { findTerminalBindingConflict } from '../terminal-topology/terminal-owner-invariants' type PtyBindingPersistenceOperationsRuntime = Pick< @@ -188,6 +193,22 @@ export class PtyBindingPersistenceOperations { throw error } } + + /** + * Detach-to-new-tab, committed before the renderer mounts the target tab (STA-9259). It lives on + * the binding domain only for its runtime and partition access; the commit module owns the write. + */ + moveTerminalLeafToNewTab(request: TerminalLeafMoveRequest): Promise { + const { runtime, sessions } = this[ptyBindingPersistenceOperationsContext] + return runtime.runDurableMutation( + moveLeaf(request, { + state: runtime.state, + hostIds: () => sessions.getWorkspaceSessionHostIds(), + getSession: (hostId) => sessions.getWorkspaceSession(hostId), + markDirty: (domain) => runtime.dirtyProfileStateDomains?.add(domain) + }) + ) + } } function writePtyBinding( diff --git a/src/main/persistence/terminal-topology/terminal-leaf-move-concurrent-close.test.ts b/src/main/persistence/terminal-topology/terminal-leaf-move-concurrent-close.test.ts new file mode 100644 index 00000000000..d5d0a89c4bc --- /dev/null +++ b/src/main/persistence/terminal-topology/terminal-leaf-move-concurrent-close.test.ts @@ -0,0 +1,120 @@ +import { tmpdir } from 'node:os' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { TerminalSurfaceCloseTarget } from '../../../shared/terminal-surface-close-target' +import { makeRepo } from '../../persistence-test-harness' +import { + closeMoveTestStores, + MOVED, + moveRequest, + newDataFile, + openStore, + seedSplitSource, + SOURCE, + TARGET, + tabsHoldingLeaf, + WT +} from './terminal-leaf-move-fixture' +import { closeLeafOrTab } from './terminal-topology-commit' + +vi.mock('electron', () => ({ + app: { + getPath: () => tmpdir(), + getName: () => 'orca-test', + getVersion: () => '0.0.0-test', + isPackaged: false, + on: () => {}, + whenReady: () => Promise.resolve() + }, + safeStorage: { + isEncryptionAvailable: () => false, + encryptString: (value: string) => Buffer.from(value), + decryptString: (value: Buffer) => value.toString() + }, + ipcMain: { on: () => {}, handle: () => {} }, + BrowserWindow: { getAllWindows: () => [] } +})) + +afterEach(closeMoveTestStores) + +const request = { ...moveRequest, ptyId: 'pty-agent' } + +/** What `session:close-terminal-surface` commits for a close the renderer already performed. */ +async function rendererClose( + store: ReturnType, + target: TerminalSurfaceCloseTarget, + reason: 'user' | 'cleanup' +): Promise { + await store.runDurableMutation( + closeLeafOrTab({ + worktreeId: WT, + target, + options: { allowMissing: true, force: true, closedByLayoutOwner: true, reason }, + requestedSession: store.getWorkspaceSession(), + ownerMatches: () => true, + hostId: () => 'local', + getSession: (hostId) => store.getWorkspaceSession(hostId), + setSession: (session, hostId) => store.setWorkspaceSession(session, hostId), + onClosed: () => {} + }) + ) +} + +async function openMovedStore() { + const dataFile = newDataFile() + const store = openStore(dataFile) + store.addRepo(makeRepo({ id: 'repo-1', path: '/tmp/move-worktree' })) + await seedSplitSource(store) + await expect(store.moveTerminalLeafToNewTab(request)).resolves.toMatchObject({ + status: 'moved' + }) + return { dataFile, store } +} + +function restartedTabs(store: ReturnType, dataFile: string) { + store.flush() + const session = openStore(dataFile).getWorkspaceSession() + return { tabIds: (session.tabsByWorktree[WT] ?? []).map((tab) => tab.id), session } +} + +// The renderer rolls a committed move forward: when the pane is gone it closes main's new tab. +describe('a close that lands while main commits the move', () => { + it('leaves no dead pane after restart when the user closed the moved pane', async () => { + const { dataFile, store } = await openMovedStore() + // The pane close reaches main after the move, so main no longer has SOURCE:leaf. + await rendererClose(store, { kind: 'pane', tabId: SOURCE, leafId: MOVED }, 'user') + expect(tabsHoldingLeaf(store.getWorkspaceSession(), MOVED)).toEqual([TARGET]) + + await rendererClose(store, { kind: 'tab', tabId: TARGET }, 'cleanup') + + const { tabIds, session } = restartedTabs(store, dataFile) + expect(tabsHoldingLeaf(session, MOVED)).toEqual([]) + expect(tabIds).toEqual([SOURCE]) + }) + + it('leaves no ghost tab after restart when the user closed the source tab', async () => { + const { dataFile, store } = await openMovedStore() + await rendererClose(store, { kind: 'tab', tabId: SOURCE }, 'user') + + await rendererClose(store, { kind: 'tab', tabId: TARGET }, 'cleanup') + + const { tabIds, session } = restartedTabs(store, dataFile) + expect(tabIds).toEqual([]) + expect(tabsHoldingLeaf(session, MOVED)).toEqual([]) + }) +}) + +// Why the renderer refuses to drag a pane whose spawn is in flight: a move with no PTY id lets the +// late spawn result bind SOURCE:leaf, and that bind grafts the leaf back into the source tab. +it('grafts a late spawn result for the moved leaf back into its source tab', async () => { + const store = openStore(newDataFile()) + await seedSplitSource(store) + await store.moveTerminalLeafToNewTab({ ...request, ptyId: null }) + await store.persistPtyBinding({ + worktreeId: WT, + tabId: SOURCE, + leafId: MOVED, + ptyId: 'pty-late', + incarnationId: 'inc-late' + }) + expect(tabsHoldingLeaf(store.getWorkspaceSession(), MOVED).sort()).toEqual([SOURCE, TARGET]) +}) diff --git a/src/main/persistence/terminal-topology/terminal-leaf-move-fixture.ts b/src/main/persistence/terminal-topology/terminal-leaf-move-fixture.ts new file mode 100644 index 00000000000..413d6cecc41 --- /dev/null +++ b/src/main/persistence/terminal-topology/terminal-leaf-move-fixture.ts @@ -0,0 +1,86 @@ +import { mkdtempSync, realpathSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { vi } from 'vitest' +import type { SleepingAgentSessionRecord } from '../../../shared/agent-session-resume' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import { closeTestStores, createSqliteTestStore } from '../../persistence-test-harness' +import { Store } from '../loading-store/store' +import { collectLayoutLeafIdsInOrder } from '../restoring-sessions/terminal-layout-normalization' + +/** Shared by the move tests; each test file mocks `electron` before importing this. */ + +export const WT = 'repo-1::/tmp/move-worktree' +export const SOURCE = 'tab-source' +export const TARGET = 'tab-target' +export const LEFT = '11111111-1111-4111-8111-111111111111' +export const MOVED = '22222222-2222-4222-8222-222222222222' +export const FROM = `${SOURCE}:${MOVED}` +export const TO = `${TARGET}:${MOVED}` + +const stores: Store[] = [] + +export async function closeMoveTestStores(): Promise { + for (const store of stores.splice(0)) { + store.freezeWrites() + } + await closeTestStores() + vi.restoreAllMocks() +} + +export function newDataFile(): string { + return join(realpathSync(mkdtempSync(join(tmpdir(), 'orca-leaf-move-'))), 'state.json') +} + +export function openStore(dataFile: string): Store { + const store = createSqliteTestStore(Store, { dataFile }) + stores.push(store) + return store +} + +export function sleeping(paneKey: string, tabId: string): SleepingAgentSessionRecord { + return { + paneKey, + tabId, + worktreeId: WT, + agent: 'codex', + providerSession: { key: 'session_id', id: 'codex-1' }, + prompt: 'go', + state: 'working', + capturedAt: 1, + updatedAt: 1 + } +} + +/** A split source tab whose second leaf runs the agent that gets moved. */ +export async function seedSplitSource(store: Store, hostId?: string): Promise { + await store.persistPtyBinding( + { worktreeId: WT, tabId: SOURCE, leafId: LEFT, ptyId: 'pty-left', incarnationId: 'inc-left' }, + hostId + ) + await store.persistPtyBinding( + { worktreeId: WT, tabId: SOURCE, leafId: MOVED, ptyId: 'pty-agent', incarnationId: 'inc-1' }, + hostId + ) +} + +export function ownersOf(session: WorkspaceSessionState, ptyId: string): string[] { + return Object.entries(session.terminalLayoutsByTabId).flatMap(([tabId, layout]) => + collectLayoutLeafIdsInOrder(layout.root) + .filter((leafId) => layout.ptyIdsByLeafId?.[leafId] === ptyId) + .map((leafId) => `${tabId}:${leafId}`) + ) +} + +export function tabsHoldingLeaf(session: WorkspaceSessionState, leafId: string): string[] { + return Object.entries(session.terminalLayoutsByTabId) + .filter(([, layout]) => collectLayoutLeafIdsInOrder(layout.root).includes(leafId)) + .map(([tabId]) => tabId) +} + +export const moveRequest = { + worktreeId: WT, + sourceTabId: SOURCE, + targetTabId: TARGET, + leafId: MOVED +} diff --git a/src/main/persistence/terminal-topology/terminal-leaf-move.test.ts b/src/main/persistence/terminal-topology/terminal-leaf-move.test.ts new file mode 100644 index 00000000000..14ff2d7b764 --- /dev/null +++ b/src/main/persistence/terminal-topology/terminal-leaf-move.test.ts @@ -0,0 +1,363 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { tmpdir } from 'node:os' +import { toSshExecutionHostId } from '../../../shared/execution-host' +import { _resetTracerForTests, setActiveSink } from '../../observability/tracer' +import { makeRepo, makeTerminalTab } from '../../persistence-test-harness' +import { planTerminalLeafMove } from './terminal-leaf-move' +import { + closeMoveTestStores, + FROM, + LEFT, + MOVED, + moveRequest, + newDataFile, + openStore, + ownersOf, + seedSplitSource, + sleeping, + SOURCE, + TARGET, + tabsHoldingLeaf, + TO, + WT +} from './terminal-leaf-move-fixture' + +vi.mock('electron', () => ({ + app: { + getPath: () => tmpdir(), + getName: () => 'orca-test', + getVersion: () => '0.0.0-test', + isPackaged: false, + on: () => {}, + whenReady: () => Promise.resolve() + }, + safeStorage: { + isEncryptionAvailable: () => false, + encryptString: (value: string) => Buffer.from(value), + decryptString: (value: Buffer) => value.toString() + }, + ipcMain: { on: () => {}, handle: () => {} }, + BrowserWindow: { getAllWindows: () => [] } +})) + +afterEach(closeMoveTestStores) + +describe('moving a pane to a new tab', () => { + it('moves the leaf and its binding in one write and re-keys pane-keyed records', async () => { + const store = openStore(newDataFile()) + await seedSplitSource(store) + store.setWorkspaceSession({ + ...store.getWorkspaceSession(), + sleepingAgentSessionsByPaneKey: { [FROM]: sleeping(FROM, SOURCE) } + }) + store.updateUI({ + acknowledgedAgentsByPaneKey: { [FROM]: 10 }, + activityClearedAtByPaneKey: { [FROM]: 11 }, + manuallyUnreadTurnsByPaneKey: { [FROM]: 12 } + }) + + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-agent' }) + ).resolves.toEqual({ status: 'moved', ptyId: 'pty-agent' }) + + const session = store.getWorkspaceSession() + expect(session.tabsByWorktree[WT]?.map((tab) => tab.id)).toEqual([SOURCE, TARGET]) + expect(session.terminalLayoutsByTabId[SOURCE]).toMatchObject({ + root: { type: 'leaf', leafId: LEFT }, + ptyIdsByLeafId: { [LEFT]: 'pty-left' } + }) + expect(session.terminalLayoutsByTabId[TARGET]).toMatchObject({ + root: { type: 'leaf', leafId: MOVED }, + ptyIdsByLeafId: { [MOVED]: 'pty-agent' } + }) + expect(session.terminalPtyIncarnationsByPaneKey).toEqual({ + [`${SOURCE}:${LEFT}`]: 'inc-left', + [TO]: 'inc-1' + }) + expect(session.sleepingAgentSessionsByPaneKey).toEqual({ + [TO]: expect.objectContaining({ paneKey: TO, tabId: TARGET }) + }) + expect(store.getUI()).toMatchObject({ + acknowledgedAgentsByPaneKey: { [TO]: 10 }, + activityClearedAtByPaneKey: { [TO]: 11 }, + manuallyUnreadTurnsByPaneKey: { [TO]: 12 } + }) + expect(ownersOf(session, 'pty-agent')).toEqual([TO]) + }) + + it('re-keys the SSH lease and moves within the partition that holds the tab', async () => { + const store = openStore(newDataFile()) + const hostId = toSshExecutionHostId('ssh-1') + await seedSplitSource(store, hostId) + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'pty-agent', + worktreeId: WT, + tabId: SOURCE, + leafId: MOVED, + state: 'attached' + }) + + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-agent' }) + ).resolves.toMatchObject({ status: 'moved' }) + + expect(tabsHoldingLeaf(store.getWorkspaceSession(hostId), MOVED)).toEqual([TARGET]) + expect(store.getWorkspaceSession().tabsByWorktree[WT]).toBeUndefined() + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ + expect.objectContaining({ ptyId: 'pty-agent', tabId: TARGET, leafId: MOVED }) + ]) + }) + + it('reports a leaf main never held instead of inventing one', async () => { + const store = openStore(newDataFile()) + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-agent' }) + ).resolves.toEqual({ status: 'not_held' }) + expect(store.getWorkspaceSession().tabsByWorktree[WT]).toBeUndefined() + }) + + it('refuses a move that names another terminal or an existing tab', async () => { + const store = openStore(newDataFile()) + await seedSplitSource(store) + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-other' }) + ).resolves.toEqual({ status: 'refused', reason: 'pty_mismatch' }) + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, targetTabId: SOURCE, ptyId: 'pty-agent' }) + ).resolves.toEqual({ status: 'refused', reason: 'target_tab_exists' }) + expect(tabsHoldingLeaf(store.getWorkspaceSession(), MOVED)).toEqual([SOURCE]) + }) + + it('moves a leaf that a stray layout of a removed tab still names', () => { + const planned = planTerminalLeafMove( + [ + { + hostId: 'local', + session: { + activeRepoId: 'repo-1', + activeWorktreeId: WT, + activeTabId: SOURCE, + tabsByWorktree: { [WT]: [makeTerminalTab({ id: SOURCE, worktreeId: WT })] }, + terminalLayoutsByTabId: { + [SOURCE]: { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: LEFT }, + second: { type: 'leaf', leafId: MOVED } + }, + activeLeafId: LEFT, + expandedLeafId: null + }, + 'tab-removed': { + root: { type: 'leaf', leafId: MOVED }, + activeLeafId: MOVED, + expandedLeafId: null + } + } + } + } + ], + { ...moveRequest, ptyId: 'pty-agent' } + ) + expect(planned.result).toEqual({ status: 'moved', ptyId: 'pty-agent' }) + }) + + it('records a persistence.terminal-topology span without pane keys or PTY ids', async () => { + const records: { name: string; attributes: Record }[] = [] + setActiveSink({ + push: (record) => { + records.push(JSON.parse(JSON.stringify(record))) + }, + flush: () => {}, + close: () => {} + }) + try { + const store = openStore(newDataFile()) + await seedSplitSource(store) + records.length = 0 + const request = { ...moveRequest, ptyId: 'pty-agent' } + await store.moveTerminalLeafToNewTab(request) + await store.moveTerminalLeafToNewTab(request) + await store.moveTerminalLeafToNewTab({ ...request, targetTabId: 'tab-other' }) + + const spans = records.filter((record) => record.name === 'persistence.terminal-topology') + expect(spans.map((span) => span.attributes)).toEqual([ + { kind: 'persistence', 'topology.kind': 'move_leaf', 'topology.outcome': 'committed' }, + { + kind: 'persistence', + 'topology.kind': 'move_leaf', + 'topology.outcome': 'refused', + 'topology.refusal': 'target_tab_exists' + }, + { + kind: 'persistence', + 'topology.kind': 'move_leaf', + 'topology.outcome': 'refused', + 'topology.refusal': 'leaf_in_other_tab' + } + ]) + expect(JSON.stringify(spans)).not.toMatch(/pty-|tab-source|tab-target|2222/) + } finally { + _resetTracerForTests() + } + }) +}) + +// After a restart the relay reattach writes the SSH pane into `local` as well as `ssh:`; a move +// that left either copy behind refused the moved pane and the next relay reattach. +describe('moving an SSH pane held by both partitions', () => { + it('moves every copy, so the target reattach and the next relay reattach both bind', async () => { + const store = openStore(newDataFile()) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const hostId = toSshExecutionHostId('ssh-1') + await seedSplitSource(store, hostId) + const relay = { worktreeId: WT, leafId: MOVED, ptyId: 'pty-agent', incarnationId: 'inc-1' } + expect( + await store.persistPtyBinding({ ...relay, tabId: SOURCE, origin: 'relay_reattach' }) + ).toBe(true) + + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-agent' }) + ).resolves.toMatchObject({ status: 'moved' }) + + expect(tabsHoldingLeaf(store.getWorkspaceSession(), MOVED)).toEqual([TARGET]) + expect(tabsHoldingLeaf(store.getWorkspaceSession(hostId), MOVED)).toEqual([TARGET]) + expect( + await store.persistPtyBinding({ ...relay, tabId: TARGET, origin: 'reattach' }, hostId) + ).toBe(true) + expect( + await store.persistPtyBinding({ + ...relay, + tabId: TARGET, + origin: 'relay_reattach', + mayReviveRetiredSurface: false + }) + ).toBe(true) + }) + + // Relay reattach into local, move, target reattach in ssh:, next-start relay reattach. + it('keeps one holder per partition across a restart, and the next relay reattach binds', async () => { + const dataFile = newDataFile() + const store = openStore(dataFile) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + store.addRepo(makeRepo({ id: 'repo-1', path: '/tmp/move-worktree' })) + const hostId = toSshExecutionHostId('ssh-1') + await seedSplitSource(store, hostId) + const relay = { worktreeId: WT, leafId: MOVED, ptyId: 'pty-agent', incarnationId: 'inc-1' } + await store.persistPtyBinding({ ...relay, tabId: SOURCE, origin: 'relay_reattach' }) + await store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-agent' }) + expect( + await store.persistPtyBinding({ ...relay, tabId: TARGET, origin: 'reattach' }, hostId) + ).toBe(true) + store.flush() + + const restarted = openStore(dataFile) + expect( + await restarted.persistPtyBinding({ + ...relay, + tabId: TARGET, + origin: 'relay_reattach', + mayReviveRetiredSurface: false + }) + ).toBe(true) + for (const session of [ + restarted.getWorkspaceSession(), + restarted.getWorkspaceSession(hostId) + ]) { + expect(tabsHoldingLeaf(session, MOVED)).toEqual([TARGET]) + expect(ownersOf(session, 'pty-agent')).toEqual([TO]) + } + }) + + it('follows the live PTY when an SSH respawn bound one partition before the other', async () => { + const store = openStore(newDataFile()) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const hostId = toSshExecutionHostId('ssh-1') + await seedSplitSource(store, hostId) + const relay = { worktreeId: WT, tabId: SOURCE, leafId: MOVED, incarnationId: 'inc-1' } + await store.persistPtyBinding({ ...relay, ptyId: 'pty-agent', origin: 'relay_reattach' }) + await store.persistPtyBinding( + { ...relay, ptyId: 'pty-respawn', incarnationId: 'inc-2' }, + hostId + ) + + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-respawn' }) + ).resolves.toEqual({ status: 'moved', ptyId: 'pty-respawn' }) + + for (const session of [store.getWorkspaceSession(), store.getWorkspaceSession(hostId)]) { + expect(tabsHoldingLeaf(session, MOVED)).toEqual([TARGET]) + expect(session.terminalLayoutsByTabId[TARGET]?.ptyIdsByLeafId).toEqual({ + [MOVED]: 'pty-respawn' + }) + } + expect(store.getWorkspaceSession(hostId).terminalPtyIncarnationsByPaneKey?.[TO]).toBe('inc-2') + expect(store.getWorkspaceSession().terminalPtyIncarnationsByPaneKey?.[TO]).toBeUndefined() + }) + + it('refuses a move while another tab already holds the leaf', async () => { + const store = openStore(newDataFile()) + await seedSplitSource(store) + const session = store.getWorkspaceSession() + store.setWorkspaceSession({ + ...session, + tabsByWorktree: { + ...session.tabsByWorktree, + [WT]: [ + ...(session.tabsByWorktree[WT] ?? []), + { ...(session.tabsByWorktree[WT] ?? [])[0]!, id: 'tab-earlier-move' } + ] + }, + terminalLayoutsByTabId: { + ...session.terminalLayoutsByTabId, + 'tab-earlier-move': { + root: { type: 'leaf', leafId: MOVED }, + activeLeafId: MOVED, + expandedLeafId: null + } + } + }) + + await expect( + store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-agent' }) + ).resolves.toEqual({ status: 'refused', reason: 'leaf_in_other_tab' }) + }) +}) + +// STA-9259: detach, then the moved pane's reattach binding, then a renderer snapshot that still +// shows the pre-move layout, then a restart. Before the move transaction this left one leaf and +// one PTY in both tabs. +describe('STA-9259 move sequence', () => { + it('keeps one owner through reattach, a stale renderer save and a restart', async () => { + const dataFile = newDataFile() + const store = openStore(dataFile) + // Load sweeps sessions of unregistered repos, so the restart needs a real owner. + store.addRepo(makeRepo({ id: 'repo-1', path: '/tmp/move-worktree' })) + await seedSplitSource(store) + const preMoveRendererSnapshot = structuredClone(store.getWorkspaceSession()) + + await store.moveTerminalLeafToNewTab({ ...moveRequest, ptyId: 'pty-agent' }) + // The moved pane mounts in the target tab and reattaches with its stable-owner fence. + const reattached = await store.persistPtyBinding({ + worktreeId: WT, + tabId: TARGET, + leafId: MOVED, + ptyId: 'pty-agent', + incarnationId: 'inc-1', + expectedBinding: { ptyId: 'pty-agent', incarnationId: 'inc-1' }, + origin: 'reattach' + }) + expect(reattached).toBe(true) + // A debounced renderer save that predates the move must not resurrect the source copy. + store.setWorkspaceSession(preMoveRendererSnapshot) + expect(ownersOf(store.getWorkspaceSession(), 'pty-agent')).toEqual([TO]) + expect(tabsHoldingLeaf(store.getWorkspaceSession(), MOVED)).toEqual([TARGET]) + + store.flush() + const restarted = openStore(dataFile) + expect(ownersOf(restarted.getWorkspaceSession(), 'pty-agent')).toEqual([TO]) + expect(tabsHoldingLeaf(restarted.getWorkspaceSession(), MOVED)).toEqual([TARGET]) + }) +}) diff --git a/src/main/persistence/terminal-topology/terminal-leaf-move.ts b/src/main/persistence/terminal-topology/terminal-leaf-move.ts new file mode 100644 index 00000000000..627e4a705e7 --- /dev/null +++ b/src/main/persistence/terminal-topology/terminal-leaf-move.ts @@ -0,0 +1,236 @@ +import type { PersistedState } from '../../../shared/persisted-state-types' +import type { SshRemotePtyLease } from '../../../shared/ssh-types' +import { + terminalLeafMovePaneKeys, + type TerminalLeafMoveRequest, + type TerminalLeafMoveResult +} from '../../../shared/terminal-leaf-move' +import type { TerminalLayoutSnapshot, TerminalTab } from '../../../shared/terminal-tab-types' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import { retireLeavesFromTerminalLayout } from '../../runtime/mobile-session-terminal-retirement' +import { createMinimalPersistedTerminalTab } from '../restoring-sessions/session-owner-fields' +import { layoutContainsLeafId } from '../restoring-sessions/terminal-layout-normalization' +import { + advanceTerminalTopologyRevision, + isTerminalOwnerPartition, + type TerminalSessionPartition +} from './terminal-topology-membership' + +type PlannedTerminalLeafMove = { + result: TerminalLeafMoveResult + /** Every partition the plan rewrote; empty when nothing changes. */ + sessions: TerminalSessionPartition[] +} + +function moveRecordKey( + record: Record | undefined, + fromKey: string, + toKey: string | null, + remap: (value: T) => T = (value) => value +): Record | undefined { + if (!record || !Object.hasOwn(record, fromKey)) { + return record + } + const next = { ...record } + const value = next[fromKey] + delete next[fromKey] + if (toKey !== null && value !== undefined) { + next[toKey] = remap(value) + } + return next +} + +function liveTabIds(session: WorkspaceSessionState): Set { + return new Set( + Object.values(session.tabsByWorktree ?? {}).flatMap((tabs) => tabs.map((tab) => tab.id)) + ) +} + +function hasTabId(session: WorkspaceSessionState, tabId: string): boolean { + return ( + liveTabIds(session).has(tabId) || Object.hasOwn(session.terminalLayoutsByTabId ?? {}, tabId) + ) +} + +type SourceHolder = TerminalSessionPartition & { + sourceTab: TerminalTab + sourceLayout: TerminalLayoutSnapshot +} + +function moveLeafInPartition( + { session, sourceTab, sourceLayout }: SourceHolder, + request: TerminalLeafMoveRequest, + ptyId: string | null +): WorkspaceSessionState { + const { worktreeId, sourceTabId, targetTabId, leafId } = request + const tabs = session.tabsByWorktree?.[worktreeId] ?? [] + const { from: fromPaneKey, to: toPaneKey } = terminalLeafMovePaneKeys(request) + const boundHere = sourceLayout.ptyIdsByLeafId?.[leafId] + const remainingLayout = retireLeavesFromTerminalLayout(sourceLayout, new Set([leafId])) + const remainingPtyIds = Object.values(remainingLayout?.ptyIdsByLeafId ?? {}) + const { pendingActivationSpawn, ...row } = createMinimalPersistedTerminalTab({ + worktreeId, + tabId: targetTabId, + ptyId: ptyId ?? '', + existingTabCount: tabs.length, + ...(sourceTab.startupCwd ? { startupCwd: sourceTab.startupCwd } : {}) + }) + const targetTab = { + ...row, + ptyId, + // A moved live pane reattaches; only an unbound one still spawns on activation. + ...(ptyId ? {} : { pendingActivationSpawn }), + ...(sourceTab.shellOverride ? { shellOverride: sourceTab.shellOverride } : {}) + } + const nextTabs = tabs.map((tab) => + tab.id === sourceTabId && tab.ptyId === ptyId + ? { ...tab, ptyId: remainingPtyIds[0] ?? null } + : tab + ) + const movedTitle = sourceLayout.titlesByLeafId?.[leafId] + const terminalLayoutsByTabId = { + ...session.terminalLayoutsByTabId, + [targetTabId]: { + root: { type: 'leaf' as const, leafId }, + activeLeafId: leafId, + expandedLeafId: null, + ...(ptyId ? { ptyIdsByLeafId: { [leafId]: ptyId } } : {}), + ...(movedTitle ? { titlesByLeafId: { [leafId]: movedTitle } } : {}), + ...(sourceLayout.chatLeafId === leafId ? { chatLeafId: leafId } : {}) + } + } + if (remainingLayout) { + terminalLayoutsByTabId[sourceTabId] = remainingLayout + } else { + // Its sibling was never bound here; the sibling's own binding mints the layout again. + delete terminalLayoutsByTabId[sourceTabId] + } + const remoteSessionIdsByTabId = { ...session.remoteSessionIdsByTabId } + if (ptyId && remoteSessionIdsByTabId[sourceTabId] === ptyId) { + remoteSessionIdsByTabId[targetTabId] = ptyId + if (remainingPtyIds[0]) { + remoteSessionIdsByTabId[sourceTabId] = remainingPtyIds[0] + } else { + delete remoteSessionIdsByTabId[sourceTabId] + } + } + return advanceTerminalTopologyRevision( + { + ...session, + tabsByWorktree: { ...session.tabsByWorktree, [worktreeId]: [...nextTabs, targetTab] }, + terminalLayoutsByTabId, + ...(session.remoteSessionIdsByTabId ? { remoteSessionIdsByTabId } : {}), + // A stale copy's incarnation belongs to the PTY it no longer names. + terminalPtyIncarnationsByPaneKey: moveRecordKey( + session.terminalPtyIncarnationsByPaneKey, + fromPaneKey, + boundHere && boundHere !== ptyId ? null : toPaneKey + ), + sleepingAgentSessionsByPaneKey: moveRecordKey( + session.sleepingAgentSessionsByPaneKey, + fromPaneKey, + toPaneKey, + (record) => ({ ...record, paneKey: toPaneKey, tabId: targetTabId }) + ) + }, + worktreeId + ) +} + +/** + * Moves one leaf and its binding into a new tab in a single session write (STA-9259). The leaf + * id and the PTY are kept; only the tab half of the pane key changes, so every pane-keyed record + * follows it here instead of being rebuilt later by a renderer save that main's membership rebase + * would discard. Every owner partition holding the pane moves together: a relay reattach writes an + * SSH pane into `local` as well as `ssh:`, and a copy left behind refuses the moved pane's bind. + */ +export function planTerminalLeafMove( + partitions: readonly TerminalSessionPartition[], + request: TerminalLeafMoveRequest +): PlannedTerminalLeafMove { + const { worktreeId, sourceTabId, leafId } = request + const refuse = ( + reason: Extract['reason'] + ): PlannedTerminalLeafMove => ({ result: { status: 'refused', reason }, sessions: [] }) + const owners = partitions.filter(({ hostId }) => isTerminalOwnerPartition(hostId)) + if (partitions.some(({ session }) => hasTabId(session, request.targetTabId))) { + return refuse('target_tab_exists') + } + // A layout left behind by a removed tab row owns nothing. + const leafElsewhere = owners.some(({ session }) => { + const tabIds = liveTabIds(session) + return Object.entries(session.terminalLayoutsByTabId ?? {}).some( + ([tabId, layout]) => + tabId !== sourceTabId && + tabIds.has(tabId) && + layoutContainsLeafId(layout?.root ?? null, leafId) + ) + }) + if (leafElsewhere) { + return refuse('leaf_in_other_tab') + } + const holders = owners.flatMap(({ hostId, session }) => { + const sourceTab = session.tabsByWorktree?.[worktreeId]?.find((tab) => tab.id === sourceTabId) + const sourceLayout = session.terminalLayoutsByTabId?.[sourceTabId] + return sourceTab && sourceLayout && layoutContainsLeafId(sourceLayout.root, leafId) + ? [{ hostId, session, sourceTab, sourceLayout }] + : [] + }) + // Main never saw this leaf, so no binding of it can be duplicated here. + if (holders.length === 0) { + return { result: { status: 'not_held' }, sessions: [] } + } + const boundPtyIds = new Set( + holders.flatMap(({ sourceLayout }) => sourceLayout.ptyIdsByLeafId?.[leafId] ?? []) + ) + // The renderer's live PTY id wins: after an SSH respawn one copy can still name the old PTY. + const ptyId = request.ptyId ?? (boundPtyIds.size === 1 ? [...boundPtyIds][0] : null) + if (boundPtyIds.size > 0 && !(ptyId && boundPtyIds.has(ptyId))) { + return refuse('pty_mismatch') + } + return { + result: { status: 'moved', ptyId }, + sessions: holders.map((holder) => ({ + hostId: holder.hostId, + session: moveLeafInPartition(holder, request, ptyId) + })) + } +} + +const PANE_KEYED_UI_RECORDS = [ + 'acknowledgedAgentsByPaneKey', + 'activityClearedAtByPaneKey', + 'manuallyUnreadTurnsByPaneKey' +] as const + +/** Pane-keyed UI marks and SSH lease leaf addresses that must follow a moved leaf. */ +export function rekeyMovedLeafProfileRecords( + state: Pick, + move: TerminalLeafMoveRequest +): { ui?: PersistedState['ui']; sshRemotePtyLeases?: SshRemotePtyLease[] } { + const { from: fromPaneKey, to: toPaneKey } = terminalLeafMovePaneKeys(move) + const ui = state.ui + let nextUi: PersistedState['ui'] | undefined + for (const key of PANE_KEYED_UI_RECORDS) { + const moved = moveRecordKey(ui?.[key], fromPaneKey, toPaneKey) + if (ui && moved !== ui[key]) { + nextUi = { ...(nextUi ?? ui), [key]: moved } + } + } + const leases = state.sshRemotePtyLeases ?? [] + const leasesChanged = leases.some( + (lease) => lease.tabId === move.sourceTabId && lease.leafId === move.leafId + ) + return { + ...(nextUi ? { ui: nextUi } : {}), + ...(leasesChanged + ? { + sshRemotePtyLeases: leases.map((lease) => + lease.tabId === move.sourceTabId && lease.leafId === move.leafId + ? { ...lease, tabId: move.targetTabId } + : lease + ) + } + : {}) + } +} diff --git a/src/main/persistence/terminal-topology/terminal-topology-commit.ts b/src/main/persistence/terminal-topology/terminal-topology-commit.ts index dee0f6283a9..dc1b562a51d 100644 --- a/src/main/persistence/terminal-topology/terminal-topology-commit.ts +++ b/src/main/persistence/terminal-topology/terminal-topology-commit.ts @@ -1,15 +1,24 @@ +import type { ExecutionHostId } from '../../../shared/execution-host' +import type { PersistedState } from '../../../shared/persisted-state-types' +import type { + TerminalLeafMoveRequest, + TerminalLeafMoveResult +} from '../../../shared/terminal-leaf-move' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { startSpan } from '../../observability/tracer' import { terminalSurfaceCloseMutation, type TerminalSurfaceCloseCommit } from '../../runtime/terminal-surface-close' import type { DurableProfileStateMutation } from '../loading-store/store-runtime-state' +import { planTerminalLeafMove, rekeyMovedLeafProfileRecords } from './terminal-leaf-move' +import { assignWorkspaceSessionPartition } from './terminal-topology-membership' -// The commit boundary for terminal layout (tabs, panes, pane-to-PTY bindings). Today it wraps only -// the close, whose transform still lives in runtime/; the other writers move here later. +// The commit boundary for terminal layout (tabs, panes, pane-to-PTY bindings). Wraps the close and +// the pane move; the close transform still lives in runtime/ and other writers move here later. /** Bindings are not listed: `persistPtyBinding` already records `persistence.pty-binding`. */ -type TerminalTopologyCommitKind = 'close_leaf' | 'close_tab' +type TerminalTopologyCommitKind = 'close_leaf' | 'close_tab' | 'move_leaf' export function closeLeafOrTab( commit: TerminalSurfaceCloseCommit @@ -22,9 +31,21 @@ export function closeLeafOrTab( ) } +/** Moves a leaf, its binding and its pane-keyed records into a new tab in one durable mutation. */ +export function moveLeaf( + request: TerminalLeafMoveRequest, + context: TerminalTopologyCommitContext +): () => DurableProfileStateMutation { + return traced( + 'move_leaf', + () => commitLeafMove(request, context), + (result) => (result.status === 'refused' ? result.reason : undefined) + ) +} + /** * One `persistence.terminal-topology` span per commit, from admission to the in-memory write. - * Attributes stay low-cardinality: no pane key, PTY id or path. + * Attributes stay low-cardinality: no pane key, PTY id or path; `refusalOf` returns a fixed code. */ function traced( kind: TerminalTopologyCommitKind, @@ -55,3 +76,74 @@ function traced( return result } } + +type TopologyState = Pick< + PersistedState, + 'workspaceSession' | 'workspaceSessionsByHostId' | 'ui' | 'sshRemotePtyLeases' +> + +type TerminalTopologyCommitContext = { + state: TopologyState + hostIds: () => ExecutionHostId[] + getSession: (hostId: ExecutionHostId) => WorkspaceSessionState + markDirty: ( + domain: 'workspaceSession' | 'workspaceSessionsByHostId' | 'ui' | 'sshRemotePtyLeases' + ) => void +} + +/** Writes `next`, returning a restore that puts the prior value back unless a later write replaced it. */ +function writeRestorable(read: () => V, write: (value: V) => void, next: V): () => void { + const prior = read() + write(next) + return () => { + if (read() === next) { + write(prior) + } + } +} + +function commitLeafMove( + request: TerminalLeafMoveRequest, + context: TerminalTopologyCommitContext +): DurableProfileStateMutation { + const { state } = context + const planned = planTerminalLeafMove( + context.hostIds().map((hostId) => ({ hostId, session: context.getSession(hostId) })), + request + ) + if (planned.sessions.length === 0) { + return { value: planned.result, persist: false } + } + const restores = planned.sessions.map(({ hostId, session }) => + writeRestorable( + () => context.getSession(hostId), + (value) => context.markDirty(assignWorkspaceSessionPartition(state, hostId, value)), + session + ) + ) + const rekeyed = rekeyMovedLeafProfileRecords(state, request) + if (rekeyed.ui) { + restores.push( + writeRestorable( + () => state.ui, + (ui) => (state.ui = ui), + rekeyed.ui + ) + ) + context.markDirty('ui') + } + if (rekeyed.sshRemotePtyLeases) { + restores.push( + writeRestorable( + () => state.sshRemotePtyLeases, + (leases) => (state.sshRemotePtyLeases = leases), + rekeyed.sshRemotePtyLeases + ) + ) + context.markDirty('sshRemotePtyLeases') + } + return { + value: planned.result, + rollback: () => restores.forEach((restore) => restore()) + } +} diff --git a/src/main/persistence/terminal-topology/terminal-topology-membership.ts b/src/main/persistence/terminal-topology/terminal-topology-membership.ts index 99437157437..358a1a34736 100644 --- a/src/main/persistence/terminal-topology/terminal-topology-membership.ts +++ b/src/main/persistence/terminal-topology/terminal-topology-membership.ts @@ -1,3 +1,9 @@ +import { + LOCAL_EXECUTION_HOST_ID, + parseExecutionHostId, + type ExecutionHostId +} from '../../../shared/execution-host' +import type { PersistedState } from '../../../shared/persisted-state-types' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { getRepoIdFromWorktreeId } from '../../../shared/worktree/id' import { layoutContainsLeafId } from '../restoring-sessions/terminal-layout-normalization' @@ -54,3 +60,29 @@ export function hasHostAuthoritativeTerminalMembership( ) ) } + +export type TerminalSessionPartition = { + hostId: ExecutionHostId + session: WorkspaceSessionState +} + +/** Partitions that own terminal panes; `runtime:` partitions mirror another host's. */ +export function isTerminalOwnerPartition(hostId: ExecutionHostId): boolean { + const kind = parseExecutionHostId(hostId)?.kind + return kind === 'local' || kind === 'ssh' +} + +/** Puts one partition's session in place and names the profile domain that now needs a write. */ +export function assignWorkspaceSessionPartition( + state: Pick, + hostId: ExecutionHostId, + session: WorkspaceSessionState +): 'workspaceSession' | 'workspaceSessionsByHostId' { + // Why: 'local' always lives in workspaceSession, never workspaceSessionsByHostId.local. + if (hostId === LOCAL_EXECUTION_HOST_ID) { + state.workspaceSession = session + return 'workspaceSession' + } + state.workspaceSessionsByHostId = { ...state.workspaceSessionsByHostId, [hostId]: session } + return 'workspaceSessionsByHostId' +} diff --git a/src/main/runtime/orchestration/db/worker-terminal/worker-terminal-resource-store.ts b/src/main/runtime/orchestration/db/worker-terminal/worker-terminal-resource-store.ts index fe26380d539..b5061de5950 100644 --- a/src/main/runtime/orchestration/db/worker-terminal/worker-terminal-resource-store.ts +++ b/src/main/runtime/orchestration/db/worker-terminal/worker-terminal-resource-store.ts @@ -244,10 +244,27 @@ export function retainReplacedWorkerTerminalResources( ) } +/** A detached pane keeps its process, so the resources it owns follow its new pane key. */ +export function rekeyWorkerTerminalResourcePaneKey( + this: OrchestrationDb, + params: { fromPaneKey: string; toPaneKey: string } +): number { + return Number( + this.db + .prepare( + `UPDATE worker_terminal_resources + SET pane_key = ?, updated_at = datetime('now') + WHERE pane_key = ? AND release_state != 'released'` + ) + .run(params.toPaneKey, params.fromPaneKey).changes + ) +} + // Finds an owned, settled, exact-match resource for an explicitly reused terminal. export type WorkerTerminalResourceStoreMethods = { retainReplacedWorkerTerminalResources: typeof retainReplacedWorkerTerminalResources + rekeyWorkerTerminalResourcePaneKey: typeof rekeyWorkerTerminalResourcePaneKey backfillWorkerTerminalResources: typeof backfillWorkerTerminalResources createWorkerTerminalResourceStatement: typeof createWorkerTerminalResourceStatement getWorkerTerminalResource: typeof getWorkerTerminalResource @@ -262,6 +279,7 @@ export type WorkerTerminalResourceStoreMethods = { export function attachWorkerTerminalResourceStore(ctor: { prototype: object }): void { Object.assign(ctor.prototype, { retainReplacedWorkerTerminalResources, + rekeyWorkerTerminalResourcePaneKey, backfillWorkerTerminalResources, createWorkerTerminalResourceStatement, getWorkerTerminalResource, diff --git a/src/preload/api/pty-api.ts b/src/preload/api/pty-api.ts index 8a7604b4ac2..8a75d7fba5a 100644 --- a/src/preload/api/pty-api.ts +++ b/src/preload/api/pty-api.ts @@ -4,6 +4,10 @@ import type { } from '../../shared/agent-session-resume' import type { StartupCommandDelivery } from '../../shared/codex-startup-delivery' import type { TerminalInputKind } from '../../shared/terminal-input-kind' +import type { + TerminalLeafMoveRequest, + TerminalLeafMoveResult +} from '../../shared/terminal-leaf-move' import type { ProjectExecutionRuntimeResolution } from '../../shared/project-execution-runtime' import type { PtyListedSession, PtySessionListScope } from '../../shared/pty-listed-session' import type { PtyMainDeliveryDiagnostics } from '../../shared/pty-delivery-diagnostics' @@ -145,6 +149,7 @@ export type PtyApi = { ids: string[] ) => Promise<{ id: string; authoritative: boolean | null }[]> hasPty: (id: string) => Promise + moveLeafToNewTab: (request: TerminalLeafMoveRequest) => Promise getMainBufferSnapshot: ( id: string, opts?: { scrollbackRows?: number } diff --git a/src/preload/api/pty-bridge-session-control.ts b/src/preload/api/pty-bridge-session-control.ts index e407163e5f6..91fdb9a5fb6 100644 --- a/src/preload/api/pty-bridge-session-control.ts +++ b/src/preload/api/pty-bridge-session-control.ts @@ -2,6 +2,10 @@ import { ipcRenderer } from 'electron' import type { ProjectExecutionRuntimeResolution } from '../../shared/project-execution-runtime' import type { StartupCommandDelivery } from '../../shared/codex-startup-delivery' import type { TerminalInputKind } from '../../shared/terminal-input-kind' +import type { + TerminalLeafMoveRequest, + TerminalLeafMoveResult +} from '../../shared/terminal-leaf-move' import type { AgentProviderSessionMetadata, SleepingAgentLaunchConfig @@ -160,6 +164,8 @@ export const ptySessionControlApi = { ): Promise<{ id: string; authoritative: boolean | null }[]> => ipcRenderer.invoke('pty:getAuthoritativeBufferSnapshotCapabilities', { ids }), hasPty: (id: string): Promise => ipcRenderer.invoke('pty:hasPty', { id }), + moveLeafToNewTab: (request: TerminalLeafMoveRequest): Promise => + ipcRenderer.invoke('pty:moveLeafToNewTab', request), getMainBufferSnapshot: ( id: string, opts?: { scrollbackRows?: number } diff --git a/src/renderer/src/components/terminal-pane/pty-transport-connect-spawn.test.ts b/src/renderer/src/components/terminal-pane/pty-transport-connect-spawn.test.ts index 59502adcca6..42171d9368d 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-connect-spawn.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-connect-spawn.test.ts @@ -66,6 +66,28 @@ describe('createIpcPtyTransport', () => { transport.disconnect() }) + // A pane drag waits out a spawn whose result would bind the leaf to its old tab. + it('reports a connect as pending only until its PTY id arrives', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + let resolveSpawn!: (value: { id: string }) => void + vi.mocked(window.api.pty.spawn).mockReturnValue( + new Promise((resolve) => { + resolveSpawn = resolve + }) + ) + const transport = createIpcPtyTransport({}) + expect(transport.isConnectPending?.()).toBe(false) + + const connecting = transport.connect({ url: '', callbacks: {} }) + expect(transport.isConnectPending?.()).toBe(true) + resolveSpawn({ id: 'pty-late' }) + await connecting + + expect(transport.getPtyId()).toBe('pty-late') + expect(transport.isConnectPending?.()).toBe(false) + transport.disconnect() + }) + it('does not create a PTY when the pane generation is stale', async () => { const { createIpcPtyTransport } = await import('./pty-transport') const spawn = window.api.pty.spawn as unknown as ReturnType diff --git a/src/renderer/src/components/terminal-pane/pty-transport-types.ts b/src/renderer/src/components/terminal-pane/pty-transport-types.ts index 39f1c7962ef..4b2de188fce 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-types.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-types.ts @@ -223,6 +223,8 @@ export type PtyTransport = { /** The user dismissed the error surface; the next occurrence of the same message must surface again. */ notifyErrorSurfaceDismissed?: () => void getPtyId: () => string | null + /** A connect (spawn or reattach) is still awaiting its PTY id. */ + isConnectPending?: () => boolean getConnectionId?: () => string | null | undefined /** The runtime captured by this transport; legacy remote PTY ids do not * encode their owner, and current worktree settings may have changed. */ diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index d1b9242243e..9f9783d6c1d 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -49,6 +49,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra let onAbandonedConnect: ((ptyId: string) => boolean) | undefined let ptyId: string | null = null let lifecycleGeneration = 0 + let pendingConnectGeneration: number | null = null let lastExitGeneration: number | null = null let suppressAttentionEvents = false let storedCallbacks: Parameters[0]['callbacks'] = {} @@ -145,6 +146,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra getPendingEscapeTailAnsi: outputProcessor.getPendingEscapeTailAnsi, connect: async (options) => { const connectGeneration = advancePtyLifecycle() + pendingConnectGeneration = connectGeneration try { return await connectIpcPty(options, { transportOptions: opts, @@ -162,12 +164,18 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra getCallbacks: () => storedCallbacks }) } finally { + if (pendingConnectGeneration === connectGeneration) { + pendingConnectGeneration = null + } if (lifecycleGeneration === connectGeneration) { await flushPreconnectInput() } } }, + isConnectPending: () => + !destroyed && ptyId === null && pendingConnectGeneration === lifecycleGeneration, + attach: (options) => { const attachGeneration = advancePtyLifecycle() try { diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-commit.test.ts b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-commit.test.ts new file mode 100644 index 00000000000..8b83383572d --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-commit.test.ts @@ -0,0 +1,272 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { + TerminalLeafMoveRequest, + TerminalLeafMoveResult +} from '../../../../shared/terminal-leaf-move' +import { detachTerminalPaneToTab } from './terminal-pane-tab-detach' +import { + createStore, + LEAF_1, + LEAF_2, + SOURCE_TAB_ID, + splitLayout, + TARGET_GROUP_ID, + unboundSplitLayout, + WORKTREE_ID +} from './terminal-pane-tab-detach-fixture' + +const toastErrorMock = vi.hoisted(() => vi.fn()) +vi.mock('sonner', () => ({ toast: { error: toastErrorMock } })) +const closeTerminalSurface = vi.fn(async () => {}) +beforeEach(() => { + toastErrorMock.mockClear() + closeTerminalSurface.mockClear() + vi.spyOn(console, 'warn').mockImplementation(() => {}) +}) + +/** Answers each call with the next result; an Error throws. */ +function mainAnswering(results: (TerminalLeafMoveResult | Error)[]) { + return vi.fn((_request: TerminalLeafMoveRequest): Promise => { + const next = results.shift() ?? { status: 'not_held' } + return next instanceof Error ? Promise.reject(next) : Promise.resolve(next) + }) +} + +function managerWithPanes(paneIds: () => number[] = () => [1, 2]) { + return { + getPanes: vi.fn(() => paneIds().map((id) => ({ id }))), + getLeafId: vi.fn((paneId: number): string | null => (paneId === 2 ? LEAF_2 : LEAF_1)), + detachPaneForExternalMove: vi.fn(() => true) + } +} + +type CommitMove = (request: TerminalLeafMoveRequest) => Promise + +function detach( + overrides: Partial[0]> & { + commitMove?: CommitMove + store?: ReturnType + } +) { + const { commitMove = mainAnswering([]), store = createStore(), ...rest } = overrides + vi.stubGlobal('window', { + api: { pty: { moveLeafToNewTab: commitMove }, session: { closeTerminalSurface } } + }) + return detachTerminalPaneToTab({ + getStore: () => store, + manager: managerWithPanes(), + persistLayoutSnapshot: vi.fn(), + sourcePaneId: 2, + sourceTabId: SOURCE_TAB_ID, + targetGroupId: TARGET_GROUP_ID, + worktreeId: WORKTREE_ID, + ...rest + }) +} + +const moved: TerminalLeafMoveResult = { status: 'moved', ptyId: 'remote:env-1@@terminal-1' } + +describe('detachTerminalPaneToTab rolls a committed move forward', () => { + it('refuses, silently, a pane whose PTY spawn is still in flight', async () => { + const commitMove = mainAnswering([{ status: 'moved', ptyId: null }]) + const store = createStore(unboundSplitLayout()) + + await expect(detach({ commitMove, store, sourceConnectPending: true })).resolves.toBeNull() + + expect(commitMove).not.toHaveBeenCalled() + expect(store.createTab).not.toHaveBeenCalled() + expect(toastErrorMock).not.toHaveBeenCalled() + }) + + it('shows the failure toast and keeps the pane when main refuses or the commit throws', async () => { + for (const results of [ + [{ status: 'refused', reason: 'pty_mismatch' } as const], + [new Error('write failed')] + ]) { + toastErrorMock.mockClear() + const store = createStore() + const manager = managerWithPanes() + + await expect( + detach({ commitMove: mainAnswering(results), store, manager }) + ).resolves.toBeNull() + + expect(manager.detachPaneForExternalMove).not.toHaveBeenCalled() + expect(store.createTab).not.toHaveBeenCalled() + expect(toastErrorMock).toHaveBeenCalledOnce() + } + }) + + it('closes main’s new tab, without a toast, when the user closed the pane meanwhile', async () => { + let paneIds = [1, 2] + const manager = managerWithPanes(() => paneIds) + const commitMove = vi.fn( + async (_request: TerminalLeafMoveRequest): Promise => { + paneIds = [1] + manager.getLeafId.mockImplementation((paneId) => (paneId === 1 ? LEAF_1 : null)) + return moved + } + ) + const store = createStore() + + await expect(detach({ commitMove, manager, store })).resolves.toBeNull() + + const targetTabId = commitMove.mock.calls[0]?.[0]?.targetTabId + expect(closeTerminalSurface).toHaveBeenCalledWith({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: targetTabId }, + reason: 'cleanup' + }) + expect(store.createTab).not.toHaveBeenCalled() + expect(toastErrorMock).not.toHaveBeenCalled() + }) + + it('closes main’s new tab when the user closed the source tab meanwhile', async () => { + const store = createStore() + const commitMove = vi.fn( + async (_request: TerminalLeafMoveRequest): Promise => { + // A tab close drops its row and its layout. + store.tabsByWorktree[WORKTREE_ID] = [] + delete store.terminalLayoutsByTabId[SOURCE_TAB_ID] + return moved + } + ) + + await expect(detach({ commitMove, store })).resolves.toBeNull() + + expect(closeTerminalSurface).toHaveBeenCalledOnce() + expect(store.createTab).not.toHaveBeenCalled() + }) + + it('finds the pane by its leaf when its pane id changed meanwhile', async () => { + const manager = managerWithPanes(() => [1, 7]) + manager.getLeafId.mockImplementation((paneId) => (paneId === 1 ? LEAF_1 : LEAF_2)) + const commitMove = vi.fn( + async (_request: TerminalLeafMoveRequest): Promise => { + manager.getLeafId.mockImplementation((paneId) => + paneId === 1 ? LEAF_1 : paneId === 7 ? LEAF_2 : null + ) + return moved + } + ) + + await expect(detach({ commitMove, manager })).resolves.toMatchObject({ leafId: LEAF_2 }) + + expect(manager.detachPaneForExternalMove).toHaveBeenCalledWith(7) + }) + + it('closes main’s new tab when the pane manager refuses to detach the pane', async () => { + const manager = managerWithPanes() + manager.detachPaneForExternalMove.mockReturnValue(false) + const commitMove = mainAnswering([moved]) + const store = createStore() + + await expect(detach({ commitMove, manager, store })).resolves.toBeNull() + + expect(closeTerminalSurface).toHaveBeenCalledWith({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: commitMove.mock.calls[0]?.[0]?.targetTabId }, + reason: 'cleanup' + }) + expect(store.createTab).not.toHaveBeenCalled() + expect(store.setTabLayout).not.toHaveBeenCalled() + }) + + it('moves the last pane, after a sibling closed meanwhile, without killing its PTY', async () => { + const store = createStore() + let paneIds = [1, 2] + const manager = managerWithPanes(() => paneIds) + const commitMove = vi.fn(async (_request: TerminalLeafMoveRequest) => { + // Closing a pane persists the layout synchronously (onLayoutChanged), as the real close does. + paneIds = [2] + store.terminalLayoutsByTabId[SOURCE_TAB_ID] = { + root: { type: 'leaf', leafId: LEAF_2 }, + activeLeafId: LEAF_2, + expandedLeafId: null, + ptyIdsByLeafId: { [LEAF_2]: 'pty-right' } + } + return { status: 'moved', ptyId: 'pty-right' } satisfies TerminalLeafMoveResult + }) + + await expect(detach({ commitMove, manager, store })).resolves.toMatchObject({ + leafId: LEAF_2, + ptyId: 'pty-right' + }) + + expect(closeTerminalSurface).not.toHaveBeenCalled() + expect(manager.detachPaneForExternalMove).not.toHaveBeenCalled() + expect(store.createTab).toHaveBeenCalledOnce() + expect(store.syncPaneDetachPtyOwnership).toHaveBeenCalledWith( + expect.objectContaining({ detachedLeafId: LEAF_2, detachedPtyId: 'pty-right' }) + ) + expect(store.closeTab).toHaveBeenCalledWith( + SOURCE_TAB_ID, + expect.objectContaining({ localPtyTeardownOwnedExternally: true }) + ) + }) + + it('commits the move in main before the target tab exists, using the same tab id', async () => { + const store = createStore() + let release!: () => void + const commitMove = vi.fn( + (_request: TerminalLeafMoveRequest) => + new Promise((resolve) => { + release = () => resolve(moved) + }) + ) + + const detaching = detach({ commitMove, store }) + await vi.waitFor(() => expect(commitMove).toHaveBeenCalledOnce()) + expect(store.createTab).not.toHaveBeenCalled() + release() + await detaching + + const request = commitMove.mock.calls[0]?.[0] + expect(request).toEqual({ + worktreeId: WORKTREE_ID, + sourceTabId: SOURCE_TAB_ID, + targetTabId: expect.any(String), + leafId: LEAF_2, + ptyId: expect.any(String) + }) + expect(store.createTab).toHaveBeenCalledWith( + WORKTREE_ID, + TARGET_GROUP_ID, + 'powershell.exe', + expect.objectContaining({ id: request?.targetTabId, initialLeafId: LEAF_2 }) + ) + }) + + it('ignores a repeat drop of a pane whose move is still committing', async () => { + const store = createStore() + let release!: () => void + const commitMove = vi.fn( + (_request: TerminalLeafMoveRequest) => + new Promise((resolve) => { + release = () => resolve(moved) + }) + ) + + const first = detach({ commitMove, store }) + await vi.waitFor(() => expect(commitMove).toHaveBeenCalledOnce()) + await expect(detach({ commitMove, store })).resolves.toBeNull() + release() + + await expect(first).resolves.toMatchObject({ leafId: LEAF_2 }) + expect(commitMove).toHaveBeenCalledOnce() + expect(toastErrorMock).not.toHaveBeenCalled() + }) + + it('binds the moved tab’s layout to the live PTY, not the saved one', async () => { + const store = createStore(splitLayout()) + const saved = store.terminalLayoutsByTabId[SOURCE_TAB_ID]?.ptyIdsByLeafId?.[LEAF_2] + expect(saved).toBeTruthy() + + const result = await detach({ livePtyId: 'pty-respawned', store }) + + expect(result?.ptyId).toBe('pty-respawned') + expect(store.terminalLayoutsByTabId['tab-detached']?.ptyIdsByLeafId?.[LEAF_2]).toBe( + 'pty-respawned' + ) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-fixture.ts b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-fixture.ts new file mode 100644 index 00000000000..b7d69404a44 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach-fixture.ts @@ -0,0 +1,122 @@ +import { vi } from 'vitest' +import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/terminal-tab-types' +import type { TerminalPaneTabDetachStore } from './terminal-pane-tab-detach' + +export const WORKTREE_ID = 'repo-1::/worktree' +export const SOURCE_TAB_ID = 'tab-source' +export const TARGET_GROUP_ID = 'group-target' +export const EXISTING_TAB_1 = 'tab-existing-1' +export const EXISTING_TAB_2 = 'tab-existing-2' +export const LEAF_1 = '11111111-1111-4111-8111-111111111111' +export const LEAF_2 = '22222222-2222-4222-8222-222222222222' + +export function splitLayout(): TerminalLayoutSnapshot { + return { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: LEAF_1 }, + second: { type: 'leaf', leafId: LEAF_2 } + }, + activeLeafId: LEAF_2, + expandedLeafId: null, + ptyIdsByLeafId: { + [LEAF_1]: 'pty-left', + [LEAF_2]: 'remote:env-1@@terminal-1' + }, + buffersByLeafId: { + [LEAF_2]: 'remote-buffer' + }, + titlesByLeafId: { + [LEAF_2]: 'remote shell' + } + } +} + +export function unboundSplitLayout(): TerminalLayoutSnapshot { + return { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: LEAF_1 }, + second: { type: 'leaf', leafId: LEAF_2 } + }, + activeLeafId: LEAF_2, + expandedLeafId: null + } +} + +export function createTerminalTab( + id: string, + ptyId: string | null, + shellOverride?: string +): TerminalTab { + return { + id, + ptyId, + worktreeId: WORKTREE_ID, + title: 'Terminal 2', + defaultTitle: 'Terminal 2', + customTitle: null, + color: null, + sortOrder: 1, + createdAt: 1, + ...(shellOverride !== undefined ? { shellOverride } : {}) + } +} + +export function createStore( + layout: TerminalLayoutSnapshot = splitLayout(), + targetTabOrder: string[] = [EXISTING_TAB_1, EXISTING_TAB_2], + sourceShellOverride = 'powershell.exe' +): TerminalPaneTabDetachStore { + const store = { + closeTab: vi.fn(), + createTab: vi.fn((_worktreeId, _targetGroupId, _shellOverride, options) => { + const tab = createTerminalTab('tab-detached', options?.initialPtyId ?? null) + const group = store.groupsByWorktree[WORKTREE_ID]?.find( + (candidate) => candidate.id === TARGET_GROUP_ID + ) + if (group && !group.tabOrder.includes(tab.id)) { + group.tabOrder = [...group.tabOrder, tab.id] + } + return tab + }), + groupsByWorktree: { + [WORKTREE_ID]: [ + { + id: TARGET_GROUP_ID, + worktreeId: WORKTREE_ID, + activeTabId: targetTabOrder[0] ?? null, + tabOrder: targetTabOrder, + recentTabIds: [] + } + ] + }, + reorderUnifiedTabs: vi.fn((groupId: string, tabIds: string[]) => { + const group = store.groupsByWorktree[WORKTREE_ID]?.find( + (candidate) => candidate.id === groupId + ) + if (group) { + group.tabOrder = tabIds + } + }), + setActiveTab: vi.fn(), + setActiveTabType: vi.fn(), + setTabLayout: vi.fn((tabId: string, nextLayout: TerminalLayoutSnapshot | null) => { + if (nextLayout) { + store.terminalLayoutsByTabId[tabId] = nextLayout + } else { + delete store.terminalLayoutsByTabId[tabId] + } + }), + syncPaneDetachPtyOwnership: vi.fn(), + tabsByWorktree: { + [WORKTREE_ID]: [createTerminalTab(SOURCE_TAB_ID, 'pty-left', sourceShellOverride)] + }, + terminalLayoutsByTabId: { + [SOURCE_TAB_ID]: layout + } + } + return store as unknown as TerminalPaneTabDetachStore +} diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.test.ts b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.test.ts index 8586db21e41..75ef69692e8 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.test.ts @@ -1,19 +1,31 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { AppState } from '@/store' -import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/terminal-tab-types' import { detachTerminalPaneToTab, - resolveTerminalTabStripDropTarget, - type TerminalPaneTabDetachStore + resolveTerminalTabStripDropTarget } from './terminal-pane-tab-detach' +import { + createStore, + EXISTING_TAB_1, + EXISTING_TAB_2, + LEAF_1, + LEAF_2, + SOURCE_TAB_ID, + splitLayout, + TARGET_GROUP_ID, + unboundSplitLayout, + WORKTREE_ID +} from './terminal-pane-tab-detach-fixture' -const WORKTREE_ID = 'repo-1::/worktree' -const SOURCE_TAB_ID = 'tab-source' -const TARGET_GROUP_ID = 'group-target' -const EXISTING_TAB_1 = 'tab-existing-1' -const EXISTING_TAB_2 = 'tab-existing-2' -const LEAF_1 = '11111111-1111-4111-8111-111111111111' -const LEAF_2 = '22222222-2222-4222-8222-222222222222' +const toastErrorMock = vi.hoisted(() => vi.fn()) +vi.mock('sonner', () => ({ toast: { error: toastErrorMock } })) +beforeEach(() => { + toastErrorMock.mockClear() + // No local main holds these sessions, so each move stays in the renderer. + vi.stubGlobal('window', { + api: { pty: { moveLeafToNewTab: () => Promise.resolve({ status: 'not_held' }) } } + }) +}) function rect(args: { left: number; top: number; width: number; height: number }): DOMRect { return { @@ -26,115 +38,9 @@ function rect(args: { left: number; top: number; width: number; height: number } } as DOMRect } -function splitLayout(): TerminalLayoutSnapshot { - return { - root: { - type: 'split', - direction: 'vertical', - first: { type: 'leaf', leafId: LEAF_1 }, - second: { type: 'leaf', leafId: LEAF_2 } - }, - activeLeafId: LEAF_2, - expandedLeafId: null, - ptyIdsByLeafId: { - [LEAF_1]: 'pty-left', - [LEAF_2]: 'remote:env-1@@terminal-1' - }, - buffersByLeafId: { - [LEAF_2]: 'remote-buffer' - }, - titlesByLeafId: { - [LEAF_2]: 'remote shell' - } - } -} - -function unboundSplitLayout(): TerminalLayoutSnapshot { - return { - root: { - type: 'split', - direction: 'vertical', - first: { type: 'leaf', leafId: LEAF_1 }, - second: { type: 'leaf', leafId: LEAF_2 } - }, - activeLeafId: LEAF_2, - expandedLeafId: null - } -} - -function createTerminalTab(id: string, ptyId: string | null, shellOverride?: string): TerminalTab { - return { - id, - ptyId, - worktreeId: WORKTREE_ID, - title: 'Terminal 2', - defaultTitle: 'Terminal 2', - customTitle: null, - color: null, - sortOrder: 1, - createdAt: 1, - ...(shellOverride !== undefined ? { shellOverride } : {}) - } -} - -function createStore( - layout: TerminalLayoutSnapshot = splitLayout(), - targetTabOrder: string[] = [EXISTING_TAB_1, EXISTING_TAB_2], - sourceShellOverride = 'powershell.exe' -): TerminalPaneTabDetachStore { - const store = { - createTab: vi.fn((_worktreeId, _targetGroupId, _shellOverride, options) => { - const tab = createTerminalTab('tab-detached', options?.initialPtyId ?? null) - const group = store.groupsByWorktree[WORKTREE_ID]?.find( - (candidate) => candidate.id === TARGET_GROUP_ID - ) - if (group && !group.tabOrder.includes(tab.id)) { - group.tabOrder = [...group.tabOrder, tab.id] - } - return tab - }), - groupsByWorktree: { - [WORKTREE_ID]: [ - { - id: TARGET_GROUP_ID, - worktreeId: WORKTREE_ID, - activeTabId: targetTabOrder[0] ?? null, - tabOrder: targetTabOrder, - recentTabIds: [] - } - ] - }, - reorderUnifiedTabs: vi.fn((groupId: string, tabIds: string[]) => { - const group = store.groupsByWorktree[WORKTREE_ID]?.find( - (candidate) => candidate.id === groupId - ) - if (group) { - group.tabOrder = tabIds - } - }), - setActiveTab: vi.fn(), - setActiveTabType: vi.fn(), - setTabLayout: vi.fn((tabId: string, nextLayout: TerminalLayoutSnapshot | null) => { - if (nextLayout) { - store.terminalLayoutsByTabId[tabId] = nextLayout - } else { - delete store.terminalLayoutsByTabId[tabId] - } - }), - syncPaneDetachPtyOwnership: vi.fn(), - tabsByWorktree: { - [WORKTREE_ID]: [createTerminalTab(SOURCE_TAB_ID, 'pty-left', sourceShellOverride)] - }, - terminalLayoutsByTabId: { - [SOURCE_TAB_ID]: layout - } - } - return store as unknown as TerminalPaneTabDetachStore -} - type SourcePaneCwd = NonNullable[0]['sourcePaneCwd']> -function expectDeferredSplitDetachRejected(sourcePaneCwd: SourcePaneCwd): void { +async function expectDeferredSplitDetachRejected(sourcePaneCwd: SourcePaneCwd): Promise { const store = createStore(unboundSplitLayout()) const manager = { getPanes: vi.fn(() => [{ id: 1 }, { id: 2 }]), @@ -143,7 +49,7 @@ function expectDeferredSplitDetachRejected(sourcePaneCwd: SourcePaneCwd): void { } const persistLayoutSnapshot = vi.fn() - const result = detachTerminalPaneToTab({ + const result = await detachTerminalPaneToTab({ getStore: () => store, manager, persistLayoutSnapshot, @@ -271,9 +177,9 @@ describe('resolveTerminalTabStripDropTarget', () => { }) describe('detachTerminalPaneToTab', () => { - it.each([LEAF_1, LEAF_2])('moves chat mode only with its owning leaf %s', (chatLeafId) => { + it.each([LEAF_1, LEAF_2])('moves chat mode only with its owning leaf %s', async (chatLeafId) => { const store = createStore({ ...splitLayout(), chatLeafId }) - detachTerminalPaneToTab({ + await detachTerminalPaneToTab({ getStore: () => store, manager: { getPanes: () => [{ id: 1 }, { id: 2 }], @@ -296,7 +202,7 @@ describe('detachTerminalPaneToTab', () => { ) }) - it('creates a new terminal tab with the detached leaf layout and PTY id', () => { + it('creates a new terminal tab with the detached leaf layout and PTY id', async () => { const store = createStore() const manager = { getPanes: vi.fn(() => [{ id: 1 }, { id: 2 }]), @@ -305,7 +211,7 @@ describe('detachTerminalPaneToTab', () => { } const persistLayoutSnapshot = vi.fn() - const result = detachTerminalPaneToTab({ + const result = await detachTerminalPaneToTab({ manager, getStore: () => store, persistLayoutSnapshot, @@ -324,8 +230,10 @@ describe('detachTerminalPaneToTab', () => { expect(result?.ptyId).toBe('remote:env-1@@terminal-1') expect(manager.detachPaneForExternalMove).toHaveBeenCalledWith(2) expect(store.createTab).toHaveBeenCalledWith(WORKTREE_ID, TARGET_GROUP_ID, 'powershell.exe', { + id: expect.any(String), activate: true, initialPtyId: 'remote:env-1@@terminal-1', + initialLeafId: LEAF_2, recordInteraction: true }) expect(store.setTabLayout).toHaveBeenCalledWith(SOURCE_TAB_ID, { @@ -361,7 +269,7 @@ describe('detachTerminalPaneToTab', () => { it.each(['powershell.exe', 'wsl.exe'])( 'preserves the moved PTY shell override when the source uses %s', - (shellOverride) => { + async (shellOverride) => { const store = createStore(splitLayout(), [EXISTING_TAB_1], shellOverride) const manager = { getPanes: vi.fn(() => [{ id: 1 }, { id: 2 }]), @@ -369,7 +277,7 @@ describe('detachTerminalPaneToTab', () => { detachPaneForExternalMove: vi.fn(() => true) } - detachTerminalPaneToTab({ + await detachTerminalPaneToTab({ getStore: () => store, manager, persistLayoutSnapshot: vi.fn(), @@ -388,7 +296,7 @@ describe('detachTerminalPaneToTab', () => { } ) - it('syncs PTY ownership when the primary source pane is detached', () => { + it('syncs PTY ownership when the primary source pane is detached', async () => { const store = createStore() const manager = { getPanes: vi.fn(() => [{ id: 1 }, { id: 2 }]), @@ -396,7 +304,7 @@ describe('detachTerminalPaneToTab', () => { detachPaneForExternalMove: vi.fn(() => true) } - const result = detachTerminalPaneToTab({ + const result = await detachTerminalPaneToTab({ getStore: () => store, manager, persistLayoutSnapshot: vi.fn(), @@ -431,7 +339,7 @@ describe('detachTerminalPaneToTab', () => { }) }) - it('moves the detached tab into the requested group slot', () => { + it('moves the detached tab into the requested group slot', async () => { const store = createStore(splitLayout(), [EXISTING_TAB_1, EXISTING_TAB_2]) const manager = { getPanes: vi.fn(() => [{ id: 1 }, { id: 2 }]), @@ -439,7 +347,7 @@ describe('detachTerminalPaneToTab', () => { detachPaneForExternalMove: vi.fn(() => true) } - detachTerminalPaneToTab({ + await detachTerminalPaneToTab({ getStore: () => store, manager, persistLayoutSnapshot: vi.fn(), @@ -457,7 +365,7 @@ describe('detachTerminalPaneToTab', () => { ) }) - it('uses the live transport PTY id when the snapshot has not persisted it yet', () => { + it('uses the live transport PTY id when the snapshot has not persisted it yet', async () => { const store = createStore({ root: { type: 'split', @@ -474,8 +382,8 @@ describe('detachTerminalPaneToTab', () => { detachPaneForExternalMove: vi.fn(() => true) } - detachTerminalPaneToTab({ - fallbackPtyId: 'remote:env-2@@terminal-9', + await detachTerminalPaneToTab({ + livePtyId: 'remote:env-2@@terminal-9', getStore: () => store, manager, persistLayoutSnapshot: vi.fn(), @@ -499,30 +407,30 @@ describe('detachTerminalPaneToTab', () => { }) }) - it('rejects a deferred split while inherited cwd is pending', () => { - expectDeferredSplitDetachRejected({ + it('rejects a deferred split while inherited cwd is pending', async () => { + await expectDeferredSplitDetachRejected({ cwd: '/remote/repo', deferredSplitSpawn: true, pendingCwd: new Promise(() => {}) }) }) - it('rejects a pending cwd even when the deferred marker is absent', () => { - expectDeferredSplitDetachRejected({ + it('rejects a pending cwd even when the deferred marker is absent', async () => { + await expectDeferredSplitDetachRejected({ cwd: '/remote/repo', pendingCwd: new Promise(() => {}) }) }) - it('still rejects a deferred split after cwd resolves but before PTY bind', () => { - expectDeferredSplitDetachRejected({ + it('still rejects a deferred split after cwd resolves but before PTY bind', async () => { + await expectDeferredSplitDetachRejected({ cwd: '/remote/repo/packages/app', confirmed: false, deferredSplitSpawn: true }) }) - it('carries resolved cwd when detaching an unbound non-deferred pane', () => { + it('carries resolved cwd when detaching an unbound non-deferred pane', async () => { const store = createStore(unboundSplitLayout()) const manager = { getPanes: vi.fn(() => [{ id: 1 }, { id: 2 }]), @@ -530,7 +438,7 @@ describe('detachTerminalPaneToTab', () => { detachPaneForExternalMove: vi.fn(() => true) } - const result = detachTerminalPaneToTab({ + const result = await detachTerminalPaneToTab({ getStore: () => store, manager, persistLayoutSnapshot: vi.fn(), @@ -546,6 +454,7 @@ describe('detachTerminalPaneToTab', () => { expect(result?.ptyId).toBeNull() expect(store.createTab).toHaveBeenCalledWith(WORKTREE_ID, TARGET_GROUP_ID, 'powershell.exe', { + id: expect.any(String), activate: true, pendingActivationSpawn: true, recordInteraction: true, diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.ts b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.ts index 27c97538d2d..b71d435af53 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-tab-detach.ts @@ -1,5 +1,10 @@ +import { toast } from 'sonner' import type { AppState } from '@/store' +import { translate } from '@/i18n/i18n' +import { createBrowserUuid } from '@/lib/browser-uuid' import type { TerminalTab } from '../../../../shared/terminal-tab-types' +import type { TerminalLeafMoveRequest } from '../../../../shared/terminal-leaf-move' +import { commitTerminalSurfaceClose } from '@/store/terminals/terminal-surface-close-intent' import type { PaneCwdEntry } from './resolve-split-cwd' import { detachTerminalLayoutLeaf } from './terminal-layout-leaf-detach' export { @@ -10,6 +15,7 @@ export type { TerminalTabStripDropTarget } from './terminal-tab-strip-drop-targe export type TerminalPaneTabDetachStore = Pick< AppState, + | 'closeTab' | 'createTab' | 'groupsByWorktree' | 'reorderUnifiedTabs' @@ -36,23 +42,6 @@ export type DetachedTerminalPaneTab = { ptyId: string | null } -function withDetachedPtyFallback(args: { - leafId: string - ptyId: string | null - detachedLayout: NonNullable>['detachedLayout'] -}): NonNullable>['detachedLayout'] { - if (!args.ptyId || args.detachedLayout.ptyIdsByLeafId?.[args.leafId]) { - return args.detachedLayout - } - return { - ...args.detachedLayout, - ptyIdsByLeafId: { - ...args.detachedLayout.ptyIdsByLeafId, - [args.leafId]: args.ptyId - } - } -} - function moveCreatedTabToIndex(args: { groupId: string store: TerminalPaneTabDetachStore @@ -76,18 +65,53 @@ function moveCreatedTabToIndex(args: { args.store.reorderUnifiedTabs(args.groupId, nextOrder, { recordInteraction: false }) } -export function detachTerminalPaneToTab(args: { - fallbackPtyId?: string | null +function reportMoveFailed(): void { + toast.error(translate('terminal.paneMove.failed', "Couldn't move the pane to a new tab.")) +} + +type DetachTerminalPaneToTabArgs = { + /** The PTY the source pane's transport is attached to now; it outranks the saved layout. */ + livePtyId?: string | null getStore: () => TerminalPaneTabDetachStore manager: TerminalPaneTabDetachManager | null persistLayoutSnapshot: () => void sourcePaneId: number sourcePaneCwd?: SourcePaneCwd + /** The pane's transport is still awaiting its PTY id from a spawn or reattach. */ + sourceConnectPending?: boolean sourceTabId: string targetGroupId: string targetIndex?: number worktreeId: string -}): DetachedTerminalPaneTab | null { +} + +// Leaves whose move main is committing; a repeat drop meanwhile is a no-op. +const leavesMovingToNewTab = new Set() + +/** + * Moves a pane into a new tab. Main commits the move (leaf, binding and pane-keyed records) before + * the target tab exists here, so the moved pane's reattach never races a second owner (STA-9259). + * Once main has moved it, this window only rolls forward: it never asks main to put it back. + */ +export async function detachTerminalPaneToTab( + args: DetachTerminalPaneToTabArgs +): Promise { + const leafId = args.manager?.getLeafId(args.sourcePaneId) + if (!leafId || leavesMovingToNewTab.has(leafId)) { + return null + } + leavesMovingToNewTab.add(leafId) + try { + return await moveLeafToNewTab(args, leafId) + } finally { + leavesMovingToNewTab.delete(leafId) + } +} + +async function moveLeafToNewTab( + args: DetachTerminalPaneToTabArgs, + leafId: string +): Promise { const initialStore = args.getStore() const targetGroupExists = initialStore.groupsByWorktree[args.worktreeId]?.some( @@ -96,49 +120,83 @@ export function detachTerminalPaneToTab(args: { if (!args.manager || !targetGroupExists || args.manager.getPanes().length <= 1) { return null } - - const sourceLeafId = args.manager.getLeafId(args.sourcePaneId) - if (!sourceLeafId) { - return null - } - const persistedPtyId = - initialStore.terminalLayoutsByTabId[args.sourceTabId]?.ptyIdsByLeafId?.[sourceLeafId] + initialStore.terminalLayoutsByTabId[args.sourceTabId]?.ptyIdsByLeafId?.[leafId] const cwdDeferred = Boolean( args.sourcePaneCwd?.pendingCwd || args.sourcePaneCwd?.deferredSplitSpawn ) - if (cwdDeferred && !persistedPtyId && !args.fallbackPtyId) { + // Why: a spawn result landing after the move binds SOURCE:leaf, and that bind grafts the leaf + // back into the source tab beside its moved copy. + if ((cwdDeferred || args.sourceConnectPending) && !persistedPtyId && !args.livePtyId) { return null } args.persistLayoutSnapshot() - const store = args.getStore() - const detached = detachTerminalLayoutLeaf( - store.terminalLayoutsByTabId[args.sourceTabId], - sourceLeafId - ) - if (!detached) { - return null + const request: TerminalLeafMoveRequest = { + worktreeId: args.worktreeId, + sourceTabId: args.sourceTabId, + targetTabId: createBrowserUuid(), + leafId, + ptyId: args.livePtyId ?? persistedPtyId ?? null } - - const ptyId = detached.ptyId ?? args.fallbackPtyId ?? null - const detachedLayout = withDetachedPtyFallback({ - leafId: sourceLeafId, - ptyId, - detachedLayout: detached.detachedLayout + const answer = await window.api.pty.moveLeafToNewTab(request).catch((error: unknown) => { + console.warn('[terminal-pane-detach] main did not answer the move', error) + return null }) + if (answer?.status === 'moved') { + return applyMove(args, request, true, answer.ptyId) + } + if (answer?.status === 'not_held') { + return applyMove(args, request, false, request.ptyId) + } + // A failed write rolls main back; one whose outcome is unknown faults persistence, and the next + // load converges on whichever side main kept. + console.warn('[terminal-pane-detach] main did not move the pane', answer) + reportMoveFailed() + return null +} - // Why: remove the renderer pane only after the layout/PTY handoff has been - // computed; the close callback detaches listeners but must not kill the PTY. - if (!args.manager.detachPaneForExternalMove(args.sourcePaneId)) { +/** Applies the move here; the pane is found by its leaf, since its pane id may have changed. */ +function applyMove( + args: DetachTerminalPaneToTabArgs, + request: TerminalLeafMoveRequest, + mainMoved: boolean, + ptyId: string | null +): DetachedTerminalPaneTab | null { + const { leafId, sourceTabId, targetTabId, worktreeId } = request + const { manager } = args + const panes = manager?.getPanes() ?? [] + const paneId = panes.find((pane) => manager?.getLeafId(pane.id) === leafId)?.id + const sourceLayout = args.getStore().terminalLayoutsByTabId[sourceTabId] + // A sibling closed meanwhile, so the pane takes the whole source layout and the source tab closes. + const lastPane = panes.length === 1 + const split = lastPane ? null : detachTerminalLayoutLeaf(sourceLayout, leafId) + const movedLayout = lastPane ? sourceLayout : split?.detachedLayout + // Why: remove the renderer pane only after the layout handoff is computed; the close callback + // detaches listeners but must not kill the PTY. + if ( + !manager || + paneId === undefined || + !movedLayout || + (!lastPane && !manager.detachPaneForExternalMove(paneId)) + ) { + // Why: the pane or its tab is gone here (or would not detach), so the tab main moved it into + // holds nothing; close it the way any tab close reaches main. + if (mainMoved) { + commitTerminalSurfaceClose(worktreeId, { kind: 'tab', tabId: targetTabId }, 'cleanup') + } return null } + const detachedLayout = ptyId + ? { ...movedLayout, ptyIdsByLeafId: { ...movedLayout.ptyIdsByLeafId, [leafId]: ptyId } } + : movedLayout const latestStore = args.getStore() - const sourceShellOverride = latestStore.tabsByWorktree[args.worktreeId]?.find( - (candidate) => candidate.id === args.sourceTabId + const sourceShellOverride = latestStore.tabsByWorktree[worktreeId]?.find( + (candidate) => candidate.id === sourceTabId )?.shellOverride - const tab = latestStore.createTab(args.worktreeId, args.targetGroupId, sourceShellOverride, { + const tab = latestStore.createTab(worktreeId, args.targetGroupId, sourceShellOverride, { + id: targetTabId, activate: true, ...(detachedLayout.chatLeafId ? { viewMode: 'chat' as const } : {}), initialPtyId: ptyId ?? undefined, @@ -147,7 +205,7 @@ export function detachTerminalPaneToTab(args: { pendingActivationSpawn: true, ...(args.sourcePaneCwd?.cwd ? { startupCwd: args.sourcePaneCwd.cwd } : {}) } - : {}), + : { initialLeafId: leafId }), recordInteraction: true }) const afterCreateStore = args.getStore() @@ -156,19 +214,30 @@ export function detachTerminalPaneToTab(args: { store: afterCreateStore, tabId: tab.id, targetIndex: args.targetIndex, - worktreeId: args.worktreeId + worktreeId }) - afterCreateStore.setTabLayout(args.sourceTabId, detached.sourceLayout) + if (split) { + afterCreateStore.setTabLayout(sourceTabId, split.sourceLayout) + } afterCreateStore.setTabLayout(tab.id, detachedLayout) afterCreateStore.syncPaneDetachPtyOwnership({ - detachedLeafId: sourceLeafId, + detachedLeafId: leafId, detachedPtyId: ptyId, - sourceLayout: detached.sourceLayout, - sourceTabId: args.sourceTabId, + sourceLayout: split?.sourceLayout ?? { root: null, activeLeafId: null, expandedLeafId: null }, + sourceTabId, targetTabId: tab.id }) + if (!split) { + // The new tab reattaches the PTY, so the source tab's close must not kill it. + afterCreateStore.closeTab(sourceTabId, { + reason: 'cleanup', + recordInteraction: false, + captureRecentlyClosed: false, + localPtyTeardownOwnedExternally: true + }) + } afterCreateStore.setActiveTab(tab.id) - afterCreateStore.setActiveTabType('terminal', args.worktreeId) + afterCreateStore.setActiveTabType('terminal', worktreeId) - return { tab, leafId: sourceLeafId, ptyId } + return { tab, leafId, ptyId } } diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts index 308711892ed..efad698fca4 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts @@ -239,22 +239,23 @@ export function useTerminalPaneCloseActions(controller: TerminalPaneBindingContr if (!isTerminalTabStripDropTarget(target)) { return false } - const fallbackPtyId = paneTransportsRef.current.get(sourcePaneId)?.getPtyId() ?? null + const sourceTransport = paneTransportsRef.current.get(sourcePaneId) + const livePtyId = sourceTransport?.getPtyId() ?? null const sourcePaneCwd = paneCwdRef.current.get(sourcePaneId) - return ( - detachTerminalPaneToTab({ - fallbackPtyId, - getStore: useAppStore.getState, - manager: managerRef.current, - persistLayoutSnapshot, - sourcePaneId, - ...(sourcePaneCwd ? { sourcePaneCwd } : {}), - sourceTabId: tabId, - targetGroupId: target.groupId, - targetIndex: target.insertionIndex, - worktreeId - }) !== null - ) + void detachTerminalPaneToTab({ + livePtyId, + getStore: useAppStore.getState, + manager: managerRef.current, + persistLayoutSnapshot, + sourcePaneId, + ...(sourcePaneCwd ? { sourcePaneCwd } : {}), + sourceConnectPending: sourceTransport?.isConnectPending?.() ?? false, + sourceTabId: tabId, + targetGroupId: target.groupId, + targetIndex: target.insertionIndex, + worktreeId + }).catch((error) => console.warn('[terminal-pane-detach] move failed', error)) + return true }, // oxlint-disable-next-line react-hooks/exhaustive-deps -- Preserve the pre-split dependency contract. [persistLayoutSnapshot, tabId, worktreeId] diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index a8320eb8c96..92322d7db16 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -19105,6 +19105,9 @@ "confirmStopDescription": "This closes any open Codex sessions that share it, including ones outside Orca.", "cancel": "Cancel", "runs": "Runs" + }, + "paneMove": { + "failed": "Couldn't move the pane to a new tab." } }, "fileExplorer": { diff --git a/src/renderer/src/web/preload-api/web-terminal-api.ts b/src/renderer/src/web/preload-api/web-terminal-api.ts index 55c0ec364eb..fcd16fab175 100644 --- a/src/renderer/src/web/preload-api/web-terminal-api.ts +++ b/src/renderer/src/web/preload-api/web-terminal-api.ts @@ -50,6 +50,8 @@ export function createPtyApi(): NonNullable['pty']> { getAuthoritativeBufferSnapshotCapabilities: (ids) => Promise.resolve(ids.map((id) => ({ id, authoritative: false }))), hasPty: () => Promise.resolve(null), + // Why: no local main owns a paired client's session, so the move stays in this window. + moveLeafToNewTab: () => Promise.resolve({ status: 'not_held' }), getMainBufferSnapshot: () => Promise.resolve(null), // Why: remote-runtime PTYs skip local main (no side-effect source); renderer byte parsing stays authoritative. onSideEffect: () => noopUnsubscribe, diff --git a/src/shared/terminal-leaf-move.test.ts b/src/shared/terminal-leaf-move.test.ts new file mode 100644 index 00000000000..1319a70c6f9 --- /dev/null +++ b/src/shared/terminal-leaf-move.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'vitest' +import { isTerminalLeafMoveRequest, terminalLeafMovePaneKeys } from './terminal-leaf-move' + +const LEAF = '22222222-2222-4222-8222-222222222222' +const valid = { + worktreeId: 'repo-1::/tmp/wt', + sourceTabId: 'tab-source', + targetTabId: 'tab-target', + leafId: LEAF, + ptyId: null +} + +describe('isTerminalLeafMoveRequest', () => { + it('accepts a move between two tabs with a stable leaf id', () => { + expect(isTerminalLeafMoveRequest(valid)).toBe(true) + expect(isTerminalLeafMoveRequest({ ...valid, ptyId: 'pty-1' })).toBe(true) + }) + + it.each([ + ['the same tab', { targetTabId: 'tab-source' }], + ['a legacy numeric leaf id', { leafId: '2' }], + ['a source tab id that would split a pane key', { sourceTabId: 'tab:source' }], + ['a target tab id that would split a pane key', { targetTabId: 'tab:target' }], + ['an empty PTY id', { ptyId: '' }] + ])('rejects %s', (_name, patch) => { + expect(isTerminalLeafMoveRequest({ ...valid, ...patch })).toBe(false) + }) + + it('builds both pane keys of a valid move', () => { + expect(terminalLeafMovePaneKeys(valid)).toEqual({ + from: `tab-source:${LEAF}`, + to: `tab-target:${LEAF}` + }) + }) +}) diff --git a/src/shared/terminal-leaf-move.ts b/src/shared/terminal-leaf-move.ts new file mode 100644 index 00000000000..ddf3a823619 --- /dev/null +++ b/src/shared/terminal-leaf-move.ts @@ -0,0 +1,59 @@ +import { makePaneKey, type PaneKey } from './stable-pane-id' + +/** Detach one pane into a new tab; the leaf id and its terminal move with it. */ +export type TerminalLeafMoveRequest = { + worktreeId: string + sourceTabId: string + targetTabId: string + leafId: string + /** The PTY the renderer's pane is attached to now, when it has one. */ + ptyId: string | null +} + +export type TerminalLeafMoveResult = + /** Main moved the leaf and every pane-keyed record into the target tab in one durable write. */ + | { status: 'moved'; ptyId: string | null } + /** Main's session never held the leaf, so there is nothing to move there. */ + | { status: 'not_held' } + | { + status: 'refused' + reason: 'invalid_request' | 'target_tab_exists' | 'pty_mismatch' | 'leaf_in_other_tab' + } + +/** The moved pane's key before and after; only valid requests reach here. */ +export function terminalLeafMovePaneKeys(request: TerminalLeafMoveRequest): { + from: PaneKey + to: PaneKey +} { + return { + from: makePaneKey(request.sourceTabId, request.leafId), + to: makePaneKey(request.targetTabId, request.leafId) + } +} + +/** The one validation of a move request: shape, distinct tabs, and ids a pane key can carry. */ +export function isTerminalLeafMoveRequest(value: unknown): value is TerminalLeafMoveRequest { + if (!value || typeof value !== 'object') { + return false + } + const candidate: Record = { ...value } + const isId = (field: unknown): field is string => + typeof field === 'string' && field.length > 0 && field.length <= 512 + if ( + !isId(candidate.worktreeId) || + !isId(candidate.sourceTabId) || + !isId(candidate.targetTabId) || + !isId(candidate.leafId) || + !(candidate.ptyId === null || isId(candidate.ptyId)) || + candidate.sourceTabId === candidate.targetTabId + ) { + return false + } + try { + makePaneKey(candidate.sourceTabId, candidate.leafId) + makePaneKey(candidate.targetTabId, candidate.leafId) + return true + } catch { + return false + } +}