From 27393108399fb7a3cb4e1207d360954230d6fc03 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 29 Jun 2026 12:12:35 -0700 Subject: [PATCH] fix(terminal): converge post-spawn PTY size reconcile to fix split-mount column desync (#6725) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): converge post-spawn PTY size reconcile to fix split-mount column desync Follow-up to #6644/#6649. Those added a post-spawn PTY reconcile but capped it at a FIXED 12 requestAnimationFrames whose counter advanced even on frames where the pane was unmeasurable or the split layout had not yet equalized. When a tab MOUNTS with a split layout already present (a new worktree opened with the side split panel on), the real narrow split width settles AFTER frame 12, so the reconcile gave up while xterm had reflowed narrow and the PTY stayed pinned at the wide spawn width. The corrective xterm onResize is dropped during the hidden mount window (isRendererPtyResizeAuthoritative() is false), so process.stdout.columns stayed wide and interactive TUIs (Claude Code) rendered garbled until a manual resize. Extract the reconcile into pty-size-reconcile.ts with an authoritative-gated convergence loop instead of a fixed frame budget: - While the pane is hidden (onResize dropped), the reconcile is the SOLE corrector: it keeps polling and forwarding every grid change (its transport.resize bypasses the visibility gate). Hidden frames never advance the settle counter. - Once visible AND stable for SETTLE_FRAMES, it hands off to the live onResize/ResizeObserver path, which reliably catches any later reflow. - Hard cap (MAX_FRAMES) guarantees termination; mobile-fit/locked frames are skipped; the reconcile handle is cancelled on dispose. Harness: pty-size-reconcile.test.ts (14 tests) drives the loop with a deterministic frame scheduler; the desync-repro tests fail against the old 12-frame logic and pass on the fix. Adds an e2e "MOUNTS with a split layout present" test. Caveat: headless Electron does not reproduce this layout-settle-after-rAF race (the existing golden e2e passes even against the old buggy logic), which is why #6644/#6649 merged with green e2e yet the bug persisted. The unit test is the real regression harness; the e2e tests are integration smoke. Made with [Orca](https://github.com/stablyai/orca) Co-authored-by: Orca * fix(terminal): re-assert PTY size on visibility resume to heal stubborn column desync Follow-up within the same fix: the user reported "sometimes even resizing doesn't fix it." Root cause beyond the mount-timing race — the renderer forwards resizes fire-and-forget and dedupes on the size it THINKS it sent, but never learns the PTY's actual size. A resize dropped main-side (the pane was hidden, a mobile take-back resize-suppression window, or a provider no-op) leaves xterm and the PTY silently diverged; a later same-cols layout fires no onResize, so it never self-corrects. Expose the PTY's last-APPLIED size to the renderer and re-assert on show: - New read-only IPC pty:getSize -> ptySizes.get(id) (the size written only when a resize actually lands or at spawn — the authoritative "what the PTY believes it is"). Wired through preload (window.api.pty.getSize) + api-types. - On visibility resume (noteVisibilityResume), the pane re-fits, reads the PTY's real size, and re-asserts via forwardPtyResize ONLY on genuine drift — so no spurious SIGWINCH on an already-synced resume (which would jar alt-screen TUIs). Routed through forwardPtyResize so the authoritative/mobile gates are re-checked at send time; remote-runtime PTYs (separate viewport channel) are skipped; overlapping resumes coalesce to one query. Also register pty:getSize in the registerPtyHandlers removeHandler cleanup block so re-registration (macOS re-activate / new window) doesn't throw on a duplicate ipcMain.handle, and make the pty IPC test mock throw on duplicate channels like real Electron so this class of omission is caught going forward. Tests: 7 resume-reassert cases (drift / match / null / remote-skip / mobile-fit-skip / hide-during-hop / overlap-coalesce), all non-vacuous. Full terminal-pane + pty IPC suites green (1494 tests); typecheck (web+node) + oxlint clean; e2e desync specs pass against a fresh build. Made with [Orca](https://github.com/stablyai/orca) Co-authored-by: Orca * Stub PTY getSize API and skip redundant Wayland GPU sandbox tests - Implement PTY `getSize` stub in `web-preload-api.ts` to satisfy API requirements for the web-preload environment. - Skip the unfixed Wayland GPU sandbox negative control test if the target base branch already contains the sandbox workaround. --------- Co-authored-by: Orca Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com> --- .../workflows/linux-wayland-gpu-sandbox.yml | 6 + src/main/ipc/pty.test.ts | 9 + src/main/ipc/pty.ts | 16 + src/preload/api-types.ts | 1 + src/preload/index.ts | 6 + .../terminal-pane/pty-connection.test.ts | 150 +++++++++ .../terminal-pane/pty-connection.ts | 118 +++++-- .../terminal-pane/pty-size-reconcile.test.ts | 315 ++++++++++++++++++ .../terminal-pane/pty-size-reconcile.ts | 144 ++++++++ src/renderer/src/web/web-preload-api.ts | 1 + .../e2e/terminal-column-desync-repro.spec.ts | 75 +++++ 11 files changed, 806 insertions(+), 35 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts create mode 100644 src/renderer/src/components/terminal-pane/pty-size-reconcile.ts diff --git a/.github/workflows/linux-wayland-gpu-sandbox.yml b/.github/workflows/linux-wayland-gpu-sandbox.yml index be4f4ecc4..fc297326d 100644 --- a/.github/workflows/linux-wayland-gpu-sandbox.yml +++ b/.github/workflows/linux-wayland-gpu-sandbox.yml @@ -95,6 +95,12 @@ jobs: run: | set -euo pipefail git fetch --no-tags --depth=1 origin "${{ github.event.pull_request.base.sha }}" + # Why: once the PR base includes this workaround, the partial + # checkout cannot reconstruct an unfixed Wayland negative control. + if git show "${{ github.event.pull_request.base.sha }}:src/main/startup/configure-process.ts" | grep -Eq "appendSwitch\\(['\"]disable-gpu-sandbox['\"]\\)"; then + echo "The base build already contains the Linux Wayland GPU sandbox workaround; skipping unfixed-path reproduction." + exit 0 + fi # Why: keep the new verifier scripts, but run them against the # unfixed production terminal/GPU path instead of a hybrid checkout. git checkout "${{ github.event.pull_request.base.sha }}" -- \ diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index add52d563..b5dffb1a2 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -305,9 +305,18 @@ describe('registerPtyHandlers', () => { mainWindow.webContents.on.mockReset() mainWindow.webContents.send.mockReset() + // Why: mirror real Electron — ipcMain.handle throws on a duplicate channel + // unless removeHandler cleared it first. This catches a re-registration + // (macOS re-activate / new window) that forgets to remove a handle channel. handleMock.mockImplementation((channel: string, handler: (...a: unknown[]) => unknown) => { + if (handlers.has(channel)) { + throw new Error(`Attempted to register a second handler for '${channel}'`) + } handlers.set(channel, handler) }) + removeHandlerMock.mockImplementation((channel: string) => { + handlers.delete(channel) + }) getPathMock.mockReturnValue('/tmp/orca-user-data') existsSyncMock.mockReturnValue(true) statSyncMock.mockReturnValue({ isDirectory: () => true, mode: 0o755 }) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 09fb8f690..a64371fb3 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -1185,6 +1185,7 @@ export function registerPtyHandlers( ipcMain.removeHandler('pty:hasChildProcesses') ipcMain.removeHandler('pty:getForegroundProcess') ipcMain.removeHandler('pty:getCwd') + ipcMain.removeHandler('pty:getSize') ipcMain.removeHandler('pty:declarePendingPaneSerializer') ipcMain.removeHandler('pty:settlePaneSerializer') ipcMain.removeHandler('pty:clearPendingPaneSerializer') @@ -3238,6 +3239,21 @@ export function registerPtyHandlers( } }) + // Why: the renderer forwards resizes fire-and-forget and otherwise has no way + // to learn the PTY's actual size. A resize dropped main-side (suppression + // window, mobile-driver gate, or a provider no-op) leaves the renderer + // believing it synced when it did not, so a later same-cols layout never + // re-forwards and the TUI stays garbled. Exposing the last APPLIED size lets + // the renderer detect true drift on resume and re-assert. ptySizes is only + // written when a resize actually lands (and at spawn), so it is the + // authoritative "what the PTY believes it is". + ipcMain.handle( + 'pty:getSize', + (_event, args: { id: string }): { cols: number; rows: number } | null => { + return ptySizes.get(args.id) ?? null + } + ) + // Why: pre-signal handshake handlers. See // docs/mobile-prefer-renderer-scrollback.md and the rationale on // `pendingByPaneKey` above. The IPC contract is: renderer awaits declare diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 0f2ac202f..655aef88d 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1140,6 +1140,7 @@ export type PreloadApi = { hasChildProcesses: (id: string) => Promise getForegroundProcess: (id: string) => Promise getCwd: (id: string) => Promise + getSize: (id: string) => Promise<{ cols: number; rows: number } | null> listSessions: () => Promise<{ id: string; cwd: string; title: string }[]> getMainBufferSnapshot: ( id: string, diff --git a/src/preload/index.ts b/src/preload/index.ts index db4dfad6b..ec69e1a0e 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -861,6 +861,12 @@ const api = { * Returns `''` when the id is unknown or the platform cannot resolve one. */ getCwd: (id: string): Promise => ipcRenderer.invoke('pty:getCwd', { id }), + /** The PTY's last APPLIED size (its real winsize), or null if unknown. + * Lets the renderer detect drift after a resize was dropped main-side and + * re-assert, instead of trusting the size it last fired blind. */ + getSize: (id: string): Promise<{ cols: number; rows: number } | null> => + ipcRenderer.invoke('pty:getSize', { id }), + onData: ( callback: (data: { id: string; data: string; seq?: number; rawLength?: number }) => void ): (() => void) => { diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index efca37633..8dd2bc348 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -606,6 +606,7 @@ describe('connectPanePty', () => { pty: { signal: vi.fn(), listSessions: vi.fn().mockResolvedValue([]), + getSize: vi.fn().mockResolvedValue(null), getMainBufferSnapshot: vi.fn().mockResolvedValue(null), getForegroundProcess: vi.fn().mockResolvedValue(null), hasChildProcesses: vi.fn().mockResolvedValue(false), @@ -10750,4 +10751,153 @@ describe('connectPanePty', () => { expect(listSessions).not.toHaveBeenCalled() }) }) + + describe('PTY size re-assert on visibility resume', () => { + // Why: a resize dropped while the pane was hidden (suppression window, + // mobile-driver gate, provider no-op) leaves xterm and the PTY silently + // diverged. The renderer dedupes on what it *thinks* it sent, so a later + // same-cols layout never re-forwards — "resizing sometimes doesn't fix it". + // On resume the binding reads the PTY's ACTUAL size and re-asserts only on + // real drift. The mock pane's fitAddon has no proposeDimensions, so safeFit + // is a no-op and pane.terminal stays at its createPane() default (120x40). + async function connectResumablePane(depsOverrides: Record = {}): Promise<{ + binding: { noteVisibilityResume: () => void } + transport: MockTransport + deps: ReturnType + }> { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('pty-pane-2') + transportFactoryQueue.push(transport) + const manager = createManager(2) + const deps = createDeps({ + restoredLeafId: LEAF_2, + paneTransportsRef: { current: new Map([[1, createMockTransport('pty-pane-1')]]) }, + ...depsOverrides + }) + const pane = createPane(2) + const binding = connectPanePty(pane as never, manager as never, deps as never) as unknown as { + noteVisibilityResume: () => void + } + return { binding, transport, deps } + } + + it('re-asserts the current size when the PTY drifted from xterm', async () => { + vi.mocked(window.api.pty.getSize).mockResolvedValue({ cols: 80, rows: 24 }) + const { binding, transport } = await connectResumablePane() + transport.resize.mockClear() + + binding.noteVisibilityResume() + await flushAsyncTicks() + + // xterm is 120x40 (createPane default), PTY reports 80x24 → re-assert. + expect(transport.resize).toHaveBeenCalledWith(120, 40) + }) + + it('does NOT re-assert when the PTY already matches xterm (no spurious SIGWINCH)', async () => { + vi.mocked(window.api.pty.getSize).mockResolvedValue({ cols: 120, rows: 40 }) + const { binding, transport } = await connectResumablePane() + transport.resize.mockClear() + + binding.noteVisibilityResume() + await flushAsyncTicks() + + expect(transport.resize).not.toHaveBeenCalled() + }) + + it('re-asserts when the PTY size is unknown (cannot confirm synced)', async () => { + vi.mocked(window.api.pty.getSize).mockResolvedValue(null) + const { binding, transport } = await connectResumablePane() + transport.resize.mockClear() + + binding.noteVisibilityResume() + await flushAsyncTicks() + + expect(transport.resize).toHaveBeenCalledWith(120, 40) + }) + + it('skips remote-runtime PTYs (their size lives outside the local ptySizes map)', async () => { + const getSize = vi.mocked(window.api.pty.getSize) + getSize.mockClear() + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('remote:env-1@@terminal-2') + transport.getConnectionId.mockReturnValue(null) + transportFactoryQueue.push(transport) + const manager = createManager(2) + const deps = createDeps({ + restoredLeafId: LEAF_2, + paneTransportsRef: { current: new Map([[1, createMockTransport('pty-pane-1')]]) } + }) + const pane = createPane(2) + const binding = connectPanePty(pane as never, manager as never, deps as never) as unknown as { + noteVisibilityResume: () => void + } + transport.resize.mockClear() + + binding.noteVisibilityResume() + await flushAsyncTicks() + + // Never even queries size for a remote pane, and never re-asserts. + expect(getSize).not.toHaveBeenCalled() + expect(transport.resize).not.toHaveBeenCalled() + }) + + it('does NOT re-assert while a mobile-fit override parks the PTY at phone dims', async () => { + const { setFitOverride } = await import('@/lib/pane-manager/mobile-fit-overrides') + vi.mocked(window.api.pty.getSize).mockResolvedValue({ cols: 80, rows: 24 }) + const { binding, transport } = await connectResumablePane() + // Park the PTY at phone dims — desktop re-assert must be suppressed. + setFitOverride('pty-pane-2', 'mobile-fit', 40, 30) + transport.resize.mockClear() + + binding.noteVisibilityResume() + await flushAsyncTicks() + + expect(transport.resize).not.toHaveBeenCalled() + setFitOverride('pty-pane-2', 'desktop-fit', 0, 0) + }) + + it('does NOT forward when the pane is hidden again before getSize resolves (stale hop)', async () => { + // The load-bearing safety property: a getSize promise resolving AFTER the + // pane was re-hidden must not emit a hidden-tab SIGWINCH (which can reset + // alt-screen TUIs). Suppression is the send-time visibility re-check. + let resolveSize: (v: { cols: number; rows: number } | null) => void = () => {} + vi.mocked(window.api.pty.getSize).mockImplementation( + () => + new Promise((resolve) => { + resolveSize = resolve + }) + ) + const { binding, transport, deps } = await connectResumablePane() + transport.resize.mockClear() + + binding.noteVisibilityResume() + // Pane is hidden again while the size query is still in flight. + deps.isVisibleRef.current = false + resolveSize({ cols: 80, rows: 24 }) // drift — would re-assert if visible + await flushAsyncTicks() + + expect(transport.resize).not.toHaveBeenCalled() + }) + + it('coalesces overlapping resumes into a single size query (re-entrancy guard)', async () => { + const getSize = vi.mocked(window.api.pty.getSize) + getSize.mockClear() + let resolveSize: (v: { cols: number; rows: number } | null) => void = () => {} + getSize.mockImplementation( + () => + new Promise((resolve) => { + resolveSize = resolve + }) + ) + const { binding } = await connectResumablePane() + getSize.mockClear() + + // Two rapid resumes before the first query resolves → only one query. + binding.noteVisibilityResume() + binding.noteVisibilityResume() + expect(getSize).toHaveBeenCalledTimes(1) + resolveSize({ cols: 120, rows: 40 }) + await flushAsyncTicks() + }) + }) }) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index de7a9e611..7ec8ac3df 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -26,6 +26,7 @@ import type { PtyConnectionDeps } from './pty-connection-types' import { safeFit } from '@/lib/pane-manager/pane-tree-ops' import { getFitOverrideForPty, bindPanePtyId } from '@/lib/pane-manager/mobile-fit-overrides' import { isPtyLocked } from '@/lib/pane-manager/mobile-driver-state' +import { reconcilePtySizeAcrossFrames, type PtySizeReconcileHandle } from './pty-size-reconcile' import { isPaneReplaying, replayIntoTerminal, replayIntoTerminalAsync } from './replay-guard' import { nativeWindowsRewriteNeedsFollowupRenderRefresh, @@ -2250,52 +2251,91 @@ export function connectPanePty( // Why: the deferred-rAF fit can spawn the PTY at a stale width when the pane's // real (e.g. split/narrower) layout has not settled by the first frame — the // PTY is born at the wide window width while xterm later reflows to the pane - // width. The corrective onResize is then dropped (cols already matched at fit - // time, or isRendererPtyResizeAuthoritative() was false mid-mount), pinning - // process.stdout.columns forever and garbling TUIs. Re-fit once layout has - // settled and force the PTY to xterm's dimensions; the initial spawn-time sync - // is authoritative by definition, so it bypasses the visibility gate (but not - // the mobile-fit override, which legitimately parks the PTY at phone dims). + // width. The corrective onResize is then dropped (isRendererPtyResizeAuthoritative() + // is false mid-mount), pinning process.stdout.columns forever and garbling + // TUIs. The reconcile re-fits across frames until the grid settles and forces + // the PTY to xterm's dimensions; the spawn-time sync is authoritative by + // definition so it bypasses the visibility gate (but not the mobile-fit + // override, which legitimately parks the PTY at phone dims). See + // pty-size-reconcile.ts for the convergence loop. + let ptySizeReconcileHandle: PtySizeReconcileHandle | null = null const reconcilePtySizeAfterSpawn = ( ptyId: string, spawnCols: number, spawnRows: number ): void => { - // Why: a single post-spawn frame is not enough — the pane's real layout can - // keep changing for several frames (split equalize, sidebar/title reflow), - // so a one-shot re-fit can still measure a stale width and leave the PTY - // pinned. Poll across frames and forward the settled size to the PTY - // whenever it differs from what the PTY was last told, mirroring the - // ResizeObserver stability loop. Each resize is gated on an actual change, - // so a TUI sees at most a couple of SIGWINCH during startup, not a loop. - const MAX_RECONCILE_FRAMES = 12 - let frame = 0 - let lastSentCols = spawnCols - let lastSentRows = spawnRows - const tick = (): void => { - if (disposed || transport.getPtyId() !== ptyId) { - return - } - // Mobile legitimately parks the PTY at phone dims; a transient guard - // should only skip this frame, not cancel the reconcile window. - if (!getFitOverrideForPty(ptyId) && !isPtyLocked(ptyId)) { + ptySizeReconcileHandle?.cancel() + ptySizeReconcileHandle = reconcilePtySizeAcrossFrames({ + spawnCols, + spawnRows, + isAlive: () => !disposed && transport.getPtyId() === ptyId, + // Mobile legitimately parks the PTY at phone dims; skip those frames + // (neither fit nor forward) instead of cancelling the reconcile window. + isParked: () => Boolean(getFitOverrideForPty(ptyId)) || isPtyLocked(ptyId), + // Once the renderer resize is authoritative (pane visible), the live + // onResize owns future corrections, so the reconcile can hand off after + // the grid stabilizes. While hidden it keeps watching for a late settle. + isAuthoritative: () => isRendererPtyResizeAuthoritative(), + measure: () => { safeFit(pane) const cols = pane.terminal.cols const rows = pane.terminal.rows - if (cols > 0 && rows > 0 && (cols !== lastSentCols || rows !== lastSentRows)) { - // Initial spawn-time sync is authoritative, so it bypasses the - // visibility gate that onResize honors (but not the mobile guards above). - transport.resize(cols, rows) - lastSentCols = cols - lastSentRows = rows + return cols > 0 && rows > 0 ? { cols, rows } : null + }, + resize: (cols, rows) => transport.resize(cols, rows), + requestFrame: (callback) => requestAnimationFrame(callback), + cancelFrame: (handle) => { + if (typeof cancelAnimationFrame === 'function') { + cancelAnimationFrame(handle) } } - frame += 1 - if (frame < MAX_RECONCILE_FRAMES) { - requestAnimationFrame(tick) - } + }) + } + + // Why: the renderer forwards resizes fire-and-forget and dedupes on the size + // it *last sent*, so a resize dropped main-side (it was hidden, a suppression + // window, or a provider no-op) leaves xterm and the PTY silently diverged — + // and a later same-cols layout fires no onResize, so it never self-corrects + // ("resizing sometimes doesn't fix it"). On becoming visible, re-fit and + // compare xterm against the PTY's ACTUAL size (not what we think we sent); if + // they truly differ, re-assert. Gated on real drift so we emit no spurious + // SIGWINCH (which would jar alt-screen TUIs) on an already-synced resume. + let reassertingPtySizeOnResume = false + const reassertPtySizeOnResume = (): void => { + const ptyId = transport.getPtyId() + // forwardPtyResize re-checks the visibility/mobile gates at send time, so + // here we only need the cheap pre-hop early-outs. Skip remote-runtime PTYs: + // their resize goes through a separate viewport channel (not pty:resize), so + // the local ptySizes map getSize reads is never populated for them. + if (disposed || reassertingPtySizeOnResume || !ptyId || isRemoteRuntimePtyId(ptyId)) { + return } - requestAnimationFrame(tick) + reassertingPtySizeOnResume = true + void window.api.pty + .getSize(ptyId) + .then((actual) => { + // The pane may have been disposed or rebound to a different PTY during + // the async hop; bail if this reconcile no longer owns it. + if (disposed || transport.getPtyId() !== ptyId) { + return + } + safeFit(pane) + const cols = pane.terminal.cols + const rows = pane.terminal.rows + if (cols <= 0 || rows <= 0) { + return + } + // Re-assert only on genuine divergence from the PTY's applied size (a + // null read = unknown id, treated as "cannot confirm synced" so we + // forward once). forwardPtyResize owns the authoritative/mobile gating. + if (!actual || actual.cols !== cols || actual.rows !== rows) { + forwardPtyResize(cols, rows) + } + }) + .catch(() => {}) + .finally(() => { + reassertingPtySizeOnResume = false + }) } // Defer PTY spawn/attach to next frame so FitAddon has time to calculate @@ -4562,10 +4602,18 @@ export function connectPanePty( // keeps the typing hot path off the listSessions IPC between resumes. noteVisibilityResume() { livenessRecheckFiredSinceResume = false + // Why: re-assert the PTY size on resume so a resize that was dropped while + // this pane was hidden self-heals on show, instead of waiting for a manual + // resize that may never change xterm's column count. + reassertPtySizeOnResume() }, reconcileIfSessionDead, dispose() { disposed = true + // Why: the post-spawn reconcile polls across frames; cancel its pending + // rAF so a torn-down pane cannot keep fitting/resizing after disposal. + ptySizeReconcileHandle?.cancel() + ptySizeReconcileHandle = null if (terminalKeyTargetSupportsEvents) { terminalKeyTarget.removeEventListener('keydown', onTerminalKeyDown, { capture: true }) } diff --git a/src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts b/src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts new file mode 100644 index 000000000..9f02527d5 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts @@ -0,0 +1,315 @@ +import { describe, expect, it, vi } from 'vitest' +import { + reconcilePtySizeAcrossFrames, + type PtySizeReconcileDimensions, + type PtySizeReconcileOptions +} from './pty-size-reconcile' + +/** + * Reproduction harness for the terminal column-desync bug (Slack: Claude Code + * renders garbled when a new worktree is opened with the side split panel on). + * + * The PTY is spawned at the wide window width, then the split/sidebar layout + * narrows the pane some frames LATER. The fix must converge the PTY to the + * settled narrow width regardless of WHEN the layout lands — including a settle + * that arrives well after any fixed frame budget (the prior fix used a fixed + * 12-frame budget that expired before the split equalized). The reconcile keeps + * polling while the pane is not yet authoritative (the hidden mount window where + * the live onResize is dropped) and hands off once authoritative + stable. + */ + +/** A deterministic frame scheduler: callbacks queue, then run() drains them. */ +function createFrameScheduler() { + const queue = new Map void>() + let nextHandle = 1 + return { + requestFrame: (callback: () => void): number => { + const handle = nextHandle++ + queue.set(handle, callback) + return handle + }, + cancelFrame: (handle: number): void => { + queue.delete(handle) + }, + /** Run up to `maxFrames` queued frames, one per tick. Returns frames run. */ + run(maxFrames = 1000): number { + let ran = 0 + while (queue.size > 0 && ran < maxFrames) { + const [handle, callback] = queue.entries().next().value as [number, () => void] + queue.delete(handle) + callback() + ran += 1 + } + return ran + }, + pending: () => queue.size + } +} + +/** + * A pane whose measured grid follows a timeline keyed by frame index: it starts + * unmeasurable (null) or wide, then narrows at some frame. `measure()` is called + * once per reconcile frame, so the call count tracks frames elapsed. + */ +function createTimelinePane(timeline: (frame: number) => PtySizeReconcileDimensions | null) { + let frame = 0 + return { + measure: vi.fn((): PtySizeReconcileDimensions | null => { + const dims = timeline(frame) + frame += 1 + return dims + }) + } +} + +function runReconcile( + overrides: Partial & Pick, + maxFrames = 1000 +): { resize: ReturnType; framesRun: number } { + const scheduler = createFrameScheduler() + const resize = vi.fn() + reconcilePtySizeAcrossFrames({ + spawnCols: 203, + spawnRows: 50, + isAlive: () => true, + isParked: () => false, + // Default: pane is visible (the common case). Specific tests override this + // to model the hidden mount window where the live onResize is dropped. + isAuthoritative: () => true, + resize, + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame, + ...overrides + }) + const framesRun = scheduler.run(maxFrames) + return { resize, framesRun } +} + +describe('reconcilePtySizeAcrossFrames', () => { + it('forwards a narrow settle that lands AFTER a fixed 12-frame budget — while hidden', () => { + // Golden repro: the pane mounts hidden (onResize dropped — the desync + // window), spawned wide (203), still measuring wide for the first 15 frames + // (split equalize / sidebar reflow in flight), then settles to 79. A + // 12-frame-budget reconcile would have stopped at frame 12 — still wide, + // leaving the PTY pinned. The convergent loop keeps watching while hidden. + const NARROW_AT = 15 + const pane = createTimelinePane((frame) => + frame < NARROW_AT ? { cols: 203, rows: 50 } : { cols: 79, rows: 50 } + ) + const { resize } = runReconcile({ measure: pane.measure, isAuthoritative: () => false }) + + expect(resize).toHaveBeenCalled() + expect(resize).toHaveBeenLastCalledWith(79, 50) + }) + + it('forwards a narrow settle that lands LATE while hidden — no fixed frame floor', () => { + // The bug the adversarial review caught: a fixed MIN-frames floor (e.g. 24) + // would treat the still-wide spawn measurement as "settled" and stop before + // the real narrowing lands. A split that equalizes at frame 40 while the + // pane is still hidden must STILL be forwarded — the loop watches until it + // becomes authoritative (where onResize takes over) or the hard cap. + const NARROW_AT = 40 + const pane = createTimelinePane((frame) => + frame < NARROW_AT ? { cols: 203, rows: 50 } : { cols: 79, rows: 50 } + ) + const { resize } = runReconcile({ measure: pane.measure, isAuthoritative: () => false }) + + expect(resize).toHaveBeenCalled() + expect(resize).toHaveBeenLastCalledWith(79, 50) + }) + + it('forwards a late settle that lands just before the pane becomes authoritative', () => { + // Realistic handoff: hidden through the settle, narrows at frame 40, then + // the pane becomes visible at frame 45. The reconcile must have already + // forwarded the narrow width during the hidden window (its resize bypasses + // the visibility gate); the later authoritative+stable state lets it stop. + const NARROW_AT = 40 + const AUTHORITATIVE_AT = 45 + let frameSeen = 0 + const pane = createTimelinePane((frame) => { + frameSeen = frame + return frame < NARROW_AT ? { cols: 203, rows: 50 } : { cols: 79, rows: 50 } + }) + const { resize } = runReconcile({ + measure: pane.measure, + isAuthoritative: () => frameSeen >= AUTHORITATIVE_AT + }) + + expect(resize).toHaveBeenCalled() + expect(resize).toHaveBeenLastCalledWith(79, 50) + }) + + it('hands off after the pane is visible+stable, leaving later reflows to the live onResize', () => { + // Handoff boundary (verified design): once the pane is authoritative AND the + // grid has been stable for the settle window, the live onResize / + // ResizeObserver path owns any FURTHER reflow (it fires on every + // grid-changing fit() and is not suppressed for a visible desktop pane). So + // the reconcile is allowed to stop here — a split that equalizes much later, + // after this handoff, is caught by that backstop, not by the reconcile. This + // pins that the reconcile terminates promptly in the steady visible state + // rather than polling to the hard cap. The narrow-while-hidden window (where + // onResize is dropped) is covered by the dedicated tests above. + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + const { resize, framesRun } = runReconcile({ measure: pane.measure }) + expect(resize).toHaveBeenCalledTimes(1) + expect(resize).toHaveBeenLastCalledWith(79, 50) + // Frame 1 forwards the single change (resets the window); frames 2..9 are + // authoritative + unchanged, so the loop settles exactly at SETTLE_FRAMES(8) + // observed-stable frames — i.e. 9 frames total, far short of the 180 cap. + expect(framesRun).toBe(9) + }) + + it('keeps polling through unmeasurable frames (pane has no layout yet)', () => { + // A fresh split mount can be unmeasurable for many frames before the real + // grid lands. Unmeasurable frames must NOT count as "settled". + const NARROW_AT = 20 + const pane = createTimelinePane((frame) => (frame < NARROW_AT ? null : { cols: 80, rows: 24 })) + const { resize } = runReconcile({ measure: pane.measure }) + + expect(resize).toHaveBeenCalledTimes(1) + expect(resize).toHaveBeenLastCalledWith(80, 24) + }) + + it('hands off (stops) once authoritative and the grid has been stable', () => { + // Once visible and stable, the live onResize owns future corrections; the + // reconcile should stop rather than poll to the hard cap forever. + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + const { framesRun } = runReconcile({ measure: pane.measure }) + // Should settle a few frames after the single resize, well short of the cap. + expect(framesRun).toBeGreaterThan(0) + expect(framesRun).toBeLessThan(180) + }) + + it('does NOT hand off while hidden — keeps watching until the hard cap', () => { + // While never authoritative, a stable grid is not a safe stopping point + // (onResize cannot back us up), so the loop runs to the hard cap. + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + const { framesRun } = runReconcile({ + measure: pane.measure, + isAuthoritative: () => false + }) + expect(framesRun).toBe(180) + }) + + it('forwards no resize when the settled size never changes from spawn dims', () => { + // If xterm already matches the spawn width the whole time, no SIGWINCH at all. + const pane = createTimelinePane(() => ({ cols: 203, rows: 50 })) + const { resize } = runReconcile({ measure: pane.measure }) + expect(resize).not.toHaveBeenCalled() + }) + + it('does not loop forever — terminates within the hard frame cap', () => { + // Pane that never stabilizes (oscillates) must still hit the hard cap. + const pane = createTimelinePane((frame) => + frame % 2 === 0 ? { cols: 100, rows: 30 } : { cols: 101, rows: 30 } + ) + const { framesRun } = runReconcile({ measure: pane.measure }, 10_000) + expect(framesRun).toBe(180) + }) + + it('issues only a couple of SIGWINCH for a monotonic narrow settle (not one per frame)', () => { + // 203 → 120 → 79 over the first frames, then stable. The TUI should see the + // size change a small, bounded number of times during its own startup. + const pane = createTimelinePane((frame) => { + if (frame < 5) { + return { cols: 203, rows: 50 } + } + if (frame < 10) { + return { cols: 120, rows: 50 } + } + return { cols: 79, rows: 50 } + }) + const { resize } = runReconcile({ measure: pane.measure }) + expect(resize.mock.calls.length).toBeLessThanOrEqual(3) + expect(resize).toHaveBeenLastCalledWith(79, 50) + }) + + it('skips parked (mobile-fit) frames without forwarding a desktop resize', () => { + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + const { resize, framesRun } = runReconcile({ + measure: pane.measure, + isParked: () => true + }) + expect(resize).not.toHaveBeenCalled() + expect(pane.measure).not.toHaveBeenCalled() + // Parked frames still count toward the cap so a parked PTY can't loop forever. + expect(framesRun).toBe(180) + }) + + it('resumes and converges after a transient park (mobile take-back during mount)', () => { + // Parked frames are SKIPPED, not cancelled: if mobile transiently drives the + // PTY during the mount window and then hands control back, the reconcile must + // resume and forward the settled desktop width — not abort permanently. + const PARKED_UNTIL = 10 + let frameSeen = 0 + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + const scheduler = createFrameScheduler() + const resize = vi.fn() + reconcilePtySizeAcrossFrames({ + spawnCols: 203, + spawnRows: 50, + isAlive: () => true, + isParked: () => frameSeen++ < PARKED_UNTIL, + isAuthoritative: () => true, + measure: pane.measure, + resize, + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame + }) + scheduler.run() + // While parked, measure()/resize() are skipped; after take-back the desktop + // width is measured and forwarded exactly once. + expect(resize).toHaveBeenCalledTimes(1) + expect(resize).toHaveBeenLastCalledWith(79, 50) + }) + + it('stops promptly once cancelled (pane disposed mid-reconcile)', () => { + const scheduler = createFrameScheduler() + const resize = vi.fn() + const pane = createTimelinePane((frame) => + frame < 30 ? { cols: 203, rows: 50 } : { cols: 79, rows: 50 } + ) + const handle = reconcilePtySizeAcrossFrames({ + spawnCols: 203, + spawnRows: 50, + isAlive: () => true, + isParked: () => false, + isAuthoritative: () => true, + measure: pane.measure, + resize, + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame + }) + // Run a few frames, then cancel — no further frames should be scheduled. + scheduler.run(3) + handle.cancel() + expect(scheduler.pending()).toBe(0) + const measuredBefore = pane.measure.mock.calls.length + scheduler.run(100) + expect(pane.measure.mock.calls.length).toBe(measuredBefore) + }) + + it('stops when the PTY is no longer alive (rebound / disposed)', () => { + const scheduler = createFrameScheduler() + const resize = vi.fn() + let alive = true + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + reconcilePtySizeAcrossFrames({ + spawnCols: 203, + spawnRows: 50, + isAlive: () => alive, + isParked: () => false, + isAuthoritative: () => true, + measure: pane.measure, + resize, + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame + }) + scheduler.run(2) + const callsBefore = resize.mock.calls.length + alive = false + scheduler.run(100) + expect(resize.mock.calls.length).toBe(callsBefore) + expect(scheduler.pending()).toBe(0) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-size-reconcile.ts b/src/renderer/src/components/terminal-pane/pty-size-reconcile.ts new file mode 100644 index 000000000..d8c290d69 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-size-reconcile.ts @@ -0,0 +1,144 @@ +// Why: the deferred-rAF fit can spawn the PTY at a stale (wide) width when the +// pane's real layout has not settled by the first frame — e.g. a tab that +// MOUNTS with a split layout already present (a new worktree opened with the +// side panel on). The PTY is born at the wide window width while xterm later +// reflows to the narrower split/pane width. The corrective xterm onResize is +// dropped during the mount window because it honors the visibility gate, which +// is not yet authoritative (deps.isVisibleRef flips true after mount). So +// process.stdout.columns stays pinned wide forever and interactive TUIs render +// garbled (output sized for the wide width wrapping ~1 char per line into the +// narrow pane). Only a later manual resize re-syncs it. +// +// This post-spawn reconcile bridges exactly that gap: it polls across frames, +// forwarding xterm's measured grid to the PTY on every actual change. Its +// resize is authoritative by definition, so it bypasses the visibility gate. +// It keeps polling while the pane is NOT yet authoritative (the hidden mount +// window where onResize is dropped) so a late-settling split/sidebar layout is +// still forwarded; once the pane is authoritative and the grid has been stable, +// it stops and hands off to the live onResize path, which catches any later +// layout change. A hard frame cap guarantees termination. +// +// This loop tracks what it last SENT, not what the PTY actually applied, so a +// forward dropped main-side (e.g. a mobile take-back resize-suppression window) +// can still leave the PTY stale here. The visibility-resume re-assert in +// pty-connection.ts is the backstop: on show it reads the PTY's real size +// (pty:getSize) and re-forwards on true drift, healing a pane that later +// hides/shows. + +export type PtySizeReconcileDimensions = { cols: number; rows: number } + +export type PtySizeReconcileOptions = { + /** Dimensions the PTY was spawned at — the size it currently believes it is. */ + spawnCols: number + spawnRows: number + /** True while this reconcile still owns a live PTY (not disposed / not rebound). */ + isAlive: () => boolean + /** + * True while the PTY is legitimately parked at non-pane dims (mobile-fit + * override / mobile driving). Such frames are skipped — neither fit nor + * forwarded — but still count toward the hard cap so a permanently-parked + * PTY cannot loop forever. + */ + isParked: () => boolean + /** + * True once the live onResize path will forward future PTY resizes itself + * (i.e. the pane is visible / renderer resize is authoritative). The + * reconcile only needs to run while this is false (the mount window where + * onResize is dropped); once true and the grid is stable it hands off. + */ + isAuthoritative: () => boolean + /** + * Fit the pane and return its current measured grid, or null when the pane is + * not yet measurable. A measured grid that differs from what the PTY was last + * told is forwarded; a matching grid counts toward the stability window. + */ + measure: () => PtySizeReconcileDimensions | null + /** Forward the settled size to the PTY (authoritative — bypasses visibility). */ + resize: (cols: number, rows: number) => void + /** Schedule the next frame; mirrors requestAnimationFrame's id contract. */ + requestFrame: (callback: () => void) => number + cancelFrame: (handle: number) => void +} + +export type PtySizeReconcileHandle = { cancel: () => void } + +// Hand off (stop) once the grid has held steady for SETTLE_FRAMES of frames +// observed *while authoritative*. Rationale for the two-part gate: +// - While hidden, the live onResize is dropped, so the reconcile is the SOLE +// corrector: it must keep polling and forwarding every change (a narrow that +// settles during the hidden window is the real shipped bug). Hidden frames +// therefore never count toward the settle window — the loop cannot hand off. +// - Once authoritative (pane visible), the live onResize/ResizeObserver path +// is the authoritative detector of any further reflow, so after a short +// stable window under authority the reconcile hands off to it. The settle +// window also gives a ~SETTLE-frame grace for a narrow landing right at the +// visibility transition before the handoff. +// MAX_FRAMES (~3s at 60fps) guarantees termination for a pane that never +// becomes authoritative (a permanently-hidden background spawn) or never +// stabilizes; such panes re-fit via the resume-time path when shown. +const POST_SPAWN_RECONCILE_SETTLE_FRAMES = 8 +const POST_SPAWN_RECONCILE_MAX_FRAMES = 180 + +export function reconcilePtySizeAcrossFrames( + options: PtySizeReconcileOptions +): PtySizeReconcileHandle { + let frame = 0 + // Counts consecutive unchanged frames observed *while authoritative*. Frames + // measured while hidden never advance it, so a long hidden-wide mount window + // cannot make the loop hand off the instant the pane becomes visible (before + // the split has equalized) on a width it never confirmed under authority. + let authoritativeStableFrames = 0 + let lastSentCols = options.spawnCols + let lastSentRows = options.spawnRows + let pendingFrame: number | null = null + let cancelled = false + + const tick = (): void => { + pendingFrame = null + if (cancelled || !options.isAlive()) { + return + } + frame += 1 + if (!options.isParked()) { + const measured = options.measure() + if (measured && measured.cols > 0 && measured.rows > 0) { + if (measured.cols !== lastSentCols || measured.rows !== lastSentRows) { + // Authoritative spawn-time correction: bypasses the visibility gate + // the live onResize honors (but the caller still skips parked frames). + // A real change resets the stability window so we wait for it to hold. + options.resize(measured.cols, measured.rows) + lastSentCols = measured.cols + lastSentRows = measured.rows + authoritativeStableFrames = 0 + } else if (options.isAuthoritative()) { + // Only stability seen *under authority* counts toward handoff — a grid + // that merely held steady while hidden is not a safe stopping point. + authoritativeStableFrames += 1 + } + } + // A null/zero measurement makes no stability progress: layout isn't ready. + } + // Why authoritative-gated rather than a fixed frame floor: while the pane is + // hidden the live onResize cannot forward, so the reconcile must keep + // watching (and forwarding, since its resize bypasses the gate) for a late + // layout settle. Once the pane has been visible AND its grid has held steady + // for the settle window, the live onResize/ResizeObserver path owns any + // further change, so we hand off. The hard cap guarantees termination. + const settled = authoritativeStableFrames >= POST_SPAWN_RECONCILE_SETTLE_FRAMES + if (!settled && frame < POST_SPAWN_RECONCILE_MAX_FRAMES) { + pendingFrame = options.requestFrame(tick) + } + } + + pendingFrame = options.requestFrame(tick) + + return { + cancel: () => { + cancelled = true + if (pendingFrame !== null) { + options.cancelFrame(pendingFrame) + pendingFrame = null + } + } + } +} diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index e7d0c976b..812e7607a 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -2542,6 +2542,7 @@ function createPtyApi(): NonNullable['pty']> { hasChildProcesses: () => Promise.resolve(false), getForegroundProcess: () => Promise.resolve(null), getCwd: () => Promise.resolve('~'), + getSize: () => Promise.resolve(null), listSessions: () => Promise.resolve([]), getMainBufferSnapshot: () => Promise.resolve(null), getRendererDeliveryDebugSnapshot: () => diff --git a/tests/e2e/terminal-column-desync-repro.spec.ts b/tests/e2e/terminal-column-desync-repro.spec.ts index f4c06de93..54fc3be17 100644 --- a/tests/e2e/terminal-column-desync-repro.spec.ts +++ b/tests/e2e/terminal-column-desync-repro.spec.ts @@ -276,6 +276,81 @@ test.describe('Terminal column desync repro', () => { } }) + // Why: the user-reported case — "start a new worktree with the side split + // panel on". A tab that MOUNTS with a split layout already present (two panes + // side by side from frame 0) spawns each PTY at the wide window width, then + // the split equalize narrows each pane AFTER the post-spawn reconcile window + // has closed. The corrective onResize is dropped by the visibility gate during + // the mount window, so the PTY stays pinned wide while xterm shows the narrow + // split width — only a later manual resize re-syncs it ("resizing fixed it"). + // We reproduce the fresh split mount by splitting then reloading: the split + // layout persists across reload, so the tab remounts with two panes already + // present, re-running the first-mount spawn for each. + test('both panes stay PTY-synced when a tab MOUNTS with a split layout present', async ({ + orcaPage + }) => { + test.setTimeout(240_000) + + // A single mount only trips the race intermittently, so reload-loop the + // restored-split first-mount and assert none of the attempts desynced. + const MOUNT_ATTEMPTS = 6 + const desyncs: { attempt: number; ptyId: string; ptyCols: number; xtermCols: number }[] = [] + + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + await closeRightSidebarAndFeatureTips(orcaPage) + await ensureTerminalVisible(orcaPage) + await orcaPage.setViewportSize({ width: 1440, height: 900 }) + await orcaPage.waitForTimeout(300) + await settleTerminal(orcaPage) + + // Establish the persisted split layout once; reloads below rebuild it. + await splitActiveTerminalPane(orcaPage, 'vertical') + await waitForPaneIdentitySnapshot(orcaPage, 2) + + for (let attempt = 0; attempt < MOUNT_ATTEMPTS; attempt += 1) { + // Re-run the split first-mount path: a wide window, reload so the tab + // remounts and re-spawns both PTYs at the wide width from the restored + // split layout, then resize down while the panes are still mounting. + await orcaPage.setViewportSize({ width: 1440, height: 900 }) + await orcaPage.reload() + await orcaPage.waitForFunction(() => Boolean(window.__store), null, { timeout: 30_000 }) + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + await closeRightSidebarAndFeatureTips(orcaPage) + await ensureTerminalVisible(orcaPage) + + // Resize narrower while the split panes are mounting / their PTYs spawn. + await orcaPage.setViewportSize({ width: 1180, height: 800 }) + await orcaPage.waitForTimeout(300) + + const snapshot = await waitForPaneIdentitySnapshot(orcaPage, 2) + // Let layout equalize and the (current) reconcile window run to completion. + await orcaPage.waitForTimeout(900) + + for (const pane of snapshot.panes) { + const ptyId = pane.ptyId + expect(ptyId, 'restored split pane should be bound to a PTY').toBeTruthy() + if (!ptyId) { + continue + } + const ptyCols = await readPtyCols(orcaPage, ptyId) + const xtermCols = await readRenderedColsForPty(orcaPage, ptyId) + if (ptyCols !== xtermCols) { + desyncs.push({ attempt, ptyId, ptyCols, xtermCols }) + } + } + } + + expect( + desyncs, + `PTY columns desynced from xterm on a restored-split mount (${desyncs.length} pane(s) ` + + `across ${MOUNT_ATTEMPTS} attempts). A PTY pinned at the wide startup width while xterm ` + + `reflowed to the narrower split width is the column-desync bug that garbles interactive ` + + `TUIs: ${JSON.stringify(desyncs)}` + ).toEqual([]) + }) + // Why: this is the tightest isolation of the real bug. A viewport resize that // lands in the terminal's initial mount window — after xterm exists but // before the PTY binding/visibility settle — reflows xterm to the new width,