Skip to content

fix(ui): reuse nowrap geometry across terminal resizes - #1200

Merged
benvinegar merged 1 commit into
mainfrom
fix/nowrap-resize-geometry-memory
Oct 18, 2026
Merged

benvinegar merged 1 commit into
mainfrom
fix/nowrap-resize-geometry-memory

Conversation

@benvinegar

@benvinegar benvinegar commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

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

  • Normalize only the geometry cache-key width when wrapping is disabled and there are no visible inline notes.
  • Keep the actual live width for measurement and rendering; preserve layout, theme, line-number, add-note-column, tab, gap, expansion/source, and visible-hunk-scope invalidation.
  • Keep width-sensitive wrapped and inline-note geometry in the existing bounded cache slots.
  • Add split/unified fresh-measurement parity, width-sensitive invalidation, expansion/source, mounted-resize, and real-PTY navigation/wrapping regressions; include a patch Changeset.

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.

Final resize sample Before Fixed source
Retained JavaScript heap 5.75 GB 2.53 GB

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 using bun 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 typecheck and bun run deps:check — passed.
  • bun run packages/hunk/src/main.tsx diff --help — passed.
  • Targeted oxfmt --check, oxlint --deny-warnings, and git 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 main 483ea47d with 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 /var versus /private/var cwd 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-linux script interface. 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, after bun 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.ts

Result: 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, and w toggles. 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.

@vercel

vercel Bot commented Oct 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
hunk-web Ignored Ignored Oct 18, 2026 12:46am UTC

Request Review

@greptile-apps

greptile-apps Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Reuses cached row bounds and lazy rows across terminal resizes when wrapping is off and no inline notes are visible.

  • Diff reviews reuse note-free, unwrapped geometry across terminal resizes.

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

@squad-robert

Copy link
Copy Markdown

Independently tested the resize-memory behavior on Linux with Bun 1.4.2, comparing base 483ea47d3db4c8e3d06c5be191ea7c1fe7051d8a with PR head 565e8bb36ad1785e938d6c37931a2f105a616547 using frozen-lockfile dependencies.

The excess resize-related retained memory reproduces on the base, and the PR substantially reduces it on a generated review.

Before/after memory measurements

Used the existing, unchanged benchmarks/resize-memory.ts: mounted headless AppHost, generated 120-line source files, split layout, wrapping off, no inline notes, 32 terminal rows. Each run starts at 200 columns and repeats widths 200,160,240,200. The benchmark forces GC before each memory sample. Base/head workloads ran sequentially in separate processes.

Workload Base final retained JS heap PR final retained JS heap Reduction
1,000 files, 12 resizes (median of 3 independent runs per revision) 340.8 MiB 165.7 MiB 51.4%
2,000 files, 12 resizes (one run per revision) 655.6 MiB 305.3 MiB 53.4%
1,000 files, 48 resizes (one run per revision) 343.2 MiB 168.0 MiB 51.0%

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.json

For the other scenarios, use --file-count 2000, or --cycles 12 for 48 resizes.

Mechanism and correctness checks

A separate scratch probe exercised split/unified geometry across widths 160,240,200,1,0,200, starting at 200:

  • Base: note-free nowrap geometry was replaced on each width transition.
  • PR: the initial geometry object was reused for all six widths.
  • Wrapped geometry remained width-sensitive on both revisions.
  • On both revisions, cached geometry matched fresh-file-identity measurements at every tested width.

Additional PR-head checks:

  • Geometry + responsive-rendering tests: 32 pass.
  • Review conformance: 118 pass.
  • Full PTY integration suite: 262 pass, 5 skip, 0 fail, including the new actual-diff resize/navigation/wrap regression. An initial attempt hit my 300-second command timeout; a rerun completed in 343.5 seconds.
  • TTY smoke: 11 pass, 0 fail.
  • Typecheck, dependency checks, diff --help, and git diff --check: passed.
  • Default suite: 5,082 pass, 44 skip, 12 fail. All 12 failing names also reproduced on pristine base using bun test packages/hunk/src/extensions/manage/npm.test.ts test/cli/npm-extension-install.test.ts (1 pass, 12 fail), with the same npm tarball fixture error at test/helpers/npmRegistryTest.ts:67: JSON.parse(pack.stdout.toString())[0] is undefined. These are not PR-only failures in this environment.

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.

@benvinegar
benvinegar merged commit 4051955 into main Oct 18, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants