perf(terminal): coalesce per-frame PTY chunks into one xterm write#179
Conversation
The rAF flush already hands writeChunks the full array of chunks staged for the frame, but writeChunks looped and wrote each chunk individually. Under heavy TUI redraw streaming (Claude Code / Ink) that per-chunk fan-out is the drain hot path: each chunk drives a full xterm VT-parser pass AND a second full parse into the predictive-echo headless model, plus a promise tick — N times per frame. The renderer couldn't keep up and typing lagged. xterm's parser is a streaming state machine, so write(a)+write(b) ≡ write(a+b) for both the live terminal and the predictive-echo model. Coalescing the frame's chunks into a single write collapses N parser passes + N model writes + N ticks into one. Byte content and order are unchanged, so the one-write-per-byte invariant holds. Per-chunk recordChunkEchoed accounting is preserved (it consumes one pending keystroke per call). Regression test asserts a 3-chunk frame lands exactly one ordered write (red: 3, green: 1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 50 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTerminal output pipeline now coalesces multiple PTY chunks from a single drain batch into one write operation before forwarding to the predictive-echo engine and live xterm terminal. A new test validates the coalescing behavior. The agent-store stress test timeout was increased to prevent CI flakiness under large workloads. ChangesPTY Output Pipeline and Test Stability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed PR #179 against the current checkout. I found no validated breakage and made no code changes. Validation passed:
Remote status checked via GitHub API: PR is Addressed comments
|
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. No validated breakage found in the PR changes, so I made no source edits. The PTY chunk coalescing keeps per-chunk typing trace accounting, preserves ordered byte delivery, and uses a single xterm/predictive-echo write per drained frame as intended. Validation run locally:
Remote PR state observed via API: Addressed comments
|
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Reviewed PR #179 and made one test reliability fix outside the PR diff after reproducing a local full-suite failure. Changed agent-store.stress.test.ts: increased the heavy stress test timeout from Validation passed:
GitHub API currently reports the PR as Addressed comments
Not ending with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/lib/terminal-runtime-registry.dom.test.ts`:
- Around line 171-203: Extend the test in terminal-runtime-registry.dom.test.ts
that uses registry.acquireTerminalRuntime, ptyBuffer.appendPtyChunk and
ptyBuffer.flushPtyChunksNow to also assert duplicate-chunk preservation and no
replay: after the initial happy-path drain (aaa+bbb+ccc) add a case where you
append duplicate chunks (e.g., 'dup','dup') before a flush and assert
term.__writes records a single combined write preserving both duplicates, then
call ptyBuffer.flushPtyChunksNow(runtime.key) again with no intervening append
and assert term.__writes did NOT increase (no replay); keep using the same
runtime and clean up with registry.disposeTerminalRuntime(runtime.key).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10de8a40-5931-42bd-96b1-03c061874ed0
📒 Files selected for processing (3)
src/renderer/src/lib/terminal-runtime-registry.dom.test.tssrc/renderer/src/lib/terminal-runtime-registry.tssrc/renderer/src/stores/agent-store.stress.test.ts
|
Reviewed PR #179 against the current checkout. I found no additional validated breakage, so I made no source edits in this run. Local validation passed:
Remote API state observed at the end: PR is Addressed comments
|
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Reviewed PR #179 and made one focused test fix. Changed terminal-runtime-registry.dom.test.ts to cover duplicate PTY chunks being preserved in the coalesced write and an empty follow-up drain not replaying prior output. Validation passed locally:
Addressed comments
Not ending with |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed the current checkout against PR #179. I found no additional validated breakage and made no new source edits in this run. Local validation passed:
Remote state observed: PR is Addressed comments
|
|
Reviewed PR #179 against the current checkout. I found no additional validated breakage and made no source edits in this run; the actionable CodeRabbit request is already present in the checked-out code. Local validation passed:
Addressed comments
Not ending with |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed PR #179 against the current checkout. I found no additional validated breakage and made no source edits; the actionable duplicate/replay coverage requested by CodeRabbit is already present at src/renderer/src/lib/terminal-runtime-registry.dom.test.ts. Validation passed locally:
Remote status I could verify through the connector only reported Addressed comments
|
|
Reviewed the PR diff and traced the touched runtime, PTY buffer, predictive echo interface, and adjacent tests. I did not find a reproducible breakage in the current checkout, so I did not make code edits. Addressed comments
Verification run:
I cannot claim PR CI or GitHub mergeability from this sandbox, so I’m not marking this READY. |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed PR #179 against the checked-out diff and current code. I did not find validated breakage requiring edits; the actionable duplicate/replay coverage requested on the PR is already present in the current checkout. Local validation passed:
Remote status available through the connector only reported Addressed comments
|
|
Reviewed PR #179 against the current checkout. I found no additional reproducible breakage and made no source edits in this run. Local validation passed:
Remote API state observed for head Addressed comments
|
1 participant
Problem
Typing felt laggy under heavy TUI redraw streaming. PR #176's fidelity harness measured the symptom: 762/1000 markers rendered after a 10s settle — xterm's
term.write()drain couldn't keep up and the main thread saturated.Root cause
The rAF flush in
pty-buffer-storealready handswriteChunksthe full array of chunks staged for the frame, butwriteChunkslooped and wrote each chunk individually. Per chunk that drives:— N times per frame. Under a redraw burst (many small chunks/frame) that per-chunk fan-out is the drain hot path.
Fix
xterm's parser is a streaming state machine, so
write(a)+write(b) ≡ write(a+b)for both the live terminal and the predictive-echo model. Coalesce the frame's chunks into a single write → N parser passes + N model writes + N ticks collapse to one. Byte content and order are unchanged (one-write-per-byte invariant holds). Per-chunkrecordChunkEchoedaccounting is preserved (it consumes one pending keystroke per call, so it stays in the loop).Test / gates
terminal-runtime-registry.dom.test.tsregression: a 3-chunk frame lands exactly one ordered write. Non-vacuous — red without the fix ("expected 3 to be 1"), green with it.tsc --noEmitclean,build:websucceeds.Scope / follow-ups
wip/lag-redraw-drain.🤖 Generated with Claude Code
Review caveat — predictive-echo branch (added on self-review)
The real-parser equivalence test covers the confirmed/live render (the
liveTerm.writepath, predictions absent). It does not unit-test thepredictiveEcho.onServerOutputpath under active typing, where per-chunk vs coalesced differ in intermediate writes: per-chunk runserasePredictionRegion+ reconcile once per chunk, coalesced once per frame.By reasoning (see
processServerChunk): the confirmed model converges (model.write(A)+model.write(B) ≡ model.write(A+B)), the survivor filter is monotonic in cursor column, andrenderPredictionsdraws the same final survivors — so the final screen + overlay converge. The dominant lag case (server redraw burst, user not mid-keystroke) hashadPredictions=false, so reconciliation is skipped entirely. Coalescing only batches chunks already staged within one animation frame.Net: authoritative content is provably identical; the optimistic-echo overlay converges and is self-correcting, but that convergence is argued, not yet covered by a test. A follow-up engine-level test (type + simultaneous redraw burst, assert overlay state matches per-chunk) would close it.