fix: stabilize WASM viewport row reads#177
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b3e85ff1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0b3e85f to
f2cb4c1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2cb4c1156
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f2cb4c1 to
d117e59
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d117e5938e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d117e59 to
d6c359c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c359c2ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d6c359c to
9940237
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Change-Id: I80440157e400c116abfd4131681fce48cbb741b7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
9940237 to
f2e4a9f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Fixes #139.
scrollbackline-count budget to Ghostty's byte-basedmax_scrollbackinside the WASM wrapper.RenderStatecached row data instead of resolving each viewport row independently, keeping rows coherent across Ghostty page boundaries.Validation
bun run build:wasm— passed; patch applies and rebuiltghostty-vt.wasmlocally for tests/build.bun test lib/terminal.test.ts -t "keeps viewport coherent"— passed.bun run fmt && bun run lint && bun run typecheck && bun test && bun run build— passed.git -C ghostty apply --check ../patches/ghostty-wasm-api.patch— passed.git diff --check origin/main...HEAD— passed.Verifier findings
The issue implementation workflow converged after one verifier run with no P1-P3 findings and no non-blocking findings reported.
Dogfooding
The workflow verifier also dogfooded the browser demo with controlled 130x39 terminal output, scrolling through ANSI-heavy
rep=/line=rows. It reported coherent bottom/history/back-to-bottom views andscrollbackLength=913.📋 Implementation Plan
Implementation Plan — #139
Goal
Fix GitHub issue #139:
getViewport()/getLine()can return corrupted rows when the visible viewport crosses Ghostty's internal page boundaries, and the JSscrollbackline-count option is currently handed to Ghostty as a byte budget. Keep the change minimal, reviewable, and limited to #139.Verified context
triage:done,bug, andaccepted. The issue describes two coupled problems:renderStateGetViewportin the WASM API patch resolves every viewport row independently withpages.pin(.active), which can become inconsistent across internal page boundaries.scrollback/scrollbackLimitrepresents lines, but GhosttyTerminal.init(.max_scrollback)expects bytes.RenderState.row_datarow pins and convert scrollback lines to bytes. Its implementation direction is useful, but its two added repro test files are large/exploratory; prefer a smaller targeted regression test footprint.lib/terminal.tsdefaultsscrollbackto10000and passes it throughbuildWasmConfig()asGhosttyTerminalConfig.scrollbackLimit.lib/ghostty.tswritesconfig.scrollbackLimit ?? 10000directly into the WASM config struct asscrollback_limit.patches/ghostty-wasm-api.patchdefinesnewWithConfig()and passesscrollback_limitdirectly toTerminal.init(... .max_scrollback = scrollback_limit ...).ghostty/src/terminal/Screen.zigdocumentsmax_scrollbackas bytes.ghostty/src/terminal/render.zigbuildsRenderState.row_data.items(.pin)using a viewportrowIteratorand stores each row pin duringRenderState.update().patches/ghostty-wasm-api.patchcurrently hasrenderStateGetViewport()bypassing that row-pin cache and callingpages.pin(.{ .active = .{ .y = ... } })per row.lib/interfaces.tsdocumentsscrollbackdefault as1000whileTerminaluses10000. I searched existing issues and opened Document actual default for ITerminalOptions.scrollback #174 withneeds-triage. Do not fold that docs cleanup into getViewport() returns corrupted data when viewport spans multiple pages #139 unless a maintainer explicitly asks.Scope boundaries
In scope:
renderStateGetViewport()row resolution inpatches/ghostty-wasm-api.patch.Terminal.init().ghostty-vt.wasmafter patch changes.Out of scope:
scrollback: 0semantics. The current wrapper has special handling for0, while Ghostty's byte-level docs have their own meaning; do not characterize, test, or change that behavior in getViewport() returns corrupted data when viewport spans multiple pages #139 unless it becomes a direct blocker, and file a separate issue if a semantic mismatch is confirmed.Recommended implementation phases
Phase 1 — Add focused regression tests first
Add minimal regression coverage in existing test files, preferably
lib/terminal.test.tsnear the existingTerminal Config/ render-state tests. ReusecreateIsolatedTerminal()fromlib/test-helpers.ts; always dispose terminals infinallyblocks.Target tests:
Viewport page-boundary stability
cols: 130,rows: 39,scrollback: 10000.rep=03 line=041plus color/style escape sequences rather than copying PR fix: use cached row pins for WASM viewport rendering #133's large repro fixture.term.wasmTerm!.update()before reading viewport data.\r\n, or explicitly account for the final blank row when computing expected viewport rows.getViewport()rows decode to the expected final visible line markers, with no horizontal merging from unrelated lines.getScrollbackLength()never decreases during append-only output before the configured scrollback limit should be reached.getLine(row)and the correspondinggetViewport()slice agree for sampled rows. Do not treat this as an independent correctness oracle becausegetLine()slicesgetViewport().Scrollback line-count budget is not treated as raw bytes
cols: 130,rows: 39,scrollback: 10000with repeated output, or a second faster case with a lower custom limit if it reliably fails before the fix.Quality gate before implementation:
Expected result before the fix: at least one new regression assertion should fail against the current
ghostty-vt.wasm. If it does not fail, tighten only the generator/threshold within #139's described reproduction dimensions before changing production code.Implementation workflow for the Zig patch phases:
patches/ghostty-wasm-api.patchto theghostty/submodule, editingghostty/src/terminal/c/terminal.zig, then regenerating or refreshing the patch from that submodule diff. This avoids hand-editing large patch hunks incorrectly.git -C ghostty apply --check ../patches/ghostty-wasm-api.patch(or rely onscripts/build-wasm.sh, which performs this check and fails early).ghostty/submodule is returned to a clean state.Phase 2 — Fix scrollback unit conversion in the WASM API patch
Update
patches/ghostty-wasm-api.patchin the generated Zig wrapper (src/terminal/c/terminal.ziginside the patch):newWithConfig(), treatcfg.scrollback_limitas a JS line count and convert finite values to bytes before passing.max_scrollbacktoTerminal.init().Preferred shape:
Implementation notes:
muloverflow fallback tostd.math.maxInt(usize)or a documented equivalent). The goal is fail-fast in tests without introducing user-visible crashes for valid inputs.cfg.scrollback_limit == 0branch as an implementation detail; do not add tests or documentation for0semantics in getViewport() returns corrupted data when viewport spans multiple pages #139.lib/ghostty.tsto say the WASM wrapper converts lines to bytes internally.Phase 3 — Fix viewport row resolution in
renderStateGetViewport()Update only
renderStateGetViewport()unless tests prove another row-coordinate API must change for #139.Minimal intended behavior:
pages.pin(.active)calls.const row_pins = rs.row_data.items(.pin);, which is populated byrenderStateUpdate()/ nativeRenderState.update().y, userow_pins[y]to obtain cells and page style/hyperlink/grapheme metadata.-1ifbuf_size < rows * colsas today.row_pins.len < rows, fill missing rows with default cells and add a debug assertion if practical. Avoid returning-1here unless the TypeScript side is also changed to prevent stale cell-pool reuse, which would broaden the fix.Important update-order assumption:
lib/ghostty.ts#getLine()already callsupdate()beforegetViewport().getViewport()should callterm.wasmTerm!.update()first.update()calls togetViewport()unless required; that would change a hot-path performance contract outside the issue's root cause.Phase 4 — Rebuild committed WASM artifact
Because
ghostty-vt.wasmis committed, patch changes are not complete until the binary is rebuilt.Commands:
If Zig is unavailable, use the repo-documented/mise path used by triage:
PATH="$(mise where zig@0.15.2):$PATH" ./scripts/build-wasm.shAfter rebuilding:
Acceptance for this phase:
ghostty-vt.wasm, and the targeted test edit(s).ghostty/submodule worktree should be clean afterscripts/build-wasm.shreverts the patch.Phase 5 — Automated validation
Run targeted checks while iterating, then the full project gate before declaring done.
Targeted checks:
Full validation gate:
Notes:
bun run buildinvokesbuild:wasm, so it may rebuild the WASM artifact again; verify no unexpected binary churn afterward.bun testhangs after reporting completion, capture the pass/fail summary and note the known hang behavior rather than claiming a clean exit that did not occur.Dogfooding / reviewable evidence
Because this bug affects the browser-visible terminal viewport and scrollback behavior, dogfooding should produce both automated logs and visual evidence.
Setup (follow the repo's demo/PTY instructions if they change; current repo instructions use this shape):
Browser exercise:
http://localhost:8000/demo/usingagent-browseror a normal browser.cols ~= 130,rows ~= 39) if feasible.rep=/line=markers remain coherent.attach_fileso reviewers can inspect the exact dogfood state.If the environment cannot provide browser screenshots/video, document the blocker explicitly and include the automated regression logs plus any terminal output snapshots that demonstrate the same invariants.
Acceptance criteria
renderStateGetViewport()uses cachedRenderState.row_datarow pins instead of independentpages.pin(.active)row resolution.scrollbackline count is converted to a Ghostty byte budget before being passed as.max_scrollback.getScrollbackLength()does not unexpectedly decrease under the issue's append-only reproduction before the configured line budget is reached.getViewport()andgetLine()return coherent, matching visible rows across page-boundary scenarios at the reported dimensions.ghostty-vt.wasmis rebuilt and included with the patch changes.Risks and mitigations
cols=130,rows=39,scrollback=10000) and the triage repro pattern. Keep assertions invariant-based rather than exact-byte-layout-based.update(). Tests and hot paths should maintain the current update-before-read ordering; add defensive checks in Zig for stale cache rather than silently reading undefined pins.renderStateGetGrapheme(),isRowWrapped(), andgetHyperlinkUri()also resolve rows, but PR fix: use cached row pins for WASM viewport rendering #133 and getViewport() returns corrupted data when viewport spans multiple pages #139 specifically targetrenderStateGetViewport(). Do not expand to them unless a getViewport() returns corrupted data when viewport spans multiple pages #139 regression test proves viewport correctness still depends on them; otherwise file/follow a separate issue.Advisor review status
Approved after two advisor review rounds. Round 1 requested refinements around test oracles, deterministic non-wrapping output, patch-edit workflow,
scrollback: 0scope, stale row-pin fallback behavior, and dogfood setup wording. The plan was updated accordingly, and round 2 approved it as implementation-ready.Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh