Repository navigation
fix(ui): reuse nowrap geometry across terminal resizes - #1200
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryReuses cached row bounds and lazy rows across terminal resizes when wrapping is off and no inline notes are visible.
No actionable issues found. Code paths and test setup were reviewed; tests were not run during this review. The whitespace check passed. Reviews (1) · Last reviewed commit: "fix(ui): reuse nowrap geometry across te..." · Reviewed by Greptile |
|
Independently tested the resize-memory behavior on Linux with Bun 1.4.2, comparing base The excess resize-related retained memory reproduces on the base, and the PR substantially reduces it on a generated review. Before/after memory measurementsUsed the existing, unchanged
For the repeated 1,000-file workload, initial retained heap was approximately 156 MiB on both revisions. Resize-induced retained heap growth was approximately 185 MiB on base versus 10 MiB on the PR. Median final RSS was 1,002 MiB versus 512 MiB (49% lower), though RSS is more allocator/environment-sensitive than post-GC JS heap. The longer sequence plateaus on both revisions in this reproduction: this demonstrates excess resize-related retention, not an unbounded leak. Reproduction command, run in each checkout (use distinct output paths): bun run benchmarks/resize-memory.ts \
--file-count 1000 \
--lines-per-file 120 \
--height 32 \
--widths 200,160,240,200 \
--cycles 3 \
--json-out /tmp/resize-memory.jsonFor the other scenarios, use Mechanism and correctness checksA separate scratch probe exercised split/unified geometry across widths
Additional PR-head checks:
Scope: these results independently support a roughly 51–53% retained-heap reduction for this synthetic workload. They do not reproduce the exact 56% result for the unavailable private 6,461-file input, and are headless mounted-app memory measurements rather than interactive-process memory measurements. Real-PTY behavior was tested separately. No actionable defect found in the behavior tested; no submitted source/tests/configuration were changed. |
Problem
Large reviews allocate substantial row geometry. Resizing a note-free review with line wrapping disabled replaced that geometry even though its row heights and row stream do not depend on terminal width. This needlessly rebuilt row bounds, lookup maps, and lazily materialized row plans.
Changes
No intended visual or interaction change: split columns still repaint at their live width, hunk navigation still crosses files, and wrapping still reflows. This stays in the existing terminal geometry cache rather than adding another rendering path or shared-review semantic model.
Measured impact
During the investigation, the same 6,461-file private diff was rendered headlessly with the installed Bun 1.4.2 runtime, initially at 200×32, then through three cycles of 160/240/200-column resizes. Samples followed full GC.
That is approximately 56% less retained heap in this reproduction. These measurements used the investigation checkout before porting the fix to current main, not a fresh benchmark of this PR's current-main base. The private input and captured incident payloads are not included. This is not a claim that the entire original long-running process's memory footprint has been explained or fixed.
Verification
Based on current
main(483ea47d) in an isolated worktree usingbun install --frozen-lockfile; no local dependency upgrades are included. Tested on macOS with Bun 1.4.3.bun test packages/hunk/src/ui/diff/diffSectionGeometry.test.ts packages/hunk/src/ui/AppHost.responsive.test.tsx— 32 passed, 0 failed.bun test ./test/review-conformance— 118 passed, 0 failed.bun run typecheckandbun run deps:check— passed.bun run packages/hunk/src/main.tsx diff --help— passed.oxfmt --check,oxlint --deny-warnings, andgit diff --check— passed; commit hooks also passed.bun run test— 5,031 passed, 55 skipped, 52 failed. All 52 failing test names were reproduced on pristine main483ea47dwith its frozen dependencies; no PR-only failing names. They span extension loading/resolution, a document observer, and history CLI tests. The local global-extension installation also produces missing-module errors. These baseline failures are not fixed here.bun run test:integration— still running at PR creation; the new actual-diff nowrap-resize regression passed. One status integration case has failed on a macOS/varversus/private/varcwd assertion. That case also fails on pristine main when run alone, but at an earlier terminal-wait checkpoint, so it is not counted as an exact failure reproduction. Final full-suite results will be added when available.bun run test:tty-smoke— 0 passed, 11 skipped: this macOS host lacks the required GNU/util-linuxscriptinterface. Do not count this suite as rendering evidence; the new real-PTY regression did execute.Pristine-main failure reproduction
In a separate, unmodified checkout of
483ea47d, afterbun install --frozen-lockfile, ran:bun test \ packages/hunk/src/ui/AppHost.extension-dialogs.test.tsx \ packages/hunk/src/ui/AppHost.workspace.test.tsx \ packages/hunk/src/ui/AppHost.extension-sidebar.test.tsx \ packages/hunk/src/ui/AppHost.extension-navigation.test.tsx \ packages/hunk/src/ui/AppHost.reload.test.tsx \ packages/hunk/src/extensions/startup.test.ts \ packages/hunk/src/extensions/installedPackageResolution.test.ts \ packages/hunk/src/extensions/hostRuntimeModules.test.ts \ packages/hunk/src/app/documents/filesystemSource.observe.test.ts \ test/cli/log.test.tsResult: 48 passed, 1 skipped, the same 52 failures, across 101 tests in 10 files.
The new PTY test uses a generated Git diff, fixed split layout, no sidebar, 24 rows, 60–180 columns,
[/]cross-file navigation, andwtoggles. There is no before/after visual change intended and no media attached; the actual-diff PTY regression verifies live repainting and wrapping. Linux and Windows were not tested locally.Registration retry churn is a separate, independently mergeable fix in #1199.