mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-20 17:19:33 +02:00
fix(terminal): drive the shipped Shift+Enter handlers and the Key tester cap in their browser tests (#520, #522 review)
- Minor: the Key tester "14 lines" test never pressed a key into the tester (the previous test blurred it, so the presses landed on <body> and the cap was never exercised). It now refocuses the field, asserts the focus, clears the log, checks 2 presses accumulate to 6 lines, then 4 presses of another key cap the log at exactly 14 with the oldest 4 lines evicted in order, and the readonly field stays empty. Verified to fail with the cap changed to 20. - Nit: the split-pane invariant implied Ctrl+Enter could use the CLI's declared newline chord. Reworded after checking the send-key route: Ctrl+Enter is always a real 0x0a, Shift+Enter is the declared capabilities.newline chord (0x0a unless the CLI declares another), sent on keydown only. The same imprecision in the auto-named sessions paragraph is corrected too. - Nit: docs/wiki/Settings-Reference.md now lists the Key tester row in the Terminal & Input table. - Nit: test/shift-enter-keypress.browser.test.ts exercised a hand-copied predicate named `shipped`. It now loads the real app from a real WebServer and presses real keys into the handlers terminal-ui.js (app.terminal, recording the real _sendInputAsync send path) and terminal-split.js (a real SplitTerminalPane) attach, recording the send-key POSTs through a fetch wrapper. It asserts no \r reaches either send path for Shift/Ctrl+Enter, exactly one send-key per press for the right session, and that Enter and Alt+Enter are untouched. The old keydown-only gate stays as a labelled reproduction of xterm's keypress behaviour on a bare Terminal. Verified to fail on both panes with the gate narrowed back to keydown. - Nit: the keypress trap is now written down beside the other key-gate rules (Command palette and shortcut registry): xterm runs the custom handler for keydown, keypress and keyup and drops only Ctrl/Alt/Meta keypresses, so a gate on a chord that can carry Shift alone must swallow every event type. The smart-copy keydown-only rule points at it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer committed
1 parent
36af183f97
commit
52267f8617
4 files changed
+223
-63
No files matched your search
@@ -191,7 +191,7 @@ Further detail (current geometry and colors, superseding the dip numbers above w
|
||||
|
||||
**Three owners, one setter.** `SessionState.nameSource` is `placeholder` | `auto` | `manual`. The constructor infers a missing value from the name (`isGeneratedSessionName()` = `w<n>-<case>` / `s<n>-<case>`, or no name at all, is a placeholder; anything else was a person's), the create routes pass none, the boot restore passes the persisted one. The `name` setter is the manual path and the ONLY thing that produces `manual` after construction; `applyAutoName()` is the only thing that produces `auto`, and it does so whether or not the string changed, which is what makes "first prompt" mean the first. A prompt whose title is null (`/clear`, `! npm test`, blank) never reaches it, so the session stays eligible: the first REAL prompt names the tab.
|
||||
|
||||
**The origin gate defaults to "system".** `write()` / `writeViaMux()` take `SessionWriteOptions.fromUser`; only the browser WS path and `POST /api/sessions/:id/input` set it. Every other caller (Ralph, respawn, cron, approvals, the orchestrator's `/compact`, auto-ops, the trust-dialog keys) is system by omission, so a new user-input path that forgets the flag fails toward a tab that keeps its placeholder, never toward a tab named after a Ralph prompt. `_lastSubmitAt` is still stamped for every write; only the tracker feed is gated. The shell gate is `getCli(mode)?.capabilities.startMode !== 'shell'`, a capability rather than an id check (the no-id-branching guard), and the send-key route feeds `trackUserInput()` by hand because its `tmux send-keys -H` newline (a line feed unless the CLI declares another `capabilities.newline` chord) never passes through the session.
|
||||
**The origin gate defaults to "system".** `write()` / `writeViaMux()` take `SessionWriteOptions.fromUser`; only the browser WS path and `POST /api/sessions/:id/input` set it. Every other caller (Ralph, respawn, cron, approvals, the orchestrator's `/compact`, auto-ops, the trust-dialog keys) is system by omission, so a new user-input path that forgets the flag fails toward a tab that keeps its placeholder, never toward a tab named after a Ralph prompt. `_lastSubmitAt` is still stamped for every write; only the tracker feed is gated. The shell gate is `getCli(mode)?.capabilities.startMode !== 'shell'`, a capability rather than an id check (the no-id-branching guard), and the send-key route feeds `trackUserInput()` by hand because its `tmux send-keys -H` newline (always a line feed for Ctrl+Enter; for Shift+Enter a line feed unless the CLI declares another `capabilities.newline` chord) never passes through the session.
|
||||
|
||||
**The tracker is a best-effort transcript with explicit per-key rules**, not a byte filter. Mirrored: printable text, backspace, Ctrl+W, Ctrl+U/Ctrl+C (composer emptied), `\n` and Alt+Enter (a newline IN the composer, joined with a space), bracketed paste (newlines inside it likewise). Ignored: cursor keys, Home/End/Delete, Shift+Tab, Tab, SGR mouse and focus reports, Alt chords, OSC/DCS, the rest of C0. Tainting: Up/Down (CSI and SS3), Ctrl+P/N/R, Ctrl+_, because the composer then holds a history line the tracker never saw and Enter must submit nothing rather than a fragment. A bare Esc is resolved at the END of the chunk it arrives in, since xterm hands each key's whole sequence to one write and the programmatic senders send Esc alone; a CSI split across chunks still resumes. The draft keeps its HEAD past 8192 code points (the title is the first sentence, so keeping the tail would title a long paste by its last line) and an escape sequence is abandoned past 64 bytes.
|
||||
|
||||
@@ -664,11 +664,13 @@ Matching semantics: a query containing `/` matches the relative path, otherwise
|
||||
|
||||
**Command palette + shortcut registry** (COD-151/153/157/192, #146): `Ctrl/Cmd/Alt+K` opens the session palette (fuzzy search over live sessions; "Browse all sessions" → the Session Manager modal backed by `GET /api/sessions/unified`); the quick-start case `<select>` is fronted by a searchable picker (`buildCasePickerOptions`/`formatCasePickerLabel` — remote cases render `name @ hostId`). Shortcuts live in a rebindable registry (`DEFAULT_SHORTCUTS`/`getShortcutRegistry()`/`matchesShortcutEvent()` in app.js; overrides persist under `settings.shortcutOverrides` via `saveAppSettingsToStorage`); App Settings → Shortcuts renders capture/disable rows; `Ctrl+?` opens the registry-driven overlay (footer links to the full `#helpModal` reference). ⚠️ Palette-chord keys must ALSO be swallowed in `attachCustomKeyEventHandler` (terminal-ui.js) or xterm writes the control byte (0x0B) into the PTY. ⚠️ The global Escape handler (app.js) calls every overlay close method on every Escape, in the capture phase and so before the focused element's own Escape handler, which means a close method with side effects beyond hiding must return early when its overlay is not open: `closeCommandPalette()` and `closeSessionManager()` hand focus back on close (`_restoreOverlayFocus()`, panels-ui.js), and run unconditionally that restore stole focus from split Pane B and from any text field, and turned the inline tab rename's Escape into a commit. ⚠️ `saveAppSettings()` rebuilds settings from the DOM — keys edited elsewhere (`shortcutOverrides`, `showTokenCount`, `showCost`) need explicit `_prev` carry-over.
|
||||
|
||||
**A key gate on a chord that can carry Shift alone must swallow the KEYPRESS too** (#520). xterm calls the custom handler from `attachCustomKeyEventHandler` for `keydown`, `keypress` AND `keyup`, and its own `_keyPress` drops a keypress carrying Ctrl, Alt or Meta but not one whose only modifier is Shift. So a gate that returns `false` for `ev.type === 'keydown'` alone is safe for a Ctrl or Alt chord (the palette's Ctrl+K, Alt+B, Ctrl+V, Ctrl+Z and the keydown-only smart copy below) and leaks for a Shift-only one. That is how Shift+Enter submitted the prompt from April to #520: its keypress fell through to `onData` as a bare `\r`, which goes out synchronously over the WebSocket ahead of the async `send-key` fetch, so the CR reached the PTY before the newline did. Ctrl+Enter worked all along only because Chrome fires no keypress for it. ⚠️ The Shift/Ctrl+Enter gate in terminal-ui.js and its twin in Pane B (terminal-split.js) return `false` for EVERY event type and key only the `send-key` POST on `keydown`; never narrow either back to keydown, and give any new Shift-only chord the same shape. Swallowing the keyup as well is harmless (xterm only skips a focus and cursor-style refresh for that one event). Tests: `test/shift-enter-keypress-swallowed.test.ts` (CI gate, a source guard on both files) and `test/shift-enter-keypress.browser.test.ts` (browser suite, which the gate does not run: real keys against both real handlers in the real app). Settings → Terminal & Input → **Key tester** shows what a device reports for each of the three events.
|
||||
|
||||
**Terminal smart copy** (#211): `Ctrl+C` copies the selection when there is one and stays the interrupt when there isn't. Three rules keep that split honest, and breaking any of them silently costs the user their interrupt key:
|
||||
|
||||
1. The branch lives in `attachCustomKeyEventHandler` (terminal-ui.js) and the **no-selection path returns `true` with no `preventDefault()`**, so xterm still evaluates `Ctrl+C` into `0x03`. Returning `false` alone does not cancel the event either way: xterm's `_keyDown` calls the custom handler *before* its own `cancel()`, which is exactly why the copy path calls `preventDefault()` explicitly (otherwise the browser also runs its native copy on top).
|
||||
2. `copyTerminalSelection` is a registry `action` **deliberately missing from `SHORTCUT_ACTIONS`** (same trick as `command-palette`): the entry stays rebindable and disableable in App Settings, while the generic document-capture loop, which `preventDefault()`s every match it dispatches, skips it and lets the terminal handler decide.
|
||||
3. The gate is keydown-only (the custom handler also runs for `keypress`/`keyup`), and `Ctrl+Shift+C` never falls through to the PTY: an "explicit copy" chord that interrupts a running agent because the selection happened to be empty is a footgun with no upside.
|
||||
3. The gate is keydown-only (the custom handler also runs for `keypress`/`keyup`; that is safe here only because xterm drops a Ctrl keypress itself, see the keypress rule above), and `Ctrl+Shift+C` never falls through to the PTY: an "explicit copy" chord that interrupts a running agent because the selection happened to be empty is a footgun with no upside.
|
||||
|
||||
Copy goes through `_copyText()` (Clipboard API, then hidden-textarea + `execCommand`), not raw `navigator.clipboard`, because `install.sh`'s LAN option serves plain HTTP where `navigator.clipboard` is undefined; the fallback steals focus, so the terminal is refocused afterwards. Related: xterm registers its own `copy` listener on the terminal element gated on `hasSelection()`, which is why right-click → Copy has always worked. Selection itself is unavailable on touch devices by design (`user-select: none` on the terminal subtree), and in `shell`/`opencode`/`antigravity` tabs the TUI owns the mouse, so selecting there needs Shift+drag. Tests: `test/terminal-copy-selection.test.ts` (gate + wiring invariants), `test/terminal-copy-shortcut.test.ts` (browser, real key presses).
|
||||
|
||||
@@ -789,7 +791,7 @@ Further detail: with many sessions the horizontal strip stops being scannable, w
|
||||
|
||||
### Split-pane sessions
|
||||
|
||||
**Split-pane sessions** (`showSplitButton`, header button, default OFF, per-device like `showFileViewerButton` — not in `SettingsUpdateSchema`, `displayKeys` in settings-ui.js): shows two live sessions side-by-side in one Codeman window. Pane A is the untouched, existing singleton terminal (`this.terminal`/`this._ws` in terminal-ui.js); Pane B is a new, independent `SplitTerminalPane` (terminal-split.js) with its own xterm instance and its own `/ws/sessions/:id/terminal` WebSocket. ⚠️ **Pane B is deliberately plainer than Pane A** — no local-echo overlay, no CJK IME, no touch/mobile handlers, no keyboard accessory bar — since this is a desktop-only feature (a split view needs a wide viewport) and those features exist for mobile/touch input; `.btn-split` is hard-hidden below 1180px regardless of the setting by the `@media (max-width: 1179px)` rule in styles.css (mobile.css only carries a comment pointing at it: that file loads up to 1023px, so it cannot cover the 1024-1179px tablet range the feature also needs to stay off), and the per-device setting means turning it on at a desk can never sync it onto a phone in the first place. No persistence: closing the browser tab or reloading always returns to the normal single-pane view; there is no localStorage key for split state. ⚠️ Splitting a session against itself is disallowed (the picker excludes the active session), **as is splitting against a popped-out (detached) session** — `buildSplitPickerSessions()` excludes `detachedSessions` because a detached session's own window is already claiming its PTY size, and `MAX_WS_PER_SESSION` needs no change since Pane A/B are always two different sessions. ⚠️ Either pane's session ending (deleted locally or from another client) auto-collapses the split — Pane A's session ending promotes Pane B to the new single pane via `selectSession(id, { auto: true })` (an app-driven selection, so it must not spend the session's idle alert — see the Approvals Inbox note above), never by trying to hot-swap the lightweight `SplitTerminalPane` object into the primary singleton state. ⚠️ Pane B refits on every window/sidebar/tab-rail resize via the SAME trailing-edge `ResizeObserver` callback that resizes Pane A (`throttledResize` in terminal-ui.js) — it only ever measured Pane A's own container, so without an explicit `this._splitPane?.fit()` call there Pane B silently kept its stale PTY size through every resize that did not happen to be a divider drag. ⚠️ A dropped WebSocket leaves Pane B visibly dead (a message written into its own xterm buffer) rather than silently swallowing keystrokes with nothing on screen to explain why — there is no reconnect logic for v1, matching the "deliberately plainer than Pane A" design. Related but distinct: `detachSession()` already opens one session in a separate OS-level browser window (`isSoloWindow`) — that is prior art for "two sessions visible at once" but not for one window with a draggable in-page divider, which is what this feature adds. ⚠️ Pane B installs its own `attachCustomKeyEventHandler` gating the same app-level chords Pane A's own handler gates (command palette, Alt+1-9/[/] tab nav, Alt+B sidebar toggle, Ctrl+Z suspend, Shift/Ctrl+Enter newline, smart-copy Ctrl+C) — without it the document capture-phase handler's `preventDefault()` (which never stops xterm) let each chord ALSO write its raw byte/escape sequence into Pane B's live PTY on top of whatever the app action did to Pane A (COD-153). Ctrl+Z is swallowed unless Pane B's own session is `mode === 'shell'`, mirroring terminal-ui.js's reasoning: in a plain shell it is the user's own job-control tool, everywhere else it silently suspends an unattended agent loop. Shift/Ctrl+Enter POSTs to `/api/sessions/:id/send-key` (`{key:'S-Enter'|'C-Enter'}`, tmux `send-keys -H` for a real 0x0a, or the CLI's declared `capabilities.newline` chord) targeting THIS pane's own `sessionId` rather than the primary pane's `activeSessionId` — without it xterm's plain `\r` would submit an incomplete prompt instead of adding a line to it. Smart-copy Ctrl+C/Ctrl+Shift+C is re-implemented against `this.terminal` (Pane B's own) rather than reusing `app.copyTerminalSelection()`, which reads Pane A's terminal and would copy the wrong pane's selection; Ctrl+Shift+C never falls through even with nothing to copy, mirroring terminal-ui.js's own `ev.shiftKey` branch. ⚠️ **This is a UX-parity fix, not an interrupt-safety one** — verified live in a real browser: xterm's `evaluateKeyboardEvent` routes a shifted ctrl-letter into a branch that assigns `c.key` only for two special cases (`_`→US, `@`→NUL), so it emits no data for Ctrl+Shift+C at all regardless of any application gate; a synthetic keydown with the gate removed produces zero WS frames, proving no accidental interrupt reaches the PTY either way. What gating the whole copy block on `hasSelection()` (an earlier draft) actually cost: with no selection, a selLine truncated
|
||||
**Split-pane sessions** (`showSplitButton`, header button, default OFF, per-device like `showFileViewerButton` — not in `SettingsUpdateSchema`, `displayKeys` in settings-ui.js): shows two live sessions side-by-side in one Codeman window. Pane A is the untouched, existing singleton terminal (`this.terminal`/`this._ws` in terminal-ui.js); Pane B is a new, independent `SplitTerminalPane` (terminal-split.js) with its own xterm instance and its own `/ws/sessions/:id/terminal` WebSocket. ⚠️ **Pane B is deliberately plainer than Pane A** — no local-echo overlay, no CJK IME, no touch/mobile handlers, no keyboard accessory bar — since this is a desktop-only feature (a split view needs a wide viewport) and those features exist for mobile/touch input; `.btn-split` is hard-hidden below 1180px regardless of the setting by the `@media (max-width: 1179px)` rule in styles.css (mobile.css only carries a comment pointing at it: that file loads up to 1023px, so it cannot cover the 1024-1179px tablet range the feature also needs to stay off), and the per-device setting means turning it on at a desk can never sync it onto a phone in the first place. No persistence: closing the browser tab or reloading always returns to the normal single-pane view; there is no localStorage key for split state. ⚠️ Splitting a session against itself is disallowed (the picker excludes the active session), **as is splitting against a popped-out (detached) session** — `buildSplitPickerSessions()` excludes `detachedSessions` because a detached session's own window is already claiming its PTY size, and `MAX_WS_PER_SESSION` needs no change since Pane A/B are always two different sessions. ⚠️ Either pane's session ending (deleted locally or from another client) auto-collapses the split — Pane A's session ending promotes Pane B to the new single pane via `selectSession(id, { auto: true })` (an app-driven selection, so it must not spend the session's idle alert — see the Approvals Inbox note above), never by trying to hot-swap the lightweight `SplitTerminalPane` object into the primary singleton state. ⚠️ Pane B refits on every window/sidebar/tab-rail resize via the SAME trailing-edge `ResizeObserver` callback that resizes Pane A (`throttledResize` in terminal-ui.js) — it only ever measured Pane A's own container, so without an explicit `this._splitPane?.fit()` call there Pane B silently kept its stale PTY size through every resize that did not happen to be a divider drag. ⚠️ A dropped WebSocket leaves Pane B visibly dead (a message written into its own xterm buffer) rather than silently swallowing keystrokes with nothing on screen to explain why — there is no reconnect logic for v1, matching the "deliberately plainer than Pane A" design. Related but distinct: `detachSession()` already opens one session in a separate OS-level browser window (`isSoloWindow`) — that is prior art for "two sessions visible at once" but not for one window with a draggable in-page divider, which is what this feature adds. ⚠️ Pane B installs its own `attachCustomKeyEventHandler` gating the same app-level chords Pane A's own handler gates (command palette, Alt+1-9/[/] tab nav, Alt+B sidebar toggle, Ctrl+Z suspend, Shift/Ctrl+Enter newline, smart-copy Ctrl+C) — without it the document capture-phase handler's `preventDefault()` (which never stops xterm) let each chord ALSO write its raw byte/escape sequence into Pane B's live PTY on top of whatever the app action did to Pane A (COD-153). Ctrl+Z is swallowed unless Pane B's own session is `mode === 'shell'`, mirroring terminal-ui.js's reasoning: in a plain shell it is the user's own job-control tool, everywhere else it silently suspends an unattended agent loop. Shift/Ctrl+Enter POSTs to `/api/sessions/:id/send-key` (`{key:'S-Enter'|'C-Enter'}`, typed by tmux `send-keys -H`: Ctrl+Enter is always a real 0x0a, Shift+Enter is the CLI's declared `capabilities.newline` chord, also 0x0a unless the CLI declares another; sent on keydown only, with the keypress and keyup swallowed too, see the keypress rule under Command palette and shortcut registry) targeting THIS pane's own `sessionId` rather than the primary pane's `activeSessionId` — without it xterm's plain `\r` would submit an incomplete prompt instead of adding a line to it. Smart-copy Ctrl+C/Ctrl+Shift+C is re-implemented against `this.terminal` (Pane B's own) rather than reusing `app.copyTerminalSelection()`, which reads Pane A's terminal and would copy the wrong pane's selection; Ctrl+Shift+C never falls through even with nothing to copy, mirroring terminal-ui.js's own `ev.shiftKey` branch. ⚠️ **This is a UX-parity fix, not an interrupt-safety one** — verified live in a real browser: xterm's `evaluateKeyboardEvent` routes a shifted ctrl-letter into a branch that assigns `c.key` only for two special cases (`_`→US, `@`→NUL), so it emits no data for Ctrl+Shift+C at all regardless of any application gate; a synthetic keydown withLine truncated
|
||||
|
||||
### Gesture control: the setting
|
||||
|
||||
|
||||
@@ -50,6 +50,7 @@ supervised by systemd or launchd; npm installs report as non-updatable. See
|
||||
| Normal / Bold font weight | xterm defaults | Per device, each slot from 100 to 900. The bundled JetBrains Mono renders every step, so a lighter normal weight makes Claude's bold headings stand out. Applies live to the terminal, both echo overlays and open team panes. |
|
||||
| WebGL Renderer | On | With a GPU-stall watchdog that falls back to DOM rendering. |
|
||||
| Gesture Control | Off | Camera hand tracking. Also needs `CODEMAN_GESTURE=1` on the server. |
|
||||
| Key tester | n/a | A diagnostic that stores nothing. Click the box and press keys to see what this browser reports (key, code, modifiers) for keydown, keypress and keyup, for when a chord such as Shift+Enter behaves differently on one device. Keys pressed there reach no session and trigger no shortcut. |
|
||||
|
||||
### Header & Panels
|
||||
|
||||
|
||||
@@ -89,8 +89,29 @@ describe('Key tester in a real browser', () => {
|
||||
});
|
||||
|
||||
it('keeps only the last 14 lines and never types into the field', async () => {
|
||||
for (let i = 0; i < 8; i++) await page.keyboard.press('a');
|
||||
expect((await log()).split('\n').length).toBeLessThanOrEqual(14);
|
||||
// The test above blurred the field, so focus it again: without this the presses land on
|
||||
// <body>, the log keeps whatever the earlier tests left, and the cap is never exercised.
|
||||
await page.focus('#keyTesterInput');
|
||||
expect(await page.evaluate(() => document.activeElement?.id)).toBe('keyTesterInput');
|
||||
await page.evaluate(() => (document.getElementById('keyTesterLog')!.textContent = ''));
|
||||
const lines = async () => (await log()).split('\n');
|
||||
|
||||
// A printable key fires keydown, keypress and keyup, so 2 presses are 6 lines: under the cap
|
||||
// the log accumulates rather than showing only the latest event.
|
||||
for (let i = 0; i < 2; i++) await page.keyboard.press('a');
|
||||
expect(await lines()).toHaveLength(6);
|
||||
|
||||
// A different key, so eviction is visible: 4 x 3 = 12 more lines makes 18, capped to 14. The
|
||||
// oldest 4 go (all of the first 'a' press and the second one's keydown), the newest stay in order.
|
||||
for (let i = 0; i < 4; i++) await page.keyboard.press('b');
|
||||
const capped = await lines();
|
||||
expect(capped).toHaveLength(14);
|
||||
expect(capped.filter((l) => l.includes('key="a"'))).toHaveLength(2);
|
||||
expect(capped.filter((l) => l.includes('key="b"'))).toHaveLength(12);
|
||||
expect(capped[0]).toMatch(/^keypress\s+key="a" code=KeyA mods=none charCode=97$/);
|
||||
expect(capped[13]).toMatch(/^keyup\s+key="b" code=KeyB mods=none$/);
|
||||
|
||||
// Readonly: none of those presses typed anything into the field itself.
|
||||
expect(await page.inputValue('#keyTesterInput')).toBe('');
|
||||
});
|
||||
});
|
||||
@@ -1,81 +1,217 @@
|
||||
// @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.
|
||||
/**
|
||||
* @fileoverview Shift/Ctrl+Enter against the SHIPPED key handlers, with real keystrokes in Chromium.
|
||||
*
|
||||
* xterm runs the custom key handler for keydown, keypress AND keyup, and drops a keypress that
|
||||
* carries Ctrl/Alt but NOT a Shift-only one. A handler that returns false for keydown alone
|
||||
* therefore lets Shift+Enter's keypress fall through to onData as a bare \r, which went out over
|
||||
* the WebSocket ahead of the async send-key fetch and submitted the prompt (#520). Ctrl+Enter
|
||||
* worked all along because Chromium fires no keypress for it.
|
||||
*
|
||||
* The page is the real app served by a real WebServer, so the handlers under test are the ones
|
||||
* terminal-ui.js (the main pane, `app.terminal`) and terminal-split.js (Pane B, a real
|
||||
* `SplitTerminalPane`) attach. Nothing restates their predicate. What stands in for the server is
|
||||
* only the edge: a fetch wrapper records the send-key POSTs instead of letting them reach tmux, and
|
||||
* no session exists behind the ids, so nothing is ever typed into a real pane.
|
||||
*
|
||||
* Browser-driven, so it is excluded from `npm run test:ci` like the other Playwright suites:
|
||||
* npm run test:browser -- test/shift-enter-keypress.browser.test.ts
|
||||
* The CI gate's guard on the same handlers is the source check in
|
||||
* test/shift-enter-keypress-swallowed.test.ts.
|
||||
*
|
||||
* Port: 3267 (per CLAUDE.md, ports 3150+ for tests)
|
||||
*/
|
||||
|
||||
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';
|
||||
import { WebServer } from '../src/web/server.js';
|
||||
|
||||
const XTERM = readFileSync(join(process.cwd(), 'node_modules/@xterm/xterm/lib/xterm.js'), 'utf8');
|
||||
const PORT = 3267;
|
||||
const BASE_URL = `http://localhost:${PORT}`;
|
||||
const MAIN_ID = 'shift-enter-probe-main';
|
||||
const PANE_B_ID = 'shift-enter-probe-pane-b';
|
||||
|
||||
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;
|
||||
};
|
||||
}
|
||||
interface Pressed {
|
||||
/** What xterm emitted through onData, i.e. what would have been written to the PTY. */
|
||||
data: string[];
|
||||
/** The send-key POSTs the handler made, as `<session id> <key>`. */
|
||||
sendKeys: string[];
|
||||
/** Whether xterm's own textarea held focus, so a key that emits nothing was really delivered. */
|
||||
focused: boolean;
|
||||
}
|
||||
|
||||
describe('Shift/Ctrl+Enter reach the PTY as a bare \\r only if the handler lets keypress through', () => {
|
||||
describe('Shift/Ctrl+Enter through the shipped key handlers (keypress must be swallowed too)', () => {
|
||||
let server: WebServer;
|
||||
let browser: Browser;
|
||||
let page: Page;
|
||||
|
||||
beforeAll(async () => {
|
||||
server = new WebServer(PORT, false, true);
|
||||
await server.start();
|
||||
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();
|
||||
});
|
||||
await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' });
|
||||
await page.waitForFunction(() => (window as any).app?.terminal, null, { timeout: 30000 });
|
||||
await page.evaluate(
|
||||
({ paneBId }) => {
|
||||
const w = window as any;
|
||||
// Record send-key instead of reaching the server (no tmux pane exists behind these ids).
|
||||
w.__sendKeys = [] as string[];
|
||||
const realFetch = window.fetch.bind(window);
|
||||
window.fetch = (input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url;
|
||||
const m = /\/api\/sessions\/([^/]+)\/send-key$/.exec(url);
|
||||
if (m) {
|
||||
w.__sendKeys.push(`${m[1]} ${JSON.parse(String(init?.body ?? '{}')).key}`);
|
||||
return Promise.resolve(
|
||||
new Response('{}', { status: 200, headers: { 'Content-Type': 'application/json' } })
|
||||
);
|
||||
}
|
||||
return realFetch(input, init);
|
||||
};
|
||||
|
||||
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);
|
||||
// Pane B, built by the real class. connect() installs the key handler synchronously,
|
||||
// before its first await; the buffer fetch and WebSocket for the missing session just fail.
|
||||
const mount = document.createElement('div');
|
||||
mount.style.cssText = 'position:fixed;left:0;top:0;width:400px;height:300px;';
|
||||
document.body.appendChild(mount);
|
||||
const pane = new w.SplitTerminalPane(paneBId, mount, { mode: 'claude' });
|
||||
void pane.connect().catch(() => {});
|
||||
// What xterm emits here is exactly what Pane B's own onData forwards to its WebSocket.
|
||||
w.__paneBData = [] as string[];
|
||||
pane.terminal.onData((d: string) => w.__paneBData.push(d));
|
||||
w.__paneB = pane;
|
||||
|
||||
// The bare xterm the old keydown-only gate is reproduced on (see the first test).
|
||||
const host = document.createElement('div');
|
||||
host.style.cssText = 'position:fixed;left:420px;top:0;width:400px;height:300px;';
|
||||
document.body.appendChild(host);
|
||||
const old = new w.Terminal();
|
||||
old.open(host);
|
||||
w.__oldData = [] as string[];
|
||||
old.onData((d: string) => w.__oldData.push(d));
|
||||
old.attachCustomKeyEventHandler(
|
||||
(ev: KeyboardEvent) => !(ev.key === 'Enter' && (ev.shiftKey || ev.ctrlKey) && ev.type === 'keydown')
|
||||
);
|
||||
w.__oldTerm = old;
|
||||
},
|
||||
{ paneBId: PANE_B_ID }
|
||||
);
|
||||
}, 90000);
|
||||
|
||||
afterAll(async () => {
|
||||
if (browser) await browser.close();
|
||||
if (server) await server.stop();
|
||||
}, 60000);
|
||||
|
||||
/**
|
||||
* Press `key` in the main pane. `sessionId` null is the welcome screen; otherwise the app thinks
|
||||
* that session is active, local echo is off (the desktop default) and its real send path,
|
||||
* `_sendInputAsync`, is recorded instead of posting.
|
||||
*/
|
||||
async function pressMain(key: string, sessionId: string | null): Promise<Pressed> {
|
||||
await page.evaluate((id) => {
|
||||
const w = window as any;
|
||||
const app = w.app;
|
||||
w.__mainSaved ??= { id: app.activeSessionId, echo: app._localEchoEnabled, send: app._sendInputAsync };
|
||||
w.__mainData = [] as string[];
|
||||
w.__sendKeys.length = 0;
|
||||
app.activeSessionId = id;
|
||||
app._localEchoEnabled = false;
|
||||
app._pendingInput = '';
|
||||
app._sendInputAsync = (_sid: string, chunk: string) => void w.__mainData.push(chunk);
|
||||
app.terminal.focus();
|
||||
}, sessionId);
|
||||
const focused = await page.evaluate(() => document.activeElement === (window as any).app.terminal.textarea);
|
||||
await page.keyboard.press(key);
|
||||
return page.evaluate((w) => [...window.__t[w].sent], which);
|
||||
// The main pane batches onData through a short flush timer before _sendInputAsync.
|
||||
await page.waitForTimeout(150);
|
||||
return page.evaluate((f) => {
|
||||
const w = window as any;
|
||||
const app = w.app;
|
||||
const out = { data: [...w.__mainData], sendKeys: [...w.__sendKeys], focused: f };
|
||||
app.activeSessionId = w.__mainSaved.id;
|
||||
app._localEchoEnabled = w.__mainSaved.echo;
|
||||
app._sendInputAsync = w.__mainSaved.send;
|
||||
app._pendingInput = '';
|
||||
return out;
|
||||
}, focused);
|
||||
}
|
||||
|
||||
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([]);
|
||||
async function pressIn(which: 'paneB' | 'old', key: string): Promise<Pressed> {
|
||||
await page.evaluate((k) => {
|
||||
const w = window as any;
|
||||
w[k === 'paneB' ? '__paneBData' : '__oldData'].length = 0;
|
||||
w.__sendKeys.length = 0;
|
||||
(k === 'paneB' ? w.__paneB.terminal : w.__oldTerm).focus();
|
||||
}, which);
|
||||
const focused = await page.evaluate((k) => {
|
||||
const w = window as any;
|
||||
return document.activeElement === (k === 'paneB' ? w.__paneB.terminal : w.__oldTerm).textarea;
|
||||
}, which);
|
||||
await page.keyboard.press(key);
|
||||
await page.waitForTimeout(50);
|
||||
return page.evaluate(
|
||||
({ k, f }) => {
|
||||
const w = window as any;
|
||||
return { data: [...w[k === 'paneB' ? '__paneBData' : '__oldData']], sendKeys: [...w.__sendKeys], focused: f };
|
||||
},
|
||||
{ k: which, f: focused }
|
||||
);
|
||||
}
|
||||
|
||||
it('reproduces the leak on a bare xterm with the old keydown-only gate (the behaviour the fix depends on)', async () => {
|
||||
// Not shipped code: this pins the xterm behaviour, so a future xterm that stops emitting the
|
||||
// Shift-only keypress shows up here instead of silently turning the tests below vacuous.
|
||||
const shift = await pressIn('old', 'Shift+Enter');
|
||||
expect(shift.focused).toBe(true);
|
||||
expect(shift.data).toEqual(['\r']);
|
||||
expect((await pressIn('old', 'Control+Enter')).data).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('main pane (terminal-ui.js): Shift+Enter and Ctrl+Enter write nothing and POST send-key exactly once', async () => {
|
||||
const shift = await pressMain('Shift+Enter', MAIN_ID);
|
||||
expect(shift.focused).toBe(true);
|
||||
expect(shift.data).toEqual([]);
|
||||
// Once, from the keydown: the swallowed keypress and keyup must not send again.
|
||||
expect(shift.sendKeys).toEqual([`${MAIN_ID} S-Enter`]);
|
||||
|
||||
const ctrl = await pressMain('Control+Enter', MAIN_ID);
|
||||
expect(ctrl.data).toEqual([]);
|
||||
expect(ctrl.sendKeys).toEqual([`${MAIN_ID} C-Enter`]);
|
||||
});
|
||||
|
||||
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']);
|
||||
it('main pane: plain Enter still reaches the send path as \\r, and Alt+Enter as ESC CR', async () => {
|
||||
const enter = await pressMain('Enter', MAIN_ID);
|
||||
expect(enter.focused).toBe(true);
|
||||
expect(enter.data).toEqual(['\r']);
|
||||
expect(enter.sendKeys).toEqual([]);
|
||||
|
||||
const alt = await pressMain('Alt+Enter', MAIN_ID);
|
||||
expect(alt.data).toEqual(['\x1b\r']);
|
||||
expect(alt.sendKeys).toEqual([]);
|
||||
});
|
||||
|
||||
it('main pane on the welcome screen: Shift+Enter sends nothing and posts no send-key', async () => {
|
||||
// With no active session the app's onData drops input anyway, so this pins the send-key
|
||||
// guard (`this.activeSessionId`), not the keypress swallow; the tests above pin that.
|
||||
const shift = await pressMain('Shift+Enter', null);
|
||||
expect(shift.focused).toBe(true);
|
||||
expect(shift.data).toEqual([]);
|
||||
expect(shift.sendKeys).toEqual([]);
|
||||
});
|
||||
|
||||
it("Pane B (terminal-split.js): Shift+Enter and Ctrl+Enter write nothing and POST send-key once for Pane B's own session", async () => {
|
||||
const shift = await pressIn('paneB', 'Shift+Enter');
|
||||
expect(shift.focused).toBe(true);
|
||||
expect(shift.data).toEqual([]);
|
||||
expect(shift.sendKeys).toEqual([`${PANE_B_ID} S-Enter`]);
|
||||
|
||||
const ctrl = await pressIn('paneB', 'Control+Enter');
|
||||
expect(ctrl.data).toEqual([]);
|
||||
expect(ctrl.sendKeys).toEqual([`${PANE_B_ID} C-Enter`]);
|
||||
|
||||
const enter = await pressIn('paneB', 'Enter');
|
||||
expect(enter.data).toEqual(['\r']);
|
||||
expect(enter.sendKeys).toEqual([]);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user