From c9a5fdab00671be9980aaddc577ea45db0629fe9 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Fri, 2 Oct 2026 19:44:58 +0800 Subject: [PATCH] fix(terminal): swallow Shift+Enter keypress so it no longer submits xterm runs the custom key handler for keypress too and drops Ctrl/Alt keypresses but not Shift-only ones, so the stray \r submitted the prompt after the newline. Swallow every event type for Shift/Ctrl+Enter and send only on keydown, in the primary pane and Pane B. Adds a static guard and a real xterm + Chromium browser test. Co-Authored-By: Claude Sonnet 5.5 --- .changeset/shift-enter-keypress.md | 5 ++ config/test-suites.ts | 1 + src/web/public/terminal-split.js | 19 +++-- src/web/public/terminal-ui.js | 7 +- test/shift-enter-keypress-swallowed.test.ts | 21 ++++++ test/shift-enter-keypress.browser.test.ts | 81 +++++++++++++++++++++ 6 files changed, 124 insertions(+), 10 deletions(-) create mode 100644 .changeset/shift-enter-keypress.md create mode 100644 test/shift-enter-keypress-swallowed.test.ts create mode 100644 test/shift-enter-keypress.browser.test.ts diff --git a/.changeset/shift-enter-keypress.md b/.changeset/shift-enter-keypress.md new file mode 100644 index 000000000..37fcf6a0f --- /dev/null +++ b/.changeset/shift-enter-keypress.md @@ -0,0 +1,5 @@ +--- +"aicodeman": patch +--- + +Shift+Enter no longer submits the prompt after inserting the newline. The terminal key handler swallowed only `keydown`, so xterm's `keypress` for Shift+Enter (which, unlike Ctrl/Alt, it does not discard) still sent a bare `\r`. diff --git a/config/test-suites.ts b/config/test-suites.ts index 9f3ed295a..ded68c600 100644 --- a/config/test-suites.ts +++ b/config/test-suites.ts @@ -31,6 +31,7 @@ export const BROWSER_TEST_GLOBS = [ 'test/capture-geometry-retry.browser.test.ts', 'test/codex-predictive-echo.test.ts', // also needs a real codex binary 'test/split-pane-terminal.browser.test.ts', + 'test/shift-enter-keypress.browser.test.ts', 'test/split-pane-orchestration.browser.test.ts', 'test/split-pane-auto-collapse.browser.test.ts', ]; diff --git a/src/web/public/terminal-split.js b/src/web/public/terminal-split.js index f8d352c74..206777dad 100644 --- a/src/web/public/terminal-split.js +++ b/src/web/public/terminal-split.js @@ -169,14 +169,17 @@ // session (this.sessionId), never the primary pane's // activeSessionId, and has no local-echo overlay of its own to flush // first (Pane B is deliberately plainer — see the fileoverview). - if (ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey) && ev.type === 'keydown') { - fetch(`/api/sessions/${this.sessionId}/send-key`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ key: ev.ctrlKey ? 'C-Enter' : 'S-Enter' }), - }).catch(() => { - /* Best-effort, matching this pane's tolerance elsewhere. */ - }); + // Swallow keypress/keyup too (xterm would send \r for a Shift-only keypress); only keydown sends. + if (ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey)) { + if (ev.type === 'keydown') { + fetch(`/api/sessions/${this.sessionId}/send-key`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ key: ev.ctrlKey ? 'C-Enter' : 'S-Enter' }), + }).catch(() => { + /* Best-effort, matching this pane's tolerance elsewhere. */ + }); + } return false; } // Smart copy (mirrors terminal-ui.js's Ctrl+C gate, #211): with a diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 657e16246..53b067807 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -447,8 +447,11 @@ Object.assign(CodemanApp.prototype, { // xterm.js sends plain \r for all Enter variants, so Claude Code (Ink) can't // distinguish them. We use tmux send-keys -H to send a line feed byte (0x0a) // which the inner application recognizes as "insert newline" vs carriage return. - if (ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey) && ev.type === 'keydown') { - if (this.activeSessionId) { + // This handler also runs for keypress/keyup: xterm drops a keypress carrying Ctrl/Alt + // but NOT one carrying only Shift, so unless every event type is swallowed here, + // Shift+Enter's keypress sends a bare \r (submit) after the newline. Only keydown sends. + if (ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey)) { + if (ev.type === 'keydown' && this.activeSessionId) { if (this._localEchoEnabled) { const text = this._localEchoOverlay?.pendingText || ''; this._localEchoOverlay?.clear(); diff --git a/test/shift-enter-keypress-swallowed.test.ts b/test/shift-enter-keypress-swallowed.test.ts new file mode 100644 index 000000000..1ca5f311a --- /dev/null +++ b/test/shift-enter-keypress-swallowed.test.ts @@ -0,0 +1,21 @@ +// @vitest-environment node +// Regression guard: xterm runs the custom key handler for keydown AND keypress. +// It discards a keypress carrying Ctrl/Alt but not one carrying only Shift, so +// a handler that returns false for keydown alone lets Shift+Enter's keypress +// through as a bare \r (submit). The Enter gate must therefore not be keyed on +// ev.type === 'keydown'; only the send-key fetch is. + +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const PUBLIC = join(new URL('.', import.meta.url).pathname, '../src/web/public'); + +describe.each(['terminal-ui.js', 'terminal-split.js'])('%s Shift/Ctrl+Enter handler', (file) => { + const src = readFileSync(join(PUBLIC, file), 'utf8'); + + it('swallows every event type for Shift/Ctrl+Enter', () => { + expect(src).toMatch(/ev\.key === 'Enter' && \(ev\.shiftKey \|\| ev\.ctrlKey\)\) \{/); + expect(src).not.toMatch(/ev\.key === 'Enter' && \(ev\.shiftKey \|\| ev\.ctrlKey\) && ev\.type === 'keydown'/); + }); +}); diff --git a/test/shift-enter-keypress.browser.test.ts b/test/shift-enter-keypress.browser.test.ts new file mode 100644 index 000000000..77b5bd35f --- /dev/null +++ b/test/shift-enter-keypress.browser.test.ts @@ -0,0 +1,81 @@ +// @vitest-environment node +// Real xterm.js in real Chromium, real keystrokes. xterm runs the custom key handler for +// keydown AND keypress, and drops a keypress that carries Ctrl/Alt but NOT a Shift-only one, +// so a handler that returns false for keydown alone lets Shift+Enter fall through to a bare +// \r (submit). That is why Ctrl+Enter and Alt+Enter inserted a newline while Shift+Enter +// submitted. Self-contained: no server, only the xterm bundle. + +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { chromium, type Browser, type Page } from 'playwright'; + +const XTERM = readFileSync(join(process.cwd(), 'node_modules/@xterm/xterm/lib/xterm.js'), 'utf8'); + +declare global { + interface Window { + __t: Record; + Terminal: new () => { + open(el: HTMLElement): void; + focus(): void; + onData(cb: (d: string) => void): void; + attachCustomKeyEventHandler(cb: (ev: KeyboardEvent) => boolean): void; + }; + } +} + +describe('Shift/Ctrl+Enter reach the PTY as a bare \\r only if the handler lets keypress through', () => { + let browser: Browser; + let page: Page; + + beforeAll(async () => { + browser = await chromium.launch({ headless: true }); + page = await browser.newPage(); + await page.setContent(''); + await page.addScriptTag({ content: XTERM }); + await page.evaluate(() => { + const mk = (handler: (ev: KeyboardEvent) => boolean) => { + const host = document.createElement('div'); + document.body.appendChild(host); + const term = new window.Terminal(); + term.open(host); + const sent: string[] = []; + term.onData((d) => sent.push(d)); + term.attachCustomKeyEventHandler(handler); + return { term, sent }; + }; + // The shipped predicate (terminal-ui.js / terminal-split.js) and the one it replaced. + const shipped = (ev: KeyboardEvent) => !(ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey)); + const keydownOnly = (ev: KeyboardEvent) => + !(ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey) && ev.type === 'keydown'); + window.__t = { shipped: mk(shipped), keydownOnly: mk(keydownOnly) }; + }); + }); + afterAll(async () => { + await browser?.close(); + }); + + async function typed(which: 'shipped' | 'keydownOnly', key: string): Promise { + await page.evaluate((w) => { + window.__t[w].sent.length = 0; + window.__t[w].term.focus(); + }, which); + await page.keyboard.press(key); + return page.evaluate((w) => [...window.__t[w].sent], which); + } + + it('reproduces the bug with the old keydown-only handler', async () => { + expect(await typed('keydownOnly', 'Shift+Enter')).toEqual(['\r']); + expect(await typed('keydownOnly', 'Control+Enter')).toEqual([]); + }); + + it('sends nothing for Shift+Enter and Ctrl+Enter with the shipped handler (the send-key route supplies the newline)', async () => { + expect(await typed('shipped', 'Shift+Enter')).toEqual([]); + expect(await typed('shipped', 'Control+Enter')).toEqual([]); + }); + + it('leaves plain Enter and Alt+Enter alone', async () => { + expect(await typed('shipped', 'Enter')).toEqual(['\r']); + expect(await typed('shipped', 'Alt+Enter')).toEqual(['\x1b\r']); + }); +});