BitSquare/Codeman
2
0
Fork 0
mirror of https://github.com/Ark0N/Codeman.git synced 2026-10-06 07:29:42 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(terminal): ship the copy clean as a trailing trim, without the shared dedent

Browse Source 
#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>
This commit is contained in:
Codeman maintainer
2026-09-19 12:18:39 +02:00
parent c376534a50
commit 4c705094f7
8 changed files with 134 additions and 160 deletions
Download Patch File Download Diff File Expand all files Collapse all files
test/terminal-copy-clean.test.ts
+60 -58
View File
@@ -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');
});
});