fix(app): reuse terminal cells during serialization - #46763
Merged
Conversation
Stop request handlers and close the page before killing the fixture-owned ConPTY so a debounced PTY resize cannot race the dead process. Await the resize acknowledgement after the viewport change and count actual canvas fillText draws when TERMINAL_DRAW_PROBE is set.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new manual Windows benchmark needs stronger platform/env guarding (and a small buffer index safety fix) to avoid accidental non-Windows discovery failures and potential flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves terminal scrollback serialization performance in the app by avoiding redundant Ghostty WASM row copies during style changes, while adding targeted regression tests and a manual performance benchmark harness to validate behavior and measure impact.
Changes:
- Reuse the already-available Ghostty cell snapshot when updating cursor style during serialization (avoids extra
buffer.getLine(row)row copies). - Add unit tests covering serializer row-read behavior and terminal writer flush ordering.
- Add a Windows-only, manual Playwright/Vite benchmark harness for end-to-end terminal workload + teardown measurement.
File summaries
| File | Description |
|---|---|
| packages/app/src/session/terminal/serialize.ts | Reuses the current cell snapshot for cursor style updates to reduce row-copy overhead. |
| packages/app/src/session/terminal/serialize.test.ts | Adds a regression test asserting bounded getLine reads and round-trip style fidelity. |
| packages/app/src/session/terminal/writer.test.ts | Adds coverage for flush behavior when writes are in-flight and more output is queued. |
| packages/app/e2e/performance/terminals/vite.config.ts | Builds a benchmark-only bundle that injects the probe module into index.html. |
| packages/app/e2e/performance/terminals/terminal-benchmark.spec.ts | Manual benchmark spec driving a ConPTY workload through the production terminal stack. |
| packages/app/e2e/performance/terminals/shell.ps1 | PowerShell fixture that emits the workload and responds to simple commands. |
| packages/app/e2e/performance/terminals/README.md | Documents how to run the manual benchmark and what it measures. |
| packages/app/e2e/performance/terminals/probe.ts | Benchmark-only instrumentation for render/write/serialize observation. |
| packages/app/e2e/performance/terminals/playwright.config.ts | Dedicated Playwright config for running only the terminal benchmark spec. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+4
to
+20
| export default defineConfig({ | ||
| ...config, | ||
| testDir: ".", | ||
| testIgnore: [], | ||
| testMatch: "terminal-benchmark.spec.ts", | ||
| workers: 1, | ||
| retries: 0, | ||
| timeout: 120_000, | ||
| outputDir: process.env.TERMINAL_RESULTS, | ||
| reporter: [["line"]], | ||
| webServer: { | ||
| command: `bun x vite preview --host 127.0.0.1 --port ${new URL(process.env.PLAYWRIGHT_BASE_URL!).port} --strictPort --outDir ${process.env.TERMINAL_BUILD}`, | ||
| url: process.env.PLAYWRIGHT_BASE_URL, | ||
| reuseExistingServer: false, | ||
| }, | ||
| use: { ...config.use, viewport: { width: 1440, height: 900 }, trace: "off", video: "off", serviceWorkers: "block" }, | ||
| }) |
Comment on lines
+12
to
+26
| // Use the same installed native PTY package as Core, with a fixture-owned process. | ||
| const native = createRequire(new URL("../../../../core/package.json", import.meta.url))("@lydell/node-pty") as { | ||
| spawn: ( | ||
| file: string, | ||
| args: string[], | ||
| options: { cols: number; rows: number; cwd: string }, | ||
| ) => { | ||
| pid: number | ||
| write: (data: string) => void | ||
| resize: (cols: number, rows: number) => void | ||
| kill: () => void | ||
| onData: (handler: (data: string) => void) => { dispose: () => void } | ||
| onExit: (handler: () => void) => { dispose: () => void } | ||
| } | ||
| } |
Comment on lines
+342
to
+346
| const buffer = term.buffer.active | ||
| return Array.from( | ||
| { length: term.rows }, | ||
| (_, i) => buffer.getLine(buffer.length - term.rows + i)?.translateToString(true) ?? "", | ||
| ) |
Hona
enabled auto-merge (squash)
September 2, 2026 08:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SerializeAddonrecorded every style change by callingbuffer.getLine(row).getCell(col)again. Inghostty-web,getLinecopies the whole row out of WASM, so a colored log paid one extra row copy per SGR change. The cell already passed to_nextCellis an immutable snapshot of the same data, so reuse it.Benchmark
Closing a session tab whose terminal holds a full colored scrollback. Baseline
335e4ca56f(v2), candidate618f154a39. 20 samples per case per build, serial ABBA blocks of 5, frozen production Vite bundles (00CB8784…/8739C1BF…), Windows 11 / i7-12800H, Playwright 1.59.1 Chromium, 1440x900 viewport.TaskDurationTerminal(172x11 cells) receiving a 12,000-line, 1,497,808-byte colored build-log fixture from a real PowerShell ConPTY (@lydell/node-pty) via a Playwright WebSocket route, without producer throttling. The app's 10,000-line setting retained 4,293 scrollback rows after the initial 80-column resize; both builds retained the same rows.SerializeAddon. Session metadata is mocked, transcript empty.896afd33…), and the new unit test round-trips colors/styles through a real Ghostty terminal while asserting bounded row reads.Controls measured in the same runs (unchanged code path; tails were noisy on this machine and are not attributed to the change):
Not claimed
fillTextcalls during hidden output on baseline). Pausing that needs a Ghostty visibility API and is out of scope here.Benchmark harness
packages/app/e2e/performance/terminals/adds a manual, Windows-only Playwright benchmark that owns its ConPTY process and preview server, checks completion in Ghostty plus completed write callbacks, and verifies input, focus, resize, no PTY deletion, and native process survival after UI teardown. It is excluded from normal discovery; see its README.Rendering of the fixture is byte-identical in both builds.