From 88ffb4744a0cfa864962c76727f4402dcafe581f Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 20:47:01 -0700 Subject: [PATCH] Cancel the keydown on Shift+Enter, or xterm submits anyway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handler returned `false` from xterm's custom key handler and a comment claimed that was enough to stop the bare CR. It is not. `_keyDown` returns the instant the handler says `false` — before it sets `_keyDownHandled` and before it cancels the event — and `_keyPress` then checks that same flag, finds it false, and emits a bare CR for Enter's charCode 13. So the headline feature of this branch did the wrong thing in a real browser: Shift+Enter inserted the newline and then submitted the half-written prompt, now with a stray blank line in it. Arguably worse than before the fix. Reproduced in Chromium and confirmed against the bundled xterm 5.5.0 source. `preventDefault()` is what stops the browser firing keypress at all. Applied at both call sites — the desktop terminal and the web terminal's copy. The test could not have caught this. jsdom never synthesizes the follow-up keypress, so `expect(sent()).not.toContain("\r")` was asserting a property the environment cannot falsify — a test named for a behaviour it could not exercise. It now asserts `defaultPrevented`, which is the mechanism that actually suppresses the keypress and which jsdom can observe. Mutation-checked: removing the `preventDefault()` fails it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- app/src-tauri/src/web_terminal/terminal.html | 8 ++++++- .../components/terminal/TerminalView.test.tsx | 23 ++++++++++++++++--- app/src/components/terminal/TerminalView.tsx | 14 ++++++++++- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/app/src-tauri/src/web_terminal/terminal.html b/app/src-tauri/src/web_terminal/terminal.html index 8e6259a..ba266ba 100644 --- a/app/src-tauri/src/web_terminal/terminal.html +++ b/app/src-tauri/src/web_terminal/terminal.html @@ -757,7 +757,13 @@ sessionType === 'claude' ) { sendTerminalInput('\x1b\r'); - return false; // xterm must not also send a bare CR, which submits + // `preventDefault()` is what stops the submit, not the `return false`. + // xterm's `_keyDown` returns before setting `_keyDownHandled`, so + // `_keyPress` still fires and emits a bare CR for Enter — inserting the + // newline and then submitting the prompt anyway. See the same comment + // in TerminalView.tsx. + e.preventDefault(); + return false; } return true; }); diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 900e388..42284d0 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -137,19 +137,36 @@ afterEach(() => { }); describe("TerminalView — Shift+Enter", () => { - it("sends ESC+CR and nothing else in a Claude session", () => { + it("sends ESC+CR and cancels the keydown, so no bare CR follows", () => { + // **The cancel is the load-bearing half, and this test could not see it.** + // + // Returning `false` from xterm's custom key handler does not cancel the + // event: `_keyDown` returns before setting `_keyDownHandled`, so + // `_keyPress` still runs and emits a bare CR for Enter's charCode 13. In a + // real browser that submitted the prompt straight after inserting the + // newline. jsdom never synthesizes the follow-up keypress, so the old + // `expect(sent()).not.toContain("\r")` assertion below could not fail no + // matter what the code did — it was named for a behaviour it could not + // exercise. + // + // Asserting `defaultPrevented` pins the actual mechanism that stops the + // keypress, which is a property jsdom *can* observe. const { container } = mountSession("claude"); - fireEvent.keyDown(helperTextarea(container), { + const event = new KeyboardEvent("keydown", { key: "Enter", keyCode: 13, shiftKey: true, + bubbles: true, + cancelable: true, }); + helperTextarea(container).dispatchEvent(event); // The bytes `/terminal-setup` installs for every other editor. expect(sent()).toEqual(["\x1b\r"]); - // And specifically not the bare CR that would have submitted the prompt. expect(sent()).not.toContain("\r"); + // Without this, the browser fires keypress and xterm submits. + expect(event.defaultPrevented).toBe(true); }); it("leaves a plain Enter alone", () => { diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index d9d1011..72d6ae0 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -414,7 +414,19 @@ export default function TerminalView({ sessionId, active }: Props) { sessionTypeRef.current === "claude" ) { sendInput(sessionId, "\x1b\r"); - return false; // xterm must not also send a bare CR, which submits + // **`preventDefault()` is what stops the submit, not the `return false`.** + // + // xterm's `_keyDown` returns the instant a custom handler says `false` + // — *before* it sets `_keyDownHandled` and before it cancels the event. + // `_keyPress` then checks that same flag, finds it still false, and + // emits a bare CR for Enter's charCode 13. So returning `false` alone + // sent ESC+CR *and* a submit: the newline was inserted and the + // half-written prompt went to Claude with a stray blank line in it. + // Cancelling the keydown is what stops the browser firing keypress at + // all. Verified in Chromium; jsdom never synthesizes the follow-up + // keypress, which is why the unit test could not see this. + event.preventDefault(); + return false; } return true; });