Repository navigation
Preserve multiline pastes as one Sidekick message - #1063
Conversation
PR #1063: Preserve multiline pastes as one Sidekick message6 files, +273 / -22 Scope✅ PR scope looks good. Structural✅ No structural bloat detected. Slop✅ Slop indicators look low. Static AnalysisOxlint: 3 diagnostics across 1 file (1 rule) no-floating-promises (3): packages/cli/src/repl/screen.ts CorrectnessFILE: packages/cli/src/repl/screen.ts FILE: packages/cli/src/repl/screen.ts No other extraneous code patterns detected. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Independent review — PR #1063 at d76e3b15
Verdict: approve (posted as a comment review — GitHub refuses an APPROVE event from the PR author's account, HTTP 422). No blocking findings. One non-blocking, pre-existing observation is left as an inline comment on packages/cli/src/repl/input.ts. Main health and the required CI checks remain merge gates; this review ran alongside them and reused the focused evidence (terminal, dispatch and presentation suites, Deno and Node typechecks) plus bounded measurements at this exact head.
Correctness
- Traced the new prefix/paste state in
packages/cli/src/repl/input.tsthrough the scanner loop inpackages/cli/src/repl/screen.ts. A held loneESCnow reportsescLatencyitself, the flush feeds the held byte and settles it, so Escape,ESC|[Asplit arrows, kittyESC[27u, the F9/Insert prefixes that shareESC[2…with the opening marker, Alt chords, Control-J and unframed CR decode identically to the parent commit. Probe of 13 sequences on both decoders: the only differences are the fixed leakage, where the parent emitted"2","0","0","~",…,Enterfor a framed paste. - Payload bytes never reach the key scanner; CR/CRLF become LF; ordinary bytes before and after the frame keep their order (P3). A frame truncated at EOF drops its payload silently, matching the plan's "no new compatibility policy". Mode 2004 rides the existing
Setting, so apply/revert ordering and the single reset are unchanged (P7 extends existing cases). @bomb.sh/tty0.9.0 exposes no paste event (esm/input.d.ts), so the hand-rolled framing is the smallest option. ItsescLatencydefault is 25, so?? 25changes nothing.
Minimalism
Scope held: decoder, one screen setting, spec and architecture paragraphs; no new owner, API, dependency or test file. Optional nits only: the paste pass object is the one unfrozen ReplInputScan, and opening/closing could be module constants.
Performance (measured at this head)
Boundary: the per-read decoder loop and one text event per paste. Byte source: in-process injected host (terminal.bytes()), production screen decoder, field, dispatch, Termless xterm.js backend at 160x36. Not an OS PTY, native terminal or clipboard. "Writes" are completed ReplTerminal writes, not frames or paint.
| workload | result |
|---|---|
decoder: 2000 typed reads (letters, ESC[A, \r), 1 warmup + 5 samples |
head p50 2.5 µs, p95 3.2 µs per read; parent p50 1.2 µs, p95 1.5 µs; same 2000 events |
| decoder: framed paste 1 KB / 16 KB / 262 KB, single, 4096- and 131-byte reads, CRLF and LF | 0.13–0.34 ms / 0.7–1.6 ms / 7.7–11.7 ms; exactly one text event, exact text every case |
| mounted: 3.4 KB framed CRLF paste at a middle caret, reads of 1/17/509/1024/4095 bytes | 42.7 ms bytes→tail visible, 1 completed write, 0 prompts, QUEUE 0/16 |
| mounted: same content unframed LF (the per-character path every terminal used before this PR) | 920.8 ms, 1 completed write, 0 prompts |
| mounted: 35 typed keystrokes | p50 6.6 ms, p95 7.4 ms bytes→visible, 1 write each (head reference; no parent run) |
Limitations: no CPU profile retained because decoder costs are microseconds to milliseconds and no hot stack was in doubt; the parent was compared at the decoder only, since the field/dispatch path is unchanged; single host, indicative numbers, no FPS budget implied.
| ordinary.push(byte); | ||
| } | ||
| } | ||
| passes.push(yield* keys(Uint8Array.from(ordinary))); |
There was a problem hiding this comment.
Non-blocking, pre-existing, outside this PR's accepted scope. Ordinary bytes still reach the tty scanner in one scan(bytes) call. @bomb.sh/tty 0.9.0 accepts at most SCAN_BUFFER_SIZE (4096) bytes per native pass and breaks when a chunk is not fully accepted, so one unframed read above 4 KB loses everything after the first ~4 KB; drain() recovers pending events, not unaccepted bytes.
Evidence, identical on the parent and this head: one 16,438-byte unframed LF read → 3,925 of 15,233 chars; one 65,586-byte read → 3,925 of 60,776; the same payload in 4,096-byte reads → 15,228 of 15,233 (multibyte sequences split at a read boundary are dropped by the scanner).
Consequence: only terminals without bracketed paste, or piped stdin, delivering more than 4 KB per read; real ttys usually deliver at most 4 KB. Framed pastes bypass this because the payload is buffered here, which is the fix for the reported case.
Recommendation: track in a follow-up issue rather than this PR. The smallest later repair is feeding ordinary to keys() in slices of at most 4096 bytes, since the per-byte loop already exists here. No change requested for #1063.
Pasting a multiline prompt into Sidekick leaves the complete text in one editable message. Blank lines, indentation and Unicode survive insertion at the caret. Paste starts no message, queued work or provider call; an explicit Send submits the entire draft once.
The CLI decoder consumes bracketed-paste framing before ordinary key scanning and normalizes CR/CRLF line breaks to LF. It preserves markers, UTF-8 and payloads split across reads, including pastes beyond one scanner pass. The existing screen owner enables and restores bracketed-paste mode. Deliberately typed submission keys and the ordinary Round draft keep their current behavior. Dispatch, queue, providers and public APIs are unchanged.
Validation: 38 focused test groups / 198 steps, Deno checks for the input/screen modules, and Node typechecking pass. Headless byte-to-editor/dispatch journeys cover idle and held-turn pastes, middle-caret insertion, zero premature work and one exact complete submission. Terminal mode setup/reset, every two-read marker/Unicode split, byte-by-byte input and long payloads are covered. This is terminal-independent injected-byte evidence; no physical clipboard or iTerm2 verification is claimed.
The original Planner passes signed commit
d76e3b15, including the direct human clarification accepting headless terminal-path evidence. The one independent Fable review reports no blocking findings at this exact head. It traced escape/key compatibility, paste framing and mode cleanup, and measured the affected headless input/editor path. Required CI and current-main health are green.A pre-existing loss of oversized unframed reads in the underlying tty scanner is documented in the review and remains outside this framed-paste fix. No correction was requested; a separate follow-up is prepared. No physical clipboard, iTerm2 or native compositor measurement is claimed.
Fixes #1046.