C0d3N1nja97342 fd76d4ddde fix(cli): let ESC cancel ongoing work before popping queued messages (#8353) ... * fix(cli): let ESC cancel ongoing work before popping queued messages When the agent is actively responding (streamingState === Responding), InputPrompt's ESC handler consumed the key before AppContainer's global cancel-work handler could fire. Users had to press ESC 3 times (pop queue, clear input, cancel work) to stop the agent. Skip the pop-queue-into-input and double-ESC-clear logic when the agent is responding, returning false so the key propagates to the global handler which cancels the ongoing request. The up-arrow key still pops queued messages into the input at any time. Fixes #8201 * fix: narrow ESC fall-through to empty buffer + add regression tests Address wenshao's review on #8353: - Gate the return false on buffer.text === '' to prevent BaseTextInput's default ESC from silently wiping typed input without double-press confirmation - Add resetEscapeState() before return false to clear any pending escPressCount/escape-prompt timer - Add two regression tests with streamingState: StreamingState.Responding: 1. queue non-empty + ESC -> popAllQueuedMessages NOT called 2. buffer has text + single ESC -> buffer NOT cleared * fix: correct ESC comment to reflect KeypressContext broadcast model Address bot suggestion: the comment claimed returning true 'consumed the key before the global cancel-work handler could fire', but KeypressContext broadcasts to all handlers regardless of return value. The real mechanism is that popQueueIntoInput() fills the shared buffer, steering AppContainer's handler into its 'input has content -> double-press to clear' branch instead of the cancel-work branch. * fix: correct comment to accurately describe return false -> BaseTextInput fall-through Bot suggestion: the comment said returning false 'avoids' BaseTextInput's wipe, but return false actually *enables* it (BaseTextInput only short-circuits on truthy returns). The buffer is safe because the buffer.text === '' gate makes the wipe a no-op, not because return false prevents it. Reworded to make this explicit and warn against relaxing the gate. * test(ui): add positive AppContainer ESC cancel regression test The PR's Responding guards were pinned only by InputPrompt-side tests (queue not popped; non-empty buffer preserved). Add the positive case the review asked for: while Responding with an empty buffer and queued follow-ups, a single Esc reaches the global handler's cancel-work branch (cancelOngoingRequest called once) and the queue is not consumed. #8201 * test(ui): clarify ESC cancel test scope vs end-to-end drain The positive ESC cancel test asserts popAllMessages is not called, but the comment framed it as 'must not consume the queue' end-to-end. In production that exact cancel path DOES drain the queue back into the buffer via the cancel handler (cancelOngoingRequest -> onCancelSubmit -> popAllMessages), under the 'never silently drop queued work' invariant. The assertion only holds because cancelOngoingRequest is replaced by a spy here, severing that hop. Reword the comment to describe the real contract: the global keypress handler itself doesn't pop the queue (InputPrompt owns that and skips it while Responding; #8201), while the end-to-end drain is a separate hop severed by the spy. Addresses the review finding on AppContainer.test.tsx:2405. * test(ui): pin ESC return-false branch and dedupe getGlobalKeypress Address two review suggestions on the ESC cancel tests: - The `return false` branch in InputPrompt.tsx (Responding + empty buffer + empty queue -> defer to AppContainer's cancel-work branch) had no test coverage: reverting it left all 334 tests green. Add an InputPrompt test that pins it (no queue pop, no buffer mutation). - The AppContainer cancel test inlined a byte-identical copy of the getGlobalKeypress() helper that already existed ~2600 lines down in the Ctrl+O describe block. Hoist the helper to the outer describe so both blocks share one definition of the fragile toString() discovery idiom. * test(ui): escape raw ESC byte in cancel test fixture Per review (R4-1): the sequence literal embedded a raw 0x1B control byte that renders as an empty string in diffs and truncates grep output, so the fixture was unreadable. Use the escaped form matching the sibling vim-INSERT fixture on the line above. * test(ui): assert buffer stays empty on ESC cancel + fix test comment Per review (R5-1/R5-2): the flagship #8201 test asserted only the mechanism (popAllQueuedMessages not called), not the effect (buffer stays empty so AppContainer takes its cancel branch). Add the buffer assertion. Also correct the return-false test comment: deleting that branch leaves the test green (KeypressContext.broadcast ignores return values, BaseTextInput's clear is a no-op on empty buffer), so the test pins the no-side-effect contract, not the branch itself. * test(ui): cover queue+text ESC and double-ESC-clear while responding Per review (R6-1/R6-2): the pop-skip guard was pinned only for the empty-buffer case, and the double-press clear contract had no test under Responding. Add: - non-empty queue AND typed text: ESC does not pop the queue and preserves the buffer (pins the guard regardless of buffer content). - double-ESC while Responding: first ESC preserves typed text, second clears it (pins the double-press contract this diff preserves). * test(ui): dedupe escKey fixture and tighten double-ESC timing Per review (R7-1/R7-2): the double-ESC test spaced presses with the default 150ms wait (~30% of the 500ms window); use 50ms to match the sibling double-ESC test. Hoist the escKey fixture to the Cancel Handler describe scope so both tests share one definition (matching the getGlobalKeypress hoist this PR already did). * test(ui): add missing removeGoalTurns to cancel-handler queue mock Per review: the cancel-handler test's useMessageQueue mock omitted removeGoalTurns, a required member that every other queue-mock override in this file includes. The real cancel handler calls removeGoalTurns() before popAllMessages(); the test passed only because cancelOngoingRequest was a spy severing that hop. * test(ui): reuse getGlobalKeypress in vim-INSERT cancel test Per review (R9-1): the vim-INSERT test still inlined a handler-discovery loop matching on 'handleExit', duplicating the hoisted getGlobalKeypress helper (matching TOGGLE_THINKING_EXPANDED). Both tokens occur in the same handleGlobalKeypress closure, so the two idioms can only drift. Reuse the shared helper. * test+docs(ui): escape ESC byte in shared fixture and document Responding Esc Per review (R10-1/R10-2): the shared escKey fixture embedded a raw 0x1B control byte (invisible in diffs, truncates grep). Use the escaped form. Also update keyboard-shortcuts.md: Esc now cancels the ongoing request while the agent is responding instead of moving queued messages back into the input. * docs(ui): correct ESC/Up-Arrow queue-pop description to match code Per review (R11-1): the previous wording said queue pop happens only when idle, but Up Arrow pops in any state (no streamingState guard) and Esc pops whenever not actively responding (including WaitingForConfirmation). Reword to match the code, and note that the responding-cancel only fires when the input is empty. * refactor(ui): drop dead Responding ESC guard per maintainer review wenshao's mutation test showed guard #2 (Responding + empty buffer -> return false) is dead code: with it gone, control falls to the escPressCount===0 branch which returns true on an empty buffer, and KeypressContext.broadcast ignores handler return values anyway - AppContainer's own cancel branch acts on the empty buffer either way. Remove it, fold the subscription-ordering invariants it relied on into guard #1's comment, and document that only Responding is gated (do not broaden to !== Idle or ESC becomes a no-op during a tool confirmation). Also tighten the docs wording: cancelled queued messages are moved back into the input, not preserved. #8201 * docs(ui): correct four review comments in ESC cancel path Round-13 review (no blockers) flagged comment inaccuracies that could misdirect future debugging: - R13-1: the invariant comment pointed at an integration test that does not exist - note the harnesses mock each other's side instead. - R13-2: the regression-test comment still said the branch returns false and that AppContainer acts on return values; it returns true and broadcast ignores return values. - R13-4: the docs row claimed Up Arrow/Esc pop in any state, but during WaitingForConfirmation Composer unmounts InputPrompt (isInputActive admits only Idle/Responding), so neither key pops. - R13-5: the guard comment warned against broadening to !== Idle as if WFC ran this branch; it never does because the component is unmounted. No behavior change. #8201 * docs(ui): correct subscription-order and double-ESC-cancel comments Round-14 review: the invariant comment overstated subscription order as load-bearing - the Responding pop guard skips the pop in either order, and InputPrompt re-subscribes after AppContainer on any remount (e.g. a tool-confirmation round trip), so only the buffer.text-liveness invariant matters. And the double-ESC clear comment now notes it composes with AppContainer's cancel on the same keypress in the initial order but lands on the next press after a remount. No behavior change. #8201 --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>		2026-08-08 05:14:42 +00:00
..
_meta.ts	feat: refactor docs	2025-12-05 10:51:57 +08:00
keyboard-shortcuts.md	fix(cli): let ESC cancel ongoing work before popping queued messages (#8353)	2026-08-08 05:14:42 +00:00