From 10ebbb9315d5cfaad795a9cda12cb7775edb14f0 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Sun, 18 Oct 2026 18:10:53 -0400 Subject: [PATCH 1/2] fix(ui): keep copy-selection rows lazy during navigation --- .changeset/lazy-navigation-copy-rows.md | 5 ++++ benchmarks/README.md | 2 +- benchmarks/geometry-memory.ts | 20 ++++++++++--- .../hunk/src/ui/diff/diffSectionGeometry.ts | 9 +++++- .../src/ui/lib/reviewVerticalStops.test.ts | 25 ++++++++++++++++ .../hunk/src/ui/lib/reviewVerticalStops.ts | 29 +++++++++---------- test/pty/notes.test.ts | 13 ++++++++- 7 files changed, 81 insertions(+), 22 deletions(-) create mode 100644 .changeset/lazy-navigation-copy-rows.md diff --git a/.changeset/lazy-navigation-copy-rows.md b/.changeset/lazy-navigation-copy-rows.md new file mode 100644 index 000000000..5c59a230f --- /dev/null +++ b/.changeset/lazy-navigation-copy-rows.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Reduce memory use in large reviews by keeping copy-selection row content lazy during keyboard navigation. diff --git a/benchmarks/README.md b/benchmarks/README.md index b0f2faa54..a6a028566 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -72,7 +72,7 @@ bun run bench:competitors - `huge-stream.ts` — opt-in huge tier (`--include-huge` or `HUNK_BENCH_INCLUDE_HUGE=1`): cold first frame, scroll-tick and hunk-navigation latency, and memory ceilings on ~1k files / 300k+ diff lines plus one giant ~50k-line file. - `large-stream-profile.ts` — optional local profiler for the main pure planning stages behind the large split-stream benchmark. - `memory.ts` — optional local RSS/heap profiler after fixture loading, planning, first frame, and next-hunk navigation. -- `geometry-memory.ts` — optional local retained-memory profiler for all-files section geometry, including JSC-native heap metrics and giant-file lazy planned-row materialization latency used by first copy selection. +- `geometry-memory.ts` — optional local retained-memory profiler for all-files section geometry, including JSC-native heap metrics, navigation projection, and giant-file first-copy latency. Copy-content heap growth is measured from the post-navigation sample; ordinary navigation should not materialize lazy copy-selection rows. - `navigation-memory.ts` — optional local retained-memory profiler for repeated hunk navigation through a mounted review stream. - `resize-memory.ts` — optional local retained-memory profiler for repeated terminal-width changes through a mounted review stream; this targets geometry-cache retention across resize variants. - `competitors.ts` — optional local informational comparisons against `git diff --no-ext-diff`, `delta`, `difftastic`, and `diff-so-fancy` when installed. diff --git a/benchmarks/geometry-memory.ts b/benchmarks/geometry-memory.ts index 53f402157..3e4d177ec 100644 --- a/benchmarks/geometry-memory.ts +++ b/benchmarks/geometry-memory.ts @@ -3,6 +3,8 @@ import { heapStats } from "bun:jsc"; import { performance } from "node:perf_hooks"; import { measureDiffSectionGeometry } from "../packages/hunk/src/ui/diff/diffSectionGeometry"; import { resolveTheme } from "../packages/hunk/src/ui/themes"; +import { buildLineCursors } from "../packages/hunk/src/ui/lib/lineCursors"; +import { buildReviewVerticalStops } from "../packages/hunk/src/ui/lib/reviewVerticalStops"; import { createGiantSingleDiffFile, createLargeSplitStreamBootstrap, @@ -139,8 +141,18 @@ const geometryMs = performance.now() - geometryStart; const afterGeometry = sampleMemory(options); printMemory("after_geometry", afterGeometry); -// Materialize the lazy planned-row streams as an upper-bound diagnostic for copy-selection style -// consumers. Normal review scrolling/navigation should not pay this retained-memory cost. +// Exercise real navigation before first copy so eager row-content materialization appears in +// retained heap rather than hiding in the copy-only upper bound. Keep both projections live. +const navigationStart = performance.now(); +const cursors = buildLineCursors(bootstrap.changeset.files, geometries); +const stops = buildReviewVerticalStops(bootstrap.changeset.files, geometries, cursors); +const navigationMs = performance.now() - navigationStart; +const afterNavigation = sampleMemory(options); +printMemory("after_navigation", afterNavigation); +console.log(`METRIC navigation_ms=${navigationMs.toFixed(2)}`); +console.log(`METRIC navigation_stops=${stops.length}`); + +// Materialize lazy row streams as an upper-bound diagnostic for copy-selection consumers. const materializeStart = performance.now(); for (const geometry of geometries) { materializedPlannedRows += geometry.plannedRows.length; @@ -162,12 +174,12 @@ console.log(`METRIC materialize_planned_rows_ms=${materializeMs.toFixed(2)}`); console.log(`METRIC materialized_planned_rows=${materializedPlannedRows}`); console.log( `METRIC materialized_planned_rows_jsc_heap_size_growth_bytes=${ - afterMaterializedRows.jscHeapSizeBytes - afterGeometry.jscHeapSizeBytes + afterMaterializedRows.jscHeapSizeBytes - afterNavigation.jscHeapSizeBytes }`, ); console.log( `METRIC materialized_planned_rows_rss_growth_bytes=${ - afterMaterializedRows.rssBytes - afterGeometry.rssBytes + afterMaterializedRows.rssBytes - afterNavigation.rssBytes }`, ); console.log(`METRIC files=${options.fileCount}`); diff --git a/packages/hunk/src/ui/diff/diffSectionGeometry.ts b/packages/hunk/src/ui/diff/diffSectionGeometry.ts index a71be4bf6..4d68f0c51 100644 --- a/packages/hunk/src/ui/diff/diffSectionGeometry.ts +++ b/packages/hunk/src/ui/diff/diffSectionGeometry.ts @@ -28,6 +28,8 @@ export interface DiffSectionRowBounds extends VerticalBounds { stableKeys: string[]; /** Exact collapsed gap that produced this synthesized source row. */ expandedGapKey?: string; + /** Selectable semantic note at this row; drafts and decoration-only cards omit it. */ + noteTarget?: { noteId: string; hunkIndex: number }; } /** @@ -374,8 +376,13 @@ export function measureDiffSectionGeometry( row.stableKey, ...(row.kind === "diff-row" ? (row.stableAliasKeys ?? []) : []), ]; - const rowBoundsEntry = { + const noteId = + row.kind === "inline-note" && row.note.source !== "draft" + ? row.note.thread?.noteId + : undefined; + const rowBoundsEntry: DiffSectionRowBounds = { key: row.key, + ...(noteId ? { noteTarget: { noteId, hunkIndex: row.hunkIndex } } : {}), stableKey: row.stableKey, stableKeys, ...(row.kind === "diff-row" && diff --git a/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts b/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts index cd509120e..6ec3be80d 100644 --- a/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts +++ b/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts @@ -21,6 +21,26 @@ function stopLabel(stop: ReturnType[number]) { } describe("review vertical stops", () => { + for (const layout of ["split", "unified"] as const) { + test(`${layout} note-free navigation leaves copy-selection rows lazy`, () => { + const file = createTestDiffFile({ + before: lines("one", "two"), + after: lines("ONE", "two"), + context: 1, + }); + const geometry = measureDiffSectionGeometry(file, layout, true, theme, [], 80); + const cursors = buildLineCursors([file], [geometry]); + Object.defineProperty(geometry, "plannedRows", { + get() { + throw new Error("Navigation must not materialize copy-selection rows"); + }, + }); + + const stops = buildReviewVerticalStops([file], [geometry], cursors); + expect(stops).toEqual(cursors.map((cursor) => ({ kind: "line", cursor }))); + }); + } + test("places semantic roots and replies after their rendered source line", () => { const file = createTestDiffFile({ id: "alpha", @@ -53,6 +73,11 @@ describe("review vertical stops", () => { ]; const geometry = measureDiffSectionGeometry(file, "unified", true, theme, notes, 80); const cursors = buildLineCursors([file], [geometry]); + Object.defineProperty(geometry, "plannedRows", { + get() { + throw new Error("Navigation must not materialize copy-selection rows"); + }, + }); const stops = buildReviewVerticalStops([file], [geometry], cursors); const labels = stops.map(stopLabel); const changedLine = labels.indexOf("line:new:3"); diff --git a/packages/hunk/src/ui/lib/reviewVerticalStops.ts b/packages/hunk/src/ui/lib/reviewVerticalStops.ts index 1026066c1..139c9198b 100644 --- a/packages/hunk/src/ui/lib/reviewVerticalStops.ts +++ b/packages/hunk/src/ui/lib/reviewVerticalStops.ts @@ -42,24 +42,23 @@ export function buildReviewVerticalStops( const geometry = sectionGeometry[sectionIndex]; if (!geometry) return []; - const plannedRows = geometry.fileViewRows ?? geometry.plannedRows; - const plannedRowByStableKey = new Map(plannedRows.map((row) => [row.stableKey, row] as const)); + // Alternate views already own a mounted row plan. Diff geometry carries just the measured + // note target, so keyboard navigation never materializes its lazy copy-selection content. + const fileViewRowByStableKey = geometry.fileViewRows + ? new Map(geometry.fileViewRows.map((row) => [row.stableKey, row] as const)) + : undefined; const stops: ReviewVerticalStop[] = []; for (const bounds of geometry.rowBounds) { - const plannedRow = plannedRowByStableKey.get(bounds.stableKey); - if (plannedRow?.kind === "inline-note") { - const noteId = - plannedRow.note.source === "draft" ? undefined : plannedRow.note.thread?.noteId; - if (noteId) { - stops.push({ - kind: "note", - fileId: file.id, - hunkIndex: plannedRow.hunkIndex, - noteId, - stableKey: bounds.stableKey, - }); - } + const fileViewRow = fileViewRowByStableKey?.get(bounds.stableKey); + const target = + fileViewRow?.kind === "inline-note" + ? fileViewRow.note.source !== "draft" && fileViewRow.note.thread?.noteId + ? { noteId: fileViewRow.note.thread.noteId, hunkIndex: fileViewRow.hunkIndex } + : undefined + : bounds.noteTarget; + if (target) { + stops.push({ kind: "note", fileId: file.id, ...target, stableKey: bounds.stableKey }); continue; } diff --git a/test/pty/notes.test.ts b/test/pty/notes.test.ts index 82712f3fd..4299d7e52 100644 --- a/test/pty/notes.test.ts +++ b/test/pty/notes.test.ts @@ -392,7 +392,7 @@ describe("PTY notes", () => { }); } - test("saved notes are visibly active and keyboard-navigable without hover", async () => { + test("saved notes remain active and keyboard-navigable after live resizes without hover", async () => { const fixture = harness.createLongWrapFilePair(); const session = await harness.launchHunk({ args: ["diff", "--files", fixture.before, fixture.after, "--mode", "unified"], @@ -427,6 +427,17 @@ describe("PTY notes", () => { expect(savedActive).toContain("E edit"); expect(savedActive).toContain("D delete"); + const noteBorderEnd = savedActive + .split("\n") + .find((line) => line.includes("● Your note"))!.length; + for (const cols of [80, 120, 100]) { + session.resize({ cols, rows: 30 }); + await harness.waitForSnapshot(session, (text) => { + const border = text.split("\n").find((line) => line.includes("● Your note")); + return border?.length === noteBorderEnd + cols - 100; + }); + } + await session.press("k"); const firstActive = await harness.pressAndWaitForSnapshot( session, From 46e145f87b36d1996c74c15eb7f20bf8a67e173c Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Sun, 18 Oct 2026 18:16:15 -0400 Subject: [PATCH 2/2] fix(ui): invalidate cached semantic note navigation targets --- .../hunk/src/ui/diff/diffSectionGeometry.ts | 5 ++- .../src/ui/lib/reviewVerticalStops.test.ts | 39 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/packages/hunk/src/ui/diff/diffSectionGeometry.ts b/packages/hunk/src/ui/diff/diffSectionGeometry.ts index 4d68f0c51..602b01c20 100644 --- a/packages/hunk/src/ui/diff/diffSectionGeometry.ts +++ b/packages/hunk/src/ui/diff/diffSectionGeometry.ts @@ -69,7 +69,7 @@ function notesCacheKey(visibleAgentNotes: VisibleAgentNote[]) { } return `:notes:${visibleAgentNotes - .map(({ annotation, actions, thread }) => { + .map(({ annotation, actions, thread, source }) => { const key = JSON.stringify({ author: annotation.author, id: annotation.id, @@ -79,6 +79,9 @@ function notesCacheKey(visibleAgentNotes: VisibleAgentNote[]) { source: annotation.source, summary: annotation.summary, title: annotation.title, + // Bounds also carry keyboard note targets, including whether this card is selectable. + noteId: thread?.noteId, + noteSource: source, parentId: thread?.parentId, threadDepth: thread?.depth, actions: actions diff --git a/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts b/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts index 6ec3be80d..6ecae37e5 100644 --- a/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts +++ b/packages/hunk/src/ui/lib/reviewVerticalStops.test.ts @@ -102,6 +102,45 @@ describe("review vertical stops", () => { expect(findNextReviewNoteStop(stops, stops[changedLine + 3]!, -1)?.noteId).toBe("reply"); }); + for (const layout of ["split", "unified"] as const) { + test(`${layout} remeasures cached note targets when semantic identity or draft status changes`, () => { + const file = createTestDiffFile({ + before: lines("one", "two"), + after: lines("ONE", "two"), + context: 1, + }); + const annotation = { + id: "stable-card", + source: "user", + summary: "Same card", + newRange: [1, 1] as [number, number], + }; + const variants = [ + { source: "user" as const, noteId: "first-note", expected: ["first-note"] }, + { source: "user" as const, noteId: "replacement-note", expected: ["replacement-note"] }, + { source: "draft" as const, noteId: "replacement-note", expected: [] }, + { source: "user" as const, noteId: "replacement-note", expected: ["replacement-note"] }, + ]; + for (const { source, noteId, expected } of variants) { + const note = createVisibleAgentNote(file.metadata.hunks, { + id: "stable-card", + annotation, + source, + thread: { noteId, depth: 0 }, + }); + const geometry = measureDiffSectionGeometry(file, layout, true, theme, [note], 80); + const stops = buildReviewVerticalStops( + [file], + [geometry], + buildLineCursors([file], [geometry]), + ); + expect(stops.filter((stop) => stop.kind === "note").map((stop) => stop.noteId)).toEqual( + expected, + ); + } + }); + } + test("uses alternate file-view inline-note rows as selectable stops", () => { const file = createTestDiffFile({ id: "alpha",