From c0b7e6607a429dd78acb4e6af2db68e03dc0c099 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 07:56:48 +0200 Subject: [PATCH 01/23] fix(server): make node-pty failures loud and actionable A broken node-pty install used to take the whole server down as a bare defect that headless startup logging could swallow, so npx t3 exited cleanly with no output. Write a diagnosis straight to stderr (toolchain hints, reinstall steps) and force a failing exit code before dying. When spawn-helper ships without its exec bit, every shell candidate fails with posix_spawnp and the error read like no shell existed on the machine. The chmod repair no longer disables itself after one failed attempt, failures are logged with the manual remedy, and spawn errors name the non-executable helper directly. Co-Authored-By: Claude Fable 5 --- .../src/terminal/NodePtyAdapter.test.ts | 110 +++++++++-- apps/server/src/terminal/NodePtyAdapter.ts | 181 ++++++++++++++---- 2 files changed, 247 insertions(+), 44 deletions(-) diff --git a/apps/server/src/terminal/NodePtyAdapter.test.ts b/apps/server/src/terminal/NodePtyAdapter.test.ts index ed87440d499..bbac4534ec2 100644 --- a/apps/server/src/terminal/NodePtyAdapter.test.ts +++ b/apps/server/src/terminal/NodePtyAdapter.test.ts @@ -62,19 +62,34 @@ it.effect("spawns through the public adapter with the provided host references", it.effect("reports native module load failures as structured startup defects", () => Effect.gen(function* () { const cause = new Error("native binding could not be loaded"); - const exit = yield* NodePtyAdapter.make(() => Promise.reject(cause)).pipe(Effect.exit); + const stderrWrite = vi.spyOn(process.stderr, "write").mockImplementation(() => true); + const previousExitCode = process.exitCode; - assert.isTrue(Exit.isFailure(exit)); - if (Exit.isFailure(exit)) { - assert.isTrue(Cause.hasDies(exit.cause)); - const error = Cause.squash(exit.cause); - assert.instanceOf(error, NodePtyAdapter.NodePtyModuleLoadError); - assert.deepInclude(error, { - _tag: "NodePtyModuleLoadError", - platform: "win32", - architecture: "x64", - }); - assert.equal(error.message, "Failed to load node-pty for win32-x64."); + try { + const exit = yield* NodePtyAdapter.make(() => Promise.reject(cause)).pipe(Effect.exit); + + assert.isTrue(Exit.isFailure(exit)); + if (Exit.isFailure(exit)) { + assert.isTrue(Cause.hasDies(exit.cause)); + const error = Cause.squash(exit.cause); + assert.instanceOf(error, NodePtyAdapter.NodePtyModuleLoadError); + assert.deepInclude(error, { + _tag: "NodePtyModuleLoadError", + platform: "win32", + architecture: "x64", + }); + assert.equal(error.message, "Failed to load node-pty for win32-x64."); + } + + // A broken install must never exit cleanly with no output. + assert.equal(process.exitCode, 1); + const written = stderrWrite.mock.calls.map((call) => String(call[0])).join(""); + assert.include(written, "Failed to load node-pty for win32-x64."); + assert.include(written, "native binding could not be loaded"); + assert.include(written, "reinstall t3"); + } finally { + stderrWrite.mockRestore(); + process.exitCode = previousExitCode; } }).pipe( Effect.provide( @@ -86,3 +101,74 @@ it.effect("reports native module load failures as structured startup defects", ( ), ), ); + +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, Error); + const error = described as Error; + assert.include(error.message, "spawn-helper"); + assert.include(error.message, 'chmod +x "/pkg/node-pty/build/Release/spawn-helper"'); + assert.equal(error.cause, 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..acf8be92326 100644 --- a/apps/server/src/terminal/NodePtyAdapter.ts +++ b/apps/server/src/terminal/NodePtyAdapter.ts @@ -20,6 +20,28 @@ export class NodePtyModuleLoadError extends Schema.TaggedErrorClass Promise; @@ -56,17 +78,69 @@ const ensureNodePtySpawnHelperExecutable = Effect.fn(function* () { if (didEnsureSpawnHelperExecutable) return; const helperPath = yield* resolveNodePtySpawnHelperPath; - if (!helperPath) return; - didEnsureSpawnHelperExecutable = true; - - if (!(yield* fs.exists(helperPath))) { + if (!helperPath) { + 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. Chmod unconditionally + // instead of stat-then-chmod: in packaged mode some fs metadata can be missing. + 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}"`, + }); }); +export const spawnHelperNotExecutableMessage = (helperPath: string): string => + `node-pty's spawn-helper at ${helperPath} is not executable, so every shell fails with "posix_spawnp failed". Fix it with: 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; +}; + +/** + * Wraps a spawn failure with an actionable message when the 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 on the machine. + */ +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 Error(spawnHelperNotExecutableMessage(input.helperPath), { cause: input.cause }); +}; + class NodePtyProcess implements PtyAdapter.PtyProcess { private readonly process: import("node-pty").IPty; @@ -126,38 +200,81 @@ export const make = Effect.fn("NodePtyAdapter.make")(function* ( architecture, cause, }), - }).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), + }).pipe( + // The defect below can be swallowed by headless startup logging, which used + // to make `npx t3` exit cleanly with no output on a broken install. Write + // the diagnosis straight to stderr and force a failing exit code first. + Effect.tapError((error) => + Effect.sync(() => { + process.exitCode = 1; + process.stderr.write(`${error.diagnostic}\n`); + }), ), + Effect.orDie, ); + 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) => + fs.stat(helperPath).pipe( + Effect.map((info) => (info.mode & 0o111) !== 0), + // When metadata is unavailable (packaged mode) assume the helper is fine + // rather than pointing users at a chmod that may not help. + Effect.orElseSucceed(() => true), + ); + 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); }), }); }); From 9f262f4a6d8cd1261b7ab98adf4510b87047b3ec Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 07:56:59 +0200 Subject: [PATCH 02/23] fix(web): terminal close shortcut can no longer reach the browser default The window-level handler owns terminal.close but bails without preventDefault when the palette is open or no thread is active, letting mod+W fall through to the tab/window close default while the terminal is focused. Prevent the default at the xterm key handler like the sibling shortcuts already do. Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ThreadTerminalDrawer.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/apps/web/src/components/ThreadTerminalDrawer.tsx b/apps/web/src/components/ThreadTerminalDrawer.tsx index dd7da738626..dfbd0c7a44f 100644 --- a/apps/web/src/components/ThreadTerminalDrawer.tsx +++ b/apps/web/src/components/ThreadTerminalDrawer.tsx @@ -527,12 +527,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; From 143087f71e7ef6e6372002068213c6a920b6365c Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 07:57:03 +0200 Subject: [PATCH 03/23] fix(desktop): holding Ctrl+W no longer closes the window Closing the last terminal disposes the xterm mount, and while focus is in flight the held key's auto-repeats resolve no command, leak past the renderer guards, and hit the windowMenu close accelerator - one hold could take out the window (and on Windows/Linux the whole app). Drop auto-repeated CmdOrCtrl+W presses in before-input-event; deliberate single presses still work everywhere. Co-Authored-By: Claude Fable 5 --- apps/desktop/src/window/DesktopWindow.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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); From 95968fd913301744d7efb0ca0e827902e74f72b4 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 08:11:17 +0200 Subject: [PATCH 04/23] fix(server): stop shell fallback once spawn-helper is diagnosed Review follow-up: the not-executable spawn-helper wrapper still carried posix_spawnp in its cause chain, so the manager retried every shell candidate (re-attempting chmod and logging a warning per candidate) before surfacing the actionable error. Treat that diagnosis as non-retryable. Also stop latching the ensure flag when the helper path resolves to null, since resolution failures are transient and folded into null. Co-Authored-By: Claude Fable 5 --- apps/server/src/terminal/Manager.test.ts | 17 +++++++++++++++++ apps/server/src/terminal/Manager.ts | 5 +++++ apps/server/src/terminal/NodePtyAdapter.ts | 7 +++---- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/apps/server/src/terminal/Manager.test.ts b/apps/server/src/terminal/Manager.test.ts index 1cf7e8dffec..40b047da930 100644 --- a/apps/server/src/terminal/Manager.test.ts +++ b/apps/server/src/terminal/Manager.test.ts @@ -1260,6 +1260,23 @@ it.layer( }), ); + it.effect("does not retry other shells when spawn-helper is not executable", () => + Effect.gen(function* () { + const { manager, ptyAdapter } = yield* createManager(); + ptyAdapter.spawnFailures.push( + new Error( + 'node-pty\'s spawn-helper at /pkg/spawn-helper is not executable, so every shell fails with "posix_spawnp failed". Fix it with: chmod +x "/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); + }), + ); + 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..e2efe38175e 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -601,6 +601,11 @@ function isRetryableShellSpawnError(error: PtyAdapter.PtySpawnError): boolean { } const message = messages.join(" ").toLowerCase(); + // A non-executable node-pty spawn-helper fails identically for every shell; + // retrying candidates would only bury the actionable error under fallbacks. + if (message.includes("spawn-helper") && message.includes("not executable")) { + return false; + } return ( message.includes("posix_spawnp failed") || message.includes("enoent") || diff --git a/apps/server/src/terminal/NodePtyAdapter.ts b/apps/server/src/terminal/NodePtyAdapter.ts index acf8be92326..bf7097e6a7e 100644 --- a/apps/server/src/terminal/NodePtyAdapter.ts +++ b/apps/server/src/terminal/NodePtyAdapter.ts @@ -77,11 +77,10 @@ const ensureNodePtySpawnHelperExecutable = Effect.fn(function* () { 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) { - didEnsureSpawnHelperExecutable = true; - return; - } + if (!helperPath) return; // npm can extract the package without the exec bit on spawn-helper, which then // surfaces as "posix_spawnp failed" for every shell. Chmod unconditionally From 8bca3c53f9d0de61a0c7b16c55f68bf38f6a672e Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 08:38:31 +0200 Subject: [PATCH 05/23] fix(web): new terminals no longer flash black while the renderer loads The ghostty surface appends an opaque canvas to the drawer before awaiting webfonts and the WASM core, and an alpha:false backing store is pure black until the first paint. Prefill the canvas with the theme background as soon as the context exists so a brand-new terminal opens straight into its themed background. Co-Authored-By: Claude Fable 5 --- apps/web/src/terminal/ghostty/renderer.ts | 2 +- apps/web/src/terminal/ghostty/surface.ts | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) 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..8111400b725 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, @@ -445,6 +446,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 { From a8ebfe17ba531102d3e2d663ad4079efee3f4673 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 08:38:44 +0200 Subject: [PATCH 06/23] fix(web): bottom-bar splits no longer collapse into a separate group The drawer reconciles its terminal list against server session metadata, but its lag guard compared raw server ids while the store filters suppressed (closed) ids. One stale server session was enough to defeat the guard on every local change: a fresh split's id is not server-known yet, so reconcile dropped it and the next metadata update re-added it as its own group. The right panel has no such reconcile pass, which is why only the bottom bar misbehaved. The guard decision now lives in the store as a pure helper that filters suppressed ids before comparing, with regression tests for the stale- session split scenario. Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 70 ++++++++--------------- apps/web/src/terminalUiStateStore.test.ts | 46 +++++++++++++++ apps/web/src/terminalUiStateStore.ts | 49 ++++++++++++++++ 3 files changed, 118 insertions(+), 47 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 2b9eda1a787..020e83cbcc9 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -197,7 +197,12 @@ import { import { appendPreviewAnnotationPrompt } from "../lib/previewAnnotation"; import { appendReviewCommentsToPrompt, type ReviewCommentContext } from "../reviewCommentContext"; import { environmentCatalog } from "../connection/catalog"; -import { selectThreadTerminalUiState, useTerminalUiStateStore } from "../terminalUiStateStore"; +import { + reconcilableServerTerminalIds, + selectSuppressedThreadTerminalIds, + selectThreadTerminalUiState, + useTerminalUiStateStore, +} from "../terminalUiStateStore"; import { useKnownTerminalSessions, useThreadRunningTerminalIds } from "../state/terminalSessions"; import { projectEnvironment } from "../state/projects"; import { useEnvironmentQuery } from "../state/query"; @@ -558,44 +563,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; @@ -637,6 +604,9 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const terminalUiState = useTerminalUiStateStore((state) => selectThreadTerminalUiState(state.terminalUiStateByThreadKey, threadRef), ); + const suppressedTerminalIds = useTerminalUiStateStore((state) => + selectSuppressedThreadTerminalIds(state.suppressedTerminalIdsByThreadKey, threadRef), + ); const knownTerminalSessions = useKnownTerminalSessions({ environmentId: threadRef.environmentId, threadId, @@ -728,16 +698,22 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const reconcileTerminalIds = useTerminalUiStateStore((state) => state.reconcileTerminalIds); useEffect(() => { - if (terminalIdListsEqual(serverOrderedTerminalIds, terminalUiState.terminalIds)) { - return; - } - if ( - serverTerminalIdsStrictSubsetOfClient(serverOrderedTerminalIds, terminalUiState.terminalIds) - ) { + const nextTerminalIds = reconcilableServerTerminalIds( + serverOrderedTerminalIds, + terminalUiState.terminalIds, + suppressedTerminalIds, + ); + if (nextTerminalIds === null) { return; } - reconcileTerminalIds(threadRef, serverOrderedTerminalIds); - }, [reconcileTerminalIds, serverOrderedTerminalIds, terminalUiState.terminalIds, threadRef]); + reconcileTerminalIds(threadRef, nextTerminalIds); + }, [ + reconcileTerminalIds, + serverOrderedTerminalIds, + suppressedTerminalIds, + terminalUiState.terminalIds, + threadRef, + ]); const [localFocusRequestId, setLocalFocusRequestId] = useState(0); const worktreePath = serverThread?.worktreePath ?? draftThread?.worktreePath ?? null; const effectiveWorktreePath = useMemo(() => { diff --git a/apps/web/src/terminalUiStateStore.test.ts b/apps/web/src/terminalUiStateStore.test.ts index b0b1df96e1f..01b6d54fb86 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,6 +14,51 @@ 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 server list matches the client list", () => { + expect(reconcilableServerTerminalIds(["terminal-1"], ["terminal-1"], [])).toBeNull(); + expect( + reconcilableServerTerminalIds(["terminal-2", "terminal-1"], ["terminal-1", "terminal-2"], []), + ).toBeNull(); + }); + + it("skips while the server list lags a freshly opened terminal", () => { + expect( + reconcilableServerTerminalIds(["terminal-1"], ["terminal-1", "terminal-2"], []), + ).toBeNull(); + }); + + it("ignores suppressed stale server sessions instead of dropping a fresh split", () => { + // A closed terminal the server still reports must not defeat the lag + // guard: reconciling here would remove the just-split terminal-2 and + // later re-add it as its own group. + expect( + reconcilableServerTerminalIds( + ["terminal-1", "terminal-stale"], + ["terminal-1", "terminal-2"], + ["terminal-stale"], + ), + ).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("filters suppressed ids out of an adopted server list", () => { + expect( + reconcilableServerTerminalIds( + ["terminal-1", "terminal-2", "terminal-stale"], + ["terminal-1"], + ["terminal-stale"], + ), + ).toEqual(["terminal-1", "terminal-2"]); + }); +}); + describe("terminalUiStateStore actions", () => { beforeEach(() => { useTerminalUiStateStore.persist.clearStorage(); diff --git a/apps/web/src/terminalUiStateStore.ts b/apps/web/src/terminalUiStateStore.ts index 290ca8e5954..9edca165405 100644 --- a/apps/web/src/terminalUiStateStore.ts +++ b/apps/web/src/terminalUiStateStore.ts @@ -489,6 +489,55 @@ export function selectThreadTerminalUiState( ); } +const EMPTY_SUPPRESSED_TERMINAL_IDS: readonly string[] = []; + +export function selectSuppressedThreadTerminalIds( + suppressedTerminalIdsByThreadKey: Record, + threadRef: ScopedThreadRef | null | undefined, +): readonly string[] { + if (!threadRef || threadRef.threadId.length === 0) { + return EMPTY_SUPPRESSED_TERMINAL_IDS; + } + return ( + suppressedTerminalIdsByThreadKey[terminalThreadKey(threadRef)] ?? EMPTY_SUPPRESSED_TERMINAL_IDS + ); +} + +/** + * Decides whether a client surface should adopt the server's session list, + * returning the ids to reconcile with or null when reconciling would destroy + * local state the server merely has not caught up with yet. + * + * Suppressed ids are removed before comparing, matching what + * `reconcileTerminalIds` persists. Comparing raw ids instead would let one + * stale server session (a close the server has not processed) defeat the lag + * guard on every local change — dropping a freshly split terminal and later + * re-adding it as its own group. + */ +export function reconcilableServerTerminalIds( + serverTerminalIds: readonly string[], + clientTerminalIds: readonly string[], + suppressedTerminalIds: readonly string[], +): string[] | null { + const suppressed = new Set(suppressedTerminalIds); + const visibleServerIds = + suppressed.size === 0 + ? [...serverTerminalIds] + : serverTerminalIds.filter((terminalId) => !suppressed.has(terminalId)); + + // Server knows about the same sessions, or fewer sessions than the client + // with every server id still present locally. The latter is typical right + // after `terminal.open`: the known-session list lags the local store; + // reconciling would drop the new id and later re-add it as a separate group + // (no split layout). + const clientIdSet = new Set(clientTerminalIds); + const everyServerIdKnown = visibleServerIds.every((terminalId) => clientIdSet.has(terminalId)); + if (everyServerIdKnown && visibleServerIds.length <= clientTerminalIds.length) { + return null; + } + return visibleServerIds; +} + function updateTerminalUiStateByThreadKey( terminalUiStateByThreadKey: Record, threadRef: ScopedThreadRef, From 45bc0dc055bd92506e29c7ca5c5847c787906e15 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 08:46:14 +0200 Subject: [PATCH 07/23] refactor(server): type the spawn-helper diagnosis and move exit policy to the CLI Review follow-ups from Macroscope: - model the non-executable spawn-helper failure as SpawnHelperNotExecutableError with a helperPath attribute, and key the terminal manager's retry decision off the tag instead of substring-matching a message - skip the chmod when the helper is already executable, so a read-only package store no longer logs the same warning on every spawn - render the node-pty load diagnostic and set the exit code in the CLI entrypoint rather than inside the adapter constructor - drop a docblock left behind in ChatView by the helper extraction Co-Authored-By: Claude Fable 5 --- apps/server/src/cli/server.ts | 24 ++++++- apps/server/src/terminal/Manager.test.ts | 8 +-- apps/server/src/terminal/Manager.ts | 11 ++-- .../src/terminal/NodePtyAdapter.test.ts | 58 ++++++++--------- apps/server/src/terminal/NodePtyAdapter.ts | 64 +++++++++++-------- apps/server/src/terminal/PtyAdapter.ts | 32 ++++++++++ apps/web/src/components/ChatView.tsx | 1 - 7 files changed, 129 insertions(+), 69 deletions(-) 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 40b047da930..59accb20953 100644 --- a/apps/server/src/terminal/Manager.test.ts +++ b/apps/server/src/terminal/Manager.test.ts @@ -1264,10 +1264,10 @@ it.layer( Effect.gen(function* () { const { manager, ptyAdapter } = yield* createManager(); ptyAdapter.spawnFailures.push( - new Error( - 'node-pty\'s spawn-helper at /pkg/spawn-helper is not executable, so every shell fails with "posix_spawnp failed". Fix it with: chmod +x "/pkg/spawn-helper"', - { cause: new Error("posix_spawnp failed.") }, - ), + new PtyAdapter.SpawnHelperNotExecutableError({ + helperPath: "/pkg/spawn-helper", + cause: new Error("posix_spawnp failed."), + }), ); const snapshot = yield* manager.open(openInput()); diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index e2efe38175e..fe846992d66 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -565,6 +565,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[] = []; @@ -601,11 +607,6 @@ function isRetryableShellSpawnError(error: PtyAdapter.PtySpawnError): boolean { } const message = messages.join(" ").toLowerCase(); - // A non-executable node-pty spawn-helper fails identically for every shell; - // retrying candidates would only bury the actionable error under fallbacks. - if (message.includes("spawn-helper") && message.includes("not executable")) { - return false; - } return ( message.includes("posix_spawnp failed") || message.includes("enoent") || diff --git a/apps/server/src/terminal/NodePtyAdapter.test.ts b/apps/server/src/terminal/NodePtyAdapter.test.ts index bbac4534ec2..1315fcd933b 100644 --- a/apps/server/src/terminal/NodePtyAdapter.test.ts +++ b/apps/server/src/terminal/NodePtyAdapter.test.ts @@ -62,34 +62,24 @@ it.effect("spawns through the public adapter with the provided host references", it.effect("reports native module load failures as structured startup defects", () => Effect.gen(function* () { const cause = new Error("native binding could not be loaded"); - const stderrWrite = vi.spyOn(process.stderr, "write").mockImplementation(() => true); - const previousExitCode = process.exitCode; + const exit = yield* NodePtyAdapter.make(() => Promise.reject(cause)).pipe(Effect.exit); - try { - const exit = yield* NodePtyAdapter.make(() => Promise.reject(cause)).pipe(Effect.exit); - - assert.isTrue(Exit.isFailure(exit)); - if (Exit.isFailure(exit)) { - assert.isTrue(Cause.hasDies(exit.cause)); - const error = Cause.squash(exit.cause); - assert.instanceOf(error, NodePtyAdapter.NodePtyModuleLoadError); - assert.deepInclude(error, { - _tag: "NodePtyModuleLoadError", - platform: "win32", - architecture: "x64", - }); - assert.equal(error.message, "Failed to load node-pty for win32-x64."); - } - - // A broken install must never exit cleanly with no output. - assert.equal(process.exitCode, 1); - const written = stderrWrite.mock.calls.map((call) => String(call[0])).join(""); - assert.include(written, "Failed to load node-pty for win32-x64."); - assert.include(written, "native binding could not be loaded"); - assert.include(written, "reinstall t3"); - } finally { - stderrWrite.mockRestore(); - process.exitCode = previousExitCode; + assert.isTrue(Exit.isFailure(exit)); + if (Exit.isFailure(exit)) { + assert.isTrue(Cause.hasDies(exit.cause)); + const error = Cause.squash(exit.cause); + assert.instanceOf(error, NodePtyAdapter.NodePtyModuleLoadError); + assert.deepInclude(error, { + _tag: "NodePtyModuleLoadError", + platform: "win32", + 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( @@ -121,11 +111,19 @@ it("explains posix_spawnp failures caused by a non-executable spawn-helper", () helperPath: "/pkg/node-pty/build/Release/spawn-helper", helperIsExecutable: false, }); - assert.instanceOf(described, Error); - const error = described as Error; - assert.include(error.message, "spawn-helper"); + 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", () => { diff --git a/apps/server/src/terminal/NodePtyAdapter.ts b/apps/server/src/terminal/NodePtyAdapter.ts index bf7097e6a7e..94bc43fbfb3 100644 --- a/apps/server/src/terminal/NodePtyAdapter.ts +++ b/apps/server/src/terminal/NodePtyAdapter.ts @@ -71,6 +71,19 @@ const resolveNodePtySpawnHelperPath = Effect.gen(function* () { return null; }).pipe(Effect.orElseSucceed(() => null)); +/** + * Whether the helper carries any exec bit, 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; + return yield* fs.stat(helperPath).pipe( + Effect.map((info) => (info.mode & 0o111) !== 0), + Effect.orElseSucceed(() => null), + ); +}); + const ensureNodePtySpawnHelperExecutable = Effect.fn(function* () { const fs = yield* FileSystem.FileSystem; const platform = yield* HostProcessPlatform; @@ -82,9 +95,15 @@ const ensureNodePtySpawnHelperExecutable = Effect.fn(function* () { const helperPath = yield* resolveNodePtySpawnHelperPath; if (!helperPath) return; - // npm can extract the package without the exec bit on spawn-helper, which then - // surfaces as "posix_spawnp failed" for every shell. Chmod unconditionally - // instead of stat-then-chmod: in packaged mode some fs metadata can be missing. + // 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; + } + + // 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; @@ -99,9 +118,6 @@ const ensureNodePtySpawnHelperExecutable = Effect.fn(function* () { }); }); -export const spawnHelperNotExecutableMessage = (helperPath: string): string => - `node-pty's spawn-helper at ${helperPath} is not executable, so every shell fails with "posix_spawnp failed". Fix it with: chmod +x "${helperPath}"`; - const causeMentionsPosixSpawnFailure = (cause: unknown): boolean => { let current: unknown = cause; const seen = new Set(); @@ -123,10 +139,9 @@ const causeMentionsPosixSpawnFailure = (cause: unknown): boolean => { }; /** - * Wraps a spawn failure with an actionable message when the 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 on the machine. + * 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; @@ -137,7 +152,10 @@ export const describeSpawnFailure = (input: { 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 Error(spawnHelperNotExecutableMessage(input.helperPath), { cause: input.cause }); + return new PtyAdapter.SpawnHelperNotExecutableError({ + helperPath: input.helperPath, + cause: input.cause, + }); }; class NodePtyProcess implements PtyAdapter.PtyProcess { @@ -199,18 +217,9 @@ export const make = Effect.fn("NodePtyAdapter.make")(function* ( architecture, cause, }), - }).pipe( - // The defect below can be swallowed by headless startup logging, which used - // to make `npx t3` exit cleanly with no output on a broken install. Write - // the diagnosis straight to stderr and force a failing exit code first. - Effect.tapError((error) => - Effect.sync(() => { - process.exitCode = 1; - process.stderr.write(`${error.diagnostic}\n`); - }), - ), - Effect.orDie, - ); + // Rendering the diagnostic and choosing an exit code is the CLI + // entrypoint's job — see `reportStartupDefect` in cli/server.ts. + }).pipe(Effect.orDie); const ensureSpawnHelperExecutable = ensureNodePtySpawnHelperExecutable().pipe( Effect.provideService(FileSystem.FileSystem, fs), @@ -225,11 +234,10 @@ export const make = Effect.fn("NodePtyAdapter.make")(function* ( Effect.provideService(HostProcessArchitecture, architecture), ); const spawnHelperIsExecutable = (helperPath: string) => - fs.stat(helperPath).pipe( - Effect.map((info) => (info.mode & 0o111) !== 0), - // When metadata is unavailable (packaged mode) assume the helper is fine - // rather than pointing users at a chmod that may not help. - Effect.orElseSucceed(() => true), + 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({ diff --git a/apps/server/src/terminal/PtyAdapter.ts b/apps/server/src/terminal/PtyAdapter.ts index 67147035bb5..2ac78a7141c 100644 --- a/apps/server/src/terminal/PtyAdapter.ts +++ b/apps/server/src/terminal/PtyAdapter.ts @@ -29,6 +29,38 @@ 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, + cause: Schema.optional(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 hasSpawnHelperNotExecutableCause = (error: unknown): boolean => { + let current: unknown = error; + const seen = new Set(); + while (current !== null && current !== undefined && !seen.has(current)) { + seen.add(current); + if (isSpawnHelperNotExecutableError(current)) return true; + if (typeof current !== "object") return false; + current = (current as { cause?: unknown }).cause; + } + return false; +}; + 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 020e83cbcc9..cbe1e31a023 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -562,7 +562,6 @@ function useLocalDispatchState(input: { }; } -/** Same terminal ids (order ignored) — avoids reconcile when only server session ordering differs. */ interface PersistentThreadTerminalDrawerProps { threadRef: { environmentId: EnvironmentId; threadId: ThreadId }; threadId: ThreadId; From ca8a88de2f06376932a373e30c088a0bcfa94fb1 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 08:53:32 +0200 Subject: [PATCH 08/23] fix(server): judge spawn-helper executability by the calling process Review follow-ups from Macroscope: a bare `mode & 0o111` calls an owner-only helper (0o100) executable for every other user, which skips the chmod repair and suppresses the actionable diagnosis for exactly the users hitting the failure. Effect's FileSystem.access cannot test X_OK, so derive executability from the mode against the process uid/gids. Also make the SpawnHelperNotExecutableError cause required, since every construction wraps a real spawn failure. Co-Authored-By: Claude Fable 5 --- .../src/terminal/NodePtyAdapter.test.ts | 75 +++++++++++++++++++ apps/server/src/terminal/NodePtyAdapter.ts | 56 +++++++++++++- apps/server/src/terminal/PtyAdapter.ts | 3 +- 3 files changed, 130 insertions(+), 4 deletions(-) diff --git a/apps/server/src/terminal/NodePtyAdapter.test.ts b/apps/server/src/terminal/NodePtyAdapter.test.ts index 1315fcd933b..93c4178bbea 100644 --- a/apps/server/src/terminal/NodePtyAdapter.test.ts +++ b/apps/server/src/terminal/NodePtyAdapter.test.ts @@ -92,6 +92,81 @@ 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({ diff --git a/apps/server/src/terminal/NodePtyAdapter.ts b/apps/server/src/terminal/NodePtyAdapter.ts index 94bc43fbfb3..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"; @@ -72,14 +73,63 @@ const resolveNodePtySpawnHelperPath = Effect.gen(function* () { }).pipe(Effect.orElseSucceed(() => null)); /** - * Whether the helper carries any exec bit, or `null` when the mode cannot be - * read — some packaged modes hide fs metadata, and "unknown" means different + * 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) => (info.mode & 0o111) !== 0), + Effect.map((info) => + modeIsExecutableFor({ + mode: info.mode, + ownerUid: Option.getOrNull(info.uid), + ownerGid: Option.getOrNull(info.gid), + ...identity, + }), + ), Effect.orElseSucceed(() => null), ); }); diff --git a/apps/server/src/terminal/PtyAdapter.ts b/apps/server/src/terminal/PtyAdapter.ts index 2ac78a7141c..77af83635f3 100644 --- a/apps/server/src/terminal/PtyAdapter.ts +++ b/apps/server/src/terminal/PtyAdapter.ts @@ -38,7 +38,8 @@ export class SpawnHelperNotExecutableError extends Schema.TaggedErrorClass Date: Sat, 1 Aug 2026 09:29:50 +0200 Subject: [PATCH 09/23] fix(terminal): close failures, held close chord, and split merges no longer lose state Four review findings: - swallow auto-repeats of the close chord at the window handler, so a hold that unmounts the last terminal cannot fall through to the browser tab-close default once the shortcut stops resolving - roll back the optimistic close suppression when both the close request and the fallback exit write fail, instead of hiding a still-running terminal for the rest of the session; the right-panel close had no failure handling at all - merge pending local ids during reconcile rather than adopting the server list wholesale, so an update that carries an unknown server id no longer drops a split opened moments earlier - publish the diagnosed spawn failure to the client, so the actionable chmod +x remedy is no longer hidden behind the generic PtySpawnError wrapper Co-Authored-By: Claude Fable 5 --- apps/server/src/terminal/Manager.test.ts | 8 +++- apps/server/src/terminal/Manager.ts | 5 +- apps/server/src/terminal/PtyAdapter.ts | 13 ++++-- apps/web/src/components/ChatView.tsx | 56 +++++++++++++++++++---- apps/web/src/terminalUiStateStore.test.ts | 48 +++++++++++++++++++ apps/web/src/terminalUiStateStore.ts | 41 ++++++++++------- 6 files changed, 140 insertions(+), 31 deletions(-) diff --git a/apps/server/src/terminal/Manager.test.ts b/apps/server/src/terminal/Manager.test.ts index 59accb20953..f1fd0653be0 100644 --- a/apps/server/src/terminal/Manager.test.ts +++ b/apps/server/src/terminal/Manager.test.ts @@ -1262,7 +1262,7 @@ it.layer( it.effect("does not retry other shells when spawn-helper is not executable", () => Effect.gen(function* () { - const { manager, ptyAdapter } = yield* createManager(); + const { manager, ptyAdapter, getEvents } = yield* createManager(); ptyAdapter.spawnFailures.push( new PtyAdapter.SpawnHelperNotExecutableError({ helperPath: "/pkg/spawn-helper", @@ -1274,6 +1274,12 @@ it.layer( 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"'); }), ); diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index fe846992d66..a94ebc6db76 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -1965,7 +1965,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, diff --git a/apps/server/src/terminal/PtyAdapter.ts b/apps/server/src/terminal/PtyAdapter.ts index 77af83635f3..6e55836003a 100644 --- a/apps/server/src/terminal/PtyAdapter.ts +++ b/apps/server/src/terminal/PtyAdapter.ts @@ -50,18 +50,23 @@ export class SpawnHelperNotExecutableError extends Schema.TaggedErrorClass { +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 true; - if (typeof current !== "object") return false; + if (isSpawnHelperNotExecutableError(current)) return current; + if (typeof current !== "object") return null; current = (current as { cause?: unknown }).cause; } - return false; + 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 cbe1e31a023..d1cbcac9f64 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -141,7 +141,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 { @@ -694,6 +698,7 @@ 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 reconcileTerminalIds = useTerminalUiStateStore((state) => state.reconcileTerminalIds); useEffect(() => { @@ -868,9 +873,12 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra deleteHistory: true, }, }); - if (closeResult._tag === "Failure" && !isAtomCommandInterrupted(closeResult)) { - await fallbackExitWrite(); - } + if (closeResult._tag !== "Failure" || isAtomCommandInterrupted(closeResult)) return; + const exitResult = await fallbackExitWrite(); + if (exitResult._tag !== "Failure" || isAtomCommandInterrupted(exitResult)) return; + // Neither path reached the session, so it is still running. Undo the + // optimistic suppression or reconcile would hide a live terminal. + storeUnsuppressTerminal(threadRef, terminalId); })(); storeCloseTerminal(threadRef, terminalId); @@ -879,6 +887,7 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra [ bumpFocusRequestId, storeCloseTerminal, + storeUnsuppressTerminal, threadId, threadRef, closeTerminalMutation, @@ -1349,6 +1358,7 @@ 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 serverThreadRefs = useThreadRefs(); const serverThreadKeys = useMemo(() => serverThreadRefs.map(scopedThreadKey), [serverThreadRefs]); const draftThreadsByThreadKey = useComposerDraftStore((store) => store.draftThreadsByThreadKey); @@ -3219,17 +3229,30 @@ 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; + void (async () => { + const result = await closeTerminalMutation({ + environmentId: threadRef.environmentId, + input: { threadId: threadRef.threadId, terminalId, deleteHistory: true }, + }); + if (result._tag !== "Failure" || isAtomCommandInterrupted(result)) return; + // The session survived the failed close, so let reconcile surface it + // again instead of suppressing a live terminal forever. + 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) => { @@ -4329,6 +4352,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/terminalUiStateStore.test.ts b/apps/web/src/terminalUiStateStore.test.ts index 01b6d54fb86..c9dee61d460 100644 --- a/apps/web/src/terminalUiStateStore.test.ts +++ b/apps/web/src/terminalUiStateStore.test.ts @@ -48,6 +48,25 @@ describe("reconcilableServerTerminalIds", () => { expect(reconcilableServerTerminalIds(["terminal-1"], [], [])).toEqual(["terminal-1"]); }); + it("keeps a fresh local split when the same update carries an unknown server id", () => { + // Mixed set: terminal-2 was just split locally and the server has not caught + // up, while terminal-3 appeared server-side. Adopting the server list + // wholesale would drop the split and re-add it later as its own group. + expect( + reconcilableServerTerminalIds(["terminal-1", "terminal-3"], ["terminal-1", "terminal-2"], []), + ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); + }); + + 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"], + ), + ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); + }); + it("filters suppressed ids out of an adopted server list", () => { expect( reconcilableServerTerminalIds( @@ -169,6 +188,35 @@ 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("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 9edca165405..5671f05c2ff 100644 --- a/apps/web/src/terminalUiStateStore.ts +++ b/apps/web/src/terminalUiStateStore.ts @@ -504,15 +504,15 @@ export function selectSuppressedThreadTerminalIds( } /** - * Decides whether a client surface should adopt the server's session list, - * returning the ids to reconcile with or null when reconciling would destroy - * local state the server merely has not caught up with yet. + * Decides which terminal ids a client surface should reconcile with, or null + * when reconciling would destroy local state the server merely has not caught + * up with yet. * - * Suppressed ids are removed before comparing, matching what - * `reconcileTerminalIds` persists. Comparing raw ids instead would let one - * stale server session (a close the server has not processed) defeat the lag - * guard on every local change — dropping a freshly split terminal and later - * re-adding it as its own group. + * Suppressed ids are removed first, matching what `reconcileTerminalIds` + * persists. Comparing raw ids instead would let one stale server session (a + * close the server has not processed) defeat the lag guard on every local + * change — dropping a freshly split terminal and later re-adding it as its own + * group. */ export function reconcilableServerTerminalIds( serverTerminalIds: readonly string[], @@ -525,17 +525,20 @@ export function reconcilableServerTerminalIds( ? [...serverTerminalIds] : serverTerminalIds.filter((terminalId) => !suppressed.has(terminalId)); - // Server knows about the same sessions, or fewer sessions than the client - // with every server id still present locally. The latter is typical right - // after `terminal.open`: the known-session list lags the local store; - // reconciling would drop the new id and later re-add it as a separate group - // (no split layout). const clientIdSet = new Set(clientTerminalIds); - const everyServerIdKnown = visibleServerIds.every((terminalId) => clientIdSet.has(terminalId)); - if (everyServerIdKnown && visibleServerIds.length <= clientTerminalIds.length) { + const unknownServerIds = visibleServerIds.filter((terminalId) => !clientIdSet.has(terminalId)); + // The client already shows every session the server reports. Typical right + // after `terminal.open`, when the known-session list still lags the local + // store; there is nothing to adopt. + if (unknownServerIds.length === 0) { return null; } - return visibleServerIds; + + // Adopt the sessions the client has not seen, but keep local ids the server + // has not caught up with. Replacing the list wholesale would drop a split + // opened moments ago whenever the same update also carries a new server id. + const pendingClientIds = clientTerminalIds.filter((terminalId) => !suppressed.has(terminalId)); + return [...pendingClientIds, ...unknownServerIds]; } function updateTerminalUiStateByThreadKey( @@ -625,6 +628,7 @@ interface TerminalUiStateStoreState { ) => void; setActiveTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; closeTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; + unsuppressTerminal: (threadRef: ScopedThreadRef, terminalId: string) => void; reconcileTerminalIds: (threadRef: ScopedThreadRef, nextIds: string[]) => void; clearTerminalUiState: (threadRef: ScopedThreadRef) => void; removeTerminalUiState: (threadRef: ScopedThreadRef) => void; @@ -736,6 +740,11 @@ export const useTerminalUiStateStore = create()( terminalId, suppressed: true, }), + // 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 }), reconcileTerminalIds: (threadRef, nextIds) => updateTerminal(threadRef, (state, suppressedTerminalIds) => { if (suppressedTerminalIds.length === 0) { From 79be8369ef2d234abb4af14d100846fd8ee4e3f8 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 09:39:37 +0200 Subject: [PATCH 10/23] fix(web): track pending opens so reconcile can drop remote closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: the previous merge kept every client id, so a terminal that ended server-side was never removed once any new id arrived in the same update, leaving a dead tab on screen for good. A client-only id is ambiguous — either a local open the server has not registered yet, or a session that ended remotely — and id sets alone cannot tell them apart. Track ids introduced by split/new/ensure in a pending set, clear an id once the server reports it, and let reconcile keep only pending or server-visible ids. An empty server list is still treated as unloaded metadata so a reconnect cannot clear the drawer. The merge decision moves into the store, where the pending set lives. Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 30 +--- apps/web/src/terminalUiStateStore.test.ts | 130 ++++++++++---- apps/web/src/terminalUiStateStore.ts | 202 +++++++++++++++------- 3 files changed, 245 insertions(+), 117 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index d1cbcac9f64..7378dbb7ee0 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -201,12 +201,7 @@ import { import { appendPreviewAnnotationPrompt } from "../lib/previewAnnotation"; import { appendReviewCommentsToPrompt, type ReviewCommentContext } from "../reviewCommentContext"; import { environmentCatalog } from "../connection/catalog"; -import { - reconcilableServerTerminalIds, - selectSuppressedThreadTerminalIds, - selectThreadTerminalUiState, - useTerminalUiStateStore, -} from "../terminalUiStateStore"; +import { selectThreadTerminalUiState, useTerminalUiStateStore } from "../terminalUiStateStore"; import { useKnownTerminalSessions, useThreadRunningTerminalIds } from "../state/terminalSessions"; import { projectEnvironment } from "../state/projects"; import { useEnvironmentQuery } from "../state/query"; @@ -607,9 +602,6 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const terminalUiState = useTerminalUiStateStore((state) => selectThreadTerminalUiState(state.terminalUiStateByThreadKey, threadRef), ); - const suppressedTerminalIds = useTerminalUiStateStore((state) => - selectSuppressedThreadTerminalIds(state.suppressedTerminalIdsByThreadKey, threadRef), - ); const knownTerminalSessions = useKnownTerminalSessions({ environmentId: threadRef.environmentId, threadId, @@ -701,23 +693,11 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const storeUnsuppressTerminal = useTerminalUiStateStore((state) => state.unsuppressTerminal); 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(() => { - const nextTerminalIds = reconcilableServerTerminalIds( - serverOrderedTerminalIds, - terminalUiState.terminalIds, - suppressedTerminalIds, - ); - if (nextTerminalIds === null) { - return; - } - reconcileTerminalIds(threadRef, nextTerminalIds); - }, [ - reconcileTerminalIds, - serverOrderedTerminalIds, - suppressedTerminalIds, - terminalUiState.terminalIds, - threadRef, - ]); + reconcileTerminalIds(threadRef, serverOrderedTerminalIds); + }, [reconcileTerminalIds, serverOrderedTerminalIds, threadRef]); const [localFocusRequestId, setLocalFocusRequestId] = useState(0); const worktreePath = serverThread?.worktreePath ?? draftThread?.worktreePath ?? null; const effectiveWorktreePath = useMemo(() => { diff --git a/apps/web/src/terminalUiStateStore.test.ts b/apps/web/src/terminalUiStateStore.test.ts index c9dee61d460..25c945faff9 100644 --- a/apps/web/src/terminalUiStateStore.test.ts +++ b/apps/web/src/terminalUiStateStore.test.ts @@ -15,66 +15,94 @@ 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 server list matches the client list", () => { - expect(reconcilableServerTerminalIds(["terminal-1"], ["terminal-1"], [])).toBeNull(); + 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"], []), + reconcilableServerTerminalIds( + ["terminal-2", "terminal-1"], + ["terminal-1", "terminal-2"], + [], + [], + ), ).toBeNull(); }); - it("skips while the server list lags a freshly opened terminal", () => { + it("keeps a pending local open the server has not registered yet", () => { expect( - reconcilableServerTerminalIds(["terminal-1"], ["terminal-1", "terminal-2"], []), + reconcilableServerTerminalIds( + ["terminal-1"], + ["terminal-1", "terminal-2"], + [], + ["terminal-2"], + ), ).toBeNull(); }); - it("ignores suppressed stale server sessions instead of dropping a fresh split", () => { - // A closed terminal the server still reports must not defeat the lag - // guard: reconciling here would remove the just-split terminal-2 and - // later re-add it as its own group. + 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-stale"], + ["terminal-1", "terminal-3"], ["terminal-1", "terminal-2"], - ["terminal-stale"], + [], + ["terminal-2"], ), - ).toBeNull(); + ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); }); - 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 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("keeps a fresh local split when the same update carries an unknown server id", () => { - // Mixed set: terminal-2 was just split locally and the server has not caught - // up, while terminal-3 appeared server-side. Adopting the server list - // wholesale would drop the split and re-add it later as its own group. + 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-1", "terminal-3"], ["terminal-1", "terminal-2"], []), - ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); + reconcilableServerTerminalIds(["terminal-stale"], ["terminal-1"], ["terminal-stale"], []), + ).toBeNull(); }); - it("drops suppressed ids from both sides of a merge", () => { + it("ignores suppressed stale server sessions", () => { expect( reconcilableServerTerminalIds( - ["terminal-1", "terminal-stale", "terminal-3"], - ["terminal-1", "terminal-2", "terminal-stale"], + ["terminal-1", "terminal-stale"], + ["terminal-1", "terminal-2"], ["terminal-stale"], + ["terminal-2"], ), - ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); + ).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("filters suppressed ids out of an adopted server list", () => { + 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-1"], ["terminal-stale"], + ["terminal-2"], ), - ).toEqual(["terminal-1", "terminal-2"]); + ).toEqual(["terminal-1", "terminal-2", "terminal-3"]); }); }); @@ -84,6 +112,7 @@ describe("terminalUiStateStore actions", () => { useTerminalUiStateStore.setState({ terminalUiStateByThreadKey: {}, suppressedTerminalIdsByThreadKey: {}, + pendingTerminalIdsByThreadKey: {}, }); }); @@ -217,6 +246,47 @@ describe("terminalUiStateStore actions", () => { ).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("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 5671f05c2ff..510cfc9d825 100644 --- a/apps/web/src/terminalUiStateStore.ts +++ b/apps/web/src/terminalUiStateStore.ts @@ -489,56 +489,51 @@ export function selectThreadTerminalUiState( ); } -const EMPTY_SUPPRESSED_TERMINAL_IDS: readonly string[] = []; - -export function selectSuppressedThreadTerminalIds( - suppressedTerminalIdsByThreadKey: Record, - threadRef: ScopedThreadRef | null | undefined, -): readonly string[] { - if (!threadRef || threadRef.threadId.length === 0) { - return EMPTY_SUPPRESSED_TERMINAL_IDS; - } - return ( - suppressedTerminalIdsByThreadKey[terminalThreadKey(threadRef)] ?? EMPTY_SUPPRESSED_TERMINAL_IDS - ); -} - /** * Decides which terminal ids a client surface should reconcile with, or null - * when reconciling would destroy local state the server merely has not caught - * up with yet. + * 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 removed first, matching what `reconcileTerminalIds` - * persists. Comparing raw ids instead would let one stale server session (a - * close the server has not processed) defeat the lag guard on every local - * change — dropping a freshly split terminal and later re-adding it as its own - * group. + * 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)); - const clientIdSet = new Set(clientTerminalIds); - const unknownServerIds = visibleServerIds.filter((terminalId) => !clientIdSet.has(terminalId)); - // The client already shows every session the server reports. Typical right - // after `terminal.open`, when the known-session list still lags the local - // store; there is nothing to adopt. - if (unknownServerIds.length === 0) { + // 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; } - // Adopt the sessions the client has not seen, but keep local ids the server - // has not caught up with. Replacing the list wholesale would drop a split - // opened moments ago whenever the same update also carries a new server id. - const pendingClientIds = clientTerminalIds.filter((terminalId) => !suppressed.has(terminalId)); - return [...pendingClientIds, ...unknownServerIds]; + 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( @@ -571,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], }; } @@ -597,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 { @@ -616,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; @@ -644,7 +645,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); @@ -654,21 +655,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, }; @@ -678,6 +690,7 @@ export const useTerminalUiStateStore = create()( return { terminalUiStateByThreadKey: {}, suppressedTerminalIdsByThreadKey: {}, + pendingTerminalIdsByThreadKey: {}, setTerminalOpen: (threadRef, open) => { const terminalState = selectThreadTerminalUiState( get().terminalUiStateByThreadKey, @@ -686,6 +699,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, @@ -697,16 +712,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( @@ -731,7 +749,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)), @@ -739,22 +757,65 @@ export const useTerminalUiStateStore = create()( updateTerminal(threadRef, (state) => closeThreadTerminal(state, terminalId), { terminalId, suppressed: true, + pending: false, }), // 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 }), - 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)), + // 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) => { @@ -766,9 +827,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; } @@ -778,6 +842,10 @@ export const useTerminalUiStateStore = create()( state.suppressedTerminalIdsByThreadKey, threadKey, ), + pendingTerminalIdsByThreadKey: removeRecordEntry( + state.pendingTerminalIdsByThreadKey, + threadKey, + ), }; }), removeTerminalUiState: (threadRef) => @@ -786,7 +854,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 { @@ -798,6 +868,10 @@ export const useTerminalUiStateStore = create()( state.suppressedTerminalIdsByThreadKey, threadKey, ), + pendingTerminalIdsByThreadKey: removeRecordEntry( + state.pendingTerminalIdsByThreadKey, + threadKey, + ), }; }), removeOrphanedTerminalUiStates: (activeThreadKeys) => @@ -806,6 +880,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) { @@ -815,13 +890,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, }; }), }; From 35f2a14c02e85d220cedce7ce0e1dd1a2ccbe4cc Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 09:47:29 +0200 Subject: [PATCH 11/23] fix(web): drop pending terminals whose open request failed Review follow-up: pending ids are immune to reconcile, and every caller fired openTerminal without handling rejection, so a failed open left a terminal tab in the drawer that nothing could ever clean up. Add abandonPendingTerminal and call it from every path that marks an id pending: both split directions, new terminal, the drawer toggle, and the project-script runner. It clears the id without suppressing it, so a session that did get created after all can still be adopted. Also rolls back the optimistic close suppression in the third close path, matching the drawer and right-panel handlers. Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 185 +++++++++++----------- apps/web/src/terminalUiStateStore.test.ts | 23 +++ apps/web/src/terminalUiStateStore.ts | 11 ++ 3 files changed, 127 insertions(+), 92 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 7378dbb7ee0..6ea04893cc2 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -691,6 +691,9 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra 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 @@ -742,6 +745,36 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra [storeSetTerminalHeight, threadRef], ); + // A rejected open leaves no server session behind, so the optimistic id must + // not linger: pending ids are deliberately immune to reconcile. + const openTerminalSession = useCallback( + (terminalId: string, sessionCwd: string) => { + void (async () => { + const result = await openTerminal({ + environmentId: threadRef.environmentId, + input: { + threadId, + terminalId, + cwd: sessionCwd, + ...(effectiveWorktreePath != null ? { worktreePath: effectiveWorktreePath } : {}), + env: runtimeEnv, + }, + }); + if (result._tag === "Failure" && !isAtomCommandInterrupted(result)) { + storeAbandonPendingTerminal(threadRef, terminalId); + } + })(); + }, + [ + effectiveWorktreePath, + openTerminal, + runtimeEnv, + storeAbandonPendingTerminal, + threadId, + threadRef, + ], + ); + const splitTerminal = useCallback(() => { if (!cwd) { return; @@ -749,26 +782,14 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const terminalId = nextTerminalId(allocatableTerminalIds); 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, threadRef, - openTerminal, ]); const splitTerminalVertical = useCallback(() => { if (!cwd) { @@ -777,25 +798,13 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const terminalId = nextTerminalId(allocatableTerminalIds); 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, threadRef, ]); @@ -806,26 +815,14 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const terminalId = nextTerminalId(allocatableTerminalIds); 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, threadRef, - openTerminal, ]); const activateTerminal = useCallback( @@ -1339,6 +1336,7 @@ function ChatViewContent(props: ChatViewProps) { 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); @@ -2599,6 +2597,40 @@ function ChatViewContent(props: ChatViewProps) { }, [activeThreadRef, storeSetTerminalOpen], ); + // A rejected open leaves no server session behind, so the optimistic id must + // not linger: pending ids are deliberately immune to reconcile. + const openThreadTerminalSession = useCallback( + (terminalId: string, sessionCwd: string, workspaceRoot: string) => { + if (!activeThreadRef || !activeThreadId) return; + const threadRef = activeThreadRef; + void (async () => { + 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" && !isAtomCommandInterrupted(result)) { + storeAbandonPendingTerminal(threadRef, terminalId); + } + })(); + }, + [ + activeThreadId, + activeThreadRef, + activeThreadWorktreePath, + environmentId, + openTerminal, + storeAbandonPendingTerminal, + ], + ); const toggleTerminalVisibility = useCallback(() => { if (!activeThreadRef) return; const nextOpen = !terminalUiState.terminalOpen; @@ -2612,19 +2644,7 @@ function ChatViewContent(props: ChatViewProps) { } const terminalId = nextTerminalId(allocatableActiveTerminalIds); 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); @@ -2632,11 +2652,9 @@ function ChatViewContent(props: ChatViewProps) { activeProject, activeThreadId, activeThreadRef, - activeThreadWorktreePath, allocatableActiveTerminalIds, - environmentId, gitCwd, - openTerminal, + openThreadTerminalSession, setTerminalOpen, storeEnsureTerminal, terminalUiState.terminalIds.length, @@ -2658,30 +2676,16 @@ function ChatViewContent(props: ChatViewProps) { 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, ], @@ -2697,26 +2701,13 @@ function ChatViewContent(props: ChatViewProps) { const terminalId = nextTerminalId(allocatableActiveTerminalIds); 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, @@ -2729,6 +2720,7 @@ function ChatViewContent(props: ChatViewProps) { environmentId, input: { threadId: activeThreadId, terminalId, data: "exit\n" }, }); + const threadRef = activeThreadRef; void (async () => { const closeResult = await closeTerminalMutation({ environmentId, @@ -2738,9 +2730,12 @@ function ChatViewContent(props: ChatViewProps) { deleteHistory: true, }, }); - if (closeResult._tag === "Failure" && !isAtomCommandInterrupted(closeResult)) { - await fallbackExitWrite(); - } + if (closeResult._tag !== "Failure" || isAtomCommandInterrupted(closeResult)) return; + const exitResult = await fallbackExitWrite(); + if (exitResult._tag !== "Failure" || isAtomCommandInterrupted(exitResult)) return; + // Neither path reached the session, so it is still running. Undo the + // optimistic suppression or reconcile would hide a live terminal. + storeUnsuppressTerminal(threadRef, terminalId); })(); storeCloseTerminal(activeThreadRef, terminalId); setTerminalFocusRequestId((value) => value + 1); @@ -2751,6 +2746,7 @@ function ChatViewContent(props: ChatViewProps) { closeTerminalMutation, environmentId, storeCloseTerminal, + storeUnsuppressTerminal, writeTerminal, ], ); @@ -2828,6 +2824,11 @@ function ChatViewContent(props: ChatViewProps) { const openResult = await openTerminal({ environmentId, input: openTerminalInput }); if (openResult._tag === "Failure") { if (!isAtomCommandInterrupted(openResult)) { + if (shouldCreateNewTerminal) { + // Nothing was created, so the optimistic id must not survive + // reconcile as a phantom tab. + storeAbandonPendingTerminal(activeThreadRef, targetTerminalId); + } const error = squashAtomCommandFailure(openResult); setThreadError( activeThreadId, diff --git a/apps/web/src/terminalUiStateStore.test.ts b/apps/web/src/terminalUiStateStore.test.ts index 25c945faff9..4820ae2b453 100644 --- a/apps/web/src/terminalUiStateStore.test.ts +++ b/apps/web/src/terminalUiStateStore.test.ts @@ -287,6 +287,29 @@ describe("terminalUiStateStore actions", () => { 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 510cfc9d825..c223e50cba4 100644 --- a/apps/web/src/terminalUiStateStore.ts +++ b/apps/web/src/terminalUiStateStore.ts @@ -630,6 +630,7 @@ interface TerminalUiStateStoreState { 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; @@ -764,6 +765,16 @@ export const useTerminalUiStateStore = create()( // 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. From 3bfce9b5983e2b806d5dc649c80a5caf54dfdc6a Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 09:51:57 +0200 Subject: [PATCH 12/23] fix(web): failed closes restore the terminal where it lived Two review findings on the close-rollback path: - a failed right-panel close re-surfaced the live terminal in the bottom drawer, because the rollback only cleared drawer-store suppression while the panel surface stayed removed; restore the panel surface as well - the drawer reconcile effect only watched the server session list, so a suppression rollback did not resurface the terminal until unrelated metadata changed; subscribe to the suppression and pending sets that feed the store's reconcile decision Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 6ea04893cc2..4e8c487384e 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -698,9 +698,25 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra // 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. + // Suppression and pending membership feed that decision, 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], + ); useEffect(() => { reconcileTerminalIds(threadRef, serverOrderedTerminalIds); - }, [reconcileTerminalIds, serverOrderedTerminalIds, threadRef]); + }, [ + pendingTerminalIds, + reconcileTerminalIds, + serverOrderedTerminalIds, + suppressedTerminalIds, + threadRef, + ]); const [localFocusRequestId, setLocalFocusRequestId] = useState(0); const worktreePath = serverThread?.worktreePath ?? draftThread?.worktreePath ?? null; const effectiveWorktreePath = useMemo(() => { @@ -3217,8 +3233,10 @@ function ChatViewContent(props: ChatViewProps) { input: { threadId: threadRef.threadId, terminalId, deleteHistory: true }, }); if (result._tag !== "Failure" || isAtomCommandInterrupted(result)) return; - // The session survived the failed close, so let reconcile surface it - // again instead of suppressing a live terminal forever. + // The session survived the failed close. Put it back where it lived — + // the right panel — and roll back the drawer-store suppression, so it + // neither vanishes nor resurfaces in the wrong surface. + useRightPanelStore.getState().openTerminal(threadRef, terminalId); storeUnsuppressTerminal(threadRef, terminalId); })(); storeCloseTerminal(activeThreadRef, terminalId); From e0ad1c8d4eb60144ad05917f96c9a8b342fa0fd1 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 09:58:20 +0200 Subject: [PATCH 13/23] fix(web): never reuse a terminal id that may still have an open in flight Review follow-up: closing an optimistic tab while its open request is pending freed the id for the next terminal, so the first request's failure handler could tear down the second terminal's tab. Suppressed and pending ids now stay reserved during allocation in both the drawer and the chat-level handlers, so a stale failure rollback can only ever touch an id that no longer exists. Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 59 ++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 4e8c487384e..e536eeda7cd 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -672,15 +672,37 @@ 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], + ); + // Suppressed ids stay reserved: a just-closed optimistic id may have an open + // request still in flight, and reusing it would let that request's failure + // handler tear down the wrong terminal. const allocatableTerminalIds = useMemo( () => [ ...new Set([ ...serverOrderedTerminalIds, ...terminalUiState.terminalIds, ...panelTerminalIds, + ...(suppressedTerminalIds ?? []), + ...(pendingTerminalIds ?? []), ]), ], - [panelTerminalIds, serverOrderedTerminalIds, terminalUiState.terminalIds], + [ + panelTerminalIds, + pendingTerminalIds, + serverOrderedTerminalIds, + suppressedTerminalIds, + terminalUiState.terminalIds, + ], ); const storeSetTerminalHeight = useTerminalUiStateStore((state) => state.setTerminalHeight); const storeSplitTerminal = useTerminalUiStateStore((state) => state.splitTerminal); @@ -698,16 +720,6 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra // 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. - // Suppression and pending membership feed that decision, 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], - ); useEffect(() => { reconcileTerminalIds(threadRef, serverOrderedTerminalIds); }, [ @@ -1513,9 +1525,30 @@ function ChatViewContent(props: ChatViewProps) { ), [rightPanelState.surfaces], ); + const activeSuppressedTerminalIds = useTerminalUiStateStore((state) => + activeThreadKey === null ? undefined : state.suppressedTerminalIdsByThreadKey[activeThreadKey], + ); + const activePendingTerminalIds = useTerminalUiStateStore((state) => + activeThreadKey === null ? undefined : state.pendingTerminalIdsByThreadKey[activeThreadKey], + ); + // Suppressed ids stay reserved: a just-closed optimistic id may have an open + // request still in flight, and reusing it would let that request's failure + // handler tear down the wrong terminal. const allocatableActiveTerminalIds = useMemo( - () => [...new Set([...activeKnownTerminalIds, ...panelTerminalIds])], - [activeKnownTerminalIds, panelTerminalIds], + () => [ + ...new Set([ + ...activeKnownTerminalIds, + ...panelTerminalIds, + ...(activeSuppressedTerminalIds ?? []), + ...(activePendingTerminalIds ?? []), + ]), + ], + [ + activeKnownTerminalIds, + activePendingTerminalIds, + activeSuppressedTerminalIds, + panelTerminalIds, + ], ); const previewPanelOpen = activeRightPanelKind === "preview" && isPreviewSupportedInRuntime(); const rightPanelOpen = rightPanelState.isOpen; From e72a9805464b3ef7973d6e75698375e0ffd4c534 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 10:10:48 +0200 Subject: [PATCH 14/23] fix(web): make terminal open/close rollbacks precise about unknown outcomes Four review findings on the rollback machinery: - a failed panel close now restores the terminal into its original surface at its original pane position and split direction via a pre-close snapshot and rightPanelStore.restoreTerminal, instead of a standalone surface that stranded split members - id reservation now tracks only opens actually in flight (a small per-thread registry held for the request window), instead of reserving every suppressed id, which would have skipped every terminal ever closed in the session and grown without bound - an interrupted open now abandons the optimistic id like a failed one: interruption is not confirmation, and if the session was created after all, reconcile re-adopts it from server metadata - an interrupted close now rolls back the optimistic suppression in all drawer, chat-level, and panel close paths, converging on server truth instead of treating interruption as success Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 238 +++++++++++------- .../src/lib/terminalOpenReservations.test.ts | 31 +++ apps/web/src/lib/terminalOpenReservations.ts | 29 +++ apps/web/src/rightPanelStore.test.ts | 56 +++++ apps/web/src/rightPanelStore.ts | 56 +++++ 5 files changed, 319 insertions(+), 91 deletions(-) create mode 100644 apps/web/src/lib/terminalOpenReservations.test.ts create mode 100644 apps/web/src/lib/terminalOpenReservations.ts diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index e536eeda7cd..a38d2e94b40 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -122,6 +122,7 @@ import { selectActiveRightPanelSurface, selectThreadRightPanelState, type RightPanelSurface, + type TerminalSurfaceSnapshot, useRightPanelStore, } from "../rightPanelStore"; import { @@ -201,6 +202,11 @@ 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 { selectThreadTerminalUiState, useTerminalUiStateStore } from "../terminalUiStateStore"; import { useKnownTerminalSessions, useThreadRunningTerminalIds } from "../state/terminalSessions"; import { projectEnvironment } from "../state/projects"; @@ -683,26 +689,15 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra const pendingTerminalIds = useTerminalUiStateStore( (state) => state.pendingTerminalIdsByThreadKey[threadKey], ); - // Suppressed ids stay reserved: a just-closed optimistic id may have an open - // request still in flight, and reusing it would let that request's failure - // handler tear down the wrong terminal. const allocatableTerminalIds = useMemo( () => [ ...new Set([ ...serverOrderedTerminalIds, ...terminalUiState.terminalIds, ...panelTerminalIds, - ...(suppressedTerminalIds ?? []), - ...(pendingTerminalIds ?? []), ]), ], - [ - panelTerminalIds, - pendingTerminalIds, - serverOrderedTerminalIds, - suppressedTerminalIds, - terminalUiState.terminalIds, - ], + [panelTerminalIds, serverOrderedTerminalIds, terminalUiState.terminalIds], ); const storeSetTerminalHeight = useTerminalUiStateStore((state) => state.setTerminalHeight); const storeSplitTerminal = useTerminalUiStateStore((state) => state.splitTerminal); @@ -773,23 +768,30 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra [storeSetTerminalHeight, threadRef], ); - // A rejected open leaves no server session behind, so the optimistic id must - // not linger: pending ids are deliberately immune to reconcile. + // 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 () => { - const result = await openTerminal({ - environmentId: threadRef.environmentId, - input: { - threadId, - terminalId, - cwd: sessionCwd, - ...(effectiveWorktreePath != null ? { worktreePath: effectiveWorktreePath } : {}), - env: runtimeEnv, - }, - }); - if (result._tag === "Failure" && !isAtomCommandInterrupted(result)) { - storeAbandonPendingTerminal(threadRef, terminalId); + 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); } })(); }, @@ -799,6 +801,7 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra runtimeEnv, storeAbandonPendingTerminal, threadId, + threadKey, threadRef, ], ); @@ -807,7 +810,10 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra if (!cwd) { return; } - const terminalId = nextTerminalId(allocatableTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableTerminalIds, + ...reservedTerminalOpenIds(threadKey), + ]); storeSplitTerminal(threadRef, terminalId); bumpFocusRequestId(); openTerminalSession(terminalId, cwd); @@ -817,13 +823,17 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra cwd, openTerminalSession, storeSplitTerminal, + threadKey, threadRef, ]); const splitTerminalVertical = useCallback(() => { if (!cwd) { return; } - const terminalId = nextTerminalId(allocatableTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableTerminalIds, + ...reservedTerminalOpenIds(threadKey), + ]); storeSplitTerminalVertical(threadRef, terminalId); bumpFocusRequestId(); openTerminalSession(terminalId, cwd); @@ -833,6 +843,7 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra cwd, openTerminalSession, storeSplitTerminalVertical, + threadKey, threadRef, ]); @@ -840,7 +851,10 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra if (!cwd) { return; } - const terminalId = nextTerminalId(allocatableTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableTerminalIds, + ...reservedTerminalOpenIds(threadKey), + ]); storeNewTerminal(threadRef, terminalId); bumpFocusRequestId(); openTerminalSession(terminalId, cwd); @@ -850,6 +864,7 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra allocatableTerminalIds, openTerminalSession, storeNewTerminal, + threadKey, threadRef, ]); @@ -878,11 +893,18 @@ const PersistentThreadTerminalDrawer = memo(function PersistentThreadTerminalDra deleteHistory: true, }, }); - if (closeResult._tag !== "Failure" || isAtomCommandInterrupted(closeResult)) return; + 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" || isAtomCommandInterrupted(exitResult)) return; - // Neither path reached the session, so it is still running. Undo the - // optimistic suppression or reconcile would hide a live terminal. + 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); })(); @@ -1525,30 +1547,9 @@ function ChatViewContent(props: ChatViewProps) { ), [rightPanelState.surfaces], ); - const activeSuppressedTerminalIds = useTerminalUiStateStore((state) => - activeThreadKey === null ? undefined : state.suppressedTerminalIdsByThreadKey[activeThreadKey], - ); - const activePendingTerminalIds = useTerminalUiStateStore((state) => - activeThreadKey === null ? undefined : state.pendingTerminalIdsByThreadKey[activeThreadKey], - ); - // Suppressed ids stay reserved: a just-closed optimistic id may have an open - // request still in flight, and reusing it would let that request's failure - // handler tear down the wrong terminal. const allocatableActiveTerminalIds = useMemo( - () => [ - ...new Set([ - ...activeKnownTerminalIds, - ...panelTerminalIds, - ...(activeSuppressedTerminalIds ?? []), - ...(activePendingTerminalIds ?? []), - ]), - ], - [ - activeKnownTerminalIds, - activePendingTerminalIds, - activeSuppressedTerminalIds, - panelTerminalIds, - ], + () => [...new Set([...activeKnownTerminalIds, ...panelTerminalIds])], + [activeKnownTerminalIds, panelTerminalIds], ); const previewPanelOpen = activeRightPanelKind === "preview" && isPreviewSupportedInRuntime(); const rightPanelOpen = rightPanelState.isOpen; @@ -2646,28 +2647,38 @@ function ChatViewContent(props: ChatViewProps) { }, [activeThreadRef, storeSetTerminalOpen], ); - // A rejected open leaves no server session behind, so the optimistic id must - // not linger: pending ids are deliberately immune to reconcile. + // 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) => { if (!activeThreadRef || !activeThreadId) return; const threadRef = activeThreadRef; + const threadKey = scopedThreadKey(threadRef); + reserveTerminalOpen(threadKey, terminalId); void (async () => { - 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" && !isAtomCommandInterrupted(result)) { - storeAbandonPendingTerminal(threadRef, terminalId); + 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); + } + } finally { + releaseTerminalOpen(threadKey, terminalId); } })(); }, @@ -2691,7 +2702,10 @@ function ChatViewContent(props: ChatViewProps) { if (!cwdForOpen) { return; } - const terminalId = nextTerminalId(allocatableActiveTerminalIds); + const terminalId = nextTerminalId([ + ...allocatableActiveTerminalIds, + ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), + ]); storeEnsureTerminal(activeThreadRef, terminalId, { open: true }); openThreadTerminalSession(terminalId, cwdForOpen, activeProject.workspaceRoot); return; @@ -2718,7 +2732,10 @@ 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 { @@ -2747,7 +2764,10 @@ 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); openThreadTerminalSession(terminalId, cwdForOpen, activeProject.workspaceRoot); @@ -2779,11 +2799,18 @@ function ChatViewContent(props: ChatViewProps) { deleteHistory: true, }, }); - if (closeResult._tag !== "Failure" || isAtomCommandInterrupted(closeResult)) return; + 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" || isAtomCommandInterrupted(exitResult)) return; - // Neither path reached the session, so it is still running. Undo the - // optimistic suppression or reconcile would hide a live terminal. + 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); @@ -2844,7 +2871,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 ? { @@ -2866,18 +2896,27 @@ 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)) { - if (shouldCreateNewTerminal) { - // Nothing was created, so the optimistic id must not survive - // reconcile as a phantom tab. - storeAbandonPendingTerminal(activeThreadRef, targetTerminalId); - } const error = squashAtomCommandFailure(openResult); setThreadError( activeThreadId, @@ -3260,16 +3299,33 @@ function ChatViewContent(props: ChatViewProps) { (terminalId: string) => { if (!activeThreadRef || activeRightPanelSurface?.kind !== "terminal") return; 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" || isAtomCommandInterrupted(result)) return; + if (result._tag !== "Failure") return; + if (isAtomCommandInterrupted(result)) { + // Unknown outcome: only roll back the suppression. If the session + // survived, drawer reconcile resurfaces it; restoring the panel + // surface here could pin a dead pane no reconcile pass would remove. + storeUnsuppressTerminal(threadRef, terminalId); + return; + } // The session survived the failed close. Put it back where it lived — - // the right panel — and roll back the drawer-store suppression, so it - // neither vanishes nor resurfaces in the wrong surface. - useRightPanelStore.getState().openTerminal(threadRef, terminalId); + // 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); 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/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) => ({ From 5e0c3c3441cc8124d5562eb6c2581ba224f9c866 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 10:51:56 +0200 Subject: [PATCH 15/23] fix(web): settle panel terminal opens and closes against server metadata Review re-check follow-ups plus a reported reflow artifact: - panel add/split now allocate against the in-flight reservation registry and open through the shared helper, so closing a panel terminal mid-open can no longer free its id for the next panel open while the request is unresolved - an interrupted panel close now parks its surface snapshot instead of guessing: authoritative session metadata restores the terminal into its original surface if the session survived, or drops the snapshot if it did not - the PTY now hears the leading edge of a resize burst as well as the settled size. Trailing-only notification left the shell on its old width for the whole drag while the grid rewrapped already-printed output, stranding fragments of a wrapped prompt above the live one on narrow panes Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 75 ++++++++++--------- .../lib/terminalPendingPanelCloses.test.ts | 56 ++++++++++++++ .../web/src/lib/terminalPendingPanelCloses.ts | 53 +++++++++++++ apps/web/src/terminal/ghostty/surface.ts | 18 ++++- 4 files changed, 162 insertions(+), 40 deletions(-) create mode 100644 apps/web/src/lib/terminalPendingPanelCloses.test.ts create mode 100644 apps/web/src/lib/terminalPendingPanelCloses.ts diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index a38d2e94b40..510b6c10096 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -207,6 +207,10 @@ import { reservedTerminalOpenIds, reserveTerminalOpen, } from "../lib/terminalOpenReservations"; +import { + recordPendingPanelClose, + resolvePendingPanelCloses, +} from "../lib/terminalPendingPanelCloses"; import { selectThreadTerminalUiState, useTerminalUiStateStore } from "../terminalUiStateStore"; import { useKnownTerminalSessions, useThreadRunningTerminalIds } from "../state/terminalSessions"; import { projectEnvironment } from "../state/projects"; @@ -1547,6 +1551,20 @@ 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. + useEffect(() => { + if (!activeThreadRef) return; + const restored = resolvePendingPanelCloses( + scopedThreadKey(activeThreadRef), + activeServerOrderedTerminalIds, + ); + for (const { terminalId, snapshot } of restored) { + useRightPanelStore.getState().restoreTerminal(activeThreadRef, snapshot, terminalId); + storeUnsuppressTerminal(activeThreadRef, terminalId); + } + }, [activeServerOrderedTerminalIds, activeThreadRef, storeUnsuppressTerminal]); const allocatableActiveTerminalIds = useMemo( () => [...new Set([...activeKnownTerminalIds, ...panelTerminalIds])], [activeKnownTerminalIds, panelTerminalIds], @@ -3215,30 +3233,24 @@ function ChatViewContent(props: ChatViewProps) { const addTerminalSurface = useCallback(() => { if (!activeThreadRef || !activeThreadId || !activeProject) return; const cwd = gitCwd ?? activeProject.workspaceRoot; - const terminalId = nextTerminalId(allocatableActiveTerminalIds); + // 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)), + ]); useRightPanelStore.getState().openTerminal(activeThreadRef, 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); }, [ activeProject, activeThreadId, activeThreadRef, - activeThreadWorktreePath, allocatableActiveTerminalIds, gitCwd, - openTerminal, + openThreadTerminalSession, ]); const splitPanelTerminal = useCallback( (direction: "horizontal" | "vertical" = "horizontal") => { @@ -3251,35 +3263,25 @@ 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); 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); }, [ activeProject, activeRightPanelSurface, activeThreadId, activeThreadRef, - activeThreadWorktreePath, allocatableActiveTerminalIds, gitCwd, - openTerminal, + openThreadTerminalSession, ], ); const splitPanelTerminalVertical = useCallback(() => { @@ -3316,10 +3318,11 @@ function ChatViewContent(props: ChatViewProps) { }); if (result._tag !== "Failure") return; if (isAtomCommandInterrupted(result)) { - // Unknown outcome: only roll back the suppression. If the session - // survived, drawer reconcile resurfaces it; restoring the panel - // surface here could pin a dead pane no reconcile pass would remove. - storeUnsuppressTerminal(threadRef, terminalId); + // 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 — diff --git a/apps/web/src/lib/terminalPendingPanelCloses.test.ts b/apps/web/src/lib/terminalPendingPanelCloses.test.ts new file mode 100644 index 00000000000..979a04ed87b --- /dev/null +++ b/apps/web/src/lib/terminalPendingPanelCloses.test.ts @@ -0,0 +1,56 @@ +import { beforeEach, describe, expect, it } from "vite-plus/test"; + +import { + clearPendingPanelCloses, + recordPendingPanelClose, + resolvePendingPanelCloses, +} from "./terminalPendingPanelCloses"; + +const snapshot = { + surfaceId: "terminal:terminal-1" as const, + resourceId: "terminal-1", + terminalIds: ["terminal-1", "terminal-2"], + splitDirection: "vertical" as const, +}; + +beforeEach(() => { + clearPendingPanelCloses("thread-a"); +}); + +describe("terminalPendingPanelCloses", () => { + it("restores a snapshot when metadata shows the session survived the interrupted close", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot); + + expect(resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"])).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"])).toEqual([]); + }); + + it("discards a snapshot when metadata shows the session really closed", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot); + + expect(resolvePendingPanelCloses("thread-a", ["terminal-1"])).toEqual([]); + expect(resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"])).toEqual([]); + }); + + it("waits for loaded metadata instead of resolving against an empty list", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot); + + // An empty list is indistinguishable from metadata that has not arrived. + expect(resolvePendingPanelCloses("thread-a", [])).toEqual([]); + expect(resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"])).toEqual([ + { terminalId: "terminal-2", snapshot }, + ]); + }); + + it("scopes pending closes per thread", () => { + recordPendingPanelClose("thread-a", "terminal-2", snapshot); + recordPendingPanelClose("thread-b", "terminal-2", snapshot); + + expect(resolvePendingPanelCloses("thread-a", ["terminal-2"])).toHaveLength(1); + expect(resolvePendingPanelCloses("thread-b", ["terminal-2"])).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..467b607d7d1 --- /dev/null +++ b/apps/web/src/lib/terminalPendingPanelCloses.ts @@ -0,0 +1,53 @@ +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. + */ +const pendingByThreadKey = new Map>(); + +export function recordPendingPanelClose( + threadKey: string, + terminalId: string, + snapshot: TerminalSurfaceSnapshot, +): void { + const pending = pendingByThreadKey.get(threadKey) ?? new Map(); + pending.set(terminalId, snapshot); + pendingByThreadKey.set(threadKey, pending); +} + +/** + * Settles every pending close for a thread against the server's session list, + * returning the ones whose session survived so the caller can restore them. + * Ids the server no longer reports were really closed and are simply dropped. + * + * `serverTerminalIds` must be a loaded, non-empty list: an empty one is + * indistinguishable from metadata that has not arrived, and would resolve every + * entry as "closed". + */ +export function resolvePendingPanelCloses( + threadKey: string, + serverTerminalIds: readonly string[], +): Array<{ terminalId: string; snapshot: TerminalSurfaceSnapshot }> { + const pending = pendingByThreadKey.get(threadKey); + if (!pending || serverTerminalIds.length === 0) return []; + + const serverIds = new Set(serverTerminalIds); + const survived: Array<{ terminalId: string; snapshot: TerminalSurfaceSnapshot }> = []; + for (const [terminalId, snapshot] of pending) { + if (serverIds.has(terminalId)) { + survived.push({ terminalId, snapshot }); + } + } + pendingByThreadKey.delete(threadKey); + return survived; +} + +export function clearPendingPanelCloses(threadKey: string): void { + pendingByThreadKey.delete(threadKey); +} diff --git a/apps/web/src/terminal/ghostty/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index 8111400b725..0e8db5e1fe1 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -604,13 +604,23 @@ export class GhosttyTerminalSurface { } /** - * 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. + * The local grid reflows on every step, so the PTY hears the start of a + * resize immediately and the settled size once the burst ends. + * + * Trailing-only notification let the shell keep its old width across the + * whole drag: the grid rewrapped the already-printed prompt while the shell + * redrew at the end, stranding the rewrapped fragments above the live prompt. + * That is only visible when a line wraps, which is why narrow panes with long + * prompts showed it. Intermediate steps stay coalesced so the shell does not + * reprint per frame, which reads as jitter. */ private notifyResize(): void { this.resizeNotified = true; - if (this.resizeNotifyTimer !== null) window.clearTimeout(this.resizeNotifyTimer); + if (this.resizeNotifyTimer === null) { + this.options.onResize(this.cols, this.rows); + } else { + window.clearTimeout(this.resizeNotifyTimer); + } this.resizeNotifyTimer = window.setTimeout(() => { this.resizeNotifyTimer = null; if (!this.disposed) this.options.onResize(this.cols, this.rows); From 5fd1ac35031e0c212dad71dbea2bb218a1e0ae0e Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 10:56:45 +0200 Subject: [PATCH 16/23] fix(web): resize the PTY in lockstep with the terminal grid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous leading-edge notify only helped the first step of a drag: the timer swallowed every intermediate step, so the shell's redraw sequences were still computed for one width and interpreted at another, and each mismatched redraw orphaned the previous prompt as a stranded fragment. Notify on every grid change instead, like a native terminal's SIGWINCH — fit() already coalesces to one call per crossed cell boundary, so a drag cannot flood the PTY. Co-Authored-By: Claude Fable 5 --- apps/web/src/terminal/ghostty/surface.ts | 34 +++++++----------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/apps/web/src/terminal/ghostty/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index 0e8db5e1fe1..20e4d61dc28 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -351,7 +351,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; @@ -604,27 +603,19 @@ export class GhosttyTerminalSurface { } /** - * The local grid reflows on every step, so the PTY hears the start of a - * resize immediately and the settled size once the burst ends. + * The PTY must track every grid change, like a native terminal's SIGWINCH. * - * Trailing-only notification let the shell keep its old width across the - * whole drag: the grid rewrapped the already-printed prompt while the shell - * redrew at the end, stranding the rewrapped fragments above the live prompt. - * That is only visible when a line wraps, which is why narrow panes with long - * prompts showed it. Intermediate steps stay coalesced so the shell does not - * reprint per frame, which reads as jitter. + * Debouncing let the grid and the shell disagree on width for the length of + * a drag: the shell's redraw sequences were computed for one width but + * interpreted at another, so its cursor-up arithmetic missed and each redraw + * orphaned the previous prompt as a stranded fragment — permanently, once it + * scrolled out of the shell's edit region. Per-step notification keeps the + * widths in lockstep; `fit()` already coalesces to one call per changed grid + * size, so a drag produces at most one notify per crossed cell boundary. */ private notifyResize(): void { this.resizeNotified = true; - if (this.resizeNotifyTimer === null) { - this.options.onResize(this.cols, this.rows); - } else { - window.clearTimeout(this.resizeNotifyTimer); - } - this.resizeNotifyTimer = window.setTimeout(() => { - this.resizeNotifyTimer = null; - if (!this.disposed) this.options.onResize(this.cols, this.rows); - }, 150); + this.options.onResize(this.cols, this.rows); } focus(): void { @@ -696,13 +687,6 @@ 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.frame !== 0) window.cancelAnimationFrame(this.frame); if (this.cursorTimer !== null) window.clearTimeout(this.cursorTimer); if (this.compositionSuppressionTimer !== null) { From 58bf3de72da0ff7a96e3be5cc82aa9e0a6d1377d Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 11:11:48 +0200 Subject: [PATCH 17/23] fix(web): apply grid resizes only once the PTY acknowledges Lockstep notification was not enough: the shell's redraw bytes are generated against the width the server pty had at generation time, and during a drag the client grid had already moved on, so each mismatched redraw still orphaned the previous prompt as a stranded fragment. Sequence the resize into the output stream instead. fit() requests the resize and keeps the old grid; the acknowledgement arrives on the same socket after all old-width output, so core.resize lands exactly where the server pty flipped and every byte is interpreted at the width it was generated for. Superseded acknowledgements are ignored via a sequence counter, and a failed request still applies locally so the grid cannot get stuck. Verified in a live paired session: clear + six wrap-crossing viewport steps leave a single correctly wrapped prompt with no stale fragments, where the debounced code stacked one fragment per redraw. Co-Authored-By: Claude Fable 5 --- .../src/components/ThreadTerminalDrawer.tsx | 5 +- apps/web/src/terminal/ghostty/surface.ts | 73 ++++++++++++------- 2 files changed, 52 insertions(+), 26 deletions(-) diff --git a/apps/web/src/components/ThreadTerminalDrawer.tsx b/apps/web/src/components/ThreadTerminalDrawer.tsx index dfbd0c7a44f..85460837cb7 100644 --- a/apps/web/src/components/ThreadTerminalDrawer.tsx +++ b/apps/web/src/components/ThreadTerminalDrawer.tsx @@ -381,7 +381,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), diff --git a/apps/web/src/terminal/ghostty/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index 20e4d61dc28..c470e5ed92c 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -314,7 +314,12 @@ 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`). + */ + readonly onResize: (cols: number, rows: number) => Promise | void; readonly onSelectionChange: () => void; readonly onCopy: (text: string) => void; readonly beforeKey: (event: KeyboardEvent) => boolean; @@ -374,6 +379,9 @@ export class GhosttyTerminalSurface { private composing = false; private focused = false; private resizeNotified = false; + private requestedCols = 0; + private requestedRows = 0; + private resizeSeq = 0; private canvasConfigured = false; private theme: GhosttyTheme; private readonly suppressedKeyCodes = new Set(); @@ -584,16 +592,35 @@ 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 request the resize first and apply the grid change only + // once the request settles: the acknowledgement arrives on the same socket + // after all old-width output, so bytes keep being interpreted at the width + // they were generated 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.requestedCols || + grid.rows !== this.requestedRows || + !this.resizeNotified + ) { + this.resizeNotified = true; + this.requestedCols = grid.cols; + this.requestedRows = grid.rows; + const seq = ++this.resizeSeq; + const apply = () => this.applyGridSize(seq, grid.cols, grid.rows); + const acknowledgement = this.options.onResize(grid.cols, grid.rows); + if (acknowledgement && typeof acknowledgement.then === "function") { + // Applied even when the request fails: a stuck grid is worse than a + // transient width mismatch, and the next fit re-requests anyway. + void acknowledgement.then(apply, apply); + } else { + apply(); + } } // Rendering synchronously keeps the repaint inside the same frame as the // layout change: ResizeObserver fires before paint, so the browser never @@ -602,20 +629,16 @@ export class GhosttyTerminalSurface { return true; } - /** - * The PTY must track every grid change, like a native terminal's SIGWINCH. - * - * Debouncing let the grid and the shell disagree on width for the length of - * a drag: the shell's redraw sequences were computed for one width but - * interpreted at another, so its cursor-up arithmetic missed and each redraw - * orphaned the previous prompt as a stranded fragment — permanently, once it - * scrolled out of the shell's edit region. Per-step notification keeps the - * widths in lockstep; `fit()` already coalesces to one call per changed grid - * size, so a drag produces at most one notify per crossed cell boundary. - */ - private notifyResize(): void { - this.resizeNotified = true; - this.options.onResize(this.cols, this.rows); + /** Applies a granted grid size, ignoring superseded acknowledgements. */ + private applyGridSize(seq: number, cols: number, rows: number): void { + if (this.disposed || seq !== this.resizeSeq) return; + 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 { From c9b75cf1dbd5f4cd2cb2ce63d688ca0257143b10 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 11:22:48 +0200 Subject: [PATCH 18/23] fix(web): never skip a PTY width the shell already redrew for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex-assisted diagnosis of the remaining resize artifact: the resize RPC layer coalesces with mode 'latest', and applyGridSize discarded acknowledgements superseded by a newer measurement. When a drag produced several measurements while one request was in flight, the server PTY visited widths the client grid skipped — the shell redrew for a width the emulator never occupied, which is exactly the stranded-fragment mechanism. This is why coarse jumps still failed while slow fine steps were clean. Resizes are now single-flight: one request at a time, its exact size always applied on settle, then the newest measurement is requested if it moved on. The emulator steps through every width the PTY entered, in order. Co-Authored-By: Claude Fable 5 --- apps/web/src/terminal/ghostty/surface.ts | 72 ++++++++++++++---------- 1 file changed, 43 insertions(+), 29 deletions(-) diff --git a/apps/web/src/terminal/ghostty/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index c470e5ed92c..28807eae253 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -378,10 +378,10 @@ export class GhosttyTerminalSurface { private selectionMoved = false; private composing = false; private focused = false; - private resizeNotified = false; - private requestedCols = 0; - private requestedRows = 0; - private resizeSeq = 0; + private resizeRequestedOnce = false; + private desiredCols = 0; + private desiredRows = 0; + private resizeRequestActive = false; private canvasConfigured = false; private theme: GhosttyTheme; private readonly suppressedKeyCodes = new Set(); @@ -597,30 +597,22 @@ export class GhosttyTerminalSurface { // 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 request the resize first and apply the grid change only - // once the request settles: the acknowledgement arrives on the same socket - // after all old-width output, so bytes keep being interpreted at the width - // they were generated 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. + // 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.requestedCols || - grid.rows !== this.requestedRows || - !this.resizeNotified + grid.cols !== this.desiredCols || + grid.rows !== this.desiredRows || + !this.resizeRequestedOnce ) { - this.resizeNotified = true; - this.requestedCols = grid.cols; - this.requestedRows = grid.rows; - const seq = ++this.resizeSeq; - const apply = () => this.applyGridSize(seq, grid.cols, grid.rows); - const acknowledgement = this.options.onResize(grid.cols, grid.rows); - if (acknowledgement && typeof acknowledgement.then === "function") { - // Applied even when the request fails: a stuck grid is worse than a - // transient width mismatch, and the next fit re-requests anyway. - void acknowledgement.then(apply, apply); - } else { - apply(); - } + 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 @@ -629,9 +621,31 @@ export class GhosttyTerminalSurface { return true; } - /** Applies a granted grid size, ignoring superseded acknowledgements. */ - private applyGridSize(seq: number, cols: number, rows: number): void { - if (this.disposed || seq !== this.resizeSeq) return; + 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 = () => { + if (this.disposed) return; + this.resizeRequestActive = false; + // Applied even when the request fails: a stuck grid is worse than a + // transient width mismatch, and the next fit re-requests anyway. + this.applyGridSize(cols, rows); + if (this.desiredCols !== cols || this.desiredRows !== rows) { + this.requestResize(); + } + }; + const acknowledgement = this.options.onResize(cols, rows); + if (acknowledgement && typeof acknowledgement.then === "function") { + void acknowledgement.then(settle, settle); + } else { + settle(); + } + } + + private applyGridSize(cols: number, rows: number): void { if (cols === this.cols && rows === this.rows) return; this.cols = cols; this.rows = rows; From 80706b04ca685dd5e015340fdf73b5b24efe2766 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 11:27:35 +0200 Subject: [PATCH 19/23] fix(web): settle recorded panel closes immediately and honor denied resizes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings: - recording a pending panel close now notifies subscribers, and the settling effect subscribes via useSyncExternalStore. Without this, a close recorded after its settling metadata already arrived — or one whose session survived, so the list never changes — left the terminal suppressed and paneless forever - a denied PTY resize no longer moves the grid: the drawer resolves the acknowledgement with the request outcome, and the surface keeps the grid at the PTY's size and re-aligns the desired size so the same measurement retries instead of being skipped. The pre-grant exception keeps mount bookkeeping aligned with the constructed core Co-Authored-By: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 19 ++++++++-- .../src/components/ThreadTerminalDrawer.tsx | 11 +++--- .../lib/terminalPendingPanelCloses.test.ts | 20 +++++++++++ .../web/src/lib/terminalPendingPanelCloses.ts | 27 ++++++++++++++ apps/web/src/terminal/ghostty/surface.ts | 35 ++++++++++++++----- 5 files changed, 97 insertions(+), 15 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 510b6c10096..d08183a89ec 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"; @@ -208,8 +209,10 @@ import { reserveTerminalOpen, } from "../lib/terminalOpenReservations"; import { + pendingPanelCloseVersion, recordPendingPanelClose, resolvePendingPanelCloses, + subscribePendingPanelCloses, } from "../lib/terminalPendingPanelCloses"; import { selectThreadTerminalUiState, useTerminalUiStateStore } from "../terminalUiStateStore"; import { useKnownTerminalSessions, useThreadRunningTerminalIds } from "../state/terminalSessions"; @@ -1553,7 +1556,14 @@ function ChatViewContent(props: ChatViewProps) { ); // 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. + // 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, + ); useEffect(() => { if (!activeThreadRef) return; const restored = resolvePendingPanelCloses( @@ -1564,7 +1574,12 @@ function ChatViewContent(props: ChatViewProps) { useRightPanelStore.getState().restoreTerminal(activeThreadRef, snapshot, terminalId); storeUnsuppressTerminal(activeThreadRef, terminalId); } - }, [activeServerOrderedTerminalIds, activeThreadRef, storeUnsuppressTerminal]); + }, [ + activePendingPanelCloseVersion, + activeServerOrderedTerminalIds, + activeThreadRef, + storeUnsuppressTerminal, + ]); const allocatableActiveTerminalIds = useMemo( () => [...new Set([...activeKnownTerminalIds, ...panelTerminalIds])], [activeKnownTerminalIds, panelTerminalIds], diff --git a/apps/web/src/components/ThreadTerminalDrawer.tsx b/apps/web/src/components/ThreadTerminalDrawer.tsx index 85460837cb7..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; diff --git a/apps/web/src/lib/terminalPendingPanelCloses.test.ts b/apps/web/src/lib/terminalPendingPanelCloses.test.ts index 979a04ed87b..5709df22754 100644 --- a/apps/web/src/lib/terminalPendingPanelCloses.test.ts +++ b/apps/web/src/lib/terminalPendingPanelCloses.test.ts @@ -2,8 +2,10 @@ import { beforeEach, describe, expect, it } from "vite-plus/test"; import { clearPendingPanelCloses, + pendingPanelCloseVersion, recordPendingPanelClose, resolvePendingPanelCloses, + subscribePendingPanelCloses, } from "./terminalPendingPanelCloses"; const snapshot = { @@ -45,6 +47,24 @@ describe("terminalPendingPanelCloses", () => { ]); }); + it("notifies subscribers when a close is recorded", () => { + // The settling effect must re-run even when session metadata never changes + // again (the session survived, or metadata arrived before the record). + let notified = 0; + const unsubscribe = subscribePendingPanelCloses(() => { + notified += 1; + }); + const versionBefore = pendingPanelCloseVersion(); + + recordPendingPanelClose("thread-a", "terminal-2", snapshot); + + expect(notified).toBe(1); + expect(pendingPanelCloseVersion()).toBeGreaterThan(versionBefore); + unsubscribe(); + recordPendingPanelClose("thread-a", "terminal-3", snapshot); + expect(notified).toBe(1); + }); + it("scopes pending closes per thread", () => { recordPendingPanelClose("thread-a", "terminal-2", snapshot); recordPendingPanelClose("thread-b", "terminal-2", snapshot); diff --git a/apps/web/src/lib/terminalPendingPanelCloses.ts b/apps/web/src/lib/terminalPendingPanelCloses.ts index 467b607d7d1..5417e9d4bc3 100644 --- a/apps/web/src/lib/terminalPendingPanelCloses.ts +++ b/apps/web/src/lib/terminalPendingPanelCloses.ts @@ -11,6 +11,32 @@ import type { TerminalSurfaceSnapshot } from "../rightPanelStore"; */ 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, @@ -19,6 +45,7 @@ export function recordPendingPanelClose( const pending = pendingByThreadKey.get(threadKey) ?? new Map(); pending.set(terminalId, snapshot); pendingByThreadKey.set(threadKey, pending); + notifyPendingPanelCloses(); } /** diff --git a/apps/web/src/terminal/ghostty/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index 28807eae253..68f9b489b08 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -317,9 +317,10 @@ export interface GhosttyTerminalSurfaceOptions { /** * 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`). + * 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 onResize: (cols: number, rows: number) => Promise | void; readonly onSelectionChange: () => void; readonly onCopy: (text: string) => void; readonly beforeKey: (event: KeyboardEvent) => boolean; @@ -379,6 +380,7 @@ export class GhosttyTerminalSurface { private composing = false; private focused = false; private resizeRequestedOnce = false; + private resizeGranted = false; private desiredCols = 0; private desiredRows = 0; private resizeRequestActive = false; @@ -627,21 +629,36 @@ export class GhosttyTerminalSurface { this.resizeRequestedOnce = true; const cols = this.desiredCols; const rows = this.desiredRows; - const settle = () => { + const settle = (granted: boolean) => { if (this.disposed) return; this.resizeRequestActive = false; - // Applied even when the request fails: a stuck grid is worse than a - // transient width mismatch, and the next fit re-requests anyway. - this.applyGridSize(cols, rows); - if (this.desiredCols !== cols || this.desiredRows !== rows) { + const hasNewerMeasurement = this.desiredCols !== cols || this.desiredRows !== rows; + if (granted || !this.resizeGranted) { + // The pre-grant exception keeps mount bookkeeping honest: 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. + this.resizeGranted = this.resizeGranted || granted; + this.applyGridSize(cols, rows); + } else if (!hasNewerMeasurement) { + // The PTY kept its old size. Keep the grid with it, and align the + // desired size with the applied one so a future fit at the failed + // size re-requests instead of being skipped. + this.desiredCols = this.cols; + this.desiredRows = this.rows; + } + if (hasNewerMeasurement) { this.requestResize(); } }; const acknowledgement = this.options.onResize(cols, rows); if (acknowledgement && typeof acknowledgement.then === "function") { - void acknowledgement.then(settle, settle); + void acknowledgement.then( + (granted) => settle(granted !== false), + () => settle(false), + ); } else { - settle(); + settle(true); } } From e38c25022fa0c7095cb157d4db2fd6e4921c56fd Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 11:41:44 +0200 Subject: [PATCH 20/23] fix(terminal): close three review gaps in resize and pending-close handling - pending panel closes settle only after a grace window, so a close whose metadata has not propagated yet cannot be restored from a stale cached session list and pin a dead pane; entries stay pending until post-grace metadata decides, with tests for the stale-cache window - the first-settle bookkeeping exception applies exactly once: repeated failed resizes before any grant no longer keep moving the grid while the PTY stays put - a resize while the terminal process is not running now records the session dimensions, so restart (which falls back to session.cols/rows) spawns at the size the client was acknowledged for instead of the stale one Co-Authored-By: Claude Fable 5 --- apps/server/src/terminal/Manager.ts | 5 + .../lib/terminalPendingPanelCloses.test.ts | 97 ++++++++++++------- .../web/src/lib/terminalPendingPanelCloses.ts | 36 +++++-- apps/web/src/terminal/ghostty/surface.ts | 15 +-- 4 files changed, 104 insertions(+), 49 deletions(-) diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index a94ebc6db76..36825836dd8 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -2528,6 +2528,11 @@ 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); diff --git a/apps/web/src/lib/terminalPendingPanelCloses.test.ts b/apps/web/src/lib/terminalPendingPanelCloses.test.ts index 5709df22754..b64ca0b0d57 100644 --- a/apps/web/src/lib/terminalPendingPanelCloses.test.ts +++ b/apps/web/src/lib/terminalPendingPanelCloses.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it } from "vite-plus/test"; +import { beforeEach, describe, expect, it, vi } from "vite-plus/test"; import { clearPendingPanelCloses, @@ -15,62 +15,89 @@ const snapshot = { 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 metadata shows the session survived the interrupted close", () => { - recordPendingPanelClose("thread-a", "terminal-2", snapshot); + 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"])).toEqual([ - { terminalId: "terminal-2", snapshot }, - ]); + 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"])).toEqual([]); + 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); + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); - expect(resolvePendingPanelCloses("thread-a", ["terminal-1"])).toEqual([]); - expect(resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"])).toEqual([]); + 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); + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); // An empty list is indistinguishable from metadata that has not arrived. - expect(resolvePendingPanelCloses("thread-a", [])).toEqual([]); - expect(resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"])).toEqual([ - { terminalId: "terminal-2", snapshot }, - ]); + expect(resolvePendingPanelCloses("thread-a", [], AFTER_GRACE)).toEqual([]); + expect( + resolvePendingPanelCloses("thread-a", ["terminal-1", "terminal-2"], AFTER_GRACE), + ).toEqual([{ terminalId: "terminal-2", snapshot }]); }); - it("notifies subscribers when a close is recorded", () => { - // The settling effect must re-run even when session metadata never changes - // again (the session survived, or metadata arrived before the record). - let notified = 0; - const unsubscribe = subscribePendingPanelCloses(() => { - notified += 1; - }); - const versionBefore = pendingPanelCloseVersion(); - - recordPendingPanelClose("thread-a", "terminal-2", snapshot); - - expect(notified).toBe(1); - expect(pendingPanelCloseVersion()).toBeGreaterThan(versionBefore); - unsubscribe(); - recordPendingPanelClose("thread-a", "terminal-3", snapshot); - expect(notified).toBe(1); + 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); - recordPendingPanelClose("thread-b", "terminal-2", snapshot); + recordPendingPanelClose("thread-a", "terminal-2", snapshot, RECORDED_AT); + recordPendingPanelClose("thread-b", "terminal-2", snapshot, RECORDED_AT); - expect(resolvePendingPanelCloses("thread-a", ["terminal-2"])).toHaveLength(1); - expect(resolvePendingPanelCloses("thread-b", ["terminal-2"])).toHaveLength(1); + 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 index 5417e9d4bc3..d7722092c72 100644 --- a/apps/web/src/lib/terminalPendingPanelCloses.ts +++ b/apps/web/src/lib/terminalPendingPanelCloses.ts @@ -9,7 +9,21 @@ import type { TerminalSurfaceSnapshot } from "../rightPanelStore"; * has no reconcile pass to remove. Holding the pre-close snapshot until * authoritative session metadata arrives lets the outcome decide. */ -const pendingByThreadKey = new Map>(); +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 @@ -41,11 +55,14 @@ 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); + const pending = pendingByThreadKey.get(threadKey) ?? new Map(); + pending.set(terminalId, { snapshot, readyAt: now + SETTLE_GRACE_MS }); pendingByThreadKey.set(threadKey, pending); - notifyPendingPanelCloses(); + // 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); } /** @@ -60,18 +77,23 @@ export function recordPendingPanelClose( export function resolvePendingPanelCloses( threadKey: string, serverTerminalIds: readonly string[], + now: number = Date.now(), ): Array<{ terminalId: string; snapshot: TerminalSurfaceSnapshot }> { const pending = pendingByThreadKey.get(threadKey); if (!pending || serverTerminalIds.length === 0) return []; const serverIds = new Set(serverTerminalIds); const survived: Array<{ terminalId: string; snapshot: TerminalSurfaceSnapshot }> = []; - for (const [terminalId, snapshot] of pending) { + for (const [terminalId, entry] of pending) { + if (entry.readyAt > now) continue; + pending.delete(terminalId); if (serverIds.has(terminalId)) { - survived.push({ terminalId, snapshot }); + survived.push({ terminalId, snapshot: entry.snapshot }); } } - pendingByThreadKey.delete(threadKey); + if (pending.size === 0) { + pendingByThreadKey.delete(threadKey); + } return survived; } diff --git a/apps/web/src/terminal/ghostty/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index 68f9b489b08..c43bf5969e4 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -380,7 +380,7 @@ export class GhosttyTerminalSurface { private composing = false; private focused = false; private resizeRequestedOnce = false; - private resizeGranted = false; + private resizeSettledOnce = false; private desiredCols = 0; private desiredRows = 0; private resizeRequestActive = false; @@ -632,13 +632,14 @@ export class GhosttyTerminalSurface { 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 || !this.resizeGranted) { - // The pre-grant exception keeps mount bookkeeping honest: 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. - this.resizeGranted = this.resizeGranted || granted; + 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. this.applyGridSize(cols, rows); } else if (!hasNewerMeasurement) { // The PTY kept its old size. Keep the grid with it, and align the From 8026b6ae183e3b27b18e8ee79eab3d7c23537bf8 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 17:28:06 +0200 Subject: [PATCH 21/23] fix(terminal): order resize replies behind queued output and close rollback gaps Review follow-ups, the first being the root ordering gap: - the server resize reply now orders itself behind every output byte queued before the resize, via a flush sentinel drained through the same per-session event queue; queue wipes release sentinels so awaiters cannot hang. The client can therefore apply its grid on the reply knowing no old-width bytes follow, with a Manager ordering test - a failed or interrupted resize now schedules a retry of the measured size until the PTY grants it, instead of leaving the emulator on a size the server never confirmed - a definite open failure now also rolls back the optimistic right-panel surface or split pane, not just the drawer-store pending id - an authoritative empty session list settles pending panel closes (the closed terminal may have been the last session), distinguished from unloaded metadata via the query's loaded state - surviving snapshots restore in reverse close order so nested split closes reinsert at their original pane positions Co-Authored-By: Claude Fable 5 --- apps/server/src/terminal/Manager.test.ts | 27 ++++++++ apps/server/src/terminal/Manager.ts | 65 +++++++++++++++---- apps/web/src/components/ChatView.tsx | 34 +++++++--- .../lib/terminalPendingPanelCloses.test.ts | 30 +++++++++ .../web/src/lib/terminalPendingPanelCloses.ts | 19 +++--- apps/web/src/state/terminalSessions.ts | 16 +++++ apps/web/src/terminal/ghostty/surface.ts | 23 +++++-- 7 files changed, 179 insertions(+), 35 deletions(-) diff --git a/apps/server/src/terminal/Manager.test.ts b/apps/server/src/terminal/Manager.test.ts index f1fd0653be0..3c398f24675 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(); diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index 36825836dd8..889644f1cdb 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; @@ -1656,7 +1673,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; @@ -1664,7 +1681,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; @@ -1672,10 +1689,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, @@ -1708,7 +1729,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) @@ -1734,6 +1755,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); @@ -1780,7 +1806,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; @@ -1875,7 +1901,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; @@ -1952,7 +1978,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); @@ -2233,7 +2259,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); @@ -2242,7 +2268,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); @@ -2539,6 +2565,21 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func 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. + const pid = session.value.pid; + if (pid !== null && session.value.pendingProcessEvents.length > 0) { + 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) => @@ -2552,7 +2593,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); @@ -2625,7 +2666,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/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index d08183a89ec..d59ba5300f5 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -215,7 +215,11 @@ import { 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 { @@ -1564,11 +1568,14 @@ function ChatViewContent(props: ChatViewProps) { 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); @@ -1577,6 +1584,7 @@ function ChatViewContent(props: ChatViewProps) { }, [ activePendingPanelCloseVersion, activeServerOrderedTerminalIds, + activeTerminalMetadataLoaded, activeThreadRef, storeUnsuppressTerminal, ]); @@ -2685,7 +2693,7 @@ function ChatViewContent(props: ChatViewProps) { // 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) => { + (terminalId: string, sessionCwd: string, workspaceRoot: string, onUncreated?: () => void) => { if (!activeThreadRef || !activeThreadId) return; const threadRef = activeThreadRef; const threadKey = scopedThreadKey(threadRef); @@ -2709,6 +2717,9 @@ function ChatViewContent(props: ChatViewProps) { }); 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); @@ -3256,9 +3267,13 @@ function ChatViewContent(props: ChatViewProps) { ...allocatableActiveTerminalIds, ...reservedTerminalOpenIds(scopedThreadKey(activeThreadRef)), ]); - useRightPanelStore.getState().openTerminal(activeThreadRef, terminalId); + const threadRef = activeThreadRef; + useRightPanelStore.getState().openTerminal(threadRef, terminalId); setTerminalFocusRequestId((value) => value + 1); - openThreadTerminalSession(terminalId, cwd, activeProject.workspaceRoot); + 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, @@ -3283,11 +3298,14 @@ function ChatViewContent(props: ChatViewProps) { ...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); - openThreadTerminalSession(terminalId, cwd, activeProject.workspaceRoot); + openThreadTerminalSession(terminalId, cwd, activeProject.workspaceRoot, () => { + // No session was created, so the optimistic split pane must go too. + useRightPanelStore.getState().closeTerminal(threadRef, surfaceId, terminalId); + }); }, [ activeProject, diff --git a/apps/web/src/lib/terminalPendingPanelCloses.test.ts b/apps/web/src/lib/terminalPendingPanelCloses.test.ts index b64ca0b0d57..e8c44b10623 100644 --- a/apps/web/src/lib/terminalPendingPanelCloses.test.ts +++ b/apps/web/src/lib/terminalPendingPanelCloses.test.ts @@ -70,6 +70,36 @@ describe("terminalPendingPanelCloses", () => { ).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 { diff --git a/apps/web/src/lib/terminalPendingPanelCloses.ts b/apps/web/src/lib/terminalPendingPanelCloses.ts index d7722092c72..f1c818a3ea3 100644 --- a/apps/web/src/lib/terminalPendingPanelCloses.ts +++ b/apps/web/src/lib/terminalPendingPanelCloses.ts @@ -66,21 +66,24 @@ export function recordPendingPanelClose( } /** - * Settles every pending close for a thread against the server's session list, - * returning the ones whose session survived so the caller can restore them. - * Ids the server no longer reports were really closed and are simply dropped. + * 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. * - * `serverTerminalIds` must be a loaded, non-empty list: an empty one is - * indistinguishable from metadata that has not arrived, and would resolve every - * entry as "closed". + * `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 || serverTerminalIds.length === 0) return []; + if (!pending || !listLoaded) return []; const serverIds = new Set(serverTerminalIds); const survived: Array<{ terminalId: string; snapshot: TerminalSurfaceSnapshot }> = []; @@ -94,7 +97,7 @@ export function resolvePendingPanelCloses( if (pending.size === 0) { pendingByThreadKey.delete(threadKey); } - return survived; + return survived.reverse(); } export function clearPendingPanelCloses(threadKey: string): void { 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/surface.ts b/apps/web/src/terminal/ghostty/surface.ts index c43bf5969e4..60ec67cdcdd 100644 --- a/apps/web/src/terminal/ghostty/surface.ts +++ b/apps/web/src/terminal/ghostty/surface.ts @@ -381,6 +381,7 @@ export class GhosttyTerminalSurface { private focused = false; private resizeRequestedOnce = false; private resizeSettledOnce = false; + private resizeRetryTimer: number | null = null; private desiredCols = 0; private desiredRows = 0; private resizeRequestActive = false; @@ -639,17 +640,16 @@ export class GhosttyTerminalSurface { // 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. + // 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); - } else if (!hasNewerMeasurement) { - // The PTY kept its old size. Keep the grid with it, and align the - // desired size with the applied one so a future fit at the failed - // size re-requests instead of being skipped. - this.desiredCols = this.cols; - this.desiredRows = this.rows; } if (hasNewerMeasurement) { this.requestResize(); + } else if (!granted) { + this.scheduleResizeRetry(); } }; const acknowledgement = this.options.onResize(cols, rows); @@ -663,6 +663,14 @@ export class GhosttyTerminalSurface { } } + 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; @@ -742,6 +750,7 @@ export class GhosttyTerminalSurface { this.dprMedia?.removeEventListener("change", this.onDevicePixelRatioChange); this.dprMedia = null; if (this.selectionScrollTimer !== null) window.clearInterval(this.selectionScrollTimer); + 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) { From 69a71325ce705611afab9d58ce8627c84ec496d6 Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 20:22:15 +0200 Subject: [PATCH 22/23] fix(server): strip replayable query traffic from terminal history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reopening a terminal replays its history, and stored capability queries made the terminal answer again — the shell then echoed the replies as junk at the prompt (the 2026;2$y…1$r0m / 1;2c / 11;rgb:… gibberish from the field reports). The history sanitizer already dropped DSR, CPR, DA, and OSC colour traffic; it now also strips DECRQM queries and DECRPM replies ($-intermediate p/y), XTVERSION and kitty-keyboard queries, and DECRQSS/XTGETTCAP DCS queries and replies. Setters sharing final bytes — DECSTR, DECSCL, DECSCUSR, restore-cursor — survive, covered by a regression test. Prevents new junk at the source; residue already flattened into stored scrollback as plain text is out of scope here (PR #3508 tackles that broader stripping). Co-Authored-By: Claude Fable 5 --- apps/server/src/terminal/Manager.test.ts | 28 +++++++++++++++++++++ apps/server/src/terminal/Manager.ts | 32 ++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/apps/server/src/terminal/Manager.test.ts b/apps/server/src/terminal/Manager.test.ts index 3c398f24675..de4a80772cf 100644 --- a/apps/server/src/terminal/Manager.test.ts +++ b/apps/server/src/terminal/Manager.test.ts @@ -1018,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", () => diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index 889644f1cdb..85748b85737 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -901,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); } @@ -1004,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; @@ -1047,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; From 013841ccc361ae6894b858272cc1aeace44f59ea Mon Sep 17 00:00:00 2001 From: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Date: Sat, 1 Aug 2026 20:27:04 +0200 Subject: [PATCH 23/23] fix(server): flush the resize reply while a drain is still publishing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: dequeue compacts the pending queue before the dequeued chunk is published, so a resize landing in that window saw an empty queue, skipped the flush sentinel, and acked ahead of the old-width bytes. A running drain now also gates the flush — the sentinel is picked up when the drain loop re-enters, behind the in-flight publication. Co-Authored-By: Claude Fable 5 --- apps/server/src/terminal/Manager.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index 85748b85737..7f0675036bb 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -2597,9 +2597,16 @@ export const makeWithOptions = Effect.fn("TerminalManager.makeWithOptions")(func // 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. + // 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) { + 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) {