Cancel the keydown on Shift+Enter, or xterm submits anyway
Build App (Preview) / compute-version (pull_request) Successful in 4s
Build Container / build-container (pull_request) Successful in 36s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m56s
Build App (Preview) / build-linux (pull_request) Successful in 5m8s
Build App (Preview) / build-windows (pull_request) Successful in 5m38s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
Build App (Preview) / compute-version (pull_request) Successful in 4s
Build Container / build-container (pull_request) Successful in 36s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m56s
Build App (Preview) / build-linux (pull_request) Successful in 5m8s
Build App (Preview) / build-windows (pull_request) Successful in 5m38s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc
This commit is contained in:
@@ -757,7 +757,13 @@
|
|||||||
sessionType === 'claude'
|
sessionType === 'claude'
|
||||||
) {
|
) {
|
||||||
sendTerminalInput('\x1b\r');
|
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;
|
return true;
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -137,19 +137,36 @@ afterEach(() => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
describe("TerminalView — Shift+Enter", () => {
|
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");
|
const { container } = mountSession("claude");
|
||||||
|
|
||||||
fireEvent.keyDown(helperTextarea(container), {
|
const event = new KeyboardEvent("keydown", {
|
||||||
key: "Enter",
|
key: "Enter",
|
||||||
keyCode: 13,
|
keyCode: 13,
|
||||||
shiftKey: true,
|
shiftKey: true,
|
||||||
|
bubbles: true,
|
||||||
|
cancelable: true,
|
||||||
});
|
});
|
||||||
|
helperTextarea(container).dispatchEvent(event);
|
||||||
|
|
||||||
// The bytes `/terminal-setup` installs for every other editor.
|
// The bytes `/terminal-setup` installs for every other editor.
|
||||||
expect(sent()).toEqual(["\x1b\r"]);
|
expect(sent()).toEqual(["\x1b\r"]);
|
||||||
// And specifically not the bare CR that would have submitted the prompt.
|
|
||||||
expect(sent()).not.toContain("\r");
|
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", () => {
|
it("leaves a plain Enter alone", () => {
|
||||||
|
|||||||
@@ -414,7 +414,19 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
sessionTypeRef.current === "claude"
|
sessionTypeRef.current === "claude"
|
||||||
) {
|
) {
|
||||||
sendInput(sessionId, "\x1b\r");
|
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;
|
return true;
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user