From 0f67f57cce663df441def01b5247cd482e869e75 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:01:42 -0700 Subject: [PATCH] fix(terminal): bound the deferred paste by wall clock, refuse partial replays, and stop hidden panes claiming the chord Adversarial review of STA-5272: - A chunked paste re-checks focus between chunks, so `stale-target` can arrive with bytes already in the PTY. Deferring that and replaying the whole payload duplicated everything written before the cancel. `chunksWritten` is now a required conjunct of isDeferrablePasteFocusCancellation. - Electron leaves backgroundThrottling on, so a hidden renderer's 2s deadline timer is aligned into 1s (then 1-minute) buckets. Restoring the window fires focusin before the overdue timer, landing the paste minutes later. The wall clock now bounds the payload; the timer only notifies. - A pane can stop being rendered while isActive stays true (closed floating panel, display:none background surface). The browser blurs it to with no focusin, so paneOwnsFocus stays stale and the document-capture listener swallowed the chord for the whole app. Ownership now requires a rendered container, and the cheap checks short-circuit first on the keystroke path. - A deferred payload whose pane was closed is released on the next focusin instead of being retained to the deadline. --- .../components/terminal-pane/TerminalPane.tsx | 1 + .../terminal-deferred-paste.test.ts | 230 ++++++++++++++++++ .../terminal-pane/terminal-deferred-paste.ts | 62 ++++- ...erminal-inert-focus-paste-fallback.test.ts | 74 ++++++ .../terminal-inert-focus-paste-fallback.ts | 31 ++- 5 files changed, 390 insertions(+), 8 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/TerminalPane.tsx b/src/renderer/src/components/terminal-pane/TerminalPane.tsx index 8017a78f2fb..a57234db9ca 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPane.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPane.tsx @@ -2042,6 +2042,7 @@ function TerminalPane( isDeferrablePasteFocusCancellation({ status: execution.status, reason: execution.reason, + chunksWritten: execution.chunksWritten, targetMounted: isPanePasteTargetMounted(pane, transport, ptyId), focusMovedToOtherPane: isFocusInsideOtherPane({ panes: managerRef.current?.getPanes() ?? [], diff --git a/src/renderer/src/components/terminal-pane/terminal-deferred-paste.test.ts b/src/renderer/src/components/terminal-pane/terminal-deferred-paste.test.ts index cc7d1f2482e..a8ac228e04c 100644 --- a/src/renderer/src/components/terminal-pane/terminal-deferred-paste.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-deferred-paste.test.ts @@ -1,6 +1,7 @@ // @vitest-environment happy-dom import { describe, expect, it, vi, type Mock } from 'vitest' +import { executeTerminalPastePlan, planTerminalPaste } from './terminal-paste-coordinator' import { createDeferredPasteFocusInHandler, createDeferredTerminalPasteQueue, @@ -144,6 +145,46 @@ describe('deferred terminal paste queue', () => { ) }) + // STA-5272 review: Electron leaves `backgroundThrottling` on, so a hidden + // renderer's timers are aligned to 1s and, after 5 minutes hidden, to 1-minute + // buckets. The deadline timer can therefore still be pending long past 2s while + // restoring the window fires focusin first — landing the paste minutes later. + // The wall clock, not the timer, has to be what bounds the payload. + it('refuses a claim made after the deadline even if the timer has not fired yet', () => { + const timers = createTimerHarness() + const onExpire = vi.fn() + let nowMs = 1_000 + const queue = createDeferredTerminalPasteQueue({ + onExpire, + now: () => nowMs, + ...timers + }) + + queue.defer(entry()) + // The window was hidden; the timer is still queued, unfired. + nowMs += TERMINAL_DEFERRED_PASTE_TIMEOUT_MS + 1 + expect(timers.live()).toHaveLength(1) + + expect(queue.claim(1, 'leaf-a')).toBe(null) + // and the user is told rather than silently robbed of the paste + expect(onExpire).toHaveBeenCalledTimes(1) + expect(queue.isPending()).toBe(false) + expect(timers.live()).toEqual([]) + }) + + it('still honours a claim made inside the deadline', () => { + const timers = createTimerHarness() + const onExpire = vi.fn() + let nowMs = 1_000 + const queue = createDeferredTerminalPasteQueue({ onExpire, now: () => nowMs, ...timers }) + + queue.defer(entry()) + nowMs += TERMINAL_DEFERRED_PASTE_TIMEOUT_MS - 1 + + expect(queue.claim(1, 'leaf-a')).toEqual(entry()) + expect(onExpire).not.toHaveBeenCalled() + }) + it('drops the payload on dispose without firing the expiry signal', () => { const timers = createTimerHarness() const onExpire = vi.fn() @@ -162,6 +203,7 @@ describe('isDeferrablePasteFocusCancellation', () => { const base = { status: 'cancelled' as const, reason: 'stale-target' as const, + chunksWritten: 0, targetMounted: true, focusMovedToOtherPane: false } @@ -179,6 +221,16 @@ describe('isDeferrablePasteFocusCancellation', () => { expect(isDeferrablePasteFocusCancellation({ ...base, targetMounted: false })).toBe(false) }) + // Ablation gap: every other case here is already excluded by the reason check, + // so the status conjunct needs a reason it would otherwise let through. Today + // the executor only pairs 'rejected' with 'payload-too-large' (see + // terminal-paste-coordinator planning), so this pins the contract, not a + // currently reachable pairing. + it('refuses to defer anything that is not a cancellation, whatever the reason says', () => { + expect(isDeferrablePasteFocusCancellation({ ...base, status: 'rejected' })).toBe(false) + expect(isDeferrablePasteFocusCancellation({ ...base, status: 'pasted' })).toBe(false) + }) + it('refuses to defer non-focus cancellations and rejections', () => { expect(isDeferrablePasteFocusCancellation({ ...base, reason: 'target-disconnected' })).toBe( false @@ -410,3 +462,181 @@ describe('deferred paste focusin handler', () => { expect(scene.onDropped).not.toHaveBeenCalled() }) }) + +// STA-5272 review: `stale-target` is not only a pre-write verdict. The chunked +// writer re-checks the target between chunks, so a focus move part-way through a +// large paste cancels with bytes already in the PTY. Deferring that payload and +// replaying it whole duplicates everything written before the cancel. +describe('partially written pastes are not deferrable', () => { + const chunkedPlan = (text: string) => + planTerminalPaste({ + text, + source: 'keyboard', + target: { + kind: 'terminal', + paneId: 1, + leafId: 'leaf-a', + ptyId: 'pty-1', + runtime: { platform: 'darwin', runtimeKey: 'local:darwin', kind: 'local' } + }, + maxDirectBytes: 4, + maxChunkBytes: 8 + }) + + it('premise: the real executor cancels mid-write with bytes already in the pty', async () => { + const writes: string[] = [] + let focusHeld = true + + const execution = await executeTerminalPastePlan(chunkedPlan('abcdefgh12345678zzzzzzzz'), { + pasteText: () => {}, + writePty: (data) => { + writes.push(data) + return true + }, + // Focus leaves the pane after the first chunk lands, exactly as a + // dictation/overlay handoff does mid-paste. + isTargetCurrent: () => { + if (writes.length >= 1) { + focusHeld = false + } + return focusHeld + } + }) + + expect(execution.status).toBe('cancelled') + expect(execution.reason).toBe('stale-target') + expect(execution.chunksWritten).toBeGreaterThan(0) + expect(writes.join('')).not.toBe('abcdefgh12345678zzzzzzzz') + }) + + it('refuses to defer a cancellation that already wrote bytes to the pty', () => { + expect( + isDeferrablePasteFocusCancellation({ + status: 'cancelled', + reason: 'stale-target', + chunksWritten: 3, + targetMounted: true, + focusMovedToOtherPane: false + }) + ).toBe(false) + }) + + it('still defers a cancellation that wrote nothing', () => { + expect( + isDeferrablePasteFocusCancellation({ + status: 'cancelled', + reason: 'stale-target', + chunksWritten: 0, + targetMounted: true, + focusMovedToOtherPane: false + }) + ).toBe(true) + }) +}) + +// STA-5272 review: the pane the payload was aimed at can be closed while the +// payload waits. Nothing may be written anywhere, and the user has to be told +// now rather than after the full deadline, because the text is still retained. +describe('a deferred paste whose pane is destroyed before focus returns', () => { + const buildScene = (): { + panes: { id: number; leafId: string; container: HTMLElement }[] + ptyWrites: string[] + queue: ReturnType + handler: () => void + onDropped: Mock<(deferred: DeferredTerminalPaste) => void> + onExpire: Mock<(deferred: DeferredTerminalPaste) => void> + outside: HTMLInputElement + } => { + document.body.innerHTML = '' + const root = document.createElement('div') + document.body.append(root) + const panes = [0, 1].map((index) => { + const container = document.createElement('div') + const helper = document.createElement('textarea') + helper.className = 'xterm-helper-textarea' + container.append(helper) + root.append(container) + return { id: index + 1, leafId: `leaf-${index}`, container } + }) + const outside = document.createElement('input') + document.body.append(outside) + const ptyWrites: string[] = [] + const onExpire = vi.fn<(deferred: DeferredTerminalPaste) => void>() + const queue = createDeferredTerminalPasteQueue({ onExpire, ...createTimerHarness() }) + const onDropped = vi.fn<(deferred: DeferredTerminalPaste) => void>() + const handler = createDeferredPasteFocusInHandler({ + queue, + getPanes: () => panes, + getFocusedElement: () => document.activeElement, + // The only thing delivery does in production is run the paste, which ends + // in a PTY write; a spy that records nothing would hide a real write. + deliver: (_pane, deferred) => ptyWrites.push(deferred.text), + onDropped + }) + return { panes, ptyWrites, queue, handler, onDropped, onExpire, outside } + } + + it('drops the payload and tells the user when focus lands outside every pane', () => { + const scene = buildScene() + scene.queue.defer(entry({ paneId: 1, leafId: 'leaf-0', text: 'secret token' })) + + // The pane is closed: it is gone from the manager and out of the DOM. + scene.panes[0]!.container.remove() + scene.panes.splice(0, 1) + scene.outside.focus() + scene.handler() + + expect(scene.ptyWrites).toEqual([]) + expect(scene.onDropped).toHaveBeenCalledTimes(1) + expect(scene.onDropped.mock.calls[0]?.[0]?.text).toBe('secret token') + // Retention: the clipboard text is released with the pane, not held to the deadline. + expect(scene.queue.isPending()).toBe(false) + expect(scene.onExpire).not.toHaveBeenCalled() + }) + + it('drops the payload and tells the user when focus lands in a surviving sibling', () => { + const scene = buildScene() + scene.queue.defer(entry({ paneId: 1, leafId: 'leaf-0' })) + + scene.panes[0]!.container.remove() + scene.panes.splice(0, 1) + ;(scene.panes[0]!.container.firstElementChild as HTMLElement).focus() + scene.handler() + + expect(scene.ptyWrites).toEqual([]) + expect(scene.onDropped).toHaveBeenCalledTimes(1) + expect(scene.queue.isPending()).toBe(false) + }) + + it('leaves the payload alone while the pane manager reports no panes at all', () => { + // A transiently empty manager during a re-render is not evidence the target + // pane was closed; treating it as one would drop a healthy deferred paste. + const scene = buildScene() + scene.queue.defer(entry({ paneId: 1, leafId: 'leaf-0' })) + const panesSnapshot = [...scene.panes] + scene.panes.length = 0 + + scene.outside.focus() + scene.handler() + + expect(scene.onDropped).not.toHaveBeenCalled() + expect(scene.queue.isPending()).toBe(true) + + // and it still lands once the manager reports the pane again + scene.panes.push(...panesSnapshot) + ;(scene.panes[0]!.container.firstElementChild as HTMLElement).focus() + scene.handler() + expect(scene.ptyWrites).toEqual(['dictated paragraph']) + }) + + it('keeps a live pane pending so the destroy check cannot swallow a normal defer', () => { + const scene = buildScene() + scene.queue.defer(entry({ paneId: 1, leafId: 'leaf-0' })) + + scene.outside.focus() + scene.handler() + + expect(scene.onDropped).not.toHaveBeenCalled() + expect(scene.queue.isPending()).toBe(true) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-deferred-paste.ts b/src/renderer/src/components/terminal-pane/terminal-deferred-paste.ts index 659d2fe6a9b..0b048243efa 100644 --- a/src/renderer/src/components/terminal-pane/terminal-deferred-paste.ts +++ b/src/renderer/src/components/terminal-pane/terminal-deferred-paste.ts @@ -25,6 +25,9 @@ export type DeferredTerminalPasteQueue = { /** Drops the payload without firing the expiry callback; returns what was dropped. */ cancel: () => DeferredTerminalPaste | null isPending: () => boolean + /** Identity of the pending payload, never its text, so callers can check the + * target is still alive without a second copy of the clipboard escaping. */ + pendingTarget: () => { paneId: number; leafId: string } | null dispose: () => void } @@ -33,16 +36,19 @@ type CreateDeferredTerminalPasteQueueArgs = { timeoutMs?: number setTimer?: (callback: () => void, ms: number) => number clearTimer?: (timerId: number) => void + now?: () => number } export function createDeferredTerminalPasteQueue({ onExpire, timeoutMs = TERMINAL_DEFERRED_PASTE_TIMEOUT_MS, setTimer = (callback, ms) => window.setTimeout(callback, ms), - clearTimer = (timerId) => window.clearTimeout(timerId) + clearTimer = (timerId) => window.clearTimeout(timerId), + now = () => Date.now() }: CreateDeferredTerminalPasteQueueArgs): DeferredTerminalPasteQueue { let pending: DeferredTerminalPaste | null = null let timerId: number | null = null + let deadlineAtMs = 0 const take = (): DeferredTerminalPaste | null => { const taken = pending @@ -54,10 +60,25 @@ export function createDeferredTerminalPasteQueue({ return taken } + // Why: Electron leaves backgroundThrottling on, so a hidden renderer's timers + // are aligned into 1s (then 1-minute) buckets. The wall clock is what bounds + // the payload; the timer is only how the user gets told promptly. + const releaseIfExpired = (): boolean => { + if (!pending || now() < deadlineAtMs) { + return false + } + const expired = take() + if (expired) { + onExpire(expired) + } + return true + } + return { defer: (entry) => { take() pending = entry + deadlineAtMs = now() + timeoutMs timerId = setTimer(() => { timerId = null // Why: release the clipboard text before notifying, so a throwing @@ -70,13 +91,23 @@ export function createDeferredTerminalPasteQueue({ }, timeoutMs) }, claim: (paneId, leafId) => { + if (releaseIfExpired()) { + return null + } if (!pending || pending.paneId !== paneId || pending.leafId !== leafId) { return null } return take() }, cancel: () => take(), - isPending: () => pending !== null, + isPending: () => { + releaseIfExpired() + return pending !== null + }, + pendingTarget: () => { + releaseIfExpired() + return pending ? { paneId: pending.paneId, leafId: pending.leafId } : null + }, dispose: () => { take() } @@ -84,20 +115,29 @@ export function createDeferredTerminalPasteQueue({ } /** A paste the focus guard stopped is only deferrable while its pane is still the - * live target and no other pane has taken focus — the case the guard exists for. */ + * live target and no other pane has taken focus — the case the guard exists for. + * `chunksWritten` is load-bearing: the chunked writer re-checks focus between + * chunks, so a cancel can arrive with bytes already in the PTY, and redelivering + * the whole payload would duplicate everything written before the cancel. */ export function isDeferrablePasteFocusCancellation({ status, reason, + chunksWritten, targetMounted, focusMovedToOtherPane }: { status: TerminalPasteExecutionResult['status'] reason: TerminalPasteExecutionReason | undefined + chunksWritten: number targetMounted: boolean focusMovedToOtherPane: boolean }): boolean { return ( - status === 'cancelled' && reason === 'stale-target' && targetMounted && !focusMovedToOtherPane + status === 'cancelled' && + reason === 'stale-target' && + chunksWritten === 0 && + targetMounted && + !focusMovedToOtherPane ) } @@ -110,7 +150,7 @@ type DeferredPasteFocusPane = { export type DeferredPasteFocusResolution = | { action: 'ignore' } | { action: 'deliver'; pane: TPane; entry: DeferredTerminalPaste } - | { action: 'drop'; pane: TPane; entry: DeferredTerminalPaste | null } + | { action: 'drop'; pane: TPane | null; entry: DeferredTerminalPaste | null } /** Focus landing back inside the deferred pane delivers it; focus landing in a * different pane drops it, because that is the wrong-target case the guard protects. */ @@ -123,10 +163,20 @@ export function resolveDeferredPasteFocusIn { - if (!queue.isPending() || !focusedElement) { + const target = queue.pendingTarget() + if (!target || !focusedElement) { return { action: 'ignore' } } const pane = panes.find((candidate) => candidate.container.contains(focusedElement)) + // Why: the target pane was closed while the payload waited. Release the + // clipboard text now rather than retaining it for a pane that cannot receive + // it; `panes.length` guards a transiently empty manager during a re-render. + const targetGone = + panes.length > 0 && + !panes.some((candidate) => candidate.id === target.paneId && candidate.leafId === target.leafId) + if (targetGone) { + return { action: 'drop', pane: pane ?? null, entry: queue.cancel() } + } if (!pane) { return { action: 'ignore' } } diff --git a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts index 2a3ab31aa6c..6908447a7d4 100644 --- a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts @@ -178,6 +178,80 @@ describe('inert-focus paste fallback', () => { expect(fallback.ownsInertFocus()).toBe(false) }) + // STA-5272 review: the pane can stop being rendered while `isActive` stays + // true (a CSS-hidden floating panel, a display:none background surface). The + // browser blurs its focus to WITHOUT a focusin, so `paneOwnsFocus` + // stays stale. These listeners sit at the document in capture phase, so a + // stale claim swallows the chord (preventDefault + stopPropagation) for the + // whole app and routes the paste into an invisible terminal. + it('does not claim inert focus once the pane is no longer rendered', () => { + const fallback = install() + inside.focus() + inside.blur() + expect(fallback.ownsInertFocus()).toBe(true) + + // Exactly how the closed floating terminal panel hides its subtree. + container.style.visibility = 'hidden' + + expect(fallback.ownsInertFocus()).toBe(false) + }) + + it('does not swallow the paste chord for the rest of the app while hidden', () => { + install() + inside.focus() + inside.blur() + container.style.visibility = 'hidden' + + pressPasteChordOn(document.body) + document.body.dispatchEvent(new Event('paste', { bubbles: true, cancelable: true })) + + expect(onPasteKey).not.toHaveBeenCalled() + expect(onPasteEvent).not.toHaveBeenCalled() + }) + + it('does not claim inert focus after its container leaves the document', () => { + const fallback = install() + inside.focus() + inside.blur() + container.remove() + + expect(fallback.ownsInertFocus()).toBe(false) + pressPasteChordOn(document.body) + expect(onPasteKey).not.toHaveBeenCalled() + }) + + it('honours the browser visibility check that covers display:none subtrees', () => { + // happy-dom has no checkVisibility; production Chromium does, and it is the + // only check that sees a display:none ancestor. Pin that we call it. + const calls: unknown[] = [] + ;(container as HTMLElement & { checkVisibility?: (o?: unknown) => boolean }).checkVisibility = ( + options + ) => { + calls.push(options) + return false + } + const fallback = install() + inside.focus() + inside.blur() + + expect(fallback.ownsInertFocus()).toBe(false) + expect(calls.length).toBeGreaterThan(0) + pressPasteChordOn(document.body) + expect(onPasteKey).not.toHaveBeenCalled() + }) + + it('still recovers when the browser reports the container as visible', () => { + ;(container as HTMLElement & { checkVisibility?: (o?: unknown) => boolean }).checkVisibility = + () => true + install() + inside.focus() + inside.blur() + + pressPasteChordOn(document.body) + + expect(onPasteKey).toHaveBeenCalledTimes(1) + }) + it('stops recovering pastes after dispose', () => { const fallback = install() inside.focus() diff --git a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts index b5f00cfe08a..2b230b3ec91 100644 --- a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts +++ b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts @@ -37,10 +37,16 @@ export function installInertFocusPasteFallback({ paneOwnsFocus = false } } + // Order matters: these run at the document on every keystroke in the app, once + // per mounted pane. The two boolean checks short-circuit before any DOM walk, + // and the layout-touching visibility check only runs for the at-most-one pane + // that both owns focus and has focus sitting on . const ownsInertFocus = (): boolean => - paneOwnsFocus && isInertDocumentFocus(documentTarget.activeElement) + paneOwnsFocus && + isInertDocumentFocus(documentTarget.activeElement) && + isContainerRendered(container) const shouldRecover = (event: Event): boolean => - !containsNode(container, event.target) && ownsInertFocus() + ownsInertFocus() && !containsNode(container, event.target) const onKeyDown = (event: KeyboardEvent): void => { if (shouldRecover(event)) { @@ -73,6 +79,27 @@ function containsNode(container: HTMLElement, node: EventTarget | Node | null): return node instanceof Node && container.contains(node) } +/** A pane that stopped being rendered (CSS-hidden floating panel, display:none + * background surface) was blurred to `` by the browser with no focusin, so + * `paneOwnsFocus` stays stale. Claiming on that swallows the chord for the whole + * app at the capture phase and routes the paste into an invisible terminal. */ +function isContainerRendered(container: HTMLElement): boolean { + if (!container.isConnected) { + return false + } + const checkVisibility = ( + container as HTMLElement & { + checkVisibility?: (options?: { visibilityProperty?: boolean }) => boolean + } + ).checkVisibility + if (typeof checkVisibility === 'function') { + return checkVisibility.call(container, { visibilityProperty: true }) + } + // Fallback for engines without checkVisibility; `visibility` is inherited, so + // reading it off the container also catches a hidden ancestor. + return container.ownerDocument.defaultView?.getComputedStyle(container).visibility !== 'hidden' +} + /** Focus is on but the pane still owns it logically; putting it back before * the shared paste path runs lets that path's dispatch-element guard see the real target. */ export function restoreInertFocusPasteTarget(