perf(terminal): coalesce WebGL redraws and cache character joining#8178
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughThe update adds renderer-aware refresh policies throughout terminal output, replay, scrollback restoration, and foreground settling. WebGL paths use debounced public refreshes while DOM paths can request synchronous core refreshes. Arabic shaping registration is now lazy and triggered by RTL output, including chunk-boundary handling and cleanup. A new ligatures addon adds bounded caching with invalidation on refresh and font changes. Unit and end-to-end tests cover these behaviors. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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.
🧹 Nitpick comments (2)
src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts (1)
506-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the cross-chunk aggregation invariant.
The resolver OR and callback fan-in are subtle queue semantics; add a brief reason for preserving all forced refresh policies and preparation hooks.
Suggested clarification
+ // Why: one drained write can combine chunks queued under different renderer + // states; preserve every forced refresh policy and producer preparation hook. if (chunk.forceForegroundRefresh) {As per coding guidelines, “When writing or modifying code driven by a design doc or another non-obvious constraint, add a brief comment explaining why the code behaves that way.”
Source: Coding guidelines
src/renderer/src/components/terminal-pane/pty-connection.ts (1)
4365-4368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplain the shaping-before-write ordering.
Registering the joiner before output bookkeeping is intentional but non-obvious; document that it must happen before xterm parses the triggering bytes.
Suggested clarification
function beforeTerminalOutputWrite(data: string): void { + // Why: the joiner must register before xterm parses the RTL bytes that require it. ensureArabicShapingJoinerForText(pane.terminal, data) recordTerminalOutput(pane.terminal) }As per coding guidelines, “When writing or modifying code driven by a design doc or another non-obvious constraint, add a brief comment explaining why the code behaves that way.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 56d9a130-c9f6-4aae-b515-b895f6879693
📒 Files selected for processing (17)
src/renderer/src/components/terminal-pane/layout-serialization.test.tssrc/renderer/src/components/terminal-pane/layout-serialization.tssrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/replay-guard.test.tssrc/renderer/src/components/terminal-pane/replay-guard.tssrc/renderer/src/lib/pane-manager/pane-lifecycle.test.tssrc/renderer/src/lib/pane-manager/pane-lifecycle.tssrc/renderer/src/lib/pane-manager/pane-manager.tssrc/renderer/src/lib/pane-manager/pane-terminal-foreground-render-settle.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.tssrc/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.tssrc/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.tssrc/renderer/src/lib/pane-manager/terminal-ligatures-addon.test.tssrc/renderer/src/lib/pane-manager/terminal-ligatures-addon.tstests/e2e/terminal-foreground-redraw-freeze.spec.ts
c390023 to
f8559b3
Compare
|
CI note: the second This is reproducible after rebasing onto current The regression arrived with #7997 ( Failed job: https://github.com/stablyai/orca/actions/runs/29132214554/job/86489555998 |
f8559b3 to
77ea614
Compare
|
Rebased onto current
Re-validated post-rebase: renderer typecheck, 2,348 pane-manager/terminal-pane unit tests (including #7214's flow-control suites), the 2-scenario Electron redraw spec, lint/format, and the max-lines ratchet — all green. A two-lane adversarial review of the rebase surface returned clean. The previously reported |
1 participant
Summary
Related issue: #6655. The issue remains open for reporter hardware confirmation.
Screenshots
No visual change.
Testing
pnpm lint— focused oxlint and oxfmt passed for all 17 changed files; the PRverifyjob passed before the comment-only follow-up.pnpm typecheck—pnpm run typecheck:webpassed.pnpm test— 521 focused Vitest tests and the 41-test registered xterm containment slice passed.pnpm build— the focused Electron run rebuilt the e2e app successfully.Additional runtime validation:
After the final comment-only change, the directly affected PTY/scheduler slice passed 402/402 tests plus focused lint/format.
Trace-backed cause
The reporter's 14.7s macOS trace was not a WebGL-loss/DOM-fallback case. It showed 548 of 549 xterm write timers immediately followed by an animation-frame render:
Measured effect
On the same live macOS Electron pane with real WebGL, replaying 180 rewrite frames at about 35 FPS:
renderRowstimeThat halves actual renders and reduced measured render work by about 53% in this replay. Ordinary output kept the character-joiner count at zero; the first Arabic output registered shaping before rendering (0 -> 1 joiner).
Reliability proof
terminal-performance.input-throughputis primary, while preservingterminal-performance.output-backpressure-budget,terminal-render.windows-cjk-repaint, andxterm-addon.boundary-containment. Foreground repair must not synchronously duplicate an already-queued WebGL frame; DOM/Windows repair remains synchronous; ordinary non-RTL panes must not pay for the RTL joiner; first RTL bytes must still be prepared before xterm parses them.terminal-performance.input-throughputremains a registered gap with protectionnone; the Electron probe is not manifest-registered and lacks cross-platform soak history.AI Review Report
The review covered renderer-mode transitions at initial parse and scheduled follow-up, replay/restore behavior, mixed-producer queue policy, xterm addon containment, lazy RTL ordering and teardown, ligature-cache mutation/invalidation/bounds, memory growth, and test oracle quality. It found and fixed early renderer-state capture, synchronous WebGL replay refreshes, overwritten per-chunk preparation, registration-failure output loss, an unbounded cache-entry count, a vacuous DOM-passing WebGL E2E, a Linux GPU-disabled harness failure, and a missing layout-manager test method. The final adversarial pass found no blockers.
Cross-platform review: no shortcuts, labels, shell commands, or paths changed. The renderer policy is provider-independent across local, daemon, SSH, WSL, and remote-runtime. macOS WebGL was tested live; Linux golden and Wayland CI passed on the prior code commit, and Windows keeps the deterministic synchronous DOM/ConPTY repair path. Live Linux WebGL and live Windows remain accepted gaps.
Security Audit
No new IPC, network, auth, secret, environment-variable, filesystem, command-execution, dependency, or persisted-data surface was added. Terminal output is only scanned in-memory for existing RTL code-point ranges; it is not logged or serialized. The new cache is pane-local, bounded by characters and entry count, invalidated on font changes/refresh, and released through addon disposal. No follow-up security work is required.
Notes