fix: clear scrolled row cells in wasm patch#180
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0edb141664
ℹ️ 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".
0edb141 to
9062250
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 906225096b
ℹ️ 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".
Change-Id: Ifa0250b9cb8a386486651f67342363ab429b11b3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
9062250 to
d101961
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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 #138.
This PR updates the Ghostty WASM integration patch so
cursorDownScroll()always clears cells for a newly grown row, even when the cursor background is the default value afterESC[0m. It also resets the recycled row's row-level metadata after clearing while preserving the backing cell offset and dirty state; the later non-default-background fill path remains conditional and intact.It adds focused regression coverage that repeatedly writes SGR/truecolor/256-color reset-heavy output, includes a soft-wrap metadata seed, scrolls enough to exercise row growth/reuse, and asserts that viewport rows do not expose stale merged cell data or stale blank-row wrap metadata.
Coordination note: #134 is the related draft/reference PR for the same issue. This branch keeps the replacement diff minimal and links that prior work to avoid duplicate-review confusion.
Validation
bun install— passed after installing local dependencies.bun run build:wasm— passed; generated local ignoredghostty-vt.wasmfor tests.bun test lib/viewport-corruption.test.ts— passed: 1 pass, 0 fail, 3723 expects.bun run fmt— passed.bun run lint— passed.bun run typecheck— passed.bun test— passed: 332 pass, 0 fail, 4487 expects.bun run build— passed.git -C ghostty apply --check ../patches/ghostty-wasm-api.patch— passed.git status --short/git status --short ghostty— clean tracked tree and clean Ghostty submodule.d101961a86— passed: fmt, lint, type check, test, build.Workflow verifier findings
The issue implementation workflow converged after 2 verifier runs. Final verifier summary:
Post-workflow Codex review found two in-scope issues, both addressed:
R00L000) and matched with/R\d{2}L\d{3}/g.Latest Codex result: “Didn't find any major issues.”
Non-blocking finding:
Dogfooding
The workflow dogfooded the interactive demo with SGR-heavy scrolling output after installing demo dependencies and running the demo on an alternate port. It captured screenshots and recordings under
/tmp/issue138-dogfood/with no browser console errors and no visible stale row merges.Screenshot upload to GitHub was attempted with
gh image, but this headless workspace has no browseruser_sessioncookie andGH_SESSION_TOKENis unset, so the images could not be embedded in this PR body from the worker environment.📋 Implementation Plan
Implementation Plan: #138
Issue summary and verified context
coder/ghostty-web#138— "Stale cell data visible on new rows after scrolling with default cursor style". State: open. Labels:triage:done,bug,accepted.ghostty/src/terminal/Screen.zigcursorDownScroll()clears newly grown row cells only whenself.cursor.style.bg_color != .none. AfterESC[0m, the cursor background is default/none, so row memory produced bypages.grow()can retain stale cells when the row is not fully overwritten.coder/ghostty-web#134— open, mergeable, review required. It contains the same fix direction plus draft regression tests. Treat this as the canonical reference, not as unrelated prior art.ghostty/.patches/ghostty-wasm-api.patch; builds apply this patch to the submodule, compile WASM, then revert the submodule patch.ghostty-vt.wasmis generated in the repo root bybun run build:wasm/./scripts/build-wasm.shand is currently ignored by git. Do not commit generated WASM unless the tracking policy changes.build:wasm,build:lib,build,test,typecheck,fmt,lint, anddemo:dev.Scope boundaries
In scope:
cursorDownScroll()clear their cells regardless of cursor background style.Out of scope:
needs-triageissue if no match exists, then return to Stale cell data visible on new rows after scrolling with default cursor style #138.Coordination with existing PR #134
At implementation start, re-check PR fix: always clear new row cells during scroll to prevent stale data #134 status and diff:
If PR fix: always clear new row cells during scroll to prevent stale data #134 has already merged by implementation time, do not open a new PR for the same fix. Pull latest
main, validate that Stale cell data visible on new rows after scrolling with default cursor style #138 is fixed, and report the result.Preferred path while PR fix: always clear new row cells during scroll to prevent stale data #134 is open: use it as the reference implementation and either help land/update it if the implementer has permission, or reproduce its minimal fix on a repo-owned branch if maintainers need a fresh PR.
If PR fix: always clear new row cells during scroll to prevent stale data #134 is closed/stale or a replacement/superseding PR is opened, link both the issue and the prior PR clearly:
Fixes #138Incorporates/supersedes #134or equivalent wordingDo not create an uncoordinated duplicate PR while fix: always clear new row cells during scroll to prevent stale data #134 remains active.
Implementation steps
Phase 1 — Preflight and clean build inputs
Install dependencies if needed:
Ensure the Ghostty submodule starts clean before applying the WASM patch:
Confirm the Zig toolchain path. Prefer a non-global invocation when the
zigshim is not configured:mise exec zig@0.15.2 -- zig versionIf
zigis already configured in the environment, directbun run build:wasmis fine. Otherwise, run build commands throughmise exec zig@0.15.2 -- ....Phase 2 — Add a focused failing regression test first
Add one focused production regression test file, preferably adapted from PR fix: always clear new row cells during scroll to prevent stale data #134's
lib/viewport-corruption.test.ts.The test should explicitly exercise all bug triggers:
ESC[0mresets;Core assertion: no viewport row should contain markers from more than one logical output line/run. A marker pattern such as
R02L05lets the test catch stale horizontal merges deterministically:Also keep a small consistency assertion that
getViewport()andgetLine()agree for visible rows after scrolling, if that is already present in the adapted PR fix: always clear new row cells during scroll to prevent stale data #134 test.Run the new test before applying the patch to confirm it is red against a known baseline WASM. Because
ghostty-vt.wasmis ignored and can be stale, rebuild the baseline first, then run the targeted test:If validating red/green after partially applying the fix, revert the patch change, rebuild WASM, confirm the test fails, then reapply the patch and rebuild.
If the adapted
viewport-corruptiontest does not fail before the fix or does not clearly cover theESC[0mdefault-background path, do not add both PR fix: always clear new row cells during scroll to prevent stale data #134 test files wholesale. Instead, extract the smallest failing case/payload from PR fix: always clear new row cells during scroll to prevent stale data #134'slib/viewport-row-merge.test.tsinto the focused test file.Phase 3 — Patch
cursorDownScroll()throughpatches/ghostty-wasm-api.patchAdd a
src/terminal/Screen.zigdiff block topatches/ghostty-wasm-api.patch.Minimal intended source change: make the new-row
clearCells(...)call unconditional incursorDownScroll().Preserve the later conditional background fill/memset for
self.cursor.style.bg_color != .none; that behavior ensures styled rows still receive the active background.The patch shape should be equivalent to:
Keep the change surgical. Do not edit renderer code, terminal APIs, scrollback data structures, or unrelated Ghostty source.
Phase 4 — Rebuild generated WASM locally and keep the submodule clean
Rebuild the local WASM artifact from the updated patch:
mise exec zig@0.15.2 -- bun run build:wasmIf the environment already has Zig configured, this can be shortened to:
Verify the build script reverted the temporary patch inside
ghostty/and did not leave the submodule dirty:Expected tracked changes after implementation should be limited to:
patches/ghostty-wasm-api.patchlib/ghostty-vt.wasmis expected to exist locally for tests but should remain ignored/untracked.Validation and quality gates
Automated checks
Run these gates in order and fix failures before claiming completion:
Targeted regression test:
bun test lib/viewport-corruption.test.tsBroader test suite:
bun testStatic checks:
Build checks:
If plain
zigis not configured, use the mise-safe form instead:mise exec zig@0.15.2 -- bun run buildFinal cleanliness check:
Notes:
bun run buildcleansdist/, rebuilds WASM, builds the library, and copies WASM intodist/.bun testcannot find/ghostty-vt.wasm, runbun run build:wasmfirst and ensure the generated rootghostty-vt.wasmexists.cd ghostty && git checkout -- . && git clean -fd, then retry the build.Acceptance criteria
The implementation is acceptable when all of these are true:
patches/ghostty-wasm-api.patchapplies a minimalScreen.zigchange that clears newly grown rows unconditionally incursorDownScroll().ESC[0mreset-heavy scrolling output.Fixes #138.Dogfooding and reviewable evidence
Because this bug is visual and transient, automated tests are necessary but not sufficient for reviewer confidence. Produce visible evidence in addition to logs.
Start the combined demo/PTY + Vite dev server:
If that workflow is unavailable in the local environment, use the demo package equivalent:
Root
bun run devstarts Vite only; use it only for supplemental static-page checks such ashttp://localhost:8000/demo/colors-demo.html, not for full interactive PTY dogfooding.Open the interactive demo in a browser:
Generate SGR-heavy scrolling output inside the terminal:
If the script path is not available from the shell's current directory, locate/copy an equivalent ANSI/256-color/truecolor stress payload that repeatedly emits
ESC[0mand scrolls.Visually inspect blank/newly created rows during and after scrolling. Expected result: no horizontal stale text fragments or row merges appear.
Capture reviewable evidence:
Attach screenshots/recording to the PR or final report. If using Mux tooling, use
attach_filefor screenshots so reviewers can inspect exact evidence.PR/report guidance for the implementer
Advisor review status
viewport-row-merge.test.tsif the primary test does not fail or does not coverESC[0m.Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh