From 44b63fc6de3fe2b53eac3109b4a20db41c663195 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Fri, 15 May 2026 21:59:28 -0500 Subject: [PATCH] fix(tui): allow transcript scroll + Esc during approval/clarify/confirm prompts (#26414) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an approval / clarify / confirm overlay was active, the global input handler in useInputHandlers returned for every key that wasn't Ctrl+C, which silently disabled transcript scrolling. On long threads the context the prompt was asking about often lived above the visible viewport, and being unable to scroll while answering felt like the prompt had locked the UI. ApprovalPrompt also had no Esc handler at all, so the one obvious 'abort' key did nothing during a permission prompt and the user had to memorize Ctrl+C or hunt for the deny number. Fixes: - Extract shouldFallThroughForScroll(key) (pure, exported) covering wheel scrolls, PageUp/PageDown, and Shift+ArrowUp/Down. When a prompt overlay is up and the pressed key is a scroll input, skip the early return so it reaches the existing wheel/PageUp/Shift+arrow handlers below. Plain arrows still drive in-prompt selection — they don't fall through. - ApprovalPrompt now maps Esc to onChoice('deny'), parity with the global Ctrl+C cancellation path that already invokes cancelOverlayFromCtrlC() for approvals. The bottom-of-prompt hint now advertises 'Esc/Ctrl+C deny'. - Extract approvalAction(ch, key, sel) — pure key-dispatch helper for the approval prompt, exported so the regression matrix (Esc, numbers, Enter, arrows, edge clamping, precedence) is testable without mounting Ink. Tests: - useInputHandlers.test.ts: 6 cases covering shouldFallThroughForScroll positives (wheel/PageUp/PageDown/Shift+arrows) and negatives (plain arrows, bare shift, no scroll key). - approvalAction.test.ts: 8 cases covering Esc→deny, numeric mapping, Enter, ↑↓ within bounds, edge clamping, Esc-beats-others precedence, unrelated keystrokes. --- ui-tui/src/__tests__/approvalAction.test.ts | 50 +++++++++++++ ui-tui/src/__tests__/useInputHandlers.test.ts | 42 ++++++++++- ui-tui/src/app/useInputHandlers.ts | 57 ++++++++++++++- ui-tui/src/components/prompts.tsx | 73 ++++++++++++++----- 4 files changed, 201 insertions(+), 21 deletions(-) create mode 100644 ui-tui/src/__tests__/approvalAction.test.ts diff --git a/ui-tui/src/__tests__/approvalAction.test.ts b/ui-tui/src/__tests__/approvalAction.test.ts new file mode 100644 index 00000000000..851b5093448 --- /dev/null +++ b/ui-tui/src/__tests__/approvalAction.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest' + +import { approvalAction } from '../components/prompts.js' + +describe('approvalAction — pure key dispatch for ApprovalPrompt', () => { + it('maps Esc to deny — parity with global Ctrl+C cancellation', () => { + expect(approvalAction('', { escape: true }, 0)).toEqual({ kind: 'choose', choice: 'deny' }) + expect(approvalAction('', { escape: true }, 2)).toEqual({ kind: 'choose', choice: 'deny' }) + }) + + it('maps number keys 1..4 to once/session/always/deny in registration order', () => { + expect(approvalAction('1', {}, 0)).toEqual({ kind: 'choose', choice: 'once' }) + expect(approvalAction('2', {}, 0)).toEqual({ kind: 'choose', choice: 'session' }) + expect(approvalAction('3', {}, 0)).toEqual({ kind: 'choose', choice: 'always' }) + expect(approvalAction('4', {}, 0)).toEqual({ kind: 'choose', choice: 'deny' }) + }) + + it('ignores out-of-range numbers', () => { + expect(approvalAction('0', {}, 1)).toEqual({ kind: 'noop' }) + expect(approvalAction('5', {}, 1)).toEqual({ kind: 'noop' }) + expect(approvalAction('9', {}, 1)).toEqual({ kind: 'noop' }) + }) + + it('confirms the current selection on Enter', () => { + expect(approvalAction('', { return: true }, 0)).toEqual({ kind: 'choose', choice: 'once' }) + expect(approvalAction('', { return: true }, 3)).toEqual({ kind: 'choose', choice: 'deny' }) + }) + + it('moves selection up/down within bounds', () => { + expect(approvalAction('', { upArrow: true }, 2)).toEqual({ kind: 'move', delta: -1 }) + expect(approvalAction('', { downArrow: true }, 1)).toEqual({ kind: 'move', delta: 1 }) + }) + + it('clamps selection movement at the edges', () => { + expect(approvalAction('', { upArrow: true }, 0)).toEqual({ kind: 'noop' }) + expect(approvalAction('', { downArrow: true }, 3)).toEqual({ kind: 'noop' }) + }) + + it('Esc beats numeric/return — denying is always the first interpretation', () => { + // If a terminal somehow delivers Esc + a digit in the same event, deny + // wins. Documents the precedence so a future refactor doesn't flip it. + expect(approvalAction('1', { escape: true }, 0)).toEqual({ kind: 'choose', choice: 'deny' }) + expect(approvalAction('', { escape: true, return: true }, 1)).toEqual({ kind: 'choose', choice: 'deny' }) + }) + + it('returns noop for unrelated keystrokes (printable letters etc.)', () => { + expect(approvalAction('a', {}, 0)).toEqual({ kind: 'noop' }) + expect(approvalAction(' ', {}, 0)).toEqual({ kind: 'noop' }) + }) +}) diff --git a/ui-tui/src/__tests__/useInputHandlers.test.ts b/ui-tui/src/__tests__/useInputHandlers.test.ts index 066292abfa5..0d3fd69c1ed 100644 --- a/ui-tui/src/__tests__/useInputHandlers.test.ts +++ b/ui-tui/src/__tests__/useInputHandlers.test.ts @@ -1,6 +1,46 @@ import { describe, expect, it, vi } from 'vitest' -import { applyVoiceRecordResponse } from '../app/useInputHandlers.js' +import { applyVoiceRecordResponse, shouldFallThroughForScroll } from '../app/useInputHandlers.js' + +const baseKey = { + downArrow: false, + pageDown: false, + pageUp: false, + shift: false, + upArrow: false, + wheelDown: false, + wheelUp: false +} + +describe('shouldFallThroughForScroll — keep transcript scrolling alive during prompt overlays', () => { + it('falls through for wheel scrolls', () => { + expect(shouldFallThroughForScroll({ ...baseKey, wheelUp: true })).toBe(true) + expect(shouldFallThroughForScroll({ ...baseKey, wheelDown: true })).toBe(true) + }) + + it('falls through for PageUp / PageDown', () => { + expect(shouldFallThroughForScroll({ ...baseKey, pageUp: true })).toBe(true) + expect(shouldFallThroughForScroll({ ...baseKey, pageDown: true })).toBe(true) + }) + + it('falls through for Shift+ArrowUp / Shift+ArrowDown', () => { + expect(shouldFallThroughForScroll({ ...baseKey, shift: true, upArrow: true })).toBe(true) + expect(shouldFallThroughForScroll({ ...baseKey, shift: true, downArrow: true })).toBe(true) + }) + + it('does NOT fall through for plain arrows — those drive in-prompt selection', () => { + expect(shouldFallThroughForScroll({ ...baseKey, upArrow: true })).toBe(false) + expect(shouldFallThroughForScroll({ ...baseKey, downArrow: true })).toBe(false) + }) + + it('does NOT fall through for plain Shift — without an arrow it is a no-op', () => { + expect(shouldFallThroughForScroll({ ...baseKey, shift: true })).toBe(false) + }) + + it('does NOT fall through for unrelated state (no scroll keys held)', () => { + expect(shouldFallThroughForScroll(baseKey)).toBe(false) + }) +}) describe('applyVoiceRecordResponse', () => { it('reverts optimistic REC state when the gateway reports voice busy', () => { diff --git a/ui-tui/src/app/useInputHandlers.ts b/ui-tui/src/app/useInputHandlers.ts index ce25af70edd..59de48a310d 100644 --- a/ui-tui/src/app/useInputHandlers.ts +++ b/ui-tui/src/app/useInputHandlers.ts @@ -23,6 +23,42 @@ import { getUiState } from './uiStore.js' const isCtrl = (key: { ctrl: boolean }, ch: string, target: string) => key.ctrl && ch.toLowerCase() === target +/** + * Approval / clarify / confirm overlays mount their own `useInput` handlers + * for the in-prompt keys (arrows, numbers, Enter, sometimes Esc). The global + * input handler used to early-return for any other key while one of those + * overlays was up, which silently disabled transcript scrolling — the user + * couldn't read context above the prompt that the prompt itself was asking + * about. Returns true when the key is a transcript-scroll input that should + * fall through to the global scroll handlers even while a prompt is active. + * + * Modifier-held wheel (precision mode) is included — a user who wants to + * scroll a single line at a time during a prompt expects it to work. + */ +export function shouldFallThroughForScroll(key: { + downArrow: boolean + pageDown: boolean + pageUp: boolean + shift: boolean + upArrow: boolean + wheelDown: boolean + wheelUp: boolean +}): boolean { + if (key.wheelUp || key.wheelDown) { + return true + } + + if (key.pageUp || key.pageDown) { + return true + } + + if (key.shift && (key.upArrow || key.downArrow)) { + return true + } + + return false +} + export function applyVoiceRecordResponse( response: null | VoiceRecordResponse, starting: boolean, @@ -224,7 +260,18 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { // handlers must receive keystrokes (arrow keys, numbers, Enter). Only // intercept Ctrl+C here so the user can deny/dismiss — all other keys // fall through to the component-level handlers. - if (overlay.approval || overlay.clarify || overlay.confirm) { + // + // Scroll inputs (wheel / PageUp / PageDown / Shift+↑↓) are special: + // they must reach the transcript scroll handlers below even with a + // prompt up. Long-thread context the prompt is asking about often + // lives above the visible viewport, and being unable to read it while + // answering felt like the prompt had locked the entire UI. Explicitly + // skip the prompt-overlay early-return for scroll keys so they fall + // through to the wheel / PageUp / Shift+arrow handlers below. + const promptOverlay = overlay.approval || overlay.clarify || overlay.confirm + const fallThroughForScroll = promptOverlay && shouldFallThroughForScroll(key) + + if (promptOverlay && !fallThroughForScroll) { if (isCtrl(key, ch, 'c')) { cancelOverlayFromCtrlC() } @@ -298,7 +345,13 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { patchOverlayState({ picker: false }) } - return + // When a prompt overlay is up and the user pressed a scroll key, fall + // through to the global scroll handlers below instead of returning. + // Otherwise nothing above this comment matched, and there's nothing + // useful to do for an arbitrary key while blocked. + if (!fallThroughForScroll) { + return + } } if (cState.completions.length && cState.input && cState.historyIdx === null && (key.upArrow || key.downArrow)) { diff --git a/ui-tui/src/components/prompts.tsx b/ui-tui/src/components/prompts.tsx index e9d42485d9b..3dfd31be869 100644 --- a/ui-tui/src/components/prompts.tsx +++ b/ui-tui/src/components/prompts.tsx @@ -11,28 +11,65 @@ const OPTS = ['once', 'session', 'always', 'deny'] as const const LABELS = { always: 'Always allow', deny: 'Deny', once: 'Allow once', session: 'Allow this session' } as const const CMD_PREVIEW_LINES = 10 +type ApprovalKey = { + downArrow?: boolean + escape?: boolean + return?: boolean + upArrow?: boolean +} + +type ApprovalAction = + | { kind: 'choose'; choice: (typeof OPTS)[number] } + | { kind: 'move'; delta: -1 | 1 } + | { kind: 'noop' } + +/** + * Pure key-dispatch for the approval prompt — exported so the regression + * matrix (Esc, Ctrl+C-equivalent, number keys, Enter, ↑↓) is testable + * without mounting React + Ink + a fake stdin. The component just maps the + * action onto its own state setters. + * + * Esc and number keys both terminate the prompt; Esc maps to deny (parity + * with the global Ctrl+C handler that already calls cancelOverlayFromCtrlC + * for approvals). Numbers 1..OPTS.length pick the labelled choice. Enter + * confirms the current selection. ↑/↓ moves the selection within bounds. + */ +export function approvalAction(ch: string, key: ApprovalKey, sel: number): ApprovalAction { + if (key.escape) { + return { kind: 'choose', choice: 'deny' } + } + + const n = parseInt(ch, 10) + + if (n >= 1 && n <= OPTS.length) { + return { kind: 'choose', choice: OPTS[n - 1]! } + } + + if (key.return) { + return { kind: 'choose', choice: OPTS[sel]! } + } + + if (key.upArrow && sel > 0) { + return { kind: 'move', delta: -1 } + } + + if (key.downArrow && sel < OPTS.length - 1) { + return { kind: 'move', delta: 1 } + } + + return { kind: 'noop' } +} + export function ApprovalPrompt({ onChoice, req, t }: ApprovalPromptProps) { const [sel, setSel] = useState(0) useInput((ch, key) => { - if (key.upArrow && sel > 0) { - setSel(s => s - 1) - } + const action = approvalAction(ch, key, sel) - if (key.downArrow && sel < OPTS.length - 1) { - setSel(s => s + 1) - } - - const n = parseInt(ch, 10) - - if (n >= 1 && n <= OPTS.length) { - onChoice(OPTS[n - 1]!) - - return - } - - if (key.return) { - onChoice(OPTS[sel]!) + if (action.kind === 'choose') { + onChoice(action.choice) + } else if (action.kind === 'move') { + setSel(s => s + action.delta) } }) @@ -71,7 +108,7 @@ export function ApprovalPrompt({ onChoice, req, t }: ApprovalPromptProps) { ))} - ↑/↓ select · Enter confirm · 1-4 quick pick · Ctrl+C deny + ↑/↓ select · Enter confirm · 1-4 quick pick · Esc/Ctrl+C deny ) }