Skip to content

fix(terminal): trim the padding and shared indent out of a copied selection - #451

Merged
Ark0N merged 1 commit into
Ark0N:masterfrom
irisitymichaelgrundberg:fix/trim-copied-terminal-selection
Sep 19, 2026
Merged

Ark0N merged 1 commit into
Ark0N:masterfrom
irisitymichaelgrundberg:fix/trim-copied-terminal-selection

Conversation

@irisitymichaelgrundberg

Copy link
Copy Markdown
Contributor

Copying several rows out of a pane puts a wall of spaces on the clipboard and repeats the program's own left margin on every line. Pasting that into a chat client or an editor means deleting the whitespace by hand, every time.

The cause is in what xterm hands back. getSelection() returns whole screen rows, and its trim drops 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 and ride along. Measured against Claude Code in a 282-column pane, single lines arrived carrying 138 trailing spaces on top of a two-space transcript indent.

Native terminals already solve this. Windows Terminal, iTerm2 and GNOME Terminal all trim trailing whitespace on copy, which is why the same selection pastes clean out of a terminal and dirty out of a Codeman tab.

Two places in this repo had already reached the same conclusion for their own paths. decideAutoCopy calls a wall of spaces "never what the gesture meant", and _selectTouchSelectionLine calls the trailing cells padding and excludes them. The mouse and keyboard paths never got the rule.

What it does

CodemanCopySelection.clean lives in constants.js beside decideAutoCopy, its pure sibling. It drops the trailing run from each line, and removes the leading run only where every selected row shares one. Sharing is what makes that run the program's margin rather than the user's text, so shell output comes through untouched, and anything nested inside a copied block keeps its relative indentation.

cleanedTerminalSelection in terminal-ui.js is the half that needs the live terminal. The main terminal's four copy paths go through it: the Ctrl+C chord, right-click, the phone selection button, and Auto Copy.

What it does not cover

Three routes still copy the raw padded rows, and all three did so before this change too. The browser's own Edit menu copy is served by xterm's own copy listener on the terminal element, which writes selectionText directly. A copy-selection shortcut the user has disabled in App Settings leaves that same native listener to run. The subagent and teammate windows build their own Terminal in panels-ui.js with no copy wiring at all.

A capture-phase copy listener on the terminal element would close all three at once. I left it out to keep this focused on the paths the shortcut owns, and the invariants document now records the gap rather than claiming every copy is cleaned.

The rules that keep it honest

A one-row selection keeps its leading run. One row shares nothing with any other row, so that run is content. Stripping it would silently reindent a single line of git log body text, or one line read out of less.

A drag that began inside a row keeps its partial first line, and that line stays out of the measurement. It has no leading run of its own, so counting it would pin the shared run to zero and leave every row below it still wearing the margin. The trade is that such a drag lowers the two-row bar to one measured row, so a two-row mid-row drag over genuinely indented content loses that indent. Both the comment and the invariants document say so.

A column selection is returned untouched. Alt+drag makes one, since shouldColumnSelect keys on altKey alone and neither Terminal this app builds passes macOptionClickForcesSelection, the one option that would disable it. A rectangle's rows lining up is the entire point of the gesture, and both halves of the clean would destroy it. xterm exposes the mode nowhere public, so the check reads terminal._core._selectionService._activeSelectionMode, the way this file already reads terminal._core for cell dimensions in four other places. If a future xterm renames that field the check reads undefined and the selection is cleaned normally, which is the safe direction to fail, so a test pins both the field name and the literal against lib/xterm.js rather than against a stub repeating them.

The Ctrl+C chord decides on the cleaned selection. A drag across the blank part of a row selects real padding spaces, so the raw text is truthy, and testing it would spend that press on a copy of nothing and make the user press again to interrupt. A padding-only selection is dropped and the press falls through to the PTY as 0x03. Ctrl+Shift+C still never falls through, per the existing rule that an explicit copy chord must not interrupt a running agent.

copyTerminalSelection gates on trim(), not truthiness. A multi-row drag across padding cleans to line breaks alone, and a bare newline pasted into a chat composer submits it.

Auto Copy reads its own toggle before it reads the selection. It is off by default, and a selection can run to the 50 000-row scrollback ceiling, so the previous order spent that work on every mouseup on the page. The clean then runs before decideAutoCopy, so its dedupe and its cap both measure the text that actually reaches the clipboard.

Every pass over a line is a scan, not a regular expression

/[ \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 whitespace position and backtracks over it. Measured over 50 000 rows with a 280-column run, that expression took 2.9 seconds against 1.3 milliseconds for a backward scan, and a 2 000-column run took 16 seconds. The scan is also the faster of the two on an ordinary padded row, so there is no trade here.

Not in this PR

A paragraph that wraps across several rows still arrives as several lines. Under tmux the program writes a real line break at each wrap point, so those rows are genuinely separate lines by the time xterm sees them, indistinguishable from short lines that were always short. Rejoining them would be guesswork that mangles code blocks and lists.

Testing

test/terminal-copy-clean.test.ts adds 32 cases. They drive the shipped transform, 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. The padding-only cases assert that nothing reaches _copyText and that the selection is cleared, since those are what protect the interrupt key.

test/keyboard-shortcuts.test.ts already pinned the smart-copy gate by source. That pin is updated rather than removed: it now matches the cleaned-selection form and records why the raw text cannot be what decides.

npm test is green: 392 files, 7399 tests. typecheck, lint, format:check, check:frontend-syntax and check:public-assets all pass.

test/terminal-copy-shortcut.test.ts is in the Playwright suite the gate excludes. It times out before its page is ready on this machine, and it does so identically on unmodified master, so I read that as not runnable here rather than as a regression. That suite is also where the Ctrl+C handler body is covered, so the new chord branch has no executed unit test — the source pin and manual testing stand in for it.

Run for real, not just tested: this branch is built and running as my own Codeman install, and I have copied out of live panes to confirm each rule by hand. That covers the multi-row paste, the single indented line keeping its indent, an Alt+drag rectangle coming across verbatim, and a padding-only drag leaving Ctrl+C free to interrupt.

Docs

docs/architecture-invariants.md gains the clean stage under terminal smart copy, the three routes it does not cover, the mid-row qualification on the one-row rule, and a note on the Auto Copy ordering. docs/wiki/Keyboard-Shortcuts.md gains a plain-language paragraph so users know what the clipboard will and will not contain.

🤖 Generated with Claude Code

…ection

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 and reach the clipboard. Measured
against Claude Code in a 282-column pane, single lines arrived carrying
138 trailing spaces, and every line carried the two-space transcript
indent as well. Windows Terminal, iTerm2 and GNOME Terminal all trim that
for you, decideAutoCopy already calls a wall of spaces "never what the
gesture meant", and _selectTouchSelectionLine already treats those cells
as padding — the mouse and keyboard paths never had the same rule.

CodemanCopySelection.clean lives in constants.js beside decideAutoCopy,
its pure sibling. It drops the trailing run from each line, and removes
the leading run only where every selected row shares one. A selection of
a single row keeps its run, because one row shares nothing with anything
and stripping it would silently reindent one line of `git log` body text
or one line out of `less`. A drag that began inside a row keeps its
partial first line untouched and out of the measurement, which otherwise
pins the shared run to zero and leaves every following row indented.

Every pass over a line is a scan rather than a regex. `/[ \t]+(\r?)$/` is
quadratic on a line whose spaces are followed by a non-space character,
which is what right-aligned or centred TUI content looks like: measured
over 50 000 rows with a 280-column run it took 2.9s, against 1.3ms for
the scan, and a 2 000-column run took 16s. The scan is also the faster of
the two on an ordinary padded row.

cleanedTerminalSelection in terminal-ui.js is the half that needs the
live terminal. It returns a COLUMN selection untouched: Alt+drag makes
one, and a rectangle's rows lining up is the point of the gesture, so
both halves of the clean would destroy it. xterm exposes the mode nowhere
public, so the check reads terminal._core._selectionService, the way this
file already reads terminal._core for cell dimensions, and cleans
normally if a future xterm renames the field. A test pins that assumption
against the library rather than against a stub repeating the literal.

The Ctrl+C chord decides on the cleaned selection, not the raw one. A
drag across the blank part of a row selects real padding spaces, so the
raw text is truthy, and testing it would spend that press on a copy of
nothing and make the user press again to interrupt. A padding-only
selection is now dropped and the press falls through to the PTY, while
Ctrl+Shift+C still never falls through. copyTerminalSelection gates on
trim() for the same reason, since a multi-row drag across padding cleans
to line breaks alone and a bare newline pasted into a chat composer
submits it.

All four of the main terminal's 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 disabled copy shortcut and the subagent
windows still copy raw rows, as they did before, and the invariants doc
now says so rather than claiming every copy is cleaned. Auto Copy
resolves its own toggle before it reads the selection, since it is off by
default and a selection can run to the 50 000-row scrollback ceiling.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@irisitymichaelgrundberg
irisitymichaelgrundberg marked this pull request as ready for review September 18, 2026 20:13
@Ark0N

Ark0N commented Sep 19, 2026

Copy link
Copy Markdown
Owner

This is going in, but with half of it removed, and you deserve the full reasoning rather than a one-line verdict.

The trailing trim ships exactly as you built it. The premise is right, the measurement behind it is right (138 trailing spaces per line in a 282-column pane is the kind of number that makes a case), and the scan-instead-of-regex decision is the part I want to call out specifically. /[ \t]+(\r?)$/ really is quadratic on a right-aligned TUI row, and 2.9s against 1.3ms over 50,000 rows is exactly the sort of thing that would have shipped as a mystery freeze if you had reached for the obvious regex. Same for the \r handling and the Alt+drag column exemption. That half is unambiguously better than what was there.

The shared leading-indent strip is not shipping. Before you read that as a taste call, here is what I found when I went looking, driving your shipped transform rather than reading the diff:

Over 401,445 three-row windows across 1,010 tracked files in this repo, the dedent fires on 73% of them. 92% inside a YAML workflow, 76% over git log output, 48% in a TypeScript source. And no width threshold can separate a margin from content, 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 right between them. I tried the qualifications before giving up on them. A diff/list marker rule spares 65% of firings in a source file but only 8% in git log output and 5% in git diff, which are precisely the cases that need it.

The asymmetry is what settled it. A wrong trailing trim costs nothing. A wrong dedent silently deletes information that was on the screen, with no signal and nothing in the clipboard to hint at it, on git log bodies, on indented Python read out of cat, on git diff context rows where the leading space is the marker, and on stack traces.

You also shipped two real bugs in that half, and they are worth knowing about independently of the decision:

  1. The mid-row flag is read off the wrong end of an upward drag. getSelectionPosition().start is the mousedown anchor, and xterm never normalises it (CoreBrowserTerminal.ts:981), so for a reversed selection it is the other end. Dragging up through a block spares the bottom row and flattens everything above it. Your test stub hardcodes start: {x, y: 0} / end: {y: 1}, so it can only express a downward drag and the suite structurally could not see this.
  2. The same three rows give three different clipboard results depending on which column the mousedown landed in, which the user never sees. Click column 0 and the block stays uniform; click the first character and it flattens; click inside the indent and it mangles.

Neither is a reason the idea is bad, and I want to be clear that the first one is a genuinely subtle xterm fact rather than carelessness.

What I changed at merge: the transform is trailing-only, cleanedTerminalSelection() no longer reads the selection position, and the tests, invariants, CLAUDE.md, the wiki line and your changeset all move with it. The test block now pins the absence as a contract, using the git log, Python and git diff cases as its examples, so nobody re-derives this in six months. I also took the four accuracy items from the review: the comments and invariant rule that justified the padding-only clear were describing 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 Nothing to copy toast got its zh-CN entry, and the invariants paragraph no longer repeats its own opening sentence verbatim.

If it is ever revisited, one qualification did measure clean: gate the strip on 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, so that is a purely textual TUI signal needing no new plumbing. Zero false positives across all 401,445 windows, and it keeps the claude gutter win. I did not take it now because it still mangles a git log body sitting inside an agent's own gutter, which is the single most likely thing anyone copies out of a Codeman pane. If you want to pick that up as its own PR with the two bugs above fixed, I would look at it seriously.

Thanks for this one, and for #435 alongside it. You have been turning rounds same-day on both and it shows.

@Ark0N
Ark0N merged commit 613b774 into Ark0N:master Sep 19, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Sep 19, 2026
…ared 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) <noreply@anthropic.com>
@github-actions github-actions Bot mentioned this pull request Sep 19, 2026
@irisitymichaelgrundberg

Copy link
Copy Markdown
Contributor Author

Thanks for being careful here and pushing back on my PR; I was actually worried that there were situations that I had not considered. At least I don't have to manually remove the trailing spaces after your merge. So remaining after it are leading indents and mid-sentence line breaks.

I totally understand your reasoning but I probably need to make another attempt at this later. I regularly copy out things from the agent sessions into other tools (chats, tickets, etc) we use at work and it's very annoying to always have to correct the formatting manually. In my mind, this is a tmux + codeman problem since copying from Claude in the normal terminal is flawless (when running in full-screen mode).

Ark0N pushed a commit that referenced this pull request Sep 19, 2026
…hole tree

A full review of the release tree found seven things, and four of them were mine.

**The gate was red, and I put it there.** Splitting `confirmed` into `confirmedContext`
and `confirmedSwap` changed the wire field without moving three assertions that check
it: `custom-model-one-shot-launch.test.ts` and two in `custom-model-run-menu-ui.test.ts`
(the swap modal and the context modal, each of which already receives exactly the right
per-question flag). Moved, with the titles.

**Worse, my own tests for the split never ran.** The four cases in
`session-custom-model.test.ts` that exist specifically to pin it call `mockRunning()`,
which was declared inside a sibling `describe`, so they threw a ReferenceError during
setup. The split would have shipped with no passing server-side coverage while the gate
reported the failure as four broken tests rather than as four tests that were never
written. `mockRunning` is hoisted to the outer describe.

**The submit verifier pressed Enter into shell panes.** `#455`'s SubmitVerifier resolved
its composer glyph as `promptGlyph ?? '❯'`, and only claude and codex declare one, so
the other eight modes fell back to claude's `❯`. That is also starship's default shell
prompt, and pure's, and spaceship's, and p10k lean's. On such a shell the line
`❯ npm run build` sits on screen for as long as the command runs, the verifier reads it
as an unsubmitted prompt, and re-presses Enter into the running program's stdin up to
nine times on its 2s..60s schedule. Mostly a stray newline; not harmless against a y/N
prompt, `read -p`, an installer or a pager, where it takes the default. The module's own
fileoverview already stated the rule this broke. Now `?? ''`, which
`promptStillInComposer()` already treats as inert, so the verifier runs only for a CLI
that actually declares a composer.

**My #451 dedent removal left a count behind**: "Two rules keep it honest" introducing
three numbered rules.

The rest is documentation the split outran. `confirmedContext`/`confirmedSwap` appeared
in no doc at all, while `docs/api-reference.md` (the SemVer-covered contract) still told
an integrator to retry with `confirmed: true` for both questions, which is precisely the
thing the split exists to stop. Documented there, in `docs/custom-model-endpoints.md`
and in CLAUDE.md. The custom-model changeset gained the split and the `CLAUDE_CONFIG_DIR`
multi-user consequence, both user-visible and both previously absent, and #454's gained
the one exception to its own claim: a Custom Endpoints launch ignores the Instance count
stepper and always starts one session.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants