mirror of
https://github.com/stablyai/orca.git
synced 2026-09-26 00:02:34 +00:00
fix(terminal): mark captured shortcut input interactive (#13800)
* fix(terminal): mark captured shortcut input interactive Refresh the interactive-redraw timestamp after a captured shortcut send succeeds, so the composer redraw that follows takes the low-latency foreground path instead of waiting out the 1s coalesce fallback. Orca's captured shortcut path sends directly through the captured pane transport to preserve pane, PTY and transport identity, and so bypasses xterm's onData -- the only place that previously stamped the timestamp. An idle pane therefore scheduled the post-Shift+Enter redraw as ordinary throughput. Measured in a real Pi pane on Windows: ~1029ms before, ~16-20ms after, at an identical ~2.3KB redraw. Stale pane bindings and rejected transport sends cannot refresh it. Fixes #10203 Refs #13598 * fix(terminal): keep captured shortcuts out of the pane-teardown signal lastTerminalInputAt has two readers, and the previous commit only meant to change one of them. onExit reads it as "the user never typed into this pane" to keep a newborn pane mounted when its shell dies on startup (the failing-.envrc direnv case, pty-connection.ts onExit) so the error stays visible and the worktree stays active. Stamping it from a captured shortcut therefore made a single Shift+Enter before that exit close the tab and bounce the user to Landing -- on every platform, since captured shortcuts are not Windows-only. Split the redraw window onto its own timestamp so captured shortcuts open the fast path without arming the teardown, and leave onExit's behaviour byte-identical to main. * test(terminal): pin the captured-shortcut wiring and its staleness guard Deleting the onAccepted block, or replacing its binding-identity check with true, both left the whole suite green — so nothing pinned the part of this PR that actually ships. Cover both against the existing IME keyboard harness. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
This commit is contained in:
@@ -29,7 +29,9 @@ function keyboardEvent(
|
||||
return event
|
||||
}
|
||||
|
||||
function createHarness(): {
|
||||
type ShortcutBinding = { markShortcutTerminalInputSent: ReturnType<typeof vi.fn> }
|
||||
|
||||
function createHarness(bindings?: Map<number, ShortcutBinding>): {
|
||||
deps: KeyboardHandlersDeps
|
||||
editable: HTMLInputElement
|
||||
sendInput: ReturnType<typeof vi.fn>
|
||||
@@ -77,7 +79,7 @@ function createHarness(): {
|
||||
keyboardScopeRef: { current: scope },
|
||||
managerRef: { current: manager },
|
||||
paneTransportsRef: { current: new Map([[pane.id, transport]]) },
|
||||
panePtyBindingsRef: { current: new Map() },
|
||||
panePtyBindingsRef: { current: (bindings ?? new Map()) as never },
|
||||
paneCwdRef: { current: new Map() },
|
||||
fallbackCwd: '',
|
||||
expandedPaneIdRef: { current: null },
|
||||
@@ -168,6 +170,56 @@ describe('Windows IME keyboard ownership', () => {
|
||||
harness.dispose()
|
||||
})
|
||||
|
||||
it('marks a captured shortcut send as interactive input', () => {
|
||||
const binding = { markShortcutTerminalInputSent: vi.fn() }
|
||||
const harness = createHarness(new Map([[1, binding]]))
|
||||
const hook = renderHook(() => useTerminalKeyboardShortcuts(harness.deps))
|
||||
|
||||
harness.terminalInput.dispatchEvent(
|
||||
keyboardEvent('keydown', {
|
||||
key: 'Enter',
|
||||
code: 'Enter',
|
||||
keyCode: 13,
|
||||
timeStamp: 10,
|
||||
shiftKey: true
|
||||
})
|
||||
)
|
||||
|
||||
expect(harness.sendInput).toHaveBeenCalledTimes(1)
|
||||
expect(binding.markShortcutTerminalInputSent).toHaveBeenCalledTimes(1)
|
||||
hook.unmount()
|
||||
harness.dispose()
|
||||
})
|
||||
|
||||
it('does not mark input for a pane binding replaced between capture and send', () => {
|
||||
// Why: the sender captures the binding, then re-reads it at send time — a rehomed
|
||||
// or reconnected pane must not have its redraw scheduling refreshed by the old one.
|
||||
const captured = { markShortcutTerminalInputSent: vi.fn() }
|
||||
const replacement = { markShortcutTerminalInputSent: vi.fn() }
|
||||
const bindings = new Map([[1, captured]])
|
||||
let reads = 0
|
||||
bindings.get = ((paneId: number) =>
|
||||
paneId === 1 ? (reads++ === 0 ? captured : replacement) : undefined) as never
|
||||
const harness = createHarness(bindings)
|
||||
const hook = renderHook(() => useTerminalKeyboardShortcuts(harness.deps))
|
||||
|
||||
harness.terminalInput.dispatchEvent(
|
||||
keyboardEvent('keydown', {
|
||||
key: 'Enter',
|
||||
code: 'Enter',
|
||||
keyCode: 13,
|
||||
timeStamp: 10,
|
||||
shiftKey: true
|
||||
})
|
||||
)
|
||||
|
||||
expect(harness.sendInput).toHaveBeenCalledTimes(1)
|
||||
expect(captured.markShortcutTerminalInputSent).not.toHaveBeenCalled()
|
||||
expect(replacement.markShortcutTerminalInputSent).not.toHaveBeenCalled()
|
||||
hook.unmount()
|
||||
harness.dispose()
|
||||
})
|
||||
|
||||
it('does not route an editable-target Enter keyup into the terminal', () => {
|
||||
const harness = createHarness()
|
||||
const hook = renderHook(() => useTerminalKeyboardShortcuts(harness.deps))
|
||||
|
||||
@@ -21,7 +21,8 @@ import { createTerminalImeDeferredChordSender } from './terminal-ime-deferred-ch
|
||||
import { hasPendingTerminalImeComposition } from './terminal-ime-composition-route'
|
||||
import {
|
||||
requestCapturedTerminalReconfirmation,
|
||||
sendCapturedTerminalInput
|
||||
sendCapturedTerminalInput,
|
||||
type TerminalCapturedInputBinding
|
||||
} from './terminal-captured-input-dispatch'
|
||||
import {
|
||||
keybindingMatchesAction,
|
||||
@@ -457,7 +458,7 @@ export function useTerminalKeyboardShortcuts({
|
||||
const capturedTransport = paneTransportsRef.current.get(pane.id)
|
||||
const capturedPtyId = capturedTransport?.getPtyId() ?? null
|
||||
const capturedBinding = panePtyBindingsRef.current.get(pane.id) as
|
||||
| (IDisposable & { requestWindowsShiftEnterReconfirmation?: () => void })
|
||||
| (IDisposable & TerminalCapturedInputBinding)
|
||||
| undefined
|
||||
const getCurrentManager = () => managerRef.current
|
||||
const getCurrentTransport = () => paneTransportsRef.current.get(pane.id)
|
||||
@@ -473,7 +474,12 @@ export function useTerminalKeyboardShortcuts({
|
||||
currentTransport: getCurrentTransport(),
|
||||
capturedTransport,
|
||||
capturedPtyId,
|
||||
data: overrideData
|
||||
data: overrideData,
|
||||
onAccepted: () => {
|
||||
if (getCurrentBinding() === capturedBinding) {
|
||||
capturedBinding?.markShortcutTerminalInputSent?.()
|
||||
}
|
||||
}
|
||||
})
|
||||
if (sent) {
|
||||
recordTerminalUserInputForLeaf(tabId, pane.leafId)
|
||||
|
||||
+26
@@ -256,6 +256,32 @@ describe('connectPanePty', () => {
|
||||
expect(transport.sendInput).toHaveBeenCalledWith('a')
|
||||
})
|
||||
|
||||
it('keeps large ANSI redraws after captured shortcut input on the immediate path', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const pane = createPane(1)
|
||||
const transport = createMockTransport('pty-1')
|
||||
const capturedDataCallback: { current: ((data: string) => void) | null } = { current: null }
|
||||
transport.connect.mockImplementation(async ({ callbacks }: { callbacks: ConnectCallbacks }) => {
|
||||
capturedDataCallback.current = callbacks.onData ?? null
|
||||
return 'pty-1'
|
||||
})
|
||||
transportFactoryQueue.push(transport)
|
||||
|
||||
const binding = connectPanePty(
|
||||
pane as never,
|
||||
createManager(1) as never,
|
||||
createDeps() as never
|
||||
) as unknown as { markShortcutTerminalInputSent: () => void }
|
||||
await flushAsyncTicks()
|
||||
binding.markShortcutTerminalInputSent()
|
||||
|
||||
const redraw = `\x1b[2J\x1b[H${'pi composer redraw '.repeat(200)}`
|
||||
expect(redraw.length).toBeGreaterThan(2_048)
|
||||
capturedDataCallback.current?.(redraw)
|
||||
|
||||
expect(pane.terminal.write).toHaveBeenCalledWith(redraw, expect.any(Function))
|
||||
})
|
||||
|
||||
it('does not let OpenTUI-style small ANSI redraw bursts monopolize foreground writes', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const pane = createPane(1)
|
||||
|
||||
@@ -335,6 +335,34 @@ describe('connectPanePty', () => {
|
||||
expect(manager.closePane).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('keeps a worktree sole terminal mounted when only a captured shortcut preceded the exit', async () => {
|
||||
// Why (regression): captured shortcuts refresh the redraw window, but that must not
|
||||
// count as "the user typed into this pane" — otherwise Shift+Enter before a direnv
|
||||
// failure closes the tab and bounces the user to Landing.
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport('tab-pty')
|
||||
transportFactoryQueue.push(transport)
|
||||
const manager = createManager(1)
|
||||
const deps = createDeps()
|
||||
|
||||
const binding = connectPanePty(
|
||||
createPane(1) as never,
|
||||
manager as never,
|
||||
deps as never
|
||||
) as unknown as { markShortcutTerminalInputSent: () => void }
|
||||
const onPtySpawn = createdTransportOptions[0]?.onPtySpawn as
|
||||
| ((ptyId: string) => void)
|
||||
| undefined
|
||||
const onPtyExit = createdTransportOptions[0]?.onPtyExit as ((ptyId: string) => void) | undefined
|
||||
|
||||
onPtySpawn?.('tab-pty')
|
||||
binding.markShortcutTerminalInputSent()
|
||||
onPtyExit?.('tab-pty')
|
||||
|
||||
expect(deps.onPtyExitRef.current).not.toHaveBeenCalled()
|
||||
expect(manager.closePane).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('tears down the sole terminal when a freshly-spawned PTY exits after the user typed input', async () => {
|
||||
// Why: an explicit `exit` (or any typed input) is a deliberate close, not a failed-startup shell, so the worktree should deactivate as before.
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
|
||||
@@ -708,6 +708,8 @@ type PanePtyBinding = IDisposable & {
|
||||
sampleForegroundAgentOnFocus: () => void
|
||||
/** Reconfirm after direct shortcut input, which bypasses PTY onData. */
|
||||
requestWindowsShiftEnterReconfirmation: () => void
|
||||
/** Refresh interactive redraw scheduling after captured shortcut input. */
|
||||
markShortcutTerminalInputSent: () => void
|
||||
reconcileIfSessionDead: (liveSessionIds: Set<string>, snapshotRequestedAt?: number) => void
|
||||
reconcileIfSessionMissing: (hasPty: HasPty, livenessRequestedAt?: number) => void
|
||||
}
|
||||
@@ -3828,16 +3830,24 @@ export function connectPanePty(
|
||||
Boolean(connectionId) && !shouldDeliverStartupViaTerminalPaste
|
||||
const hadExistingPaneTransportAtConnect = deps.paneTransportsRef.current.size > 0
|
||||
let lastTerminalInputAt = Number.NEGATIVE_INFINITY
|
||||
// Why: separate from lastTerminalInputAt because onExit reads that one as
|
||||
// "the user never typed into this pane" to keep a dead newborn pane mounted.
|
||||
// Captured shortcuts must open the redraw window without arming that teardown.
|
||||
let lastInteractiveRedrawInputAt = Number.NEGATIVE_INFINITY
|
||||
let hasReceivedPtyOutput = false
|
||||
let deferredReattachLiveData: DeferredReattachLiveDataQueue | null = null
|
||||
let reattachLiveDataDeferralDepth = 0
|
||||
let deferredReattachLiveDataOwners = new Map<number, { failed: boolean }>()
|
||||
let transportStreamGeneration = 0
|
||||
const markTerminalInputSent = (): void => {
|
||||
lastTerminalInputAt = performance.now()
|
||||
const markInteractiveRedrawInput = (): void => {
|
||||
lastInteractiveRedrawInputAt = performance.now()
|
||||
// Why: input must probe a wedged xterm even when the PTY produces no renderer output.
|
||||
requestTerminalWritePipelineProbe(pane.terminal)
|
||||
}
|
||||
const markTerminalInputSent = (): void => {
|
||||
lastTerminalInputAt = performance.now()
|
||||
markInteractiveRedrawInput()
|
||||
}
|
||||
const recordTerminalInputForHibernation = (): void => {
|
||||
useAppStore.getState().recordTerminalInput(cacheKey)
|
||||
}
|
||||
@@ -6347,7 +6357,7 @@ export function connectPanePty(
|
||||
return consumeForegroundImmediateBudget(data.length)
|
||||
}
|
||||
const recentInput =
|
||||
performance.now() - lastTerminalInputAt <= FOREGROUND_INTERACTIVE_REDRAW_WINDOW_MS
|
||||
performance.now() - lastInteractiveRedrawInputAt <= FOREGROUND_INTERACTIVE_REDRAW_WINDOW_MS
|
||||
if (
|
||||
recentInput &&
|
||||
data.length <= FOREGROUND_INTERACTIVE_REDRAW_CHARS &&
|
||||
@@ -6387,7 +6397,7 @@ export function connectPanePty(
|
||||
} {
|
||||
const rewriteOutputPrefersRenderRefresh = foregroundRewriteOutputPrefersRenderRefresh(data)
|
||||
const recentInput =
|
||||
performance.now() - lastTerminalInputAt <= FOREGROUND_INTERACTIVE_REDRAW_WINDOW_MS
|
||||
performance.now() - lastInteractiveRedrawInputAt <= FOREGROUND_INTERACTIVE_REDRAW_WINDOW_MS
|
||||
if (foregroundRendererRiskOutputPrefersRenderRefresh(data)) {
|
||||
return {
|
||||
refresh: true,
|
||||
@@ -6517,7 +6527,7 @@ export function connectPanePty(
|
||||
// Why: recompute the latch on every synchronized START so each frame's interactivity is judged by its own open time and can't leak across a same-chunk close+open; clear only on leaving synchronized output.
|
||||
if (synchronizedForegroundOutput && synchronizedOutputStarted) {
|
||||
synchronizedForegroundFrameInteractive =
|
||||
performance.now() - lastTerminalInputAt <=
|
||||
performance.now() - lastInteractiveRedrawInputAt <=
|
||||
FOREGROUND_SYNCHRONIZED_FRAME_INTERACTIVE_WINDOW_MS
|
||||
} else if (!nextSynchronizedForegroundOutputActive && !synchronizedOutputEnded) {
|
||||
synchronizedForegroundFrameInteractive = false
|
||||
@@ -9484,6 +9494,9 @@ export function connectPanePty(
|
||||
sampleVisiblePaneForegroundAgent()
|
||||
}, SHIFT_ENTER_RECONFIRM_IDLE_MS)
|
||||
},
|
||||
markShortcutTerminalInputSent() {
|
||||
markInteractiveRedrawInput()
|
||||
},
|
||||
reconcileIfSessionDead,
|
||||
reconcileIfSessionMissing,
|
||||
dispose() {
|
||||
|
||||
@@ -5,10 +5,10 @@ import {
|
||||
sendCapturedTerminalInput
|
||||
} from './terminal-captured-input-dispatch'
|
||||
|
||||
function createTransport(ptyId: string | null): PtyTransport {
|
||||
function createTransport(ptyId: string | null, sendResult = true): PtyTransport {
|
||||
return {
|
||||
getPtyId: vi.fn(() => ptyId),
|
||||
sendInput: vi.fn(() => true)
|
||||
sendInput: vi.fn(() => sendResult)
|
||||
} as unknown as PtyTransport
|
||||
}
|
||||
|
||||
@@ -31,11 +31,48 @@ describe('sendCapturedTerminalInput', () => {
|
||||
}
|
||||
)
|
||||
|
||||
it('runs the accepted callback only after a successful captured send', () => {
|
||||
const transport = createTransport('pty-original')
|
||||
const onAccepted = vi.fn()
|
||||
|
||||
expect(
|
||||
sendCapturedTerminalInput({
|
||||
targetPaneMounted: true,
|
||||
currentTransport: transport,
|
||||
capturedTransport: transport,
|
||||
capturedPtyId: 'pty-original',
|
||||
data: '\x1b[13;2u',
|
||||
onAccepted
|
||||
})
|
||||
).toBe(true)
|
||||
|
||||
expect(onAccepted).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('does not run the accepted callback for a rejected captured send', () => {
|
||||
const transport = createTransport('pty-original', false)
|
||||
const onAccepted = vi.fn()
|
||||
|
||||
expect(
|
||||
sendCapturedTerminalInput({
|
||||
targetPaneMounted: true,
|
||||
currentTransport: transport,
|
||||
capturedTransport: transport,
|
||||
capturedPtyId: 'pty-original',
|
||||
data: '\x1b[13;2u',
|
||||
onAccepted
|
||||
})
|
||||
).toBe(false)
|
||||
|
||||
expect(onAccepted).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it.each(['local IPC', 'SSH remote runtime'])(
|
||||
'does not deliver to a replacement %s transport for a reused pane',
|
||||
() => {
|
||||
const original = createTransport('pty-original')
|
||||
const replacement = createTransport('pty-replacement')
|
||||
const onAccepted = vi.fn()
|
||||
|
||||
expect(
|
||||
sendCapturedTerminalInput({
|
||||
@@ -43,11 +80,13 @@ describe('sendCapturedTerminalInput', () => {
|
||||
currentTransport: replacement,
|
||||
capturedTransport: original,
|
||||
capturedPtyId: 'pty-original',
|
||||
data: '\r'
|
||||
data: '\r',
|
||||
onAccepted
|
||||
})
|
||||
).toBe(false)
|
||||
expect(original.sendInput).not.toHaveBeenCalled()
|
||||
expect(replacement.sendInput).not.toHaveBeenCalled()
|
||||
expect(onAccepted).not.toHaveBeenCalled()
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
@@ -6,10 +6,12 @@ type CapturedTerminalInputDispatch = {
|
||||
capturedTransport: PtyTransport | undefined
|
||||
capturedPtyId: string | null
|
||||
data: string
|
||||
onAccepted?: () => void
|
||||
}
|
||||
|
||||
export type TerminalReconfirmationBinding = {
|
||||
export type TerminalCapturedInputBinding = {
|
||||
requestWindowsShiftEnterReconfirmation?: () => void
|
||||
markShortcutTerminalInputSent?: () => void
|
||||
}
|
||||
|
||||
export function sendCapturedTerminalInput({
|
||||
@@ -17,7 +19,8 @@ export function sendCapturedTerminalInput({
|
||||
currentTransport,
|
||||
capturedTransport,
|
||||
capturedPtyId,
|
||||
data
|
||||
data,
|
||||
onAccepted
|
||||
}: CapturedTerminalInputDispatch): boolean {
|
||||
if (
|
||||
!targetPaneMounted ||
|
||||
@@ -28,12 +31,16 @@ export function sendCapturedTerminalInput({
|
||||
) {
|
||||
return false
|
||||
}
|
||||
return capturedTransport.sendInput(data)
|
||||
const sent = capturedTransport.sendInput(data)
|
||||
if (sent) {
|
||||
onAccepted?.()
|
||||
}
|
||||
return sent
|
||||
}
|
||||
|
||||
export function requestCapturedTerminalReconfirmation(
|
||||
currentBinding: object | undefined,
|
||||
capturedBinding: TerminalReconfirmationBinding | undefined
|
||||
capturedBinding: TerminalCapturedInputBinding | undefined
|
||||
): void {
|
||||
if (currentBinding === capturedBinding) {
|
||||
capturedBinding?.requestWindowsShiftEnterReconfirmation?.()
|
||||
|
||||
Reference in New Issue
Block a user