Repository navigation
perf(web): pack terminal snapshot rows in WASM - #17188
StiensWout wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This change replaces the shipped terminal snapshot marshalling path with a new C/WASM packing ABI, updated build integration, and shared-memory allocation protocol. Its intended output is compatible and well tested, but the cross-language runtime refactor affects every terminal snapshot and warrants human review. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe browser WASM build now exports a Ghostty row-packing helper. The terminal core uses its packed output to decode rows. Tests cover graphemes, memory growth, resizing, and dirty-row behavior. ChangesPacked Ghostty row decoding
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant Core as GhosttyTerminalCore
participant Pack as t3_ghostty_pack_row
participant API as Ghostty API
Core->>Pack: Pass row, cells, columns, and output buffer
Pack->>API: Read row flags and cell data
API-->>Pack: Return row and cell values
Pack-->>Core: Return packed row data
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change speeds up terminal snapshot decoding by packing rows in WASM. The review found no concrete defect, and the buffer retry and memory-growth paths are covered by tests. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is confined to terminal rendering and retains buffer ownership and size checks. The new row format requires the application and its binary to remain compatible. No introduced security concern was established, but deployed behavior was not independently verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Terminal snapshots cross the JS/WASM boundary once per cell, which makes large grids expensive. Pack each changed row in WASM and decode its cells and graphemes in one pass, retaining the existing row cache.
A 120×40 Node benchmark measured full snapshots at 7.15–7.95 ms on main, 4.05–4.53 ms with #17091, and 1.31–1.38 ms here. This measures snapshot work, not browser FPS. The binary grows by 1,037 bytes. #17091's navigation changes remain separate.
Verified 31 focused tests, web typecheck, scoped lint, differential parity, and two independent reviews. Mac Chrome checked ANSI, Unicode, wrapping, alternate screen, scrolling, selection, and split-pane resizing. Desktop shares this renderer; the Electron shell was not tested.
Before
After
Scroll, selection, and split-pane verification is an action sequence, not a frame-rate recording.
Prepared for Wout by
gpt-6.1-solin the Codex harness, with delegated GPT and Claude reviews.