From 4c705094f72b2963a9a4a965a3c868c55c4eb039 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sat, 19 Sep 2026 11:54:52 +0200 Subject: [PATCH] fix(terminal): ship the copy clean as a trailing trim, without the shared dedent #451 cleaned two things on copy. The trailing trim is right and every native terminal does it. The shared leading-indent strip is this project's own rule, and it is dropped here rather than shipped. Measured against the shipped transform over 401,445 three-row windows across 1,010 tracked files in this repo, it fired on 73% of them: 92% inside a YAML workflow, 76% over `git log` output, 48% in a TypeScript source. No width threshold separates a margin from content because they are the same widths, a live Claude Code pane's own margins measuring 2 and 5 columns while the most common non-TUI shared run is 4. The failure modes are not symmetric either: a wrong trailing trim costs nothing, while a wrong dedent silently deletes information that was on the screen, with nothing in the clipboard to hint at it, on git log bodies, on indented code read out of cat (semantic in Python), on git diff context rows where the leading space is the marker, and on stack traces. It also could not be made self-consistent cheaply. Whether the first row joined the measurement depended on the mousedown COLUMN, which the user never sees, so one block of three rows produced three different clipboard results; and the flag read getSelectionPosition().start, which is xterm's mousedown anchor and is never normalised, so dragging UP through a block read it off the bottom row. The PR's test stub hardcoded a downward drag, so its suite could not express that case. The transform, the wiring, the tests, the invariants, CLAUDE.md, the wiki page and the changeset all move together. The test block now pins the ABSENCE as a contract, with the git log, Python and git diff cases as its examples, so this is not re-derived later. If it is ever revisited, the one qualification that measured clean is painted trailing padding: zero false positives over all 401,445 windows. Also from the review: the comments and invariant rule justifying the padding-only clear described the pre-change code (the Ctrl+C gate reads the CLEANED selection now, so such a selection falls through to the PTY on its own and the clear is feedback rather than protection), the new 'Nothing to copy' toast gained its zh-CN entry, and the invariants paragraph no longer repeats its own opening sentence. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/trim-copied-terminal-selection.md | 39 ++---- CLAUDE.md | 2 +- docs/architecture-invariants.md | 8 +- docs/wiki/Keyboard-Shortcuts.md | 2 +- src/web/public/constants.js | 83 +++++-------- src/web/public/i18n.js | 1 + src/web/public/terminal-ui.js | 41 ++++--- test/terminal-copy-clean.test.ts | 118 ++++++++++--------- 8 files changed, 134 insertions(+), 160 deletions(-) diff --git a/.changeset/trim-copied-terminal-selection.md b/.changeset/trim-copied-terminal-selection.md index e660142c..48620697 100644 --- a/.changeset/trim-copied-terminal-selection.md +++ b/.changeset/trim-copied-terminal-selection.md @@ -2,30 +2,17 @@ "aicodeman": patch --- -fix(terminal): trim the padding and shared indent out of a copied selection +fix(terminal): trim the padding out of a copied selection -Copying several rows out of a pane put a wall of spaces on the clipboard and -repeated the program's own margin on every line. xterm hands back whole screen -rows and trims only the cells that were never written to, so the real spaces a -full-screen TUI paints across the unused part of a row count as content: -measured against Claude Code in a 282-column pane, single lines arrived -carrying 138 trailing spaces on top of a two-space transcript indent. Pasting -that into a chat client or an editor meant deleting the whitespace by hand, -while Windows Terminal, iTerm2 and GNOME Terminal all trim it for you. - -A copy now drops the trailing run from every line. It also removes the leading -run, but only the one that every selected row shares and only when the -selection covers more than one row, so shell output is untouched, a single line -keeps the indentation you can see, and anything nested inside a copied block -keeps its relative indentation. A drag that starts inside a row keeps that -partial first line exactly as it was. An `Alt+drag` rectangular selection is -copied verbatim, because its columns lining up is the point of that gesture. - -The main terminal's four copy paths go through it: the Ctrl+C chord, -right-click, the phone selection button and Auto Copy. The browser's own -Edit menu copy, a copy shortcut you disabled in settings, and the subagent -windows still copy the raw rows, as they did before. A selection holding nothing but padding is now -refused rather than copied as bare line breaks, and it clears the selection on -the way out so Ctrl+C goes straight back to interrupting. Auto Copy reads its -own toggle before it reads the selection, so a pane nobody is copying from -costs nothing on mouseup. +Copying out of a pane put a wall of spaces on the clipboard. xterm hands back +whole screen rows and trims only the cells that were never written to, so the +real spaces a full-screen program paints across the unused part of a row count +as content: measured against Claude Code in a 282-column pane, single lines +arrived carrying 138 trailing spaces. Pasting that into a chat client or an +editor meant deleting the whitespace by hand, while Windows Terminal, iTerm2 and +GNOME Terminal all trim it for you. A copy now drops the trailing run from every +line, on all four paths (the Ctrl+C chord, right-click, the phone selection +button and Auto Copy), while leading indentation is left exactly as it is. An +Alt+drag rectangular selection is copied verbatim, because its columns lining up +is the point of that gesture. A selection holding nothing but padding is refused +rather than copied as bare line breaks. diff --git a/CLAUDE.md b/CLAUDE.md index 7b525ecd..739c8e08 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -318,7 +318,7 @@ Frontend JS modules have `@fileoverview` with `@dependency`/`@loadorder` tags. L **Welcome "Resume Conversation" list** (terminal-ui.js): `loadHistorySessions()` fetches once and caches the corpus on `_historyAll`/`_historyCases`; every subsequent view (filter box, sort select, expand, the periodic refresh in panels-ui.js) goes through `_renderHistoryList()`, so never append rows to `#historyList` directly or re-fetch to re-sort. ⚠️ The box height is **class-driven**: expanding the list without `.history-list.expanded` leaves the collapsed `max-height` in place and just deepens a scroll well, which is the bug #260 reported (35 sessions in a ~4-row box). ⚠️ The A–Z sort keys off `_historyRowLabel()`, the SAME string the row renders (`name || firstPrompt || path`), most rows are transcript-backed and have no session name, so sorting on `name` alone silently does nothing. ⚠️ A filter implies expansion, and `_renderSearch()` hides `#historyHeader` (title + controls) as one unit while a search is active. Tests: `test/history-list-controls.test.ts`. -**Command palette + shortcut registry**: `Ctrl/Cmd/Alt+K` opens the session palette; shortcuts live in a rebindable registry (`DEFAULT_SHORTCUTS`/`getShortcutRegistry()`/`matchesShortcutEvent()` in app.js, overrides in `settings.shortcutOverrides`). ⚠️ Palette-chord keys must ALSO be swallowed in `attachCustomKeyEventHandler` (terminal-ui.js) or xterm writes the control byte (0x0B) into the PTY. ⚠️ `saveAppSettings()` rebuilds settings from the DOM, so keys edited elsewhere (`shortcutOverrides`, `showTokenCount`, `showCost`) need explicit `_prev` carry-over. ⚠️ **Smart copy (`Ctrl+C`)** lives in that same handler: with a selection it copies, with none it must `return true` **without** `preventDefault()` or the interrupt is lost. `copyTerminalSelection` is deliberately absent from `SHORTCUT_ACTIONS` because the generic capture loop preventDefaults every match it dispatches. → [architecture-invariants#command-palette-and-shortcut-registry](docs/architecture-invariants.md#command-palette-and-shortcut-registry) +**Command palette + shortcut registry**: `Ctrl/Cmd/Alt+K` opens the session palette; shortcuts live in a rebindable registry (`DEFAULT_SHORTCUTS`/`getShortcutRegistry()`/`matchesShortcutEvent()` in app.js, overrides in `settings.shortcutOverrides`). ⚠️ Palette-chord keys must ALSO be swallowed in `attachCustomKeyEventHandler` (terminal-ui.js) or xterm writes the control byte (0x0B) into the PTY. ⚠️ `saveAppSettings()` rebuilds settings from the DOM, so keys edited elsewhere (`shortcutOverrides`, `showTokenCount`, `showCost`) need explicit `_prev` carry-over. ⚠️ **Smart copy (`Ctrl+C`)** lives in that same handler: with a selection it copies, with none it must `return true` **without** `preventDefault()` or the interrupt is lost. `copyTerminalSelection` is deliberately absent from `SHORTCUT_ACTIONS` because the generic capture loop preventDefaults every match it dispatches. ⚠️ **The gate tests the CLEANED selection, not the raw one** (`CodemanCopySelection.clean` in constants.js, pure; `cleanedTerminalSelection()` reads the live terminal): xterm returns whole screen ROWS and trims only never-written cells, so the spaces a full-screen TUI paints across the rest of a row are content and reach the clipboard (138 of them per line, measured in a 282-column pane). The clean drops each line's TRAILING run and nothing else. ⚠️ **A shared LEADING indent is deliberately NOT stripped**, and that is a decision, not a gap: it was built, measured and dropped before #451 merged, because it fired on 73% of real non-TUI text (401,445 windows sampled) and no width threshold separates a TUI margin from content (a claude pane's own margins are 2 and 5 columns; the commonest non-TUI shared run is 4). Do not re-add it without reading the rule in architecture-invariants. ⚠️ Alt+drag COLUMN selections are returned untouched (`_activeSelectionMode === 3`), and the padding-only clear is FEEDBACK rather than interrupt protection, since a padding-only selection now cleans to `''` and falls through to the PTY on its own. → [architecture-invariants#command-palette-and-shortcut-registry](docs/architecture-invariants.md#command-palette-and-shortcut-registry) **Per-device vs synced settings**: the `displayKeys` set in settings-ui.js is a **client-side merge policy**, not a wire filter. A display key seeds from the server only when localStorage has no value for it, which is what prevents one device overwriting another; `showPlanUsageLimits` is additionally `delete`d from the incoming payload outright. Separately, `SettingsUpdateSchema` is `.strict()` and simply **does not declare** `skin`, `showFileViewerButton`, `showCronButton`, `webglRendererEnabled`, `localEchoEnabled`, `cjkInputEnabled`, or `extendedKeyboardBar`, so sending one of those is a validation error. The rest (`showResponseViewer`, `showPlanUsageLimits`, `language`, and most `show*` keys) ARE in the schema and do persist server-side; they are per-device by client policy only. ⚠️ Adding a new per-device setting means deciding **both** questions: membership in `displayKeys`, and presence in the schema. diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index a50d34b8..e8484471 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -306,13 +306,13 @@ Invariants: 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). -**The main terminal's four copy paths clean the selection first** (`CodemanCopySelection.clean` in constants.js, pure; `cleanedTerminalSelection()` in terminal-ui.js is the half that reads the live terminal). Those four are the `Ctrl+C` chord, right-click, the phone selection button and Auto Copy. ⚠️ Three routes still copy the RAW padded rows, all of them predating the clean: the browser's own Edit → Copy, which xterm's own `copy` listener on the terminal element serves with `selectionText` directly; a `copy-selection` shortcut the user disabled in App Settings, where nothing calls `preventDefault()` and that native listener runs; and the subagent/teammate windows, which build their own `Terminal` in panels-ui.js with no copy wiring at all. xterm hands back whole screen ROWS and its own trim drops only cells that were never written to, so the real spaces a full-screen TUI paints across the unused part of a row count as content and reach the clipboard. Measured against Claude Code in a 282-column pane, single lines arrived carrying 138 trailing spaces on top of the two-space transcript indent. The clean drops each line's trailing run, and removes the leading run only when every selected row shares one, which makes it a no-op for shell output. Three rules keep it honest: +**The main terminal's four copy paths clean the selection first** (`CodemanCopySelection.clean` in constants.js, pure; `cleanedTerminalSelection()` in terminal-ui.js is the half that reads the live terminal). Those four are the `Ctrl+C` chord, right-click, the phone selection button and Auto Copy. ⚠️ Three routes still copy the RAW padded rows, all of them predating the clean: the browser's own Edit → Copy, which xterm's own `copy` listener on the terminal element serves with `selectionText` directly; a `copy-selection` shortcut the user disabled in App Settings, where nothing calls `preventDefault()` and that native listener runs; and the subagent/teammate windows, which build their own `Terminal` in panels-ui.js with no copy wiring at all. xterm hands back whole screen ROWS and its own trim drops only cells that were never written to, so the real spaces a full-screen TUI paints across the unused part of a row count as content and reach the clipboard. Measured against Claude Code in a 282-column pane, single lines arrived carrying 138 trailing spaces on top of the two-space transcript indent. The clean drops each line's trailing run. Two rules keep it honest: -1. **A one-row selection keeps its leading run.** One row shares nothing with anything, so that run is content — stripping it would silently reindent a single line of `git log` body text or one line out of `less`. ⚠️ A drag that STARTED mid-row lowers that bar to one measured row, because the partial first line is counted toward the two-row total and then skipped by the measurement. That is deliberate: both rows of a wrapped paragraph wear the TUI's margin and the drag only hid the first one's, so the second row's margin still goes. The cost is that a two-row mid-row drag over genuinely indented content loses that indent. +1. **Trailing padding only. A shared LEADING indent is deliberately NOT stripped**, and that is a decision rather than an omission: it was built, measured and dropped before #451 merged. It looks like the mirror image of the trailing trim and is not, because no native terminal does it and the transform cannot tell a TUI's margin from content that is genuinely indented. Measured over 401,445 three-row windows across 1,010 tracked files in this repo it fired on **73%** of them (92% inside a YAML workflow, 76% over `git log` output, 48% in a TypeScript source), and no width threshold separates the two because they are the same widths: a live Claude Code pane's own margins measure 2 and 5 columns while the most common non-TUI shared run is 4, sitting between them. The failure modes are what settle it. A wrong trailing trim costs nothing; a wrong dedent silently deletes information that was on screen, with no signal and nothing in the clipboard to hint at it, and it is wrong on `git log` bodies, on indented code read out of `cat` (semantic in Python), on `git diff` context rows where the leading space is the marker, and on stack traces. ⚠️ It also could not be made self-consistent cheaply: whether the first row joined the measurement depended on the mousedown COLUMN, which the user never sees, so one block of three rows produced three different clipboard results, and the flag read `getSelectionPosition().start`, which is the mousedown anchor xterm never normalises, so dragging UP through a block read it off the bottom row (the PR's test stub hardcoded a downward drag, so its suite could not express the case). If it is ever revisited, the one qualification that measured clean is **painted trailing padding** (a full-screen TUI writes real spaces across every row, while a shell pane leaves those cells never-written for xterm to trim): zero false positives over all 401,445 windows, no new plumbing. It still mangles a `git log` body sitting inside an agent's own gutter, which is why it was not taken. 2. **A COLUMN selection is returned untouched.** Alt+drag makes one (xterm's `shouldColumnSelect` keys on `altKey` alone, and neither `Terminal` Codeman builds passes the one option, `macOptionClickForcesSelection`, that would disable it), and a rectangle's rows lining up is the whole point of the gesture. xterm exposes the mode nowhere public, so the check reads `terminal._core._selectionService._activeSelectionMode` (`SelectionMode.COLUMN` is 3) and cleans normally if a future xterm renames it. -3. **The emptiness gate is `trim()`, not truthiness, and it still clears the selection.** A multi-row drag across padding cleans to line breaks alone, which are truthy, and a bare newline pasted into a chat composer submits it. ⚠️ Returning without `clearSelection()` there would leave the raw selection set, and since the `Ctrl+C` gate above tests the RAW selection, every later `Ctrl+C` would copy nothing instead of interrupting — the exact way rule 1 of smart copy gets lost. +3. **The emptiness gate is `trim()`, not truthiness, and it still clears the selection.** A multi-row drag across padding cleans to line breaks alone, which are truthy, and a bare newline pasted into a chat composer submits it. ⚠️ The clear is FEEDBACK, not protection for the interrupt, and the comments that said otherwise were describing the pre-clean code: the `Ctrl+C` gate above now tests the CLEANED selection, so a padding-only selection left set cleans to `''` on every later press and falls through to the PTY as `0x03` anyway. What the clear buys is that a highlight which copied nothing does not linger unexplained, which is also what the toast is for. -All four paths go through it: the `Ctrl+C` chord, right-click, the phone selection button and Auto Copy. Tests: `test/terminal-copy-clean.test.ts`. +Tests: `test/terminal-copy-clean.test.ts`. ### Auto Copy (copy-on-select) diff --git a/docs/wiki/Keyboard-Shortcuts.md b/docs/wiki/Keyboard-Shortcuts.md index 92ff82df..098f7e05 100644 --- a/docs/wiki/Keyboard-Shortcuts.md +++ b/docs/wiki/Keyboard-Shortcuts.md @@ -34,7 +34,7 @@ Press `Ctrl+?` in the app for the same list in a floating overlay. | Right-click | Copy the selection. With nothing selected the native menu is left alone. | | `Ctrl+Z` | Swallowed in agent sessions so a running CLI cannot be suspended. Normal job control in a shell. | -Anything you copy is cleaned on the way to the clipboard. Each line loses the padding spaces a full-screen program paints across the rest of the row, and a selection covering several rows also loses the indent every one of those rows shares, which is usually the program's own margin rather than your text. A one-row selection keeps its indent, and an `Alt+drag` rectangular selection is copied exactly as it looks, so its columns stay lined up. +Anything you copy is cleaned on the way to the clipboard: each line loses the padding spaces a full-screen program paints across the rest of the row. Leading indentation is left exactly as it is, so indented code, a `git log` message body and `git diff` context lines paste back the way they looked on screen. An `Alt+drag` rectangular selection is copied exactly as it looks, so its columns stay lined up. ## Everything else diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 57682eba..57d97a25 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -819,29 +819,40 @@ function decideAutoCopy({ enabled, text, lastCopied, pending } = {}) { // _selectTouchSelectionLine already treats those cells as padding. This is that // same rule for the mouse and keyboard paths, which never had it. // -// The leading run is the other half, and it applies ACROSS ROWS ONLY. A TUI -// that indents its whole transcript repeats the indent on every row, so a -// multi-row selection arrives with the chrome baked into each line; only the -// run every selected row shares is removed, which is a no-op for shell output -// and keeps the relative indentation of anything nested inside. A selection of -// ONE row shares nothing with anything, so its leading spaces are content and -// stay put — otherwise a single line of `git log` body text, or one line out of -// `less`, would silently lose its indentation. +// ⚠ Trailing padding ONLY. A shared LEADING indent is deliberately left alone, +// and this note is here so the idea is not re-derived: it was built, measured +// and dropped before merge. Removing the longest leading run every selected row +// shares looks like the mirror image of the trailing trim and is not, because +// no native terminal does it and the transform cannot tell a TUI's margin from +// content that is genuinely indented. Measured over 401 445 three-row windows +// across 1 010 tracked files in this repo, it fired on 73% of them: 92% inside +// a YAML workflow, 76% over `git log` output, 48% in a TypeScript source file. +// No width threshold separates the two, because they are the same widths: a +// live Claude Code pane's own margins measure 2 and 5 columns while the most +// common non-TUI shared run is 4, sitting between them. // -// ⚠ The trailing trim takes spaces AND tabs while the leading run counts spaces -// only, so one tab-led row disables the strip for its whole block. Terminals -// expand tabs into cells, so a tab should never reach either rule; the -// asymmetry is deliberate caution rather than an oversight. +// The asymmetry that settles it is in the failure modes. A wrong trailing trim +// costs nothing. A wrong dedent silently deletes information that was on the +// screen, with no signal to the user and nothing in the clipboard to hint at +// it, and it is wrong on `git log` bodies, on indented code read out of `cat` +// (semantic in Python), on `git diff` context rows where the leading space is +// the marker, and on stack traces. // -// ⚠ A WRAPPED logical line keeps its continuation indent. xterm appends a -// wrapped row to the previous entry instead of starting a new line, so rows -// 2..n of one wrapped line sit mid-string where no line rule can see them. That -// is inherent to cleaning xterm's output rather than a gap to fix here. -function cleanCopiedSelection(text, { startedMidRow = false } = {}) { +// ⚠ It also cannot be made consistent cheaply. Whether the first row joins the +// measurement depended on the mousedown COLUMN, which the user never sees, so +// one block of three rows produced three different clipboard results; and the +// flag read `getSelectionPosition().start`, which is the mousedown anchor that +// xterm never normalises, so dragging UP through a block read it off the bottom +// row. If it is ever revisited, the one qualification that measured clean is +// painted trailing padding (a full-screen TUI writes real spaces across every +// row; a shell pane leaves those cells never-written, so xterm trims them): +// zero false positives over all 401 445 windows. It still mangles a `git log` +// body sitting inside an agent's own gutter, which is why it was not taken now. +function cleanCopiedSelection(text) { if (typeof text !== 'string' || !text) return ''; // Split on \n and leave any \r in place: xterm joins rows with \r\n on // Windows, and the clipboard should keep the endings xterm chose. - // Scanned rather than matched, throughout. A selection can run to the 50 000-row + // Scanned rather than matched. A selection can run to the 50 000-row // scrollback ceiling, and `/[ \t]+(\r?)$/` is QUADRATIC on a line whose spaces // are followed by any non-space character, which is what right-aligned or // centred TUI content looks like: the engine retries the run from every @@ -857,41 +868,7 @@ function cleanCopiedSelection(text, { startedMidRow = false } = {}) { while (cut > 0 && (line[cut - 1] === ' ' || line[cut - 1] === '\t')) cut--; return cut === end ? line : line.slice(0, cut) + line.slice(end); }; - const lines = text.split('\n').map(trimEnd); - const bareLen = (line) => (line.endsWith('\r') ? line.length - 1 : line.length); - const leadingRun = (line) => { - let n = 0; - while (n < line.length && line[n] === ' ') n++; - return n; - }; - - // ⚠ Counted over EVERY line, including a partial first line the loop below - // skips. A mid-row drag across two rows therefore measures one row and strips - // it. That is deliberate: both rows of a wrapped paragraph wear the TUI's - // margin, and the drag only hid the first one's. The cost is that a two-row - // mid-row drag over genuinely indented content loses that indent. - let contentRows = 0; - for (const line of lines) if (bareLen(line)) contentRows++; - if (contentRows < 2) return lines.join('\n'); - - // startedMidRow keeps the first line out of the measurement. A drag that - // begins inside a row gives a first line with no leading run at all, which - // would otherwise pin the shared run to zero and leave every row after it - // still wearing the indent. That partial line is never stripped either. - let shared = Infinity; - for (let i = startedMidRow ? 1 : 0; i < lines.length; i++) { - // A row left blank by the trailing trim says nothing about the indent, and - // counting it as zero would disable the strip for the whole block. - if (!bareLen(lines[i])) continue; - shared = Math.min(shared, leadingRun(lines[i])); - if (shared === 0) break; - } - if (!shared || shared === Infinity) return lines.join('\n'); - // The clamp is what keeps a blank row's lone \r intact when the shared run is - // wider than that row is long. - return lines - .map((line, i) => (startedMidRow && i === 0 ? line : line.slice(Math.min(shared, leadingRun(line))))) - .join('\n'); + return text.split('\n').map(trimEnd).join('\n'); } if (typeof window !== 'undefined') { diff --git a/src/web/public/i18n.js b/src/web/public/i18n.js index bee8fcab..a5289255 100644 --- a/src/web/public/i18n.js +++ b/src/web/public/i18n.js @@ -521,6 +521,7 @@ 'Respawn Blocked': '重生已阻止', 'Task Complete': '任务完成', 'Copied to clipboard': '已复制到剪贴板', + 'Nothing to copy': '没有可复制的内容', // Terminal touch-selection bar (long-press to select). The bar is a sibling of // `.xterm`, not a descendant, so SKIP_SELECTOR does not cover it and these apply. Copy: '复制', diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 722abad1..e32b5832 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -374,9 +374,13 @@ Object.assign(CodemanApp.prototype, { void this.copyTerminalSelection(selection); return false; } - // Nothing worth copying. Drop a padding-only selection first, or it would - // intercept every following press too, then fall through exactly as an - // empty selection does so this press still reaches the PTY as 0x03. + // Nothing worth copying. The clear is for feedback, not for the + // interrupt: the gate above tests the CLEANED selection, so a + // padding-only selection left set cleans to '' on every later press and + // falls through to the PTY anyway. What it buys is that a highlight + // which copies nothing does not linger with no explanation, which is + // also what the toast is for. Falls through exactly as an empty + // selection does, so this press still reaches the PTY as 0x03. if (this.terminal?.hasSelection?.()) { this.terminal.clearSelection?.(); this.showToast('Nothing to copy', 'warning'); @@ -4159,16 +4163,20 @@ Object.assign(CodemanApp.prototype, { * * `text` is for the callers that already read the selection to decide whether * to copy at all (the Ctrl+C gate and the right-click handler), so the read is - * not repeated. It must be the selection xterm holds RIGHT NOW, because the - * mid-row flag below comes from the live selection rather than from `text`. + * not repeated. The transform is idempotent on xterm output, so an + * already-cleaned string is an acceptable argument: a CR is consumed by the + * parser as a cursor move and never stored in a cell, so the only \r the + * selection can carry is the Windows line join, and that is what makes the + * trailing scan a fixed point. Fuzzed over 300 000 realistic selections. * - * A COLUMN selection comes back untouched. Alt+drag makes one — xterm's + * A COLUMN selection comes back untouched. Alt+drag makes one (xterm's * shouldColumnSelect keys on altKey alone, and Codeman sets neither of the - * terminals it creates with the one option that would disable it — and a rectangle's whole point is that its rows line - * up, which both halves of the clean would destroy. xterm exposes the mode - * nowhere public, so this reads the private field the way this file already - * reads terminal._core for cell dimensions, and falls back to cleaning - * normally if a future xterm renames it. SelectionMode.COLUMN is 3. + * terminals it creates with the one option that would disable it), and a + * rectangle's whole point is that its rows line up, which trimming each row + * to its own last glyph would destroy. xterm exposes the mode nowhere public, + * so this reads the private field the way this file already reads + * terminal._core for cell dimensions, and falls back to cleaning normally if + * a future xterm renames it. SelectionMode.COLUMN is 3. */ cleanedTerminalSelection(text) { const raw = text ?? (this.terminal?.hasSelection?.() ? this.terminal.getSelection() : ''); @@ -4176,8 +4184,7 @@ Object.assign(CodemanApp.prototype, { if (this.terminal?._core?._selectionService?._activeSelectionMode === 3) return raw; const clean = window.CodemanCopySelection?.clean; if (!clean) return raw; - const start = this.terminal?.getSelectionPosition?.()?.start; - return clean(raw, { startedMidRow: !!start && start.x > 0 }); + return clean(raw); }, // Copy the current terminal selection. Goes through _copyText (Clipboard API, @@ -4189,10 +4196,10 @@ Object.assign(CodemanApp.prototype, { // alone, which are truthy, and a bare newline pasted into a chat composer // or a shell submits the line. decideAutoCopy applies the same rule. if (!selection.trim()) { - // Clearing matters as much as the toast. The Ctrl+C gate tests the RAW - // selection, so a padding-only selection left set would make every later - // Ctrl+C copy nothing instead of interrupting — the exact failure - // docs/architecture-invariants.md warns about under Terminal smart copy. + // Clearing is feedback, not protection. The Ctrl+C gate tests the CLEANED + // selection, so a padding-only selection left set can no longer swallow a + // later interrupt; it cleans to '' and the press reaches the PTY. What the + // clear avoids is a highlight that sits there having copied nothing. this.terminal?.clearSelection?.(); this.showToast('Nothing to copy', 'warning'); return false; diff --git a/test/terminal-copy-clean.test.ts b/test/terminal-copy-clean.test.ts index badd27e5..63ac18b4 100644 --- a/test/terminal-copy-clean.test.ts +++ b/test/terminal-copy-clean.test.ts @@ -2,13 +2,16 @@ * What a copy actually puts on the clipboard. * * xterm returns whole screen rows and trims only the cells that were never - * written to, so a full-screen TUI's padding spaces reach the clipboard and the - * indent it repeats on every row arrives baked into every line. These tests - * drive the SHIPPED transform (`CodemanCopySelection.clean` in constants.js), - * the SHIPPED wiring that decides column mode and the mid-row flag, and both - * SHIPPED copy paths, because the interesting failures live in the paths rather - * than in the string handling: a padding-only selection must not silently keep - * the user's Ctrl+C, and must not put a bare newline on the clipboard. + * written to, so a full-screen TUI's padding spaces reach the clipboard. These + * tests drive the SHIPPED transform (`CodemanCopySelection.clean` in + * constants.js), the SHIPPED wiring that decides column mode, and both SHIPPED + * copy paths, because the interesting failures live in the paths rather than in + * the string handling: a padding-only selection must not silently keep the + * user's Ctrl+C, and must not put a bare newline on the clipboard. + * + * A shared LEADING indent is deliberately left alone. The block below pins that + * as a contract rather than an accident, because stripping it was built and + * dropped before merge: see the rule in docs/architecture-invariants.md. * * Strategy: constants.js and terminal-ui.js in one vm with a stub CodemanApp, * the harness shape test/terminal-auto-copy.test.ts uses. No DOM, no xterm. @@ -73,8 +76,7 @@ function loadHarness() { return { app, windowRef, toasts, setSelection }; } -const clean = (text: unknown, startedMidRow = false) => - loadHarness().windowRef.CodemanCopySelection.clean(text, { startedMidRow }); +const clean = (text: unknown) => loadHarness().windowRef.CodemanCopySelection.clean(text); describe('CodemanCopySelection.clean — trailing padding', () => { it('drops the padding a full-screen TUI writes across the rest of each row', () => { @@ -98,36 +100,45 @@ describe('CodemanCopySelection.clean — trailing padding', () => { }); }); -describe('CodemanCopySelection.clean — shared leading indent', () => { - it('removes the indent every selected row shares', () => { - expect(clean(' first line\n second line')).toBe('first line\nsecond line'); +describe('CodemanCopySelection.clean — a shared leading indent is kept', () => { + // Measured over 401,445 three-row windows across 1,010 tracked files, stripping + // the run every row shares fired on 73% of them, and the transform cannot tell + // a TUI margin from content. These are the cases that settled it: each one is + // real output a user copies, and each one loses information if this changes. + it('keeps the indent every selected row shares', () => { + expect(clean(' first line\n second line')).toBe(' first line\n second line'); }); - it('keeps the relative indentation of anything nested inside the block', () => { - expect(clean(' outer\n inner\n outer again')).toBe('outer\n inner\nouter again'); + it('keeps a git log body at its four-space indent', () => { + expect(clean(' fix(terminal): trim the padding \n xterm hands back whole rows ')).toBe( + ' fix(terminal): trim the padding\n xterm hands back whole rows' + ); }); - it('is a no-op when the rows share no indent, as shell output does not', () => { + it('keeps indented Python, where the indent is semantic', () => { + expect(clean(' for item in items:\n if item.ready:')).toBe( + ' for item in items:\n if item.ready:' + ); + }); + + it('keeps the leading space on git diff context rows, where it is the marker', () => { + expect(clean(' const x = 1;\n }')).toBe(' const x = 1;\n }'); + }); + + it('is a no-op on shell output, which shares no indent anyway', () => { expect(clean('$ ls\n indented output\ndone')).toBe('$ ls\n indented output\ndone'); }); - it('is not disabled by a blank row in the middle of the block', () => { - expect(clean(' first \n \n second ')).toBe('first\n\nsecond'); + it('still drops trailing padding on every one of those rows', () => { + expect(clean(' first \n \n second ')).toBe(' first\n\n second'); }); - it('does not eat a \\r when the row is blank and the shared indent is wider', () => { - expect(clean(' first\r\n\r\n second\r\n')).toBe('first\r\n\r\nsecond\r\n'); - }); - - it('treats a leading tab as no indent at all, so nothing is stripped', () => { - expect(clean('\tfirst\n second')).toBe('\tfirst\n second'); + it('does not eat a \\r on a blank row', () => { + expect(clean(' first\r\n\r\n second\r\n')).toBe(' first\r\n\r\n second\r\n'); }); }); -describe('CodemanCopySelection.clean — one row keeps its own indent', () => { - // A single row shares its leading run with nothing, so that run is content. - // Stripping it would silently reindent one line of `git log` body text or one - // line read out of `less`. +describe('CodemanCopySelection.clean — one row is treated like any other', () => { it('leaves the indent on a single-row selection', () => { expect(clean(' hello world ')).toBe(' hello world'); }); @@ -136,22 +147,8 @@ describe('CodemanCopySelection.clean — one row keeps its own indent', () => { expect(clean(' hello world\n ')).toBe(' hello world\n'); }); - it('strips as soon as a second row carries content', () => { - expect(clean(' hello\n world')).toBe('hello\nworld'); - }); -}); - -describe('CodemanCopySelection.clean — a drag that began inside a row', () => { - it('measures the shared indent without the partial first line', () => { - expect(clean('That sample is clean.\n the next row continues.', true)).toBe( - 'That sample is clean.\nthe next row continues.' - ); - }); - - it('leaves the partial first line exactly as it is, indent included', () => { - expect(clean(' already mid-row\n following row\n deeper row', true)).toBe( - ' already mid-row\nfollowing row\n deeper row' - ); + it('leaves it when a second row carries content, exactly as for one row', () => { + expect(clean(' hello\n world')).toBe(' hello\n world'); }); }); @@ -173,16 +170,21 @@ describe('CodemanCopySelection.clean — nothing to clean', () => { }); describe('cleanedTerminalSelection — wiring', () => { - it('strips the shared indent when the drag began at column 0', () => { + it('trims each row and leaves the shared indent alone', () => { const { app, setSelection } = loadHarness(); - setSelection(' first\n second'); - expect(app.cleanedTerminalSelection()).toBe('first\nsecond'); + setSelection(' first \n second '); + expect(app.cleanedTerminalSelection()).toBe(' first\n second'); }); - it('spares the first line when the drag began inside a row', () => { + it('does not read the selection position at all', () => { + // The mid-row flag is gone. It read getSelectionPosition().start, which is + // xterm's mousedown ANCHOR and is never normalised, so an upward drag read + // it off the bottom row of the selection. const { app, setSelection } = loadHarness(); - setSelection('first\n second', { startX: 6 }); - expect(app.cleanedTerminalSelection()).toBe('first\nsecond'); + const terminal = setSelection(' first\n second'); + terminal.getSelectionPosition = vi.fn(() => ({ start: { x: 6, y: 0 }, end: { x: 0, y: 1 } })); + expect(app.cleanedTerminalSelection()).toBe(' first\n second'); + expect(terminal.getSelectionPosition).not.toHaveBeenCalled(); }); it('uses the text it is given without reading the selection again', () => { @@ -190,7 +192,7 @@ describe('cleanedTerminalSelection — wiring', () => { // The contract is that `text` IS the live selection, so the stub agrees with // it; the assertion that carries weight is that getSelection went unread. const terminal = setSelection(' given text \n second row '); - expect(app.cleanedTerminalSelection(' given text \n second row ')).toBe('given text\nsecond row'); + expect(app.cleanedTerminalSelection(' given text \n second row ')).toBe(' given text\n second row'); expect(terminal.getSelection).not.toHaveBeenCalled(); }); @@ -235,11 +237,11 @@ describe('copyTerminalSelection — what reaches the clipboard', () => { setSelection(' first line \n second line '); return app.copyTerminalSelection().then((ok: boolean) => { expect(ok).toBe(true); - expect(app._copyText).toHaveBeenCalledWith('first line\nsecond line'); + expect(app._copyText).toHaveBeenCalledWith(' first line\n second line'); }); }); - it('cleans a realistic TUI block on both rules at once', () => { + it('cleans a realistic TUI block, padding only', () => { const { app, setSelection } = loadHarness(); const pane = [' That last point is the important one. ', ' Claude Code writes each paragraph. '].join( '\n' @@ -247,14 +249,14 @@ describe('copyTerminalSelection — what reaches the clipboard', () => { setSelection(pane); return app.copyTerminalSelection().then(() => { expect(app._copyText).toHaveBeenCalledWith( - 'That last point is the important one.\nClaude Code writes each paragraph.' + ' That last point is the important one.\n Claude Code writes each paragraph.' ); }); }); - it('clears a padding-only selection so Ctrl+C goes back to interrupting', () => { - // The Ctrl+C gate tests the RAW selection. Leaving a padding-only selection - // set would make every later Ctrl+C copy nothing instead of interrupting. + it('clears a padding-only selection rather than leaving a dead highlight', () => { + // The clear is feedback, not protection: the Ctrl+C gate tests the CLEANED + // selection, so a padding-only one falls through to the PTY either way. const { app, toasts, setSelection } = loadHarness(); const terminal = setSelection(' '); return app.copyTerminalSelection().then((ok: boolean) => { @@ -282,8 +284,8 @@ describe('_flushAutoCopySelection — cleaned text is what Auto Copy handles', ( setSelection(' first line \n second line '); app._autoCopyPending = true; return app._flushAutoCopySelection().then(() => { - expect(app._copyText).toHaveBeenCalledWith('first line\nsecond line'); - expect(app._autoCopyLastText).toBe('first line\nsecond line'); + expect(app._copyText).toHaveBeenCalledWith(' first line\n second line'); + expect(app._autoCopyLastText).toBe(' first line\n second line'); }); });