Repository navigation
fix(xterm): remove scrollback decorations by identity - #13178
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
Merge Risk: ⚪ Minimal · up to The change removes quadratic search-decoration cleanup with broad regression coverage and no established behavioral risk. The branch conflicts with main must still be resolved before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch (3)
20-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep native commits visible in screen reader mode.
Line 20 returns before the existing
!screenReaderModeguard. The adjacent contract requiresinsertTextto bubble in screen reader mode for character announcement. Do not consume composition input on this path when screen reader mode is enabled.Proposed fix
- if (ev.data && ev.inputType === 'insertText' && this._compositionHelper?.handleCompositionInput(ev.data, true)) return true; + if (ev.data && ev.inputType === 'insertText' && !this.optionsService.rawOptions.screenReaderMode && this._compositionHelper?.handleCompositionInput(ev.data, true)) return true;
32-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn the non-composing default from the mock.
CoreBrowserTerminal.inputnow calls this method. This mock reportsisComposing === false, but it throws instead of allowing normal input to continue. Returnfalse.Proposed fix
public handleCompositionInput(data: string, nativeCommit: boolean): boolean { - throw new Error('Method not implemented.'); + return false; }
295-295: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace the raw NUL byte with a TypeScript escape.
The delimiter is a raw NUL byte. Git can classify files containing NUL bytes as binary, which breaks reliable patch regeneration and application. Use the textual
\0escape instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c60be8c-b6ea-454e-be46-5f8e0834f826
📥 Commits
Reviewing files that changed from the base of the PR and between 25a8c51 and ac790a5ac8dd3bba7809dac24bbad6781ce373ef.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
config/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patchsrc/renderer/src/components/terminal-marker-dispose-key-order.test.ts
|
Backlog note (cleanup pass): auto-rebase onto current |
ac790a5 to
511e383
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Scrollback trim under search highlights was O(k²) because dispose set marker.line to -1 before onDispose, collapsing SortedList keys. Fire listeners first so delete still sees the real line, then clear the line. Fixes stablyai#10879
511e383 to
decd687
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Supersede the old beta.287 marker-event reorder with the tested beta.303 SortedList fix. Preserve the existing PR history while retaining current main patches and regenerated artifacts.
nwparker
changed the title
|
Too many files changed for review (3000 files, 100 file limit). |
`pr-code-change-scope.mjs` read stdin with `readFileSync(0, 'utf8')`, a single read of fd 0. Once the writer outgrows the 64 KB pipe buffer that read returns early or throws EAGAIN, the script exits 0 having emitted no `name=value` pairs, and `tee -a "$GITHUB_OUTPUT"` records nothing -- so every lane the classifier gates is silently skipped rather than failing loudly. A PR opened long ago carries a stale `pull_request.base.sha`, so the gate's merge-base diff spans the whole base branch. PR #13178 diffed 13,294 files (773 KB) against a base 1,592 commits behind main and lost typecheck, test, static analysis, xterm patch sync, package and e2e to this. Stream stdin instead, matching how the sibling `pr-e2e-source-routing.mjs` already reads the same list in the same workflow.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ELI5
Heavy output while terminal search highlights are active repeatedly scans highlights already removed from scrollback. Removing 1,000 highlights requires over 800,000 key reads. Remove decorations by identity so the terminal does constant lookup work per removal, while keeping the same highlights and scrollback.
What Changed
Replace the existing SortedList keyed-delete retry patch with an identity-to-index map. Unique entries store a scalar index; index arrays are allocated only for duplicate identities. Preserve the sorted array, live-key queries, deferred insertion/compaction, duplicate entries, and snapshot iteration. Use a set of deleted indices for linear compaction. Clear/rebuild the index at the existing array lifecycle boundaries.
Regenerate xterm 6.1.0-beta.303 bundles and sourcemaps through the pinned source-patch workflow and update the lockfile hash. No dependency version change or unrelated source-patch change.
Why
The issue still reproduces on current main. The previous version of this PR reordered Marker.dispose notifications, but Buffer.addMarker already subtracts the trim amount before disposal. Ordinary one-line trims still collapse evicted keys to -1. This replacement leaves the public marker event contract unchanged and handles mutated keys directly.
Identity indexing also subsumes the previous missed-delete retry fix. Search limits, retained output, marker disposal timing, and sorted overview-ruler iteration remain unchanged. The index uses O(n) additional storage bounded by entries already retained in the list; it is cleared on reset and rebuilt after compaction.
Linked Issue
Fixes #10879
Visual Proof
Background Electron / Playwright CDP validation on local macOS, using an isolated profile and a folder workspace. App identity was verified against this worktree. Screenshots below show successive states on the final implementation, not a timing comparison against old code.
echo UI_OK_10879through the terminal input and observed the exact output.Highlights before trim
After trim, with find bar still open
Search reused successfully
Search closed; keyboard input/output verified
Testing
xterm-addon.boundary-containmentcommand, and patch-generator contracts.pnpm tc:web.git diff --checkexcluding generated patch files (unified-diff blank context lines intentionally contain a space).ORCA_BACKGROUND_LAUNCH=1 node config/scripts/regenerate-xterm-patches.mjs --check --work-dir=/tmp/orca-xterm-10879reproduces all shipped patches and lockfile hashes.Deterministic lookup evidence (250 / 500 / 1,000 disposed entries): baseline 52,789 / 209,788 / 836,287 key reads; replacement 0 / 0 / 0. Real search eviction previously used 838,041 key reads and now satisfies the linear budget. New tests also cover duplicate identity removal, repeated deletes, live-key queries after shifts, snapshot order, reinsertion, clear, and 2,000 seeded mixed operations.
Final scalar-index benchmark (plain Node/V8, 1,000 entries, median of 100 samples after warmup on local macOS): dispose-then-delete plus compaction 1.374 ms → 0.069 ms; normal delete plus compaction 0.055 ms → 0.062 ms; insertion approximately 0.041 ms → 0.067 ms. Whole-terminal timing depends on renderer and other work; the deterministic key-read counts are the performance oracle.
Forced-GC memory measurement across 200 lists, each containing the same 1,000 shared decoration objects (isolates collection storage): original collection approximately 8.5 KB/list; first array-index candidate 93.5 KB/list; final scalar-index candidate 37.5 KB/list. Incremental overhead is therefore approximately 29 KB per 1,000 decorations, down from 85 KB (66% reduction). Clearing drops storage to roughly 0.6 KB per empty final list, with no per-entry retention.
nis decoration entries, not scrollback lines. Duplicate identities retain their supported semantics through lazy index arrays and a new partial-compaction/reinsertion test.Review
Readiness checklist: PASS for this diff. No proven P0, P1, or P2 findings; counsel skipped under the checklist's zero-candidate exception.
Reliability invariant:
terminal-decoration.identity-removal— removing a decoration must notify consumers exactly once even after its marker key changes, retain live decorations in buffer-line order, and leave output intact. Failure source: #10879 and the existing search-highlight leak regression. Oracle: shipped-runtime removal counts, empty/retained decoration iteration, search reuse, output assertions, and deterministic lookup budget. Related existing gate:xterm-addon.boundary-containment(executed). Dedicated eviction performance coverage is in ordinary Vitest CI; a dedicated manifest gate is not added in this change.Residual validation gaps: physical Windows/Linux/WSL/SSH/relay/mobile, native-focus transitions, and real-network fault recovery were not run. Local macOS has actual Electron screenshot and terminal/input evidence in addition to happy-dom contracts. The first CDP command immediately after focus lost its initial character and the shell rejected it; that attempt was not counted as a pass. After input settled, a complete keyboard-typed command and exact shell output passed. This initial automation anomaly was not diagnosed as a production regression, and native-focus transitions remain unproved. Those implementation paths are unchanged. No negative user-facing behavior trade-off was introduced or observed; this is evidence, not an all-platform guarantee.
Agent skill upstream boundary
Checklist