diff --git a/apps/desktop/src/window/DesktopWindow.ts b/apps/desktop/src/window/DesktopWindow.ts index 40788009d3b..cf0387ad10a 100644 --- a/apps/desktop/src/window/DesktopWindow.ts +++ b/apps/desktop/src/window/DesktopWindow.ts @@ -514,6 +514,19 @@ export const make = Effect.gen(function* () { } }); + // Electron's windowMenu close role owns CmdOrCtrl+W. Holding the + // close-terminal shortcut auto-repeats faster than the renderer can re-arm + // its guard after the last terminal closes, and a single leaked repeat + // closes the whole window (and on Windows/Linux, the app). Only deliberate, + // non-repeated presses may act. + window.webContents.on("before-input-event", (event, input) => { + if (input.type !== "keyDown" || !input.isAutoRepeat) return; + const modifier = environment.platform === "darwin" ? input.meta : input.control; + if (modifier && !input.alt && !input.shift && input.key.toLowerCase() === "w") { + event.preventDefault(); + } + }); + window.on("page-title-updated", (event) => { event.preventDefault(); window.setTitle(environment.displayName); diff --git a/apps/server/src/cli/server.ts b/apps/server/src/cli/server.ts index bc4b3f45706..0d0414b834b 100644 --- a/apps/server/src/cli/server.ts +++ b/apps/server/src/cli/server.ts @@ -5,6 +5,25 @@ import { ServerConfig, type StartupPresentation } from "../config.ts"; import { runServer } from "../server.ts"; import { type CliServerFlags, resolveServerConfig, sharedServerCommandFlags } from "./config.ts"; +/** + * Defects thrown while building the server layer can be swallowed by headless + * startup logging, which used to make `npx t3` exit cleanly with no output on a + * broken install. The entrypoint owns process-exit policy, so render whatever + * diagnosis the defect carries and make sure the exit code is non-zero. + */ +const hasDiagnostic = (defect: unknown): defect is { readonly diagnostic: string } => + typeof defect === "object" && + defect !== null && + typeof (defect as { diagnostic?: unknown }).diagnostic === "string"; + +const reportStartupDefect = (defect: unknown) => + Effect.sync(() => { + process.exitCode = 1; + if (hasDiagnostic(defect)) { + process.stderr.write(`${defect.diagnostic}\n`); + } + }); + export const runServerCommand = ( flags: CliServerFlags, options?: { @@ -15,7 +34,10 @@ export const runServerCommand = ( Effect.gen(function* () { const logLevel = yield* GlobalFlag.LogLevel; const config = yield* resolveServerConfig(flags, logLevel, options); - return yield* runServer.pipe(Effect.provideService(ServerConfig, config)); + return yield* runServer.pipe( + Effect.provideService(ServerConfig, config), + Effect.tapDefect(reportStartupDefect), + ); }); export const startCommand = Command.make("start", { ...sharedServerCommandFlags }).pipe( diff --git a/apps/server/src/terminal/Manager.test.ts b/apps/server/src/terminal/Manager.test.ts index 1cf7e8dffec..de4a80772cf 100644 --- a/apps/server/src/terminal/Manager.test.ts +++ b/apps/server/src/terminal/Manager.test.ts @@ -541,6 +541,33 @@ it.layer( }), ); + it.effect("orders the resize reply behind output queued at the old width", () => + Effect.gen(function* () { + const { manager, ptyAdapter, getEvents } = yield* createManager(); + yield* manager.open(openInput()); + const process = ptyAdapter.processes[0]; + expect(process).toBeDefined(); + if (!process) return; + + // Queued but not yet drained: produced at the pre-resize width. + process.emitData("old-width bytes\n"); + + yield* manager.resize({ + threadId: "thread-1", + terminalId: DEFAULT_TERMINAL_ID, + cols: 42, + rows: 20, + }); + + // By the time the resize reply lands, the old-width output must already + // be published — the client applies its grid on this reply. + const events = yield* getEvents; + const outputEvent = events.find((event) => event.type === "output"); + expect(outputEvent).toBeDefined(); + expect(process.resizeCalls).toEqual([{ cols: 42, rows: 20 }]); + }), + ); + it.effect("preserves structured context and causes for PTY I/O failures", () => Effect.gen(function* () { const { manager, ptyAdapter } = yield* createManager(); @@ -991,6 +1018,34 @@ it.layer( }), ); + it.effect("strips mode, version, and DCS query traffic that reopening would replay", () => + Effect.gen(function* () { + const { manager, ptyAdapter } = yield* createManager(); + yield* manager.open(openInput()); + const process = ptyAdapter.processes[0]; + expect(process).toBeDefined(); + if (!process) return; + + // The families behind the reopen gibberish from the field reports: + // DECRQM query + DECRPM replies, DECRQSS/XTGETTCAP query + reply, and + // XTVERSION/kitty-keyboard queries. + process.emitData("prompt "); + process.emitData("\u001b[?2026$p\u001b[?2026;2$y\u001b[?2027;0$y"); + process.emitData("\u001bP$q m\u001b\\\u001bP1$r0m\u001b\\"); + process.emitData("\u001bP+q544e\u001b\\\u001bP1+r544e=1b\u001b\\"); + process.emitData("\u001b[>q\u001b[?u\u001b[?31u"); + // Setters that share final bytes with the stripped queries must survive: + // DECSCUSR (cursor style) and restore-cursor. + process.emitData("\u001b[4 q\u001b[umid "); + process.emitData("done\n"); + + yield* manager.close({ threadId: "thread-1" }); + + const reopened = yield* manager.open(openInput()); + assert.equal(reopened.history, "prompt \u001b[4 q\u001b[umid done\n"); + }), + ); + it.effect( "preserves clear and style control sequences while dropping chunk-split query traffic", () => @@ -1260,6 +1315,29 @@ it.layer( }), ); + it.effect("does not retry other shells when spawn-helper is not executable", () => + Effect.gen(function* () { + const { manager, ptyAdapter, getEvents } = yield* createManager(); + ptyAdapter.spawnFailures.push( + new PtyAdapter.SpawnHelperNotExecutableError({ + helperPath: "/pkg/spawn-helper", + cause: new Error("posix_spawnp failed."), + }), + ); + + const snapshot = yield* manager.open(openInput()); + + assert.equal(snapshot.status, "error"); + expect(ptyAdapter.spawnInputs).toHaveLength(1); + + // The user must see the fix, not the generic "tried these shells" wrapper. + const events = yield* getEvents; + const errorEvent = events.find((event) => event.type === "error"); + assert.isDefined(errorEvent); + assert.include(errorEvent?.message ?? "", 'chmod +x "/pkg/spawn-helper"'); + }), + ); + it.effect("prefers PowerShell over ComSpec for Windows terminals", () => Effect.gen(function* () { const { manager, ptyAdapter } = yield* createManager(5, { diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index caa5106bb9f..7f0675036bb 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -40,6 +40,7 @@ import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; import * as Encoding from "effect/Encoding"; import * as Equal from "effect/Equal"; +import * as Deferred from "effect/Deferred"; import * as Exit from "effect/Exit"; import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; @@ -263,10 +264,26 @@ interface PersistHistoryRequest { type PendingProcessEvent = | { type: "output"; data: string } - | { type: "exit"; event: PtyAdapter.PtyExitEvent }; + | { type: "exit"; event: PtyAdapter.PtyExitEvent } + // Drain-ordering sentinel: completes once every event queued before it has + // been published. Lets resize replies order themselves behind old-width + // output without a marker in the wire protocol. + | { type: "flush"; deferred: Deferred.Deferred }; + +/** Queue wipes must release flush sentinels or their awaiters hang forever. */ +function discardPendingProcessEvents(session: TerminalSessionState): void { + for (const event of session.pendingProcessEvents) { + if (event.type === "flush") { + Deferred.doneUnsafe(event.deferred, Effect.void); + } + } + session.pendingProcessEvents = []; + session.pendingProcessEventIndex = 0; +} type DrainProcessEventAction = | { type: "idle" } + | { type: "flush"; deferred: Deferred.Deferred } | { type: "output"; threadId: string; @@ -565,6 +582,12 @@ function resolveShellCandidates( } function isRetryableShellSpawnError(error: PtyAdapter.PtySpawnError): boolean { + // A non-executable node-pty spawn-helper fails identically for every shell; + // retrying candidates would only bury the actionable error under fallbacks. + if (PtyAdapter.hasSpawnHelperNotExecutableCause(error)) { + return false; + } + const queue: unknown[] = [error]; const seen = new Set(); const messages: string[] = []; @@ -878,9 +901,31 @@ function shouldStripCsiSequence(body: string, finalByte: string): boolean { if (finalByte === "c" && /^[>0-9;?]*$/.test(body)) { return true; } + // DECRQM mode queries (…$p) and DECRPM replies (…$y): replaying a stored + // query makes the terminal answer again, and the shell echoes the answer as + // junk at the prompt. The `$` guard keeps setters like DECSTR (!p) and + // DECSCL ("p) intact. + if ((finalByte === "p" || finalByte === "y") && /^[0-9;?]*\$$/.test(body)) { + return true; + } + // XTVERSION query (>q). DECSCUSR (space-intermediate q) stays. + if (finalByte === "q" && /^>[0-9;]*$/.test(body)) { + return true; + } + // Kitty keyboard protocol query/reply (?u). Restore-cursor (bare u) stays. + if (finalByte === "u" && body.startsWith("?")) { + return true; + } return false; } +// DECRQSS ($q) and XTGETTCAP (+q) queries plus their replies ([01]$r / [01]+r): +// pure request/response traffic with no visual value, and replaying a stored +// query triggers a fresh reply. +function shouldStripDcsSequence(content: string): boolean { + return /^[01]?[$+][qr]/.test(content); +} + function shouldStripOscSequence(content: string): boolean { return /^(10|11|12);(?:\?|rgb:)/.test(content); } @@ -981,7 +1026,10 @@ function sanitizeTerminalHistoryChunk( } const sequence = input.slice(index, terminatorIndex); const content = stripStringTerminator(input.slice(index + 2, terminatorIndex)); - if (nextCodePoint !== 0x5d || !shouldStripOscSequence(content)) { + const strip = + (nextCodePoint === 0x5d && shouldStripOscSequence(content)) || + (nextCodePoint === 0x50 && shouldStripDcsSequence(content)); + if (!strip) { append(sequence); } index = terminatorIndex; @@ -1024,7 +1072,10 @@ function sanitizeTerminalHistoryChunk( } const sequence = input.slice(index, terminatorIndex); const content = stripStringTerminator(input.slice(index + 1, terminatorIndex)); - if (codePoint !== 0x9d || !shouldStripOscSequence(content)) { + const strip = + (codePoint === 0x9d && shouldStripOscSequence(content)) || + (codePoint === 0x90 && shouldStripDcsSequence(content)); + if (!strip) { append(sequence); } index = terminatorIndex; @@ -1650,7 +1701,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func while (true) { const action: DrainProcessEventAction = yield* Effect.sync(() => { if (session.pid !== expectedPid || !session.process || session.status !== "running") { - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; return { type: "idle" } as const; @@ -1658,7 +1709,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func const nextEvent = session.pendingProcessEvents[session.pendingProcessEventIndex]; if (!nextEvent) { - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; return { type: "idle" } as const; @@ -1666,10 +1717,14 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func session.pendingProcessEventIndex += 1; if (session.pendingProcessEventIndex >= session.pendingProcessEvents.length) { - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; } + if (nextEvent.type === "flush") { + return { type: "flush", deferred: nextEvent.deferred } as const; + } + if (nextEvent.type === "output") { const sanitized = sanitizeTerminalHistoryChunk( session.pendingHistoryControlSequence, @@ -1702,7 +1757,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func session.childCommandLabel = null; session.status = "exited"; session.pendingHistoryControlSequence = ""; - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; session.exitCode = Number.isInteger(nextEvent.event.exitCode) @@ -1728,6 +1783,11 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func return; } + if (action.type === "flush") { + yield* Deferred.succeed(action.deferred, undefined); + continue; + } + if (action.type === "output") { if (action.history !== null) { yield* queuePersist(action.threadId, action.terminalId, action.history); @@ -1774,7 +1834,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func session.childCommandLabel = null; session.status = "exited"; session.pendingHistoryControlSequence = ""; - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; session.updatedAt = updatedAt; @@ -1869,7 +1929,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func session.exitSignal = null; session.hasRunningSubprocess = false; session.childCommandLabel = null; - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; session.updatedAt = startingAt; @@ -1946,7 +2006,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func session.process = null; session.hasRunningSubprocess = false; session.childCommandLabel = null; - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; advanceEventSequence(session); @@ -1959,7 +2019,10 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func yield* evictInactiveSessionsIfNeeded(); - const message = error.message; + // The outer spawn error only says which shells were tried. When the real + // culprit is diagnosed further down the chain, publish that instead so the + // user sees the fix rather than a generic failure. + const message = (PtyAdapter.findSpawnHelperNotExecutableCause(error) ?? error).message; yield* publishEvent({ type: "error", threadId: session.threadId, @@ -2224,7 +2287,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func liveSession.runtimeEnv = nextRuntimeEnv; liveSession.history = ""; liveSession.pendingHistoryControlSequence = ""; - liveSession.pendingProcessEvents = []; + discardPendingProcessEvents(liveSession); liveSession.pendingProcessEventIndex = 0; liveSession.processEventDrainRunning = false; yield* persistHistory(liveSession.threadId, liveSession.terminalId, liveSession.history); @@ -2233,7 +2296,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func liveSession.worktreePath = nextWorktreePath; liveSession.history = ""; liveSession.pendingHistoryControlSequence = ""; - liveSession.pendingProcessEvents = []; + discardPendingProcessEvents(liveSession); liveSession.pendingProcessEventIndex = 0; liveSession.processEventDrainRunning = false; yield* persistHistory(liveSession.threadId, liveSession.terminalId, liveSession.history); @@ -2519,12 +2582,39 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func } const process = session.value.process; if (!process || session.value.status !== "running") { + // Still authoritative for the next spawn: restart falls back to + // session.cols/rows, and the client grid follows the acknowledged size. + session.value.cols = input.cols; + session.value.rows = input.rows; + session.value.updatedAt = yield* nowIso; return; } yield* resizePtyProcess(session.value, process, input.cols, input.rows); session.value.cols = input.cols; session.value.rows = input.rows; session.value.updatedAt = yield* nowIso; + + // The client applies its grid when this reply lands, so the reply must + // order itself behind every output byte queued before the resize — those + // were produced at the old width. A flush sentinel through the same drain + // queue provides that ordering without a wire-protocol change. A running + // drain counts even when the queue looks empty: dequeue compacts the queue + // before the dequeued chunk is published, and an ack in that window would + // arrive ahead of the old-width bytes. The sentinel is picked up when the + // drain loop re-enters, behind the in-flight publication. + const pid = session.value.pid; + if ( + pid !== null && + (session.value.pendingProcessEvents.length > 0 || session.value.processEventDrainRunning) + ) { + const flushed = yield* Deferred.make(); + session.value.pendingProcessEvents.push({ type: "flush", deferred: flushed }); + if (!session.value.processEventDrainRunning) { + session.value.processEventDrainRunning = true; + runFork(drainProcessEvents(session.value, pid)); + } + yield* Deferred.await(flushed); + } }); const resize: TerminalManager["Service"]["resize"] = (input) => @@ -2538,7 +2628,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func const session = yield* requireSession(input.threadId, terminalId); session.history = ""; session.pendingHistoryControlSequence = ""; - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; const eventStamp = advanceEventSequence(session); @@ -2611,7 +2701,7 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func session.history = ""; session.pendingHistoryControlSequence = ""; - session.pendingProcessEvents = []; + discardPendingProcessEvents(session); session.pendingProcessEventIndex = 0; session.processEventDrainRunning = false; yield* persistHistory(input.threadId, terminalId, session.history); diff --git a/apps/server/src/terminal/NodePtyAdapter.test.ts b/apps/server/src/terminal/NodePtyAdapter.test.ts index ed87440d499..93c4178bbea 100644 --- a/apps/server/src/terminal/NodePtyAdapter.test.ts +++ b/apps/server/src/terminal/NodePtyAdapter.test.ts @@ -75,6 +75,11 @@ it.effect("reports native module load failures as structured startup defects", ( architecture: "x64", }); assert.equal(error.message, "Failed to load node-pty for win32-x64."); + // The CLI entrypoint renders this; a broken install must never exit + // cleanly with no output. + assert.include(error.diagnostic, "Failed to load node-pty for win32-x64."); + assert.include(error.diagnostic, "native binding could not be loaded"); + assert.include(error.diagnostic, "reinstall t3"); } }).pipe( Effect.provide( @@ -86,3 +91,157 @@ it.effect("reports native module load failures as structured startup defects", ( ), ), ); + +it("resolves helper executability against the calling process, not any exec bit", () => { + const owned = { ownerUid: 501, ownerGid: 20 }; + + // Owner-only binary: executable for its owner, not for anyone else. + assert.isTrue( + NodePtyAdapter.modeIsExecutableFor({ + mode: 0o100, + ...owned, + processUid: 501, + processGids: [20], + }), + ); + assert.isFalse( + NodePtyAdapter.modeIsExecutableFor({ + mode: 0o100, + ...owned, + processUid: 502, + processGids: [21], + }), + ); + + // Group and other bits are honoured independently of the owner bit. + assert.isTrue( + NodePtyAdapter.modeIsExecutableFor({ + mode: 0o010, + ...owned, + processUid: 502, + processGids: [20], + }), + ); + assert.isFalse( + NodePtyAdapter.modeIsExecutableFor({ + mode: 0o600, + ...owned, + processUid: 501, + processGids: [20], + }), + ); + assert.isTrue( + NodePtyAdapter.modeIsExecutableFor({ + mode: 0o001, + ...owned, + processUid: 502, + processGids: [21], + }), + ); + + // Root and unknown identities fall back to "somebody can execute this". + assert.isTrue( + NodePtyAdapter.modeIsExecutableFor({ mode: 0o100, ...owned, processUid: 0, processGids: [0] }), + ); + assert.isFalse( + NodePtyAdapter.modeIsExecutableFor({ mode: 0o644, ...owned, processUid: 0, processGids: [0] }), + ); + assert.isTrue( + NodePtyAdapter.modeIsExecutableFor({ + mode: 0o100, + ownerUid: null, + ownerGid: null, + processUid: null, + processGids: [], + }), + ); + + // The reported bug: a 0644 helper is never executable for anyone. + assert.isFalse( + NodePtyAdapter.modeIsExecutableFor({ + mode: 0o644, + ...owned, + processUid: 501, + processGids: [20], + }), + ); +}); + +it("leaves non-posix_spawnp failures untouched", () => { + const cause = new Error("cwd does not exist"); + const described = NodePtyAdapter.describeSpawnFailure({ + cause, + platform: "darwin", + helperPath: "/pkg/node-pty/build/Release/spawn-helper", + helperIsExecutable: false, + }); + assert.equal(described, cause); +}); + +it("explains posix_spawnp failures caused by a non-executable spawn-helper", () => { + const cause = new Error("posix_spawnp failed."); + const described = NodePtyAdapter.describeSpawnFailure({ + cause, + platform: "darwin", + helperPath: "/pkg/node-pty/build/Release/spawn-helper", + helperIsExecutable: false, + }); + assert.instanceOf(described, PtyAdapter.SpawnHelperNotExecutableError); + const error = described as PtyAdapter.SpawnHelperNotExecutableError; + assert.equal(error.helperPath, "/pkg/node-pty/build/Release/spawn-helper"); + assert.include(error.message, 'chmod +x "/pkg/node-pty/build/Release/spawn-helper"'); + assert.equal(error.cause, cause); + // The terminal manager keys its retry decision off the tag, not the wording. + assert.isTrue(PtyAdapter.hasSpawnHelperNotExecutableCause(error)); + assert.isTrue( + PtyAdapter.hasSpawnHelperNotExecutableCause( + new PtyAdapter.PtySpawnError({ adapter: "node-pty", shell: "/bin/zsh", cause: error }), + ), + ); + assert.isFalse(PtyAdapter.hasSpawnHelperNotExecutableCause(cause)); +}); + +it("keeps posix_spawnp failures as-is when the helper is executable or missing", () => { + const cause = new Error("posix_spawnp failed."); + assert.equal( + NodePtyAdapter.describeSpawnFailure({ + cause, + platform: "darwin", + helperPath: "/pkg/node-pty/build/Release/spawn-helper", + helperIsExecutable: true, + }), + cause, + ); + assert.equal( + NodePtyAdapter.describeSpawnFailure({ + cause, + platform: "darwin", + helperPath: null, + helperIsExecutable: false, + }), + cause, + ); + assert.equal( + NodePtyAdapter.describeSpawnFailure({ + cause, + platform: "win32", + helperPath: "/pkg/node-pty/build/Release/spawn-helper", + helperIsExecutable: false, + }), + cause, + ); +}); + +it("finds posix_spawnp mentions through nested error causes", () => { + const nested = new Error("outer wrapper", { + cause: new Error("posix_spawnp failed."), + }); + const described = NodePtyAdapter.describeSpawnFailure({ + cause: nested, + platform: "darwin", + helperPath: "/pkg/spawn-helper", + helperIsExecutable: false, + }); + assert.instanceOf(described, Error); + assert.include((described as Error).message, "chmod +x"); +}); diff --git a/apps/server/src/terminal/NodePtyAdapter.ts b/apps/server/src/terminal/NodePtyAdapter.ts index ac06e1edfab..388f4961b85 100644 --- a/apps/server/src/terminal/NodePtyAdapter.ts +++ b/apps/server/src/terminal/NodePtyAdapter.ts @@ -3,6 +3,7 @@ import * as NodeModule from "node:module"; import * as Effect from "effect/Effect"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; +import * as Option from "effect/Option"; import * as Path from "effect/Path"; import * as Schema from "effect/Schema"; import { HostProcessArchitecture, HostProcessPlatform } from "@t3tools/shared/hostProcess"; @@ -20,6 +21,28 @@ export class NodePtyModuleLoadError extends Schema.TaggedErrorClass Promise; @@ -49,24 +72,142 @@ const resolveNodePtySpawnHelperPath = Effect.gen(function* () { return null; }).pipe(Effect.orElseSucceed(() => null)); +/** + * Whether `mode` grants execute permission to the identified process. + * + * Effect's `FileSystem.access` cannot test `X_OK`, so executability is derived + * from the mode against the caller's identity: a bare `mode & 0o111` would call + * an owner-only binary (`0o100`) executable for every other user, skipping the + * repair and suppressing the diagnosis for exactly the users who need it. + */ +export const modeIsExecutableFor = (input: { + mode: number; + ownerUid: number | null; + ownerGid: number | null; + processUid: number | null; + processGids: readonly number[]; +}): boolean => { + const anyExecuteBit = (input.mode & 0o111) !== 0; + // Unknown identity, or root, which bypasses the permission bits entirely. + if (input.processUid === null || input.processUid === 0) return anyExecuteBit; + if (input.ownerUid !== null && input.ownerUid === input.processUid) { + return (input.mode & 0o100) !== 0; + } + if (input.ownerGid !== null && input.processGids.includes(input.ownerGid)) { + return (input.mode & 0o010) !== 0; + } + return (input.mode & 0o001) !== 0; +}; + +const currentProcessIdentity = (): { + processUid: number | null; + processGids: readonly number[]; +} => { + const processUid = process.getuid?.() ?? null; + const primaryGid = process.getgid?.() ?? null; + // getgroups() omits the primary gid on some platforms, so add it explicitly. + const supplementaryGids = process.getgroups?.() ?? []; + return { + processUid, + processGids: primaryGid === null ? supplementaryGids : [primaryGid, ...supplementaryGids], + }; +}; + +/** + * Whether this process can execute the helper, or `null` when the mode cannot + * be read — some packaged modes hide fs metadata, and "unknown" means different + * things to the repair path (try anyway) and the diagnosis path (do not blame). + */ +const readSpawnHelperExecutable = Effect.fn(function* (helperPath: string) { + const fs = yield* FileSystem.FileSystem; + const identity = currentProcessIdentity(); + return yield* fs.stat(helperPath).pipe( + Effect.map((info) => + modeIsExecutableFor({ + mode: info.mode, + ownerUid: Option.getOrNull(info.uid), + ownerGid: Option.getOrNull(info.gid), + ...identity, + }), + ), + Effect.orElseSucceed(() => null), + ); +}); + const ensureNodePtySpawnHelperExecutable = Effect.fn(function* () { const fs = yield* FileSystem.FileSystem; const platform = yield* HostProcessPlatform; if (platform === "win32") return; if (didEnsureSpawnHelperExecutable) return; + // Resolution can fail transiently (and is folded into null), so leave the + // flag unset and let the next spawn rescan instead of giving up for good. const helperPath = yield* resolveNodePtySpawnHelperPath; if (!helperPath) return; - didEnsureSpawnHelperExecutable = true; - if (!(yield* fs.exists(helperPath))) { + // Nothing to repair. Skipping the chmod also keeps a read-only package store + // (where spawning works fine) from failing and re-logging on every spawn. + if ((yield* readSpawnHelperExecutable(helperPath)) === true) { + didEnsureSpawnHelperExecutable = true; return; } - // Best-effort: avoid FileSystem.stat in packaged mode where some fs metadata can be missing. - yield* fs.chmod(helperPath, 0o755).pipe(Effect.orElseSucceed(() => undefined)); + // npm can extract the package without the exec bit on spawn-helper, which + // then surfaces as "posix_spawnp failed" for every shell. + const chmodResult = yield* Effect.result(fs.chmod(helperPath, 0o755)); + if (chmodResult._tag === "Success") { + didEnsureSpawnHelperExecutable = true; + return; + } + // Leave the flag unset so the next spawn retries; a transient failure (e.g. a + // fleeting read-only mount) should not disable the repair forever. + yield* Effect.logWarning("failed to mark node-pty spawn-helper executable", { + helperPath, + error: chmodResult.failure, + remedy: `chmod +x "${helperPath}"`, + }); }); +const causeMentionsPosixSpawnFailure = (cause: unknown): boolean => { + let current: unknown = cause; + const seen = new Set(); + while (current !== null && current !== undefined && !seen.has(current)) { + seen.add(current); + if (typeof current === "string") { + return current.toLowerCase().includes("posix_spawnp failed"); + } + if (current instanceof Error) { + if (current.message.toLowerCase().includes("posix_spawnp failed")) { + return true; + } + current = current.cause; + continue; + } + return false; + } + return false; +}; + +/** + * Diagnoses a spawn failure whose real culprit is a non-executable spawn-helper. + * Without this, the shell-candidate fallback in the terminal manager retries + * every shell and reports the failure as if no working shell existed. + */ +export const describeSpawnFailure = (input: { + cause: unknown; + platform: string; + helperPath: string | null; + helperIsExecutable: boolean; +}): unknown => { + if (input.platform === "win32") return input.cause; + if (!causeMentionsPosixSpawnFailure(input.cause)) return input.cause; + if (input.helperPath === null || input.helperIsExecutable) return input.cause; + return new PtyAdapter.SpawnHelperNotExecutableError({ + helperPath: input.helperPath, + cause: input.cause, + }); +}; + class NodePtyProcess implements PtyAdapter.PtyProcess { private readonly process: import("node-pty").IPty; @@ -126,38 +267,71 @@ export const make = Effect.fn("NodePtyAdapter.make")(function* ( architecture, cause, }), + // Rendering the diagnostic and choosing an exit code is the CLI + // entrypoint's job — see `reportStartupDefect` in cli/server.ts. }).pipe(Effect.orDie); - const ensureNodePtySpawnHelperExecutableCached = yield* Effect.cached( - ensureNodePtySpawnHelperExecutable().pipe( - Effect.provideService(FileSystem.FileSystem, fs), - Effect.provideService(Path.Path, path), - Effect.provideService(HostProcessPlatform, platform), - Effect.provideService(HostProcessArchitecture, architecture), - Effect.orElseSucceed(() => undefined), - ), + const ensureSpawnHelperExecutable = ensureNodePtySpawnHelperExecutable().pipe( + Effect.provideService(FileSystem.FileSystem, fs), + Effect.provideService(Path.Path, path), + Effect.provideService(HostProcessPlatform, platform), + Effect.provideService(HostProcessArchitecture, architecture), ); + const resolveSpawnHelperPath = resolveNodePtySpawnHelperPath.pipe( + Effect.provideService(FileSystem.FileSystem, fs), + Effect.provideService(Path.Path, path), + Effect.provideService(HostProcessPlatform, platform), + Effect.provideService(HostProcessArchitecture, architecture), + ); + const spawnHelperIsExecutable = (helperPath: string) => + readSpawnHelperExecutable(helperPath).pipe( + // Unknown metadata must not point users at a chmod that may not help. + Effect.map((isExecutable) => isExecutable !== false), + Effect.provideService(FileSystem.FileSystem, fs), + ); return PtyAdapter.PtyAdapter.of({ spawn: Effect.fn("NodePtyAdapter.spawn")(function* (input) { - yield* ensureNodePtySpawnHelperExecutableCached; - const ptyProcess = yield* Effect.try({ - try: () => - nodePty.spawn(input.shell, input.args ?? [], { - cwd: input.cwd, - cols: input.cols, - rows: input.rows, - env: input.env, - name: platform === "win32" ? "xterm-color" : "xterm-256color", - }), - catch: (cause) => - new PtyAdapter.PtySpawnError({ - adapter: "node-pty", - shell: input.shell, - cause, - }), + yield* ensureSpawnHelperExecutable; + const attempt = yield* Effect.result( + Effect.try({ + try: () => + nodePty.spawn(input.shell, input.args ?? [], { + cwd: input.cwd, + cols: input.cols, + rows: input.rows, + env: input.env, + name: platform === "win32" ? "xterm-color" : "xterm-256color", + }), + catch: (cause) => + new PtyAdapter.PtySpawnError({ + adapter: "node-pty", + shell: input.shell, + cause, + }), + }), + ); + if (attempt._tag === "Success") { + return new NodePtyProcess(attempt.success); + } + + const spawnCause = attempt.failure.cause; + if (platform === "win32" || !causeMentionsPosixSpawnFailure(spawnCause)) { + return yield* attempt.failure; + } + const helperPath = yield* resolveSpawnHelperPath; + const helperIsExecutable = + helperPath === null ? true : yield* spawnHelperIsExecutable(helperPath); + return yield* new PtyAdapter.PtySpawnError({ + adapter: "node-pty", + shell: input.shell, + cause: describeSpawnFailure({ + cause: spawnCause, + platform, + helperPath, + helperIsExecutable, + }), }); - return new NodePtyProcess(ptyProcess); }), }); }); diff --git a/apps/server/src/terminal/PtyAdapter.ts b/apps/server/src/terminal/PtyAdapter.ts index 67147035bb5..6e55836003a 100644 --- a/apps/server/src/terminal/PtyAdapter.ts +++ b/apps/server/src/terminal/PtyAdapter.ts @@ -29,6 +29,44 @@ export class PtySpawnError extends Schema.TaggedErrorClass()("Pty } } +/** + * A spawn failure whose real culprit is node-pty's `spawn-helper` missing its + * exec bit. It fails identically for every shell, so the terminal manager must + * not bury it under the shell-candidate fallback. + */ +export class SpawnHelperNotExecutableError extends Schema.TaggedErrorClass()( + "SpawnHelperNotExecutableError", + { + helperPath: Schema.String, + // Always the originating spawn failure, so the error chain is never lost. + cause: Schema.Defect(), + }, +) { + override get message(): string { + return `node-pty's spawn-helper at ${this.helperPath} is not executable, so every shell fails with "posix_spawnp failed". Fix it with: chmod +x "${this.helperPath}"`; + } +} + +const isSpawnHelperNotExecutableError = Schema.is(SpawnHelperNotExecutableError); + +/** Walks the cause chain so wrapping a diagnosed failure never hides the tag. */ +export const findSpawnHelperNotExecutableCause = ( + error: unknown, +): SpawnHelperNotExecutableError | null => { + let current: unknown = error; + const seen = new Set(); + while (current !== null && current !== undefined && !seen.has(current)) { + seen.add(current); + if (isSpawnHelperNotExecutableError(current)) return current; + if (typeof current !== "object") return null; + current = (current as { cause?: unknown }).cause; + } + return null; +}; + +export const hasSpawnHelperNotExecutableCause = (error: unknown): boolean => + findSpawnHelperNotExecutableCause(error) !== null; + export interface PtyExitEvent { exitCode: number; signal: number | null; diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 2b9eda1a787..d59ba5300f5 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -53,6 +53,7 @@ import { useMemo, useRef, useState, + useSyncExternalStore, } from "react"; import { flushSync } from "react-dom"; import { useNavigate } from "@tanstack/react-router"; @@ -122,6 +123,7 @@ import { selectActiveRightPanelSurface, selectThreadRightPanelState, type RightPanelSurface, + type TerminalSurfaceSnapshot, useRightPanelStore, } from "../rightPanelStore"; import { @@ -141,7 +143,11 @@ import { import { RightPanelTabs } from "./RightPanelTabs"; import { DiffWorkerPoolProvider } from "./DiffWorkerPoolProvider"; import { BranchToolbar } from "./BranchToolbar"; -import { resolveShortcutCommand, shortcutLabelForCommand } from "../keybindings"; +import { + isTerminalCloseShortcut, + resolveShortcutCommand, + shortcutLabelForCommand, +} from "../keybindings"; import PlanSidebar from "./PlanSidebar"; import ThreadTerminalDrawer from "./ThreadTerminalDrawer"; import { @@ -197,8 +203,23 @@ import { import { appendPreviewAnnotationPrompt } from "../lib/previewAnnotation"; import { appendReviewCommentsToPrompt, type ReviewCommentContext } from "../reviewCommentContext"; import { environmentCatalog } from "../connection/catalog"; +import { + releaseTerminalOpen, + reservedTerminalOpenIds, + reserveTerminalOpen, +} from "../lib/terminalOpenReservations"; +import { + pendingPanelCloseVersion, + recordPendingPanelClose, + resolvePendingPanelCloses, + subscribePendingPanelCloses, +} from "../lib/terminalPendingPanelCloses"; import { selectThreadTerminalUiState, useTerminalUiStateStore } from "../terminalUiStateStore"; -import { useKnownTerminalSessions, useThreadRunningTerminalIds } from "../state/terminalSessions"; +import { + useKnownTerminalSessions, + useTerminalMetadataLoaded, + useThreadRunningTerminalIds, +} from "../state/terminalSessions"; import { projectEnvironment } from "../state/projects"; import { useEnvironmentQuery } from "../state/query"; import { @@ -557,45 +578,6 @@ function useLocalDispatchState(input: { }; } -/** Same terminal ids (order ignored) — avoids reconcile when only server session ordering differs. */ -function terminalIdListsEqual(left: readonly string[], right: readonly string[]): boolean { - if (left.length !== right.length) { - return false; - } - if (left.length === 0) { - return true; - } - const sortedLeft = left.toSorted((a, b) => a.localeCompare(b)); - const sortedRight = right.toSorted((a, b) => a.localeCompare(b)); - for (let index = 0; index < sortedLeft.length; index += 1) { - if (sortedLeft[index] !== sortedRight[index]) { - return false; - } - } - return true; -} - -/** - * Server knows about fewer sessions than the client, but every server id still exists locally. - * Typical right after `terminal.open`: known-session list lags; reconciling would drop the new id - * and later re-add it as a separate group (no split layout). - */ -function serverTerminalIdsStrictSubsetOfClient( - serverIds: readonly string[], - clientIds: readonly string[], -): boolean { - if (serverIds.length >= clientIds.length || clientIds.length === 0) { - return false; - } - const clientSet = new Set(clientIds); - for (const id of serverIds) { - if (!clientSet.has(id)) { - return false; - } - } - return true; -} - interface PersistentThreadTerminalDrawerProps { threadRef: { environmentId: EnvironmentId; threadId: ThreadId }; threadId: ThreadId; @@ -707,6 +689,17 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra // Every client-side id source participates in allocation: the server list // lags fresh opens, and panel terminals are filtered out of the drawer's // sessions — an id collision attaches two viewports to one PTY session. + // Suppression and pending membership feed the store's reconcile decision and + // id allocation, so subscribe to both — a failed close rolls back suppression + // without any server-list change, and the unsuppressed terminal must + // resurface without waiting for one. + const threadKey = scopedThreadKey(threadRef); + const suppressedTerminalIds = useTerminalUiStateStore( + (state) => state.suppressedTerminalIdsByThreadKey[threadKey], + ); + const pendingTerminalIds = useTerminalUiStateStore( + (state) => state.pendingTerminalIdsByThreadKey[threadKey], + ); const allocatableTerminalIds = useMemo( () => [ ...new Set([ @@ -725,19 +718,23 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const storeNewTerminal = useTerminalUiStateStore((state) => state.newTerminal); const storeSetActiveTerminal = useTerminalUiStateStore((state) => state.setActiveTerminal); const storeCloseTerminal = useTerminalUiStateStore((state) => state.closeTerminal); + const storeUnsuppressTerminal = useTerminalUiStateStore((state) => state.unsuppressTerminal); + const storeAbandonPendingTerminal = useTerminalUiStateStore( + (state) => state.abandonPendingTerminal, + ); const reconcileTerminalIds = useTerminalUiStateStore((state) => state.reconcileTerminalIds); + // The store owns the merge: it holds the pending set that separates a local + // open the server has not registered yet from a session that ended remotely. useEffect(() => { - if (terminalIdListsEqual(serverOrderedTerminalIds, terminalUiState.terminalIds)) { - return; - } - if ( - serverTerminalIdsStrictSubsetOfClient(serverOrderedTerminalIds, terminalUiState.terminalIds) - ) { - return; - } reconcileTerminalIds(threadRef, serverOrderedTerminalIds); - }, [reconcileTerminalIds, serverOrderedTerminalIds, terminalUiState.terminalIds, threadRef]); + }, [ + pendingTerminalIds, + reconcileTerminalIds, + serverOrderedTerminalIds, + suppressedTerminalIds, + threadRef, + ]); const [localFocusRequestId, setLocalFocusRequestId] = useState(0); const worktreePath = serverThread?.worktreePath ?? draftThread?.worktreePath ?? null; const effectiveWorktreePath = useMemo(() => { @@ -782,60 +779,82 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra [storeSetTerminalHeight, threadRef], ); + // A settled-but-not-successful open must not leave the optimistic id behind: + // pending ids are deliberately immune to reconcile. Interruption gets the + // same rollback — it is not confirmation either way, and if the session was + // created after all, reconcile re-adopts it from server metadata. + const openTerminalSession = useCallback( + (terminalId: string, sessionCwd: string) => { + reserveTerminalOpen(threadKey, terminalId); + void (async () => { + try { + const result = await openTerminal({ + environmentId: threadRef.environmentId, + input: { + threadId, + terminalId, + cwd: sessionCwd, + ...(effectiveWorktreePath != null ? { worktreePath: effectiveWorktreePath } : {}), + env: runtimeEnv, + }, + }); + if (result._tag === "Failure") { + storeAbandonPendingTerminal(threadRef, terminalId); + } + } finally { + releaseTerminalOpen(threadKey, terminalId); + } + })(); + }, + [ + effectiveWorktreePath, + openTerminal, + runtimeEnv, + storeAbandonPendingTerminal, + threadId, + threadKey, + threadRef, + ], + ); + const splitTerminal = useCallback(() => { if (!cwd) { return; } - const terminalId = nextTerminalId(allocatableTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableTerminalIds, + ...reservedTerminalOpenIds(threadKey), + ]); storeSplitTerminal(threadRef, terminalId); bumpFocusRequestId(); - void openTerminal({ - environmentId: threadRef.environmentId, - input: { - threadId, - terminalId, - cwd, - ...(effectiveWorktreePath != null ? { worktreePath: effectiveWorktreePath } : {}), - env: runtimeEnv, - }, - }); + openTerminalSession(terminalId, cwd); }, [ allocatableTerminalIds, bumpFocusRequestId, cwd, - effectiveWorktreePath, - runtimeEnv, + openTerminalSession, storeSplitTerminal, - threadId, + threadKey, threadRef, - openTerminal, ]); const splitTerminalVertical = useCallback(() => { if (!cwd) { return; } - const terminalId = nextTerminalId(allocatableTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableTerminalIds, + ...reservedTerminalOpenIds(threadKey), + ]); storeSplitTerminalVertical(threadRef, terminalId); bumpFocusRequestId(); - void openTerminal({ - environmentId: threadRef.environmentId, - input: { - threadId, - terminalId, - cwd, - ...(effectiveWorktreePath != null ? { worktreePath: effectiveWorktreePath } : {}), - env: runtimeEnv, - }, - }); + openTerminalSession(terminalId, cwd); }, [ allocatableTerminalIds, bumpFocusRequestId, cwd, - effectiveWorktreePath, - openTerminal, - runtimeEnv, + openTerminalSession, storeSplitTerminalVertical, - threadId, + threadKey, threadRef, ]); @@ -843,29 +862,21 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra if (!cwd) { return; } - const terminalId = nextTerminalId(allocatableTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableTerminalIds, + ...reservedTerminalOpenIds(threadKey), + ]); storeNewTerminal(threadRef, terminalId); bumpFocusRequestId(); - void openTerminal({ - environmentId: threadRef.environmentId, - input: { - threadId, - terminalId, - cwd, - ...(effectiveWorktreePath != null ? { worktreePath: effectiveWorktreePath } : {}), - env: runtimeEnv, - }, - }); + openTerminalSession(terminalId, cwd); }, [ bumpFocusRequestId, cwd, - effectiveWorktreePath, allocatableTerminalIds, - runtimeEnv, + openTerminalSession, storeNewTerminal, - threadId, + threadKey, threadRef, - openTerminal, ]); const activateTerminal = useCallback( @@ -893,9 +904,19 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra deleteHistory: true, }, }); - if (closeResult._tag === "Failure" && !isAtomCommandInterrupted(closeResult)) { - await fallbackExitWrite(); + if (closeResult._tag !== "Failure") return; + if (isAtomCommandInterrupted(closeResult)) { + // Interruption is not confirmation. Roll back the suppression: if the + // server did close the session it vanishes from metadata and + // reconcile drops the id; if not, the terminal must resurface. + storeUnsuppressTerminal(threadRef, terminalId); + return; } + const exitResult = await fallbackExitWrite(); + if (exitResult._tag !== "Failure") return; + // Neither path confirmably reached the session. Undo the optimistic + // suppression or reconcile would hide a live terminal. + storeUnsuppressTerminal(threadRef, terminalId); })(); storeCloseTerminal(threadRef, terminalId); @@ -904,6 +925,7 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra [ bumpFocusRequestId, storeCloseTerminal, + storeUnsuppressTerminal, threadId, threadRef, closeTerminalMutation, @@ -1374,6 +1396,8 @@ function ChatViewContent(props: ChatViewProps) { const storeNewTerminal = useTerminalUiStateStore((s) => s.newTerminal); const storeSetActiveTerminal = useTerminalUiStateStore((s) => s.setActiveTerminal); const storeCloseTerminal = useTerminalUiStateStore((s) => s.closeTerminal); + const storeUnsuppressTerminal = useTerminalUiStateStore((s) => s.unsuppressTerminal); + const storeAbandonPendingTerminal = useTerminalUiStateStore((s) => s.abandonPendingTerminal); const serverThreadRefs = useThreadRefs(); const serverThreadKeys = useMemo(() => serverThreadRefs.map(scopedThreadKey), [serverThreadRefs]); const draftThreadsByThreadKey = useComposerDraftStore((store) => store.draftThreadsByThreadKey); @@ -1534,6 +1558,36 @@ function ChatViewContent(props: ChatViewProps) { ), [rightPanelState.surfaces], ); + // Settles right-panel closes whose result never arrived. Only the server list + // is authoritative here, so a still-reported session is put back exactly where + // it was and one the server dropped stays closed. Subscribes to the pending + // registry as well: a close can be recorded after the metadata that settles + // it already arrived (and a survived session never changes the list), so the + // effect must also re-run on record. + const activePendingPanelCloseVersion = useSyncExternalStore( + subscribePendingPanelCloses, + pendingPanelCloseVersion, + ); + const activeTerminalMetadataLoaded = useTerminalMetadataLoaded(environmentId); + useEffect(() => { + if (!activeThreadRef) return; + const restored = resolvePendingPanelCloses( + scopedThreadKey(activeThreadRef), + activeServerOrderedTerminalIds, + Date.now(), + activeTerminalMetadataLoaded, + ); + for (const { terminalId, snapshot } of restored) { + useRightPanelStore.getState().restoreTerminal(activeThreadRef, snapshot, terminalId); + storeUnsuppressTerminal(activeThreadRef, terminalId); + } + }, [ + activePendingPanelCloseVersion, + activeServerOrderedTerminalIds, + activeTerminalMetadataLoaded, + activeThreadRef, + storeUnsuppressTerminal, + ]); const allocatableActiveTerminalIds = useMemo( () => [...new Set([...activeKnownTerminalIds, ...panelTerminalIds])], [activeKnownTerminalIds, panelTerminalIds], @@ -2634,6 +2688,53 @@ function ChatViewContent(props: ChatViewProps) { }, [activeThreadRef, storeSetTerminalOpen], ); + // A settled-but-not-successful open must not leave the optimistic id behind: + // pending ids are deliberately immune to reconcile. Interruption gets the + // same rollback — it is not confirmation either way, and if the session was + // created after all, reconcile re-adopts it from server metadata. + const openThreadTerminalSession = useCallback( + (terminalId: string, sessionCwd: string, workspaceRoot: string, onUncreated?: () => void) => { + if (!activeThreadRef || !activeThreadId) return; + const threadRef = activeThreadRef; + const threadKey = scopedThreadKey(threadRef); + reserveTerminalOpen(threadKey, terminalId); + void (async () => { + try { + const result = await openTerminal({ + environmentId, + input: { + threadId: activeThreadId, + terminalId, + cwd: sessionCwd, + ...(activeThreadWorktreePath != null + ? { worktreePath: activeThreadWorktreePath } + : {}), + env: projectScriptRuntimeEnv({ + project: { cwd: workspaceRoot }, + worktreePath: activeThreadWorktreePath, + }), + }, + }); + if (result._tag === "Failure") { + storeAbandonPendingTerminal(threadRef, terminalId); + // Optimistic UI outside the drawer store (right-panel surfaces) + // must be rolled back by its owner too. + onUncreated?.(); + } + } finally { + releaseTerminalOpen(threadKey, terminalId); + } + })(); + }, + [ + activeThreadId, + activeThreadRef, + activeThreadWorktreePath, + environmentId, + openTerminal, + storeAbandonPendingTerminal, + ], + ); const toggleTerminalVisibility = useCallback(() => { if (!activeThreadRef) return; const nextOpen = !terminalUiState.terminalOpen; @@ -2645,21 +2746,12 @@ function ChatViewContent(props: ChatViewProps) { if (!cwdForOpen) { return; } - const terminalId = nextTerminalId(allocatableActiveTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableActiveTerminalIds, + ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), + ]); storeEnsureTerminal(activeThreadRef, terminalId, { open: true }); - void openTerminal({ - environmentId, - input: { - threadId: activeThreadId, - terminalId, - cwd: cwdForOpen, - ...(activeThreadWorktreePath != null ? { worktreePath: activeThreadWorktreePath } : {}), - env: projectScriptRuntimeEnv({ - project: { cwd: activeProject.workspaceRoot }, - worktreePath: activeThreadWorktreePath, - }), - }, - }); + openThreadTerminalSession(terminalId, cwdForOpen, activeProject.workspaceRoot); return; } setTerminalOpen(nextOpen); @@ -2667,11 +2759,9 @@ function ChatViewContent(props: ChatViewProps) { activeProject, activeThreadId, activeThreadRef, - activeThreadWorktreePath, allocatableActiveTerminalIds, - environmentId, gitCwd, - openTerminal, + openThreadTerminalSession, setTerminalOpen, storeEnsureTerminal, terminalUiState.terminalIds.length, @@ -2686,37 +2776,26 @@ function ChatViewContent(props: ChatViewProps) { if (!cwdForOpen) { return; } - const terminalId = nextTerminalId(allocatableActiveTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableActiveTerminalIds, + ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), + ]); if (direction === "vertical") { storeSplitTerminalVertical(activeThreadRef, terminalId); } else { storeSplitTerminal(activeThreadRef, terminalId); } setTerminalFocusRequestId((value) => value + 1); - void openTerminal({ - environmentId, - input: { - threadId: activeThreadId, - terminalId, - cwd: cwdForOpen, - ...(activeThreadWorktreePath != null ? { worktreePath: activeThreadWorktreePath } : {}), - env: projectScriptRuntimeEnv({ - project: { cwd: activeProject.workspaceRoot }, - worktreePath: activeThreadWorktreePath, - }), - }, - }); + openThreadTerminalSession(terminalId, cwdForOpen, activeProject.workspaceRoot); }, [ activeProject, activeThreadId, allocatableActiveTerminalIds, activeThreadRef, - openTerminal, - activeThreadWorktreePath, - environmentId, gitCwd, hasReachedSplitLimit, + openThreadTerminalSession, storeSplitTerminal, storeSplitTerminalVertical, ], @@ -2729,29 +2808,19 @@ function ChatViewContent(props: ChatViewProps) { if (!cwdForOpen) { return; } - const terminalId = nextTerminalId(allocatableActiveTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableActiveTerminalIds, + ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), + ]); storeNewTerminal(activeThreadRef, terminalId); setTerminalFocusRequestId((value) => value + 1); - void openTerminal({ - environmentId, - input: { - threadId: activeThreadId, - terminalId, - cwd: cwdForOpen, - ...(activeThreadWorktreePath != null ? { worktreePath: activeThreadWorktreePath } : {}), - env: projectScriptRuntimeEnv({ - project: { cwd: activeProject.workspaceRoot }, - worktreePath: activeThreadWorktreePath, - }), - }, - }); + openThreadTerminalSession(terminalId, cwdForOpen, activeProject.workspaceRoot); }, [ activeProject, activeThreadId, allocatableActiveTerminalIds, activeThreadRef, - openTerminal, - activeThreadWorktreePath, + openThreadTerminalSession, environmentId, gitCwd, storeNewTerminal, @@ -2764,6 +2833,7 @@ function ChatViewContent(props: ChatViewProps) { environmentId, input: { threadId: activeThreadId, terminalId, data: "exit\n" }, }); + const threadRef = activeThreadRef; void (async () => { const closeResult = await closeTerminalMutation({ environmentId, @@ -2773,9 +2843,19 @@ function ChatViewContent(props: ChatViewProps) { deleteHistory: true, }, }); - if (closeResult._tag === "Failure" && !isAtomCommandInterrupted(closeResult)) { - await fallbackExitWrite(); + if (closeResult._tag !== "Failure") return; + if (isAtomCommandInterrupted(closeResult)) { + // Interruption is not confirmation. Roll back the suppression: if the + // server did close the session it vanishes from metadata and + // reconcile drops the id; if not, the terminal must resurface. + storeUnsuppressTerminal(threadRef, terminalId); + return; } + const exitResult = await fallbackExitWrite(); + if (exitResult._tag !== "Failure") return; + // Neither path confirmably reached the session. Undo the optimistic + // suppression or reconcile would hide a live terminal. + storeUnsuppressTerminal(threadRef, terminalId); })(); storeCloseTerminal(activeThreadRef, terminalId); setTerminalFocusRequestId((value) => value + 1); @@ -2786,6 +2866,7 @@ function ChatViewContent(props: ChatViewProps) { closeTerminalMutation, environmentId, storeCloseTerminal, + storeUnsuppressTerminal, writeTerminal, ], ); @@ -2834,7 +2915,10 @@ function ChatViewContent(props: ChatViewProps) { ...(options?.env ? { extraEnv: options.env } : {}), }); const targetTerminalId = shouldCreateNewTerminal - ? nextTerminalId(allocatableActiveTerminalIds) + ? nextTerminalId([ + ...allocatableActiveTerminalIds, + ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), + ]) : baseTerminalId; const openTerminalInput: TerminalOpenInput = shouldCreateNewTerminal ? { @@ -2856,12 +2940,26 @@ function ChatViewContent(props: ChatViewProps) { if (shouldCreateNewTerminal) { storeNewTerminal(activeThreadRef, targetTerminalId); + reserveTerminalOpen(scopedThreadKey(activeThreadRef), targetTerminalId); } else { storeSetActiveTerminal(activeThreadRef, targetTerminalId); } - const openResult = await openTerminal({ environmentId, input: openTerminalInput }); + let openResult: Awaited>; + try { + openResult = await openTerminal({ environmentId, input: openTerminalInput }); + } finally { + if (shouldCreateNewTerminal) { + releaseTerminalOpen(scopedThreadKey(activeThreadRef), targetTerminalId); + } + } if (openResult._tag === "Failure") { + if (shouldCreateNewTerminal) { + // No confirmed session behind the optimistic id (interrupted opens + // included) — abandoning is safe either way, since reconcile re-adopts + // a session that was created after all. + storeAbandonPendingTerminal(activeThreadRef, targetTerminalId); + } if (!isAtomCommandInterrupted(openResult)) { const error = squashAtomCommandFailure(openResult); setThreadError( @@ -3161,30 +3259,28 @@ function ChatViewContent(props: ChatViewProps) { const addTerminalSurface = useCallback(() => { if (!activeThreadRef || !activeThreadId || !activeProject) return; const cwd = gitCwd ?? activeProject.workspaceRoot; - const terminalId = nextTerminalId(allocatableActiveTerminalIds); - useRightPanelStore.getState().openTerminal(activeThreadRef, terminalId); + // Panel opens go through the shared helper so their ids are reserved for the + // request window too: closing a panel terminal mid-open drops it from + // panelTerminalIds, and an unreserved id could be handed straight to the + // next panel open while the first request is still unresolved. + const terminalId = nextTerminalId([ + ...allocatableActiveTerminalIds, + ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), + ]); + const threadRef = activeThreadRef; + useRightPanelStore.getState().openTerminal(threadRef, terminalId); setTerminalFocusRequestId((value) => value + 1); - void openTerminal({ - environmentId: activeThreadRef.environmentId, - input: { - threadId: activeThreadId, - terminalId, - cwd, - ...(activeThreadWorktreePath != null ? { worktreePath: activeThreadWorktreePath } : {}), - env: projectScriptRuntimeEnv({ - project: { cwd: activeProject.workspaceRoot }, - worktreePath: activeThreadWorktreePath, - }), - }, + openThreadTerminalSession(terminalId, cwd, activeProject.workspaceRoot, () => { + // No session was created, so the optimistic panel surface must go too. + useRightPanelStore.getState().closeTerminal(threadRef, `terminal:${terminalId}`, terminalId); }); }, [ activeProject, activeThreadId, activeThreadRef, - activeThreadWorktreePath, allocatableActiveTerminalIds, gitCwd, - openTerminal, + openThreadTerminalSession, ]); const splitPanelTerminal = useCallback( (direction: "horizontal" | "vertical" = "horizontal") => { @@ -3197,24 +3293,18 @@ function ChatViewContent(props: ChatViewProps) { ) { return; } - const terminalId = nextTerminalId(allocatableActiveTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableActiveTerminalIds, + ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), + ]); const cwd = gitCwd ?? activeProject.workspaceRoot; - useRightPanelStore - .getState() - .splitTerminal(activeThreadRef, activeRightPanelSurface.id, terminalId, direction); + const threadRef = activeThreadRef; + const surfaceId = activeRightPanelSurface.id; + useRightPanelStore.getState().splitTerminal(threadRef, surfaceId, terminalId, direction); setTerminalFocusRequestId((value) => value + 1); - void openTerminal({ - environmentId: activeThreadRef.environmentId, - input: { - threadId: activeThreadId, - terminalId, - cwd, - ...(activeThreadWorktreePath != null ? { worktreePath: activeThreadWorktreePath } : {}), - env: projectScriptRuntimeEnv({ - project: { cwd: activeProject.workspaceRoot }, - worktreePath: activeThreadWorktreePath, - }), - }, + openThreadTerminalSession(terminalId, cwd, activeProject.workspaceRoot, () => { + // No session was created, so the optimistic split pane must go too. + useRightPanelStore.getState().closeTerminal(threadRef, surfaceId, terminalId); }); }, [ @@ -3222,10 +3312,9 @@ function ChatViewContent(props: ChatViewProps) { activeRightPanelSurface, activeThreadId, activeThreadRef, - activeThreadWorktreePath, allocatableActiveTerminalIds, gitCwd, - openTerminal, + openThreadTerminalSession, ], ); const splitPanelTerminalVertical = useCallback(() => { @@ -3244,17 +3333,50 @@ function ChatViewContent(props: ChatViewProps) { const closePanelTerminal = useCallback( (terminalId: string) => { if (!activeThreadRef || activeRightPanelSurface?.kind !== "terminal") return; - void closeTerminalMutation({ - environmentId: activeThreadRef.environmentId, - input: { threadId: activeThreadRef.threadId, terminalId, deleteHistory: true }, - }); + const threadRef = activeThreadRef; + // Captured before the optimistic removal, so a failed close can put the + // terminal back into its original surface, position, and split direction. + const surfaceSnapshot: TerminalSurfaceSnapshot = { + surfaceId: activeRightPanelSurface.id, + resourceId: activeRightPanelSurface.resourceId, + terminalIds: [...activeRightPanelSurface.terminalIds], + ...(activeRightPanelSurface.splitDirection === "vertical" + ? { splitDirection: "vertical" as const } + : {}), + }; + void (async () => { + const result = await closeTerminalMutation({ + environmentId: threadRef.environmentId, + input: { threadId: threadRef.threadId, terminalId, deleteHistory: true }, + }); + if (result._tag !== "Failure") return; + if (isAtomCommandInterrupted(result)) { + // Unknown outcome. Hold the snapshot instead of guessing: unsuppressing + // now would let drawer reconcile adopt a surviving split terminal into + // the wrong surface, and restoring now could pin a dead pane the panel + // has no reconcile pass to remove. Metadata settles it. + recordPendingPanelClose(scopedThreadKey(threadRef), terminalId, surfaceSnapshot); + return; + } + // The session survived the failed close. Put it back where it lived — + // its original panel surface — and roll back the drawer-store + // suppression, so it neither vanishes nor resurfaces in the wrong place. + useRightPanelStore.getState().restoreTerminal(threadRef, surfaceSnapshot, terminalId); + storeUnsuppressTerminal(threadRef, terminalId); + })(); storeCloseTerminal(activeThreadRef, terminalId); useRightPanelStore .getState() .closeTerminal(activeThreadRef, activeRightPanelSurface.id, terminalId); setTerminalFocusRequestId((value) => value + 1); }, - [activeRightPanelSurface, activeThreadRef, closeTerminalMutation, storeCloseTerminal], + [ + activeRightPanelSurface, + activeThreadRef, + closeTerminalMutation, + storeCloseTerminal, + storeUnsuppressTerminal, + ], ); const activateRightPanelSurface = useCallback( (surface: RightPanelSurface) => { @@ -4354,6 +4476,21 @@ function ChatViewContent(props: ChatViewProps) { useEffect(() => { const handler = (event: globalThis.KeyboardEvent) => { + // Closing the last terminal unmounts its focus owner, so a held close + // chord stops resolving mid-hold and its auto-repeats fall through to the + // browser's tab-close default. Swallow repeats before any early return; + // deliberate presses always arrive with repeat=false. Evaluated with + // terminalFocus forced on, because by now focus may already be gone. + if ( + event.repeat && + isTerminalCloseShortcut(event, keybindings, { + context: { terminalFocus: true, terminalOpen: true }, + }) + ) { + event.preventDefault(); + event.stopPropagation(); + return; + } if (!activeThreadId || isCommandPaletteOpen()) { return; } diff --git a/apps/web/src/components/ThreadTerminalDrawer.tsx b/apps/web/src/components/ThreadTerminalDrawer.tsx index dd7da738626..7ab8305530c 100644 --- a/apps/web/src/components/ThreadTerminalDrawer.tsx +++ b/apps/web/src/components/ThreadTerminalDrawer.tsx @@ -321,12 +321,15 @@ export function TerminalViewport({ input: { threadId, terminalId, data }, }), ); - const resizeTerminal = useEffectEvent((cols: number, rows: number) => - runTerminalResize({ + const resizeTerminal = useEffectEvent(async (cols: number, rows: number) => { + const result = await runTerminalResize({ environmentId, input: { threadId, terminalId, cols, rows }, - }), - ); + }); + // A failed request means the PTY kept its old size; the surface must not + // move the grid onto a width the shell never redrew for. + return result._tag === "Success"; + }); const terminalBuffer = terminalSession.buffer; const terminalError = terminalSession.error; const terminalStatus = terminalSession.status; @@ -381,7 +384,10 @@ export function TerminalViewport({ const terminalOptions: GhosttyTerminalSurfaceOptions = { theme: terminalThemeFromApp(mount), onData: (data) => handleData(data), - onResize: (cols, rows) => void resizeTerminal(cols, rows), + // Returns the request promise: the surface holds its grid at the old + // width until the PTY acknowledges, keeping bytes interpreted at the + // width they were generated for. + onResize: (cols, rows) => resizeTerminal(cols, rows), onSelectionChange: () => handleSelectionChange(), onCopy: (text) => handleCopy(text), beforeKey: (event) => handleBeforeKey(event), @@ -527,12 +533,19 @@ export function TerminalViewport({ function handleBeforeKey(event: KeyboardEvent): boolean { const currentKeybindings = keybindingsRef.current; const options = { context: { terminalFocus: true, terminalOpen: true } }; + if (isTerminalCloseShortcut(event, currentKeybindings, options)) { + // The window-level handler owns the close action but can bail without + // preventDefault (palette open, no active thread). The browser or + // Electron default for this chord closes the whole tab/window, so it + // must never fire from inside the terminal. + event.preventDefault(); + return false; + } if ( isTerminalToggleShortcut(event, currentKeybindings, options) || isTerminalSplitShortcut(event, currentKeybindings, options) || isTerminalSplitVerticalShortcut(event, currentKeybindings, options) || isTerminalNewShortcut(event, currentKeybindings, options) || - isTerminalCloseShortcut(event, currentKeybindings, options) || isDiffToggleShortcut(event, currentKeybindings, options) ) { return false; diff --git a/apps/web/src/lib/terminalOpenReservations.test.ts b/apps/web/src/lib/terminalOpenReservations.test.ts new file mode 100644 index 00000000000..65c87773fd2 --- /dev/null +++ b/apps/web/src/lib/terminalOpenReservations.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { + releaseTerminalOpen, + reservedTerminalOpenIds, + reserveTerminalOpen, +} from "./terminalOpenReservations"; + +describe("terminalOpenReservations", () => { + it("reserves an id only while its open request is in flight", () => { + // Open terminal-2, close its tab mid-request, allocate again: the id must + // stay reserved until the request settles, then become reusable. + reserveTerminalOpen("thread-a", "terminal-2"); + expect(reservedTerminalOpenIds("thread-a")).toEqual(["terminal-2"]); + + releaseTerminalOpen("thread-a", "terminal-2"); + expect(reservedTerminalOpenIds("thread-a")).toEqual([]); + }); + + it("scopes reservations per thread and tolerates unknown releases", () => { + reserveTerminalOpen("thread-a", "terminal-2"); + reserveTerminalOpen("thread-b", "terminal-2"); + releaseTerminalOpen("thread-a", "terminal-2"); + releaseTerminalOpen("thread-a", "never-reserved"); + releaseTerminalOpen("thread-c", "terminal-2"); + + expect(reservedTerminalOpenIds("thread-a")).toEqual([]); + expect(reservedTerminalOpenIds("thread-b")).toEqual(["terminal-2"]); + releaseTerminalOpen("thread-b", "terminal-2"); + }); +}); diff --git a/apps/web/src/lib/terminalOpenReservations.ts b/apps/web/src/lib/terminalOpenReservations.ts new file mode 100644 index 00000000000..55bbedd46b5 --- /dev/null +++ b/apps/web/src/lib/terminalOpenReservations.ts @@ -0,0 +1,29 @@ +/** + * Terminal ids with an `terminal.open` request still in flight, per thread key. + * + * Reserving them keeps `nextTerminalId` from handing a just-freed id (its + * optimistic tab closed mid-request) to a new terminal — the first request's + * failure rollback would then tear down the second terminal's tab. Reservations + * live only for the request window, so closed ids become reusable again the + * moment their open settles, and the set never grows with session history. + */ +const inFlightOpenIdsByThreadKey = new Map>(); + +export function reserveTerminalOpen(threadKey: string, terminalId: string): void { + const ids = inFlightOpenIdsByThreadKey.get(threadKey) ?? new Set(); + ids.add(terminalId); + inFlightOpenIdsByThreadKey.set(threadKey, ids); +} + +export function releaseTerminalOpen(threadKey: string, terminalId: string): void { + const ids = inFlightOpenIdsByThreadKey.get(threadKey); + if (!ids) return; + ids.delete(terminalId); + if (ids.size === 0) { + inFlightOpenIdsByThreadKey.delete(threadKey); + } +} + +export function reservedTerminalOpenIds(threadKey: string): string[] { + return [...(inFlightOpenIdsByThreadKey.get(threadKey) ?? [])]; +} diff --git a/apps/web/src/lib/terminalPendingPanelCloses.test.ts b/apps/web/src/lib/terminalPendingPanelCloses.test.ts new file mode 100644 index 00000000000..e8c44b10623 --- /dev/null +++ b/apps/web/src/lib/terminalPendingPanelCloses.test.ts @@ -0,0 +1,133 @@ +import { beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +import { + clearPendingPanelCloses, + pendingPanelCloseVersion, + recordPendingPanelClose, + resolvePendingPanelCloses, + subscribePendingPanelCloses, +} from "./terminalPendingPanelCloses"; + +const snapshot = { + surfaceId: "terminal:terminal-1" as const, + resourceId: "terminal-1", + terminalIds: ["terminal-1", "terminal-2"], + splitDirection: "vertical" as const, +}; + +// Recorded at t=1000; the settle grace makes entries eligible at t=2500. +const RECORDED_AT = 1_000; +const BEFORE_GRACE = 2_000; +const AFTER_GRACE = 3_000; + +beforeEach(() => { + clearPendingPanelCloses("thread-a"); +}); + +describe("terminalPendingPanelCloses", () => { + it("restores a snapshot when post-grace metadata shows the session survived", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + + expect( + resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"], AFTER_GRACE), + ).toEqual([{ terminalId: "terminal-2", snapshot }]); + // Settled: a later metadata update must not restore it a second time. + expect( + resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"], AFTER_GRACE), + ).toEqual([]); + }); + + it("does not settle against metadata cached from before the close attempt", () => { + // The cached list still contains the id because the close's metadata has + // not propagated yet; restoring from it would pin a dead pane. + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + + expect( + resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"], BEFORE_GRACE), + ).toEqual([]); + // Still pending: post-grace metadata settles it. + expect( + resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"], AFTER_GRACE), + ).toEqual([{ terminalId: "terminal-2", snapshot }]); + }); + + it("discards a snapshot when metadata shows the session really closed", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + + expect(resolvePendingPanelCloses("thread-a", ["terminal-1"], AFTER_GRACE)).toEqual([]); + expect( + resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"], AFTER_GRACE), + ).toEqual([]); + }); + + it("waits for loaded metadata instead of resolving against an empty list", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + + // An empty list is indistinguishable from metadata that has not arrived. + expect(resolvePendingPanelCloses("thread-a", [], AFTER_GRACE)).toEqual([]); + expect( + resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"], AFTER_GRACE), + ).toEqual([{ terminalId: "terminal-2", snapshot }]); + }); + + it("settles a close as real against an authoritative empty list", () => { + // The closed terminal was the last session: loaded [] must settle the + // entry as closed, or the snapshot survives until the id is reused and + // resurrects a stale surface. + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + + expect(resolvePendingPanelCloses("thread-a", [], AFTER_GRACE, true)).toEqual([]); + expect(resolvePendingPanelCloses("thread-a", ["terminal-2"], AFTER_GRACE, true)).toEqual([]); + }); + + it("restores surviving snapshots in reverse close order", () => { + // Nested closes of panes 2 then 3: LIFO restore reproduces [1,2,3]; + // insertion order would land on [1,3,2]. + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + recordPendingPanelClose( + "thread-a", + "terminal-3", + { ...snapshot, terminalIds: ["terminal-1", "terminal-3"] }, + RECORDED_AT, + ); + + const restored = resolvePendingPanelCloses( + "thread-a", + ["terminal-1", "terminal-2", "terminal-3"], + AFTER_GRACE, + true, + ); + expect(restored.map((entry) => entry.terminalId)).toEqual(["terminal-3", "terminal-2"]); + }); + + it("notifies subscribers when a recorded close becomes eligible", () => { + vi.useFakeTimers(); + try { + let notified = 0; + const unsubscribe = subscribePendingPanelCloses(() => { + notified += 1; + }); + const versionBefore = pendingPanelCloseVersion(); + + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + + // The notification is deferred by the grace timer, not synchronous. + expect(notified).toBe(0); + vi.advanceTimersByTime(1_500); + expect(notified).toBe(1); + expect(pendingPanelCloseVersion()).toBeGreaterThan(versionBefore); + unsubscribe(); + } finally { + vi.useRealTimers(); + } + }); + + it("scopes pending closes per thread", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + recordPendingPanelClose("thread-b", "terminal-2", snapshot, RECORDED_AT); + + expect(resolvePendingPanelCloses("thread-a", ["terminal-2"], AFTER_GRACE)).toHaveLength(1); + expect(resolvePendingPanelCloses("thread-b", ["terminal-2"], AFTER_GRACE)).toHaveLength(1); + clearPendingPanelCloses("thread-b"); + }); +}); diff --git a/apps/web/src/lib/terminalPendingPanelCloses.ts b/apps/web/src/lib/terminalPendingPanelCloses.ts new file mode 100644 index 00000000000..f1c818a3ea3 --- /dev/null +++ b/apps/web/src/lib/terminalPendingPanelCloses.ts @@ -0,0 +1,105 @@ +import type { TerminalSurfaceSnapshot } from "../rightPanelStore"; + +/** + * Right-panel closes whose outcome the server never confirmed, keyed by thread. + * + * An interrupted `terminal.close` is not evidence either way: the optimistic + * removal already took the pane out of its surface, so rolling back blind would + * either strand a live split terminal in the drawer or pin a dead pane the panel + * has no reconcile pass to remove. Holding the pre-close snapshot until + * authoritative session metadata arrives lets the outcome decide. + */ +interface PendingPanelClose { + readonly snapshot: TerminalSurfaceSnapshot; + /** + * Earliest time the cached session list may settle this entry. The cached + * list can predate the close attempt (a close that succeeded server-side but + * reported interruption before the pruned metadata arrived), and restoring + * from it would pin a dead pane the panel cannot reconcile away. Metadata + * that arrives after the grace window has had time to reflect the close. + */ + readonly readyAt: number; +} + +const SETTLE_GRACE_MS = 1500; + +const pendingByThreadKey = new Map>(); + +// Recording can happen after the metadata that would settle it has already +// arrived (the close promise reports interruption late, or the session survived +// so the list never changes). Version + subscribe lets the settling effect +// re-run on every record instead of waiting for a metadata change that may +// never come. +let version = 0; +const listeners = new Set<() => void>(); + +function notifyPendingPanelCloses(): void { + version += 1; + for (const listener of [...listeners]) { + listener(); + } +} + +export function subscribePendingPanelCloses(listener: () => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +export function pendingPanelCloseVersion(): number { + return version; +} + +export function recordPendingPanelClose( + threadKey: string, + terminalId: string, + snapshot: TerminalSurfaceSnapshot, + now: number = Date.now(), +): void { + const pending = pendingByThreadKey.get(threadKey) ?? new Map(); + pending.set(terminalId, { snapshot, readyAt: now + SETTLE_GRACE_MS }); + pendingByThreadKey.set(threadKey, pending); + // One notification when the entry becomes eligible; metadata changes in the + // meantime re-run the settling effect on their own but skip unready entries. + setTimeout(notifyPendingPanelCloses, SETTLE_GRACE_MS); +} + +/** + * Settles every eligible pending close for a thread against the server's + * session list, returning the ones whose session survived so the caller can + * restore them — in reverse close order, so nested split closes reinsert at + * their original pane positions. Ids the server no longer reports were really + * closed and are simply dropped. + * + * `listLoaded` distinguishes an authoritative empty list (the closed terminal + * was the last session — the entry must settle as closed) from metadata that + * has not arrived yet, which must never settle anything. + */ +export function resolvePendingPanelCloses( + threadKey: string, + serverTerminalIds: readonly string[], + now: number = Date.now(), + listLoaded: boolean = serverTerminalIds.length > 0, +): Array<{ terminalId: string; snapshot: TerminalSurfaceSnapshot }> { + const pending = pendingByThreadKey.get(threadKey); + if (!pending || !listLoaded) return []; + + const serverIds = new Set(serverTerminalIds); + const survived: Array<{ terminalId: string; snapshot: TerminalSurfaceSnapshot }> = []; + for (const [terminalId, entry] of pending) { + if (entry.readyAt > now) continue; + pending.delete(terminalId); + if (serverIds.has(terminalId)) { + survived.push({ terminalId, snapshot: entry.snapshot }); + } + } + if (pending.size === 0) { + pendingByThreadKey.delete(threadKey); + } + return survived.reverse(); +} + +export function clearPendingPanelCloses(threadKey: string): void { + pendingByThreadKey.delete(threadKey); +} diff --git a/apps/web/src/rightPanelStore.test.ts b/apps/web/src/rightPanelStore.test.ts index c7457cfd304..b4ded9ec54a 100644 --- a/apps/web/src/rightPanelStore.test.ts +++ b/apps/web/src/rightPanelStore.test.ts @@ -18,6 +18,62 @@ beforeEach(() => { }); describe("rightPanelStore", () => { + it("restores a failed split-pane close into its original surface position", () => { + const store = useRightPanelStore.getState(); + store.openTerminal(refA, "terminal-1"); + store.splitTerminal(refA, "terminal:terminal-1", "terminal-2", "vertical"); + store.splitTerminal(refA, "terminal:terminal-1", "terminal-3", "vertical"); + const snapshot = { + surfaceId: "terminal:terminal-1" as const, + resourceId: "terminal-1", + terminalIds: ["terminal-1", "terminal-2", "terminal-3"], + splitDirection: "vertical" as const, + }; + + store.closeTerminal(refA, "terminal:terminal-1", "terminal-2"); + store.restoreTerminal(refA, snapshot, "terminal-2"); + + const surfaces = selectThreadRightPanelState( + useRightPanelStore.getState().byThreadKey, + refA, + ).surfaces; + expect(surfaces).toEqual([ + { + id: "terminal:terminal-1", + kind: "terminal", + resourceId: "terminal-1", + terminalIds: ["terminal-1", "terminal-2", "terminal-3"], + activeTerminalId: "terminal-2", + splitDirection: "vertical", + }, + ]); + }); + + it("recreates the surface when a failed close removed its last terminal", () => { + const store = useRightPanelStore.getState(); + store.openTerminal(refA, "terminal-1"); + const snapshot = { + surfaceId: "terminal:terminal-1" as const, + resourceId: "terminal-1", + terminalIds: ["terminal-1"], + }; + + store.closeTerminal(refA, "terminal:terminal-1", "terminal-1"); + store.restoreTerminal(refA, snapshot, "terminal-1"); + + const state = selectThreadRightPanelState(useRightPanelStore.getState().byThreadKey, refA); + expect(state.surfaces).toEqual([ + { + id: "terminal:terminal-1", + kind: "terminal", + resourceId: "terminal-1", + terminalIds: ["terminal-1"], + activeTerminalId: "terminal-1", + }, + ]); + expect(state.activeSurfaceId).toBe("terminal:terminal-1"); + }); + it("drops the legacy singleton terminal surface during migration", () => { expect( migratePersistedRightPanelState({ diff --git a/apps/web/src/rightPanelStore.ts b/apps/web/src/rightPanelStore.ts index 70d163306cc..edac2198348 100644 --- a/apps/web/src/rightPanelStore.ts +++ b/apps/web/src/rightPanelStore.ts @@ -39,6 +39,14 @@ export type RightPanelSurface = } | { id: "plan"; kind: "plan" }; +/** Pre-close shape of a terminal surface, captured to undo an optimistic close. */ +export interface TerminalSurfaceSnapshot { + surfaceId: `terminal:${string}`; + resourceId: string; + terminalIds: string[]; + splitDirection?: "horizontal" | "vertical"; +} + const RIGHT_PANEL_STORAGE_KEY = "t3code:right-panel-state:v2"; const RIGHT_PANEL_STORAGE_VERSION = 7; @@ -54,6 +62,11 @@ interface RightPanelStoreState { openBrowser: (ref: ScopedThreadRef, tabId: string | null) => void; openFile: (ref: ScopedThreadRef, relativePath: string, line?: number) => void; openTerminal: (ref: ScopedThreadRef, terminalId: string) => void; + restoreTerminal: ( + ref: ScopedThreadRef, + snapshot: TerminalSurfaceSnapshot, + terminalId: string, + ) => void; splitTerminal: ( ref: ScopedThreadRef, surfaceId: string, @@ -292,6 +305,49 @@ export const useRightPanelStore = create()( upsertSurface(current, terminalSurface(terminalId)), ), })), + // Undo for an optimistic close the server rejected: put the terminal back + // into its original surface at its original pane position (clamped to the + // panes that still exist), or recreate the surface if it is gone. A plain + // openTerminal would strand a split member in a new standalone surface. + restoreTerminal: (ref, snapshot, terminalId) => + set((state) => ({ + byThreadKey: updateThread(state.byThreadKey, scopedThreadKey(ref), (current) => { + const existing = current.surfaces.find( + (surface) => surface.id === snapshot.surfaceId && surface.kind === "terminal", + ); + if (!existing) { + return upsertSurface(current, { + id: snapshot.surfaceId, + kind: "terminal", + resourceId: snapshot.resourceId, + terminalIds: [terminalId], + activeTerminalId: terminalId, + ...(snapshot.splitDirection === "vertical" + ? { splitDirection: "vertical" as const } + : {}), + }); + } + return { + ...current, + isOpen: true, + activeSurfaceId: snapshot.surfaceId, + surfaces: current.surfaces.map((surface) => { + if (surface.id !== snapshot.surfaceId || surface.kind !== "terminal") { + return surface; + } + if (surface.terminalIds.includes(terminalId)) { + return { ...surface, activeTerminalId: terminalId }; + } + const insertAt = snapshot.terminalIds + .filter((id) => id === terminalId || surface.terminalIds.includes(id)) + .indexOf(terminalId); + const terminalIds = [...surface.terminalIds]; + terminalIds.splice(insertAt < 0 ? terminalIds.length : insertAt, 0, terminalId); + return { ...surface, terminalIds, activeTerminalId: terminalId }; + }), + }; + }), + })), splitTerminal: (ref, surfaceId, terminalId, direction = "horizontal") => set((state) => ({ byThreadKey: updateThread(state.byThreadKey, scopedThreadKey(ref), (current) => ({ diff --git a/apps/web/src/state/terminalSessions.ts b/apps/web/src/state/terminalSessions.ts index 9e480df08b2..28a64878066 100644 --- a/apps/web/src/state/terminalSessions.ts +++ b/apps/web/src/state/terminalSessions.ts @@ -48,6 +48,22 @@ export function useAttachedTerminalSession(input: { }, [attach.data, attach.error, input.environmentId, input.terminal, metadata.data]); } +/** + * Whether the environment's terminal metadata has loaded at least once. + * Distinguishes an authoritative empty session list from one not yet fetched. + */ +export function useTerminalMetadataLoaded(environmentId: EnvironmentId | null): boolean { + const metadata = useEnvironmentQuery( + environmentId === null + ? null + : terminalEnvironment.metadata({ + environmentId, + input: null, + }), + ); + return metadata.data !== null; +} + export function useKnownTerminalSessions(input: { readonly environmentId: EnvironmentId | null; readonly threadId: ThreadId | null; diff --git a/apps/web/src/terminal/ghostty/renderer.ts b/apps/web/src/terminal/ghostty/renderer.ts index 0cb7d6f535d..7ace3d52396 100644 --- a/apps/web/src/terminal/ghostty/renderer.ts +++ b/apps/web/src/terminal/ghostty/renderer.ts @@ -14,7 +14,7 @@ export interface GhosttyCellMetrics { const DEFAULT_SELECTION_BACKGROUND = "rgba(72, 122, 191, 0.35)"; -function cssColor(color: GhosttyColor): string { +export function cssColor(color: GhosttyColor): string { return `rgb(${color.r}, ${color.g}, ${color.b})`; } diff --git a/apps/web/src/terminal/ghostty/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index 86c1ad5329c..60ec67cdcdd 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -7,6 +7,7 @@ import { type GhosttyTheme, } from "./core"; import { + cssColor, measureGhosttyCell, renderGhosttySnapshot, terminalGridSize, @@ -313,7 +314,13 @@ export interface GhosttyTerminalSurfaceOptions { readonly theme: GhosttyTheme; readonly font?: GhosttyTerminalFont; readonly onData: (data: string) => void; - readonly onResize: (cols: number, rows: number) => void; + /** + * Requests a PTY resize. When it returns a promise, the grid is resized only + * once it settles, so output keeps being interpreted at the width it was + * generated for (see `fit`). Resolving `false` means the PTY kept its old + * size and the grid must not follow. + */ + readonly onResize: (cols: number, rows: number) => Promise | void; readonly onSelectionChange: () => void; readonly onCopy: (text: string) => void; readonly beforeKey: (event: KeyboardEvent) => boolean; @@ -350,7 +357,6 @@ export class GhosttyTerminalSurface { private scrollbarPointerId: number | null = null; private scrollbarPointerOffset = 0; private disposed = false; - private resizeNotifyTimer: number | null = null; private originY = CONTENT_PADDING; private mountHeight = 0; private selectionEnd: { x: number; y: number } | null = null; @@ -373,7 +379,12 @@ export class GhosttyTerminalSurface { private selectionMoved = false; private composing = false; private focused = false; - private resizeNotified = false; + private resizeRequestedOnce = false; + private resizeSettledOnce = false; + private resizeRetryTimer: number | null = null; + private desiredCols = 0; + private desiredRows = 0; + private resizeRequestActive = false; private canvasConfigured = false; private theme: GhosttyTheme; private readonly suppressedKeyCodes = new Set(); @@ -445,6 +456,11 @@ export class GhosttyTerminalSurface { const context = canvas.getContext("2d", { alpha: false }); if (!context) throw new Error("Canvas 2D is unavailable"); + // An opaque backing store starts out black and stays on screen through the + // font and WASM awaits below; prefill with the theme background so a + // brand-new terminal opens without a black flash. + context.fillStyle = cssColor(options.theme.background); + context.fillRect(0, 0, canvas.width, canvas.height); const fontFamily = terminalFontFamily(options.font?.family); const fontSize = terminalFontSize(options.font?.size); try { @@ -579,16 +595,27 @@ export class GhosttyTerminalSurface { } const grid = terminalGridSize(width, height, this.metrics, CONTENT_PADDING); this.mountHeight = height; - // onResize is the only PTY resize channel, so the first successful fit must - // notify even when the measured grid equals the 1x1 construction sentinel. - if (grid.cols !== this.cols || grid.rows !== this.rows || !this.resizeNotified) { - this.cols = grid.cols; - this.rows = grid.rows; - this.core.resize(grid.cols, grid.rows, this.metrics.width, this.metrics.height); - this.notifyResize(); - this.forceFullRender = true; - this.scrollbarDirty = true; - shouldRender = true; + // The PTY and the grid must agree on width for every byte, or the shell's + // redraw sequences get interpreted at a width they were not computed for — + // each such redraw orphans the previous prompt as a stranded fragment that + // survives above the shell's edit region. A native terminal gets this for + // free because VT resize and SIGWINCH are synchronous; here the PTY lives + // across an RPC, so resizes are single-flight: request one size, apply it + // to the grid when the request settles, then request the newest + // measurement if it moved on. The emulator therefore steps through every + // width the PTY entered, in the same order — discarding a superseded + // acknowledgement instead would skip a width the shell already redrew for. + // The first successful fit must request even when the measured grid equals + // the 1x1 construction sentinel, because onResize is the only PTY resize + // channel. + if ( + grid.cols !== this.desiredCols || + grid.rows !== this.desiredRows || + !this.resizeRequestedOnce + ) { + this.desiredCols = grid.cols; + this.desiredRows = grid.rows; + this.requestResize(); } // Rendering synchronously keeps the repaint inside the same frame as the // layout change: ResizeObserver fires before paint, so the browser never @@ -597,18 +624,61 @@ export class GhosttyTerminalSurface { return true; } - /** - * The local grid reflows immediately, but the PTY only hears about settled - * dimensions: notifying on every drag step makes the shell reprint its - * prompt mid-drag, which reads as jitter. - */ - private notifyResize(): void { - this.resizeNotified = true; - if (this.resizeNotifyTimer !== null) window.clearTimeout(this.resizeNotifyTimer); - this.resizeNotifyTimer = window.setTimeout(() => { - this.resizeNotifyTimer = null; - if (!this.disposed) this.options.onResize(this.cols, this.rows); - }, 150); + private requestResize(): void { + if (this.disposed || this.resizeRequestActive) return; + this.resizeRequestActive = true; + this.resizeRequestedOnce = true; + const cols = this.desiredCols; + const rows = this.desiredRows; + const settle = (granted: boolean) => { + if (this.disposed) return; + this.resizeRequestActive = false; + const firstSettle = !this.resizeSettledOnce; + this.resizeSettledOnce = true; + const hasNewerMeasurement = this.desiredCols !== cols || this.desiredRows !== rows; + if (granted || firstSettle) { + // The first-settle exception keeps mount bookkeeping honest exactly + // once: the core was constructed at the first measured grid, so a + // failed initial request must still align this.cols with it rather + // than stay at the 1x1 sentinel. Later failures never move the grid, + // and the retry below keeps re-requesting until the PTY grants — a + // failed or interrupted request has an unknown server outcome, so + // convergence needs an authoritative answer, not a guess. + this.applyGridSize(cols, rows); + } + if (hasNewerMeasurement) { + this.requestResize(); + } else if (!granted) { + this.scheduleResizeRetry(); + } + }; + const acknowledgement = this.options.onResize(cols, rows); + if (acknowledgement && typeof acknowledgement.then === "function") { + void acknowledgement.then( + (granted) => settle(granted !== false), + () => settle(false), + ); + } else { + settle(true); + } + } + + private scheduleResizeRetry(): void { + if (this.disposed || this.resizeRetryTimer !== null) return; + this.resizeRetryTimer = window.setTimeout(() => { + this.resizeRetryTimer = null; + if (!this.disposed) this.requestResize(); + }, 1000); + } + + private applyGridSize(cols: number, rows: number): void { + if (cols === this.cols && rows === this.rows) return; + this.cols = cols; + this.rows = rows; + this.core.resize(cols, rows, this.metrics.width, this.metrics.height); + this.forceFullRender = true; + this.scrollbarDirty = true; + this.renderFrame(); } focus(): void { @@ -680,13 +750,7 @@ export class GhosttyTerminalSurface { this.dprMedia?.removeEventListener("change", this.onDevicePixelRatioChange); this.dprMedia = null; if (this.selectionScrollTimer !== null) window.clearInterval(this.selectionScrollTimer); - if (this.resizeNotifyTimer !== null) { - window.clearTimeout(this.resizeNotifyTimer); - this.resizeNotifyTimer = null; - // Flush the settled dimensions so the PTY keeps the final size even when - // the surface unmounts inside the debounce window. - this.options.onResize(this.cols, this.rows); - } + if (this.resizeRetryTimer !== null) window.clearTimeout(this.resizeRetryTimer); if (this.frame !== 0) window.cancelAnimationFrame(this.frame); if (this.cursorTimer !== null) window.clearTimeout(this.cursorTimer); if (this.compositionSuppressionTimer !== null) { diff --git a/apps/web/src/terminalUiStateStore.test.ts b/apps/web/src/terminalUiStateStore.test.ts index b0b1df96e1f..4820ae2b453 100644 --- a/apps/web/src/terminalUiStateStore.test.ts +++ b/apps/web/src/terminalUiStateStore.test.ts @@ -4,6 +4,7 @@ import { beforeEach, describe, expect, it } from "vite-plus/test"; import { migratePersistedTerminalUiStateStoreState, + reconcilableServerTerminalIds, selectThreadTerminalUiState, useTerminalUiStateStore, } from "./terminalUiStateStore"; @@ -13,12 +14,105 @@ const THREAD_ID = ThreadId.make("thread-1"); const THREAD_REF = scopeThreadRef("environment-a" as never, THREAD_ID); const OTHER_THREAD_REF = scopeThreadRef("environment-b" as never, THREAD_ID); +describe("reconcilableServerTerminalIds", () => { + it("skips when the client already reflects the server", () => { + expect(reconcilableServerTerminalIds(["terminal-1"], ["terminal-1"], [], [])).toBeNull(); + // Ordering differences alone are not a reason to rewrite the list. + expect( + reconcilableServerTerminalIds( + ["terminal-2", "terminal-1"], + ["terminal-1", "terminal-2"], + [], + [], + ), + ).toBeNull(); + }); + + it("keeps a pending local open the server has not registered yet", () => { + expect( + reconcilableServerTerminalIds( + ["terminal-1"], + ["terminal-1", "terminal-2"], + [], + ["terminal-2"], + ), + ).toBeNull(); + }); + + it("keeps a pending split when the same update carries an unknown server id", () => { + // Mixed set: terminal-2 was just split locally, terminal-3 appeared + // server-side. Adopting the server list wholesale would drop the split. + expect( + reconcilableServerTerminalIds( + ["terminal-1", "terminal-3"], + ["terminal-1", "terminal-2"], + [], + ["terminal-2"], + ), + ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); + }); + + it("drops a confirmed terminal the server stopped reporting", () => { + // terminal-2 is not pending, so its absence is a real server-side close and + // must not linger as a dead tab. + expect( + reconcilableServerTerminalIds( + ["terminal-1", "terminal-3"], + ["terminal-1", "terminal-2"], + [], + [], + ), + ).toEqual(["terminal-1", "terminal-3"]); + expect( + reconcilableServerTerminalIds(["terminal-1"], ["terminal-1", "terminal-2"], [], []), + ).toEqual(["terminal-1"]); + }); + + it("never clears a populated list from an empty server response", () => { + // Indistinguishable from metadata that has not loaded yet. + expect(reconcilableServerTerminalIds([], ["terminal-1", "terminal-2"], [], [])).toBeNull(); + expect( + reconcilableServerTerminalIds(["terminal-stale"], ["terminal-1"], ["terminal-stale"], []), + ).toBeNull(); + }); + + it("ignores suppressed stale server sessions", () => { + expect( + reconcilableServerTerminalIds( + ["terminal-1", "terminal-stale"], + ["terminal-1", "terminal-2"], + ["terminal-stale"], + ["terminal-2"], + ), + ).toBeNull(); + }); + + it("adopts server sessions the client does not know", () => { + expect( + reconcilableServerTerminalIds(["terminal-1", "terminal-2"], ["terminal-1"], [], []), + ).toEqual(["terminal-1", "terminal-2"]); + expect(reconcilableServerTerminalIds(["terminal-1"], [], [], [])).toEqual(["terminal-1"]); + }); + + it("drops suppressed ids from both sides of a merge", () => { + expect( + reconcilableServerTerminalIds( + ["terminal-1", "terminal-stale", "terminal-3"], + ["terminal-1", "terminal-2", "terminal-stale"], + ["terminal-stale"], + ["terminal-2"], + ), + ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); + }); +}); + describe("terminalUiStateStore actions", () => { beforeEach(() => { useTerminalUiStateStore.persist.clearStorage(); useTerminalUiStateStore.setState({ terminalUiStateByThreadKey: {}, suppressedTerminalIdsByThreadKey: {}, + pendingTerminalIdsByThreadKey: {}, }); }); @@ -123,6 +217,99 @@ describe("terminalUiStateStore actions", () => { ]); }); + it("restores a terminal whose close request failed", () => { + const store = useTerminalUiStateStore.getState(); + store.setTerminalOpen(THREAD_REF, true); + store.splitTerminal(THREAD_REF, "terminal-2"); + store.closeTerminal(THREAD_REF, "terminal-2"); + + const threadKey = scopedThreadKey(THREAD_REF); + expect( + useTerminalUiStateStore.getState().suppressedTerminalIdsByThreadKey[threadKey], + ).toContain("terminal-2"); + + store.unsuppressTerminal(THREAD_REF, "terminal-2"); + + // Suppression cleared, so the next reconcile can surface the still-live + // session instead of hiding it for the rest of the session. + expect( + useTerminalUiStateStore.getState().suppressedTerminalIdsByThreadKey[threadKey] ?? [], + ).not.toContain("terminal-2"); + useTerminalUiStateStore + .getState() + .reconcileTerminalIds(THREAD_REF, [DEFAULT_THREAD_TERMINAL_ID, "terminal-2"]); + expect( + selectThreadTerminalUiState( + useTerminalUiStateStore.getState().terminalUiStateByThreadKey, + THREAD_REF, + ).terminalIds, + ).toEqual([DEFAULT_THREAD_TERMINAL_ID, "terminal-2"]); + }); + + it("keeps a split alive across reconciles until the server confirms it", () => { + const store = useTerminalUiStateStore.getState(); + store.setTerminalOpen(THREAD_REF, true); + store.splitTerminal(THREAD_REF, "terminal-2"); + + const readIds = () => + selectThreadTerminalUiState( + useTerminalUiStateStore.getState().terminalUiStateByThreadKey, + THREAD_REF, + ).terminalIds; + + // Server has not registered the split yet: it must survive, still grouped. + useTerminalUiStateStore + .getState() + .reconcileTerminalIds(THREAD_REF, [DEFAULT_THREAD_TERMINAL_ID]); + expect(readIds()).toEqual([DEFAULT_THREAD_TERMINAL_ID, "terminal-2"]); + expect( + selectThreadTerminalUiState( + useTerminalUiStateStore.getState().terminalUiStateByThreadKey, + THREAD_REF, + ).terminalGroups, + ).toEqual([ + { + id: `group-${DEFAULT_THREAD_TERMINAL_ID}`, + terminalIds: [DEFAULT_THREAD_TERMINAL_ID, "terminal-2"], + }, + ]); + + // Once confirmed it stops being pending, so a later disappearance is a real + // close and the dead tab goes away. + useTerminalUiStateStore + .getState() + .reconcileTerminalIds(THREAD_REF, [DEFAULT_THREAD_TERMINAL_ID, "terminal-2"]); + expect(readIds()).toEqual([DEFAULT_THREAD_TERMINAL_ID, "terminal-2"]); + + useTerminalUiStateStore + .getState() + .reconcileTerminalIds(THREAD_REF, [DEFAULT_THREAD_TERMINAL_ID]); + expect(readIds()).toEqual([DEFAULT_THREAD_TERMINAL_ID]); + }); + + it("abandons a pending terminal whose open request failed", () => { + const store = useTerminalUiStateStore.getState(); + store.setTerminalOpen(THREAD_REF, true); + store.splitTerminal(THREAD_REF, "terminal-2"); + + store.abandonPendingTerminal(THREAD_REF, "terminal-2"); + + const terminalUiState = selectThreadTerminalUiState( + useTerminalUiStateStore.getState().terminalUiStateByThreadKey, + THREAD_REF, + ); + expect(terminalUiState.terminalIds).toEqual([DEFAULT_THREAD_TERMINAL_ID]); + // Not suppressed: a session that did get created after all may still be + // adopted, and the id must no longer be immune to reconcile. + const threadKey = scopedThreadKey(THREAD_REF); + expect( + useTerminalUiStateStore.getState().suppressedTerminalIdsByThreadKey[threadKey] ?? [], + ).not.toContain("terminal-2"); + expect( + useTerminalUiStateStore.getState().pendingTerminalIdsByThreadKey[threadKey] ?? [], + ).not.toContain("terminal-2"); + }); + it("creates new terminals in a separate group", () => { useTerminalUiStateStore.getState().newTerminal(THREAD_REF, "terminal-2"); diff --git a/apps/web/src/terminalUiStateStore.ts b/apps/web/src/terminalUiStateStore.ts index 290ca8e5954..c223e50cba4 100644 --- a/apps/web/src/terminalUiStateStore.ts +++ b/apps/web/src/terminalUiStateStore.ts @@ -489,6 +489,53 @@ export function selectThreadTerminalUiState( ); } +/** + * Decides which terminal ids a client surface should reconcile with, or null + * when the local list already reflects the server. + * + * A client id the server does not report is ambiguous: it is either a local + * open the server has not registered yet, or a session that disappeared + * server-side. `pendingTerminalIds` — ids introduced locally and not yet + * confirmed — is what separates the two. Keeping every client id would leave a + * dead tab on screen forever; keeping none would drop a split opened moments + * ago whenever the same update carries an unrelated new server id. + * + * Suppressed ids are dropped from both sides, so a close the server has not + * processed cannot resurrect itself. + */ +export function reconcilableServerTerminalIds( + serverTerminalIds: readonly string[], + clientTerminalIds: readonly string[], + suppressedTerminalIds: readonly string[], + pendingTerminalIds: readonly string[], +): string[] | null { + const suppressed = new Set(suppressedTerminalIds); + const pending = new Set(pendingTerminalIds); + const visibleServerIds = + suppressed.size === 0 + ? [...serverTerminalIds] + : serverTerminalIds.filter((terminalId) => !suppressed.has(terminalId)); + + // An empty list is indistinguishable from "session metadata has not loaded + // yet" (a reconnect serves `[]` before the first response), so it must never + // clear a populated drawer. + if (visibleServerIds.length === 0) { + return null; + } + + const visibleServerIdSet = new Set(visibleServerIds); + const clientIdSet = new Set(clientTerminalIds); + const keptClientIds = clientTerminalIds.filter( + (terminalId) => + !suppressed.has(terminalId) && + (visibleServerIdSet.has(terminalId) || pending.has(terminalId)), + ); + const unknownServerIds = visibleServerIds.filter((terminalId) => !clientIdSet.has(terminalId)); + + const nextTerminalIds = [...keptClientIds, ...unknownServerIds]; + return arraysEqual(nextTerminalIds, [...clientTerminalIds]) ? null : nextTerminalIds; +} + function updateTerminalUiStateByThreadKey( terminalUiStateByThreadKey: Record, threadRef: ScopedThreadRef, @@ -519,25 +566,26 @@ function updateTerminalUiStateByThreadKey( }; } -function updateSuppressedTerminalId( - suppressedTerminalIdsByThreadKey: Record, +/** Adds or removes one terminal id from a per-thread id set (suppressed, pending). */ +function updateThreadTerminalIdSet( + idsByThreadKey: Record, threadRef: ScopedThreadRef, terminalId: string, - suppressed: boolean, + member: boolean, ): Record { const normalizedTerminalId = terminalId.trim(); if (normalizedTerminalId.length === 0) { - return suppressedTerminalIdsByThreadKey; + return idsByThreadKey; } const threadKey = terminalThreadKey(threadRef); - const currentIds = suppressedTerminalIdsByThreadKey[threadKey] ?? []; - const currentlySuppressed = currentIds.includes(normalizedTerminalId); - if (currentlySuppressed === suppressed) { - return suppressedTerminalIdsByThreadKey; + const currentIds = idsByThreadKey[threadKey] ?? []; + const currentlyMember = currentIds.includes(normalizedTerminalId); + if (currentlyMember === member) { + return idsByThreadKey; } - if (suppressed) { + if (member) { return { - ...suppressedTerminalIdsByThreadKey, + ...idsByThreadKey, [threadKey]: [...currentIds, normalizedTerminalId], }; } @@ -545,11 +593,11 @@ function updateSuppressedTerminalId( const remainingIds = currentIds.filter((id) => id !== normalizedTerminalId); if (remainingIds.length > 0) { return { - ...suppressedTerminalIdsByThreadKey, + ...idsByThreadKey, [threadKey]: remainingIds, }; } - return removeRecordEntry(suppressedTerminalIdsByThreadKey, threadKey); + return removeRecordEntry(idsByThreadKey, threadKey); } function removeRecordEntry(record: Record, key: string): Record { @@ -564,6 +612,11 @@ interface TerminalUiStateStoreState { terminalUiStateByThreadKey: Record; /** Closed ids hidden from stale server metadata until that id is explicitly opened again. */ suppressedTerminalIdsByThreadKey: Record; + /** + * Ids opened locally that the server has not confirmed yet. Reconcile keeps + * these; every other client-only id is treated as a server-side close. + */ + pendingTerminalIdsByThreadKey: Record; setTerminalOpen: (threadRef: ScopedThreadRef, open: boolean) => void; setTerminalHeight: (threadRef: ScopedThreadRef, height: number) => void; splitTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; @@ -576,6 +629,8 @@ interface TerminalUiStateStoreState { ) => void; setActiveTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; closeTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; + unsuppressTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; + abandonPendingTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; reconcileTerminalIds: (threadRef: ScopedThreadRef, nextIds: string[]) => void; clearTerminalUiState: (threadRef: ScopedThreadRef) => void; removeTerminalUiState: (threadRef: ScopedThreadRef) => void; @@ -591,7 +646,7 @@ export const useTerminalUiStateStore = create()( state: ThreadTerminalUiState, suppressedTerminalIds: readonly string[], ) => ThreadTerminalUiState, - suppression?: { terminalId: string; suppressed: boolean }, + membership?: { terminalId: string; suppressed: boolean; pending?: boolean }, ) => { set((state) => { const threadKey = terminalThreadKey(threadRef); @@ -601,21 +656,32 @@ export const useTerminalUiStateStore = create()( threadRef, (terminalState) => updater(terminalState, suppressedTerminalIds), ); - const nextSuppressedTerminalIdsByThreadKey = suppression - ? updateSuppressedTerminalId( + const nextSuppressedTerminalIdsByThreadKey = membership + ? updateThreadTerminalIdSet( state.suppressedTerminalIdsByThreadKey, threadRef, - suppression.terminalId, - suppression.suppressed, + membership.terminalId, + membership.suppressed, ) : state.suppressedTerminalIdsByThreadKey; + const nextPendingTerminalIdsByThreadKey = + membership?.pending === undefined + ? state.pendingTerminalIdsByThreadKey + : updateThreadTerminalIdSet( + state.pendingTerminalIdsByThreadKey, + threadRef, + membership.terminalId, + membership.pending, + ); if ( nextTerminalUiStateByThreadKey === state.terminalUiStateByThreadKey && - nextSuppressedTerminalIdsByThreadKey === state.suppressedTerminalIdsByThreadKey + nextSuppressedTerminalIdsByThreadKey === state.suppressedTerminalIdsByThreadKey && + nextPendingTerminalIdsByThreadKey === state.pendingTerminalIdsByThreadKey ) { return state; } return { + pendingTerminalIdsByThreadKey: nextPendingTerminalIdsByThreadKey, terminalUiStateByThreadKey: nextTerminalUiStateByThreadKey, suppressedTerminalIdsByThreadKey: nextSuppressedTerminalIdsByThreadKey, }; @@ -625,6 +691,7 @@ export const useTerminalUiStateStore = create()( return { terminalUiStateByThreadKey: {}, suppressedTerminalIdsByThreadKey: {}, + pendingTerminalIdsByThreadKey: {}, setTerminalOpen: (threadRef, open) => { const terminalState = selectThreadTerminalUiState( get().terminalUiStateByThreadKey, @@ -633,6 +700,8 @@ export const useTerminalUiStateStore = create()( updateTerminal( threadRef, (state) => setThreadTerminalOpen(state, open), + // Not marked pending: this only materializes the default id in the + // UI, without an open request behind it, so server truth still wins. open && terminalState.terminalIds.length === 0 ? { terminalId: DEFAULT_THREAD_TERMINAL_ID, suppressed: false } : undefined, @@ -644,16 +713,19 @@ export const useTerminalUiStateStore = create()( updateTerminal(threadRef, (state) => splitThreadTerminal(state, terminalId), { terminalId, suppressed: false, + pending: true, }), splitTerminalVertical: (threadRef, terminalId) => updateTerminal(threadRef, (state) => splitThreadTerminal(state, terminalId, "vertical"), { terminalId, suppressed: false, + pending: true, }), newTerminal: (threadRef, terminalId) => updateTerminal(threadRef, (state) => newThreadTerminal(state, terminalId), { terminalId, suppressed: false, + pending: true, }), ensureTerminal: (threadRef, terminalId, options) => updateTerminal( @@ -678,7 +750,7 @@ export const useTerminalUiStateStore = create()( } return normalizeThreadTerminalUiState(nextState); }, - { terminalId, suppressed: false }, + { terminalId, suppressed: false, pending: true }, ), setActiveTerminal: (threadRef, terminalId) => updateTerminal(threadRef, (state) => setThreadActiveTerminal(state, terminalId)), @@ -686,17 +758,75 @@ export const useTerminalUiStateStore = create()( updateTerminal(threadRef, (state) => closeThreadTerminal(state, terminalId), { terminalId, suppressed: true, + pending: false, }), - reconcileTerminalIds: (threadRef, nextIds) => - updateTerminal(threadRef, (state, suppressedTerminalIds) => { - if (suppressedTerminalIds.length === 0) { - return reconcileThreadTerminalSessionIds(state, nextIds); - } - const suppressedIds = new Set(suppressedTerminalIds); - return reconcileThreadTerminalSessionIds( - state, - nextIds.filter((terminalId) => !suppressedIds.has(terminalId)), + // Rollback for an optimistic close the server rejected: the session is + // still alive, and staying suppressed would hide it from reconcile for + // the rest of the session. + unsuppressTerminal: (threadRef, terminalId) => + updateTerminal(threadRef, (state) => state, { terminalId, suppressed: false }), + // Rollback for an open the server rejected. Pending ids are immune to + // reconcile, so without this the never-created terminal would sit in the + // drawer as a phantom tab forever. Not suppressed: if the session did + // get created after all, reconcile is free to adopt it back. + abandonPendingTerminal: (threadRef, terminalId) => + updateTerminal(threadRef, (state) => closeThreadTerminal(state, terminalId), { + terminalId, + suppressed: false, + pending: false, + }), + // Takes the raw server session list. The decision lives here because it + // needs the pending set, which is what tells a local open the server has + // not registered yet apart from a session that ended server-side. + reconcileTerminalIds: (threadRef, serverTerminalIds) => + set((state) => { + const threadKey = terminalThreadKey(threadRef); + const suppressedTerminalIds = state.suppressedTerminalIdsByThreadKey[threadKey] ?? []; + const pendingTerminalIds = state.pendingTerminalIdsByThreadKey[threadKey] ?? []; + + // Anything the server now reports is confirmed and stops being pending, + // so a later disappearance is read as a real close. + const serverIdSet = new Set(serverTerminalIds); + const confirmedPendingIds = pendingTerminalIds.filter( + (terminalId) => !serverIdSet.has(terminalId), ); + const nextPendingTerminalIdsByThreadKey = + confirmedPendingIds.length === pendingTerminalIds.length + ? state.pendingTerminalIdsByThreadKey + : confirmedPendingIds.length > 0 + ? { ...state.pendingTerminalIdsByThreadKey, [threadKey]: confirmedPendingIds } + : removeRecordEntry(state.pendingTerminalIdsByThreadKey, threadKey); + + const clientTerminalIds = selectThreadTerminalUiState( + state.terminalUiStateByThreadKey, + threadRef, + ).terminalIds; + const nextTerminalIds = reconcilableServerTerminalIds( + serverTerminalIds, + clientTerminalIds, + suppressedTerminalIds, + confirmedPendingIds, + ); + const nextTerminalUiStateByThreadKey = + nextTerminalIds === null + ? state.terminalUiStateByThreadKey + : updateTerminalUiStateByThreadKey( + state.terminalUiStateByThreadKey, + threadRef, + (terminalState) => + reconcileThreadTerminalSessionIds(terminalState, nextTerminalIds), + ); + + if ( + nextTerminalUiStateByThreadKey === state.terminalUiStateByThreadKey && + nextPendingTerminalIdsByThreadKey === state.pendingTerminalIdsByThreadKey + ) { + return state; + } + return { + terminalUiStateByThreadKey: nextTerminalUiStateByThreadKey, + pendingTerminalIdsByThreadKey: nextPendingTerminalIdsByThreadKey, + }; }), clearTerminalUiState: (threadRef) => set((state) => { @@ -708,9 +838,12 @@ export const useTerminalUiStateStore = create()( ); const hadSuppressedTerminalIds = state.suppressedTerminalIdsByThreadKey[threadKey] !== undefined; + const hadPendingTerminalIds = + state.pendingTerminalIdsByThreadKey[threadKey] !== undefined; if ( nextTerminalUiStateByThreadKey === state.terminalUiStateByThreadKey && - !hadSuppressedTerminalIds + !hadSuppressedTerminalIds && + !hadPendingTerminalIds ) { return state; } @@ -720,6 +853,10 @@ export const useTerminalUiStateStore = create()( state.suppressedTerminalIdsByThreadKey, threadKey, ), + pendingTerminalIdsByThreadKey: removeRecordEntry( + state.pendingTerminalIdsByThreadKey, + threadKey, + ), }; }), removeTerminalUiState: (threadRef) => @@ -728,7 +865,9 @@ export const useTerminalUiStateStore = create()( const hadTerminalUiState = state.terminalUiStateByThreadKey[threadKey] !== undefined; const hadSuppressedTerminalIds = state.suppressedTerminalIdsByThreadKey[threadKey] !== undefined; - if (!hadTerminalUiState && !hadSuppressedTerminalIds) { + const hadPendingTerminalIds = + state.pendingTerminalIdsByThreadKey[threadKey] !== undefined; + if (!hadTerminalUiState && !hadSuppressedTerminalIds && !hadPendingTerminalIds) { return state; } return { @@ -740,6 +879,10 @@ export const useTerminalUiStateStore = create()( state.suppressedTerminalIdsByThreadKey, threadKey, ), + pendingTerminalIdsByThreadKey: removeRecordEntry( + state.pendingTerminalIdsByThreadKey, + threadKey, + ), }; }), removeOrphanedTerminalUiStates: (activeThreadKeys) => @@ -748,6 +891,7 @@ export const useTerminalUiStateStore = create()( [ ...Object.keys(state.terminalUiStateByThreadKey), ...Object.keys(state.suppressedTerminalIdsByThreadKey), + ...Object.keys(state.pendingTerminalIdsByThreadKey), ].filter((key) => !activeThreadKeys.has(key)), ); if (orphanedIds.size === 0) { @@ -757,13 +901,16 @@ export const useTerminalUiStateStore = create()( const nextSuppressedTerminalIdsByThreadKey = { ...state.suppressedTerminalIdsByThreadKey, }; + const nextPendingTerminalIdsByThreadKey = { ...state.pendingTerminalIdsByThreadKey }; for (const id of orphanedIds) { delete nextTerminalUiStateByThreadKey[id]; delete nextSuppressedTerminalIdsByThreadKey[id]; + delete nextPendingTerminalIdsByThreadKey[id]; } return { terminalUiStateByThreadKey: nextTerminalUiStateByThreadKey, suppressedTerminalIdsByThreadKey: nextSuppressedTerminalIdsByThreadKey, + pendingTerminalIdsByThreadKey: nextPendingTerminalIdsByThreadKey, }; }), };