BitSquare/Codeman
2
0
Fork 0
mirror of https://github.com/Ark0N/Codeman.git synced 2026-09-30 12:39:42 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(split-pane): port Ctrl+Shift+C's never-falls-through guarantee to Pane B

Browse Source 
The smart-copy gate only entered its selection-check block behind
hasSelection(), so a selection-less Ctrl+Shift+C skipped straight to
`return true` and ceded the keystroke to the browser's own handling
(e.g. Chrome's Inspect-Element binding) instead of matching Pane A's
"never falls through" contract for that chord.

Verified live in a real browser that this is a UX-parity fix, not an
interrupt-safety one: xterm's evaluateKeyboardEvent never emits PTY
data for a shifted ctrl-letter regardless of any gate (only "_" and
"@" get special-cased), so no accidental 0x03 was ever at risk. The
regression test added here asserts on the dispatched event's
defaultPrevented rather than the absence of a WS frame, since the
frame-count check passes vacuously for this exact key combo whether
or not the gate fires.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
timkjr
2026-09-20 13:10:34 -05:00
co-authored by Claude Sonnet 5
parent 0b3e086334
commit 46d8b92049
3 changed files with 54 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/architecture-invariants.md
+1 -1
View File
File diff suppressed because one or more lines are too long
src/web/public/terminal-split.js
+33 -19
View File
@@ -95,14 +95,11 @@
// action already did to Pane A (COD-153; mirrors the primary pane's own
// gates at terminal-ui.js's attachCustomKeyEventHandler: command palette,
// Alt+1-9/[/] tab nav, Alt+B sidebar toggle, Ctrl+Z suspend, Shift/Ctrl+Enter
// newline, and smart-copy Ctrl+C). Routed through the same registry-aware
// predicates so a rebind or a disable restores plain terminal behavior
// here too. Ctrl+V is deliberately left on xterm's own default
// (plain-text paste): Pane B has no image-paste trap to route it to, so
// intercepting it here would only break paste. Ctrl+Shift+C (the
// explicit, never-falls-through copy chord) is also left un-ported —
// lower value than the plain Ctrl+C case above, since Pane B is rarely
// the pane a user is actively selecting text in.
// newline, and smart-copy Ctrl+C/Ctrl+Shift+C). Routed through the same
// registry-aware predicates so a rebind or a disable restores plain
// terminal behavior here too. Ctrl+V is deliberately left on xterm's own
// default (plain-text paste): Pane B has no image-paste trap to route it
// to, so intercepting it here would only break paste.
this.terminal.attachCustomKeyEventHandler((ev) => {
if (ev.isComposing || ev.key === 'Process' || ev.keyCode === 229) return true;
if (
@@ -158,17 +155,26 @@
}
// Smart copy (mirrors terminal-ui.js's Ctrl+C gate, #211): with a
// selection, Ctrl+C copies THIS pane's own selection instead of
// sending ^C; with none, it must fall through unchanged or the
// interrupt key is lost. Re-implemented against this.terminal rather
// than reusing app.copyTerminalSelection(), which reads app.terminal
// — Pane A's — and would copy the wrong pane's selection.
if (
ev.type === 'keydown' &&
global.app?.shouldCopyTerminalSelectionFromShortcut?.(ev) &&
this.terminal?.hasSelection?.()
) {
const raw = this.terminal.getSelection();
const isColumnSelection = this.terminal._core?._selectionService?._activeSelectionMode === 3;
// sending ^C; with none, plain Ctrl+C must fall through unchanged or
// the interrupt key is lost. Ctrl+Shift+C is different: it is the
// explicit, never-falls-through copy chord, and the predicate above
// does not distinguish it from plain Ctrl+C — ev.shiftKey does, below.
// xterm's own evaluateKeyboardEvent routes a shifted ctrl-letter into
// a branch that assigns c.key only for a couple of special cases
// ("_"->US, "@"->NUL), neither of which is "c", so it emits NOTHING
// for Ctrl+Shift+C either way — this is not about an accidental
// interrupt byte reaching the PTY (verified live: it does not).
// Gating this whole block on hasSelection() (an earlier draft) meant
// that with no selection Ctrl+Shift+C skipped straight to `return
// true`, silently ceding the keystroke to the BROWSER's own handling
// (e.g. Chrome's Inspect-Element binding) with no feedback and no
// attempt to copy, unlike Pane A, which always intercepts it.
// Re-implemented against this.terminal rather than reusing
// app.copyTerminalSelection(), which reads app.terminal — Pane A's —
// and would copy the wrong pane's selection.
if (ev.type === 'keydown' && global.app?.shouldCopyTerminalSelectionFromShortcut?.(ev)) {
const raw = this.terminal?.getSelection?.() || '';
const isColumnSelection = this.terminal?._core?._selectionService?._activeSelectionMode === 3;
const selection = isColumnSelection ? raw : (global.CodemanCopySelection?.clean?.(raw) ?? raw);
if (selection.trim()) {
ev.preventDefault();
@@ -181,9 +187,17 @@
// Nothing worth copying — clear for feedback (a padding-only
// selection cleans to '' and this press still falls through to the
// PTY as 0x03, matching the primary pane's own rule).
if (this.terminal?.hasSelection?.()) {
this.terminal.clearSelection?.();
global.app.showToast?.('Nothing to copy', 'warning');
}
// Ctrl+Shift+C never falls through, even with nothing to copy —
// matches terminal-ui.js's own ev.shiftKey branch.
if (ev.shiftKey) {
ev.preventDefault();
return false;
}
}
return true;
});
test/split-pane-terminal.browser.test.ts
+18 -2
View File
@@ -213,7 +213,9 @@ describe('SplitTerminalPane in a real browser', () => {
// which is not what this test is isolating.
const textarea = (pane.terminal as any)._core?.textarea || (pane.terminal as any).textarea;
const fire = (init: KeyboardEventInit) => {
textarea.dispatchEvent(new KeyboardEvent('keydown', { bubbles: true, cancelable: true, ...init }));
const event = new KeyboardEvent('keydown', { bubbles: true, cancelable: true, ...init });
textarea.dispatchEvent(event);
return event.defaultPrevented;
};
// keyCode is what xterm's evaluateKeyboardEvent switches on to decide
// whether to produce a data frame at all — at keyCode 0 (unset) it can
@@ -262,6 +264,19 @@ describe('SplitTerminalPane in a real browser', () => {
await new Promise((r) => setTimeout(r, 50));
app._copyText = realCopyText;
// Ctrl+Shift+C with NO selection: the blanket "no 'i' frames" check
// below is NOT what proves this gate works — xterm's own
// evaluateKeyboardEvent never emits data for a shifted ctrl-letter in
// the first place (verified live: removing the gate entirely still
// produces zero WS frames for this exact key), so an absent 'i' frame
// is true whether or not the app-level shiftKey branch fires. What the
// branch actually buys is `preventDefault()`, so the browser's own
// handling of the chord (e.g. Chrome's Inspect-Element binding) is
// pre-empted, mirroring Pane A's own "never falls through" contract —
// asserted directly via the dispatched event's defaultPrevented.
pane.terminal.clearSelection();
const ctrlShiftCPrevented = fire({ key: 'c', code: 'KeyC', keyCode: 67, ctrlKey: true, shiftKey: true });
// Alt+B only reaches shouldToggleSessionSidebarFromShortcut's gate when
// the sidebar layout is actually active (app.js:4325) — under the
// default header-strip layout the app doesn't treat Alt+B as its own
@@ -286,12 +301,13 @@ describe('SplitTerminalPane in a real browser', () => {
pane.destroy();
document.body.removeChild(mount);
return { sent, sendKeyCalls, copiedText };
return { sent, sendKeyCalls, copiedText, ctrlShiftCPrevented };
}, sessionId);
expect(result.sent.every((f) => JSON.parse(f).t !== 'i')).toBe(true);
expect(result.sendKeyCalls).toEqual([{ key: 'S-Enter' }]);
expect(result.copiedText).toContain('SPLITPANE_COPY_MARKER');
expect(result.ctrlShiftCPrevented).toBe(true);
await page.evaluate(async (id) => {
await fetch(`/api/sessions/${id}`, { method: 'DELETE' });