mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-07 16:09:43 +02:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5.5
parent
9240493c43
commit
c9a5fdab00
@@ -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`.
|
||||||
@@ -31,6 +31,7 @@ export const BROWSER_TEST_GLOBS = [
|
|||||||
'test/capture-geometry-retry.browser.test.ts',
|
'test/capture-geometry-retry.browser.test.ts',
|
||||||
'test/codex-predictive-echo.test.ts', // also needs a real codex binary
|
'test/codex-predictive-echo.test.ts', // also needs a real codex binary
|
||||||
'test/split-pane-terminal.browser.test.ts',
|
'test/split-pane-terminal.browser.test.ts',
|
||||||
|
'test/shift-enter-keypress.browser.test.ts',
|
||||||
'test/split-pane-orchestration.browser.test.ts',
|
'test/split-pane-orchestration.browser.test.ts',
|
||||||
'test/split-pane-auto-collapse.browser.test.ts',
|
'test/split-pane-auto-collapse.browser.test.ts',
|
||||||
];
|
];
|
||||||
|
|||||||
@@ -169,14 +169,17 @@
|
|||||||
// session (this.sessionId), never the primary pane's
|
// session (this.sessionId), never the primary pane's
|
||||||
// activeSessionId, and has no local-echo overlay of its own to flush
|
// activeSessionId, and has no local-echo overlay of its own to flush
|
||||||
// first (Pane B is deliberately plainer — see the fileoverview).
|
// first (Pane B is deliberately plainer — see the fileoverview).
|
||||||
if (ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey) && ev.type === 'keydown') {
|
// Swallow keypress/keyup too (xterm would send \r for a Shift-only keypress); only keydown sends.
|
||||||
fetch(`/api/sessions/${this.sessionId}/send-key`, {
|
if (ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey)) {
|
||||||
method: 'POST',
|
if (ev.type === 'keydown') {
|
||||||
headers: { 'Content-Type': 'application/json' },
|
fetch(`/api/sessions/${this.sessionId}/send-key`, {
|
||||||
body: JSON.stringify({ key: ev.ctrlKey ? 'C-Enter' : 'S-Enter' }),
|
method: 'POST',
|
||||||
}).catch(() => {
|
headers: { 'Content-Type': 'application/json' },
|
||||||
/* Best-effort, matching this pane's tolerance elsewhere. */
|
body: JSON.stringify({ key: ev.ctrlKey ? 'C-Enter' : 'S-Enter' }),
|
||||||
});
|
}).catch(() => {
|
||||||
|
/* Best-effort, matching this pane's tolerance elsewhere. */
|
||||||
|
});
|
||||||
|
}
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
// Smart copy (mirrors terminal-ui.js's Ctrl+C gate, #211): with a
|
// Smart copy (mirrors terminal-ui.js's Ctrl+C gate, #211): with a
|
||||||
|
|||||||
@@ -447,8 +447,11 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
// xterm.js sends plain \r for all Enter variants, so Claude Code (Ink) can't
|
// 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)
|
// 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.
|
// which the inner application recognizes as "insert newline" vs carriage return.
|
||||||
if (ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey) && ev.type === 'keydown') {
|
// This handler also runs for keypress/keyup: xterm drops a keypress carrying Ctrl/Alt
|
||||||
if (this.activeSessionId) {
|
// 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) {
|
if (this._localEchoEnabled) {
|
||||||
const text = this._localEchoOverlay?.pendingText || '';
|
const text = this._localEchoOverlay?.pendingText || '';
|
||||||
this._localEchoOverlay?.clear();
|
this._localEchoOverlay?.clear();
|
||||||
|
|||||||
@@ -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'/);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -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<string, { term: { focus(): void }; sent: string[] }>;
|
||||||
|
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('<body></body>');
|
||||||
|
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<string[]> {
|
||||||
|
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']);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user