diff --git a/CHANGELOG.md b/CHANGELOG.md index 4915b40..29d050f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ## [Unreleased] +### Fixed + +- Preserve scroll position when a comment is added, edited, or deleted (and when `.dunk/comments.json` reloads externally). The review pane re-anchors on a surviving diff row so inserting or removing inline comment cards no longer pushes the code you were reading out from under the viewport. + ## [0.12.1] - 2026-05-11 ### Fixed diff --git a/src/core/jj.test.ts b/src/core/jj.test.ts index a31ba09..ad6f412 100644 --- a/src/core/jj.test.ts +++ b/src/core/jj.test.ts @@ -1,9 +1,13 @@ -import { afterEach, describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { buildJjDiffArgs, runJjText } from "./jj"; +// jj sub-process spawns are dramatically slower on Windows (~70× the Linux time observed in CI), +// so give the suite generous headroom to absorb that overhead without flaking. +setDefaultTimeout(30_000); + const tempDirs: string[] = []; const realJjTest = Bun.which("jj") ? test : test.skip; diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 8e20c0e..1408555 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -879,39 +879,71 @@ export function DiffPane({ const layoutChanged = previousLayoutRef.current !== layout; const explicitLayoutToggle = previousLayoutToggleRequestIdRef.current !== layoutToggleRequestId; const wrapChanged = previousWrapLinesRef.current !== wrapLines; + const filesChanged = previousFilesRef.current !== files; const previousSectionMetrics = previousSectionGeometryRef.current; const previousFiles = previousFilesRef.current; + const shouldRestoreViewport = layoutChanged || wrapChanged || filesChanged; - if ((layoutChanged || wrapChanged) && previousSectionMetrics && previousFiles.length > 0) { + if (shouldRestoreViewport && previousSectionMetrics && previousFiles.length > 0) { const previousSectionHeaderHeights = buildInStreamFileHeaderHeights(previousFiles); const previousScrollTop = // Prefer the synchronously captured pre-toggle position so anchor restoration does not - // race the polling-based viewport snapshot. + // race the polling-based viewport snapshot. For files-only changes (comment add/edit/delete + // and external `.dunk/comments.json` reloads) the live scrollTop is still the pre-swap + // position because OpenTUI does not auto-reset scrollTop when content height changes. wrapChanged && wrapToggleScrollTop != null ? wrapToggleScrollTop : layoutChanged && explicitLayoutToggle && layoutToggleScrollTop != null ? layoutToggleScrollTop : (scrollRef.current?.scrollTop ?? Math.max(prevScrollTopRef.current, scrollViewport.top)); - const anchor = findViewportRowAnchor( - previousFiles, - previousSectionMetrics, - previousScrollTop, - previousSectionHeaderHeights, - lastViewportRowAnchorRef.current?.stableKey, - ); - if (anchor) { - const nextTop = resolveViewportRowAnchorTop( - files, - sectionGeometry, - anchor, - sectionHeaderHeights, + // Resolve a viewport anchor using `previousScrollTop` against the pre-swap geometry, then + // translate it into the post-swap geometry. Returns null when no row in the new geometry + // covers the captured anchor (e.g. the only comment in a file was deleted). + const resolveStrategy = (options?: { preferDiffRows?: boolean }) => { + const candidate = findViewportRowAnchor( + previousFiles, + previousSectionMetrics, + previousScrollTop, + previousSectionHeaderHeights, + lastViewportRowAnchorRef.current?.stableKey, + options, ); + const top = candidate + ? resolveViewportRowAnchorTop(files, sectionGeometry, candidate, sectionHeaderHeights) + : null; + return { anchor: candidate, top }; + }; + // Prefer the natural row anchor first. Inline-note stable keys are note-id based, so a + // comment-edit reload still resolves cleanly and keeps the reader's position inside the + // card. Only when the natural anchor cannot resolve in the new geometry do we fall back + // to a survivable diff row — that path handles deletes, where the inline-note row is gone. + const natural = resolveStrategy(); + const filesOnlyChange = filesChanged && !layoutChanged && !wrapChanged; + const diffRowFallback = + natural.top === null && filesOnlyChange ? resolveStrategy({ preferDiffRows: true }) : null; + const anchor = natural.top !== null ? natural.anchor : diffRowFallback?.anchor; + const resolvedTop = natural.top !== null ? natural.top : (diffRowFallback?.top ?? null); + // When the anchor row vanishes (rare: e.g. the only comment in a file was deleted from a + // viewport sitting on it, or a filter removed the anchored file), fall back to clamping the + // pre-mutation scrollTop into the new content extent so the user does not snap to file top. + const fallbackTop = + resolvedTop === null && filesChanged + ? clampReviewScrollTop( + previousScrollTop, + Math.max(scrollViewport.height, scrollRef.current?.viewport.height ?? 0), + ) + : null; + const targetTop = resolvedTop ?? fallbackTop; + + if (targetTop !== null) { const restoreViewportAnchor = () => { - scrollRef.current?.scrollTo(nextTop); + scrollRef.current?.scrollTo(targetTop); }; - lastViewportRowAnchorRef.current = anchor; + if (anchor && resolvedTop !== null) { + lastViewportRowAnchorRef.current = anchor; + } suppressViewportSelectionSync(); restoreViewportAnchor(); // Retry across a couple of repaint cycles so the restored top-row anchor sticks @@ -937,11 +969,13 @@ export function DiffPane({ previousSectionGeometryRef.current = sectionGeometry; previousFilesRef.current = files; }, [ + clampReviewScrollTop, files, layout, layoutToggleRequestId, layoutToggleScrollTop, scrollRef, + scrollViewport.height, scrollViewport.top, sectionGeometry, sectionHeaderHeights, diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 34d2ea4..b3badbf 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -540,7 +540,10 @@ describe("UI components", () => { try { await settleDiffPane(setup); - const frame = setup.captureCharFrame(); + const frame = await waitForFrame( + setup, + (text) => text.includes("11 - export const line11 = 11;") && !text.includes("intro.ts"), + ); expect(frame).toContain("11 - export const line11 = 11;"); expect(frame).toContain("11 + export const line11 = 1100;"); @@ -907,7 +910,12 @@ describe("UI components", () => { try { await settleDiffPane(setup); - const frame = setup.captureCharFrame(); + const frame = await waitForFrame( + setup, + (text) => + text.includes("16 - export const line16 = 16;") && + !text.includes("2 - export const line2 = 2;"), + ); expect(frame).toContain("export const line11 = 11;"); expect(frame).toContain("14 - export const line14 = 14;"); @@ -946,7 +954,12 @@ describe("UI components", () => { try { await settleDiffPane(setup); - const frame = setup.captureCharFrame(); + const frame = await waitForFrame( + setup, + (text) => + text.includes("18 export const line18 = 18;") && + !text.includes("2 - export const line2 = 2;"), + ); expect(frame).toContain("11 export const line11 = 11;"); expect(frame).toContain("14 + export const line14 = 'this is a"); @@ -1014,7 +1027,9 @@ describe("UI components", () => { try { await settleDiffPane(setup); - const frame = setup.captureCharFrame(); + const frame = await waitForFrame(setup, (text) => + text.includes("Keep the selected hunk visible with its note."), + ); // The minimal note card (left accent + title + body, no box border) keeps // the selected hunk and its inline comment visible together. @@ -1080,7 +1095,7 @@ describe("UI components", () => { try { await settleDiffPane(setupWithout); - const frameWithout = setupWithout.captureCharFrame(); + const frameWithout = await waitForFrame(setupWithout, (text) => text.includes("line57")); // dunk context (lines near 57-59) should be visible at the top. expect(frameWithout).toContain("line57"); @@ -1110,7 +1125,9 @@ describe("UI components", () => { try { await settleDiffPane(setupWith); - const frameWith = setupWith.captureCharFrame(); + const frameWith = await waitForFrame(setupWith, (text) => + text.includes("Note anchored on second hunk."), + ); // Note should be visible. expect(frameWith).toContain("Note anchored on second hunk."); diff --git a/src/ui/diff/rowWindowing.test.ts b/src/ui/diff/rowWindowing.test.ts index 3dd47c3..82fab0d 100644 --- a/src/ui/diff/rowWindowing.test.ts +++ b/src/ui/diff/rowWindowing.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import type { DiffSectionGeometry } from "../lib/diffSectionGeometry"; +import type { DiffSectionGeometry, DiffSectionRowBounds } from "../lib/diffSectionGeometry"; import type { PlannedReviewRow } from "./reviewRenderPlan"; import { resolveVisiblePlannedRowWindow } from "./rowWindowing"; @@ -26,10 +26,11 @@ function createTestSectionGeometry( rowBounds: Array<{ key: string; top: number; height: number }>, bodyHeight: number, ): DiffSectionGeometry { - const normalizedRowBounds = rowBounds.map((row) => ({ + const normalizedRowBounds: DiffSectionRowBounds[] = rowBounds.map((row) => ({ ...row, stableKey: row.key, stableKeys: [row.key], + kind: "diff-row", })); return { diff --git a/src/ui/lib/diffSectionGeometry.ts b/src/ui/lib/diffSectionGeometry.ts index a20520b..07f71bb 100644 --- a/src/ui/lib/diffSectionGeometry.ts +++ b/src/ui/lib/diffSectionGeometry.ts @@ -14,6 +14,8 @@ export interface DiffSectionRowBounds extends VerticalBounds { key: string; stableKey: string; stableKeys: string[]; + /** Distinguishes content rows from inline comment cards so anchor pickers can prefer survivable rows. */ + kind: PlannedReviewRow["kind"]; } /** Cached placeholder sizing and hunk navigation geometry for one file section. */ @@ -146,10 +148,11 @@ export function measureDiffSectionGeometry( row.stableKey, ...(row.kind === "diff-row" ? (row.stableAliasKeys ?? []) : []), ]; - const rowBoundsEntry = { + const rowBoundsEntry: DiffSectionRowBounds = { key: row.key, stableKey: row.stableKey, stableKeys, + kind: row.kind, // Record both the starting top and the measured height so callers can translate between // scroll positions and stable review-row identities across wrap/layout changes. top: bodyHeight, diff --git a/src/ui/lib/viewportAnchor.test.ts b/src/ui/lib/viewportAnchor.test.ts index a0132be..f2577ad 100644 --- a/src/ui/lib/viewportAnchor.test.ts +++ b/src/ui/lib/viewportAnchor.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import type { VisibleAgentNote } from "./agentAnnotations"; import { resolveTheme } from "../themes"; import { buildInStreamFileHeaderHeights } from "./fileSectionLayout"; import { measureDiffSectionGeometry } from "./diffSectionGeometry"; @@ -128,10 +129,13 @@ describe("viewport row anchors", () => { stackDeletionAnchor!, headerHeights, ); + + expect(splitTop).not.toBeNull(); + const splitAnchor = findViewportRowAnchor( [file], [splitGeometry], - splitTop, + splitTop!, headerHeights, stackDeletionAnchor?.stableKey, ); @@ -144,4 +148,280 @@ describe("viewport row anchors", () => { expect(roundTripTop).toBe(stackDeletionTop!); }); + + function createAnnotatedFile() { + return createTestDiffFile({ + after: lines( + "const alpha = 2;", + "const beta = 2;", + "const gamma = 30;", + "const stable = true;", + ), + before: lines( + "const alpha = 1;", + "const beta = 2;", + "const gamma = 3;", + "const stable = true;", + ), + id: "viewport-anchor-annotated", + path: "viewport-anchor-annotated.ts", + annotations: [ + { + id: "annotation:1", + newRange: [1, 1], + summary: "alpha changed", + rationale: "increment", + }, + ], + }); + } + + function buildAgentNotes(file: ReturnType): VisibleAgentNote[] { + return file.annotations.map((annotation, index) => ({ + id: `annotation:${file.id}:${annotation.id ?? index}`, + annotation, + })); + } + + test("preferDiffRows skips an inline-note row at the viewport top in favor of the nearest diff row", () => { + const file = createAnnotatedFile(); + const headerHeights = buildInStreamFileHeaderHeights([file]); + const notes = buildAgentNotes(file); + const geometry = measureDiffSectionGeometry( + file, + "stack", + false, + theme, + notes, + 120, + true, + false, + ); + const inlineNoteRow = geometry.rowBounds.find((row) => row.kind === "inline-note"); + + expect(inlineNoteRow).toBeDefined(); + + const naiveAnchor = findViewportRowAnchor( + [file], + [geometry], + inlineNoteRow!.top, + headerHeights, + ); + const survivableAnchor = findViewportRowAnchor( + [file], + [geometry], + inlineNoteRow!.top, + headerHeights, + undefined, + { preferDiffRows: true }, + ); + + expect(naiveAnchor?.stableKey.startsWith("inline-note:")).toBe(true); + expect(survivableAnchor).not.toBeNull(); + expect(survivableAnchor!.stableKey.startsWith("inline-note:")).toBe(false); + expect(geometry.rowBoundsByStableKey.get(survivableAnchor!.stableKey)?.kind).toBe("diff-row"); + }); + + test("resolveViewportRowAnchorTop returns null when an inline-note anchor is removed by deleting the comment", () => { + const annotatedFile = createAnnotatedFile(); + const annotatedHeaderHeights = buildInStreamFileHeaderHeights([annotatedFile]); + const annotatedGeometry = measureDiffSectionGeometry( + annotatedFile, + "stack", + false, + theme, + buildAgentNotes(annotatedFile), + 120, + true, + false, + ); + const inlineNoteRow = annotatedGeometry.rowBounds.find((row) => row.kind === "inline-note"); + + expect(inlineNoteRow).toBeDefined(); + + const noteAnchor = findViewportRowAnchor( + [annotatedFile], + [annotatedGeometry], + inlineNoteRow!.top, + annotatedHeaderHeights, + ); + + expect(noteAnchor?.stableKey.startsWith("inline-note:")).toBe(true); + + const cleanedFile = { ...annotatedFile, annotations: [] }; + const cleanedHeaderHeights = buildInStreamFileHeaderHeights([cleanedFile]); + const cleanedGeometry = measureDiffSectionGeometry( + cleanedFile, + "stack", + false, + theme, + [], + 120, + true, + false, + ); + + const restoredTop = resolveViewportRowAnchorTop( + [cleanedFile], + [cleanedGeometry], + noteAnchor!, + cleanedHeaderHeights, + ); + + expect(restoredTop).toBeNull(); + }); + + test("preferDiffRows yields a survivable anchor that resolves cleanly after a comment deletion", () => { + const annotatedFile = createAnnotatedFile(); + const annotatedHeaderHeights = buildInStreamFileHeaderHeights([annotatedFile]); + const annotatedGeometry = measureDiffSectionGeometry( + annotatedFile, + "stack", + false, + theme, + buildAgentNotes(annotatedFile), + 120, + true, + false, + ); + const inlineNoteRow = annotatedGeometry.rowBounds.find((row) => row.kind === "inline-note"); + + expect(inlineNoteRow).toBeDefined(); + + const survivableAnchor = findViewportRowAnchor( + [annotatedFile], + [annotatedGeometry], + inlineNoteRow!.top, + annotatedHeaderHeights, + undefined, + { preferDiffRows: true }, + ); + + expect(survivableAnchor).not.toBeNull(); + + const cleanedFile = { ...annotatedFile, annotations: [] }; + const cleanedHeaderHeights = buildInStreamFileHeaderHeights([cleanedFile]); + const cleanedGeometry = measureDiffSectionGeometry( + cleanedFile, + "stack", + false, + theme, + [], + 120, + true, + false, + ); + + const restoredTop = resolveViewportRowAnchorTop( + [cleanedFile], + [cleanedGeometry], + survivableAnchor!, + cleanedHeaderHeights, + ); + + expect(restoredTop).not.toBeNull(); + expect(typeof restoredTop).toBe("number"); + }); + + test("inline-note anchor survives a comment edit that changes the card height", () => { + const annotatedFile = createAnnotatedFile(); + const headerHeights = buildInStreamFileHeaderHeights([annotatedFile]); + // A narrow width forces card prose to wrap so the edited rationale guarantees a height bump, + // rather than relying on incidental layout coincidence. + const cardWidth = 40; + const beforeGeometry = measureDiffSectionGeometry( + annotatedFile, + "stack", + false, + theme, + buildAgentNotes(annotatedFile), + cardWidth, + true, + false, + ); + const inlineNoteRowBefore = beforeGeometry.rowBounds.find((row) => row.kind === "inline-note"); + + expect(inlineNoteRowBefore).toBeDefined(); + + // Without `preferDiffRows`, the helper picks the inline-note row as the anchor when the + // viewport top sits inside the card. That id-keyed anchor must continue to resolve after a + // comment body edit so the reader's position inside the card is preserved. + const noteAnchor = findViewportRowAnchor( + [annotatedFile], + [beforeGeometry], + inlineNoteRowBefore!.top, + headerHeights, + ); + + expect(noteAnchor?.stableKey.startsWith("inline-note:")).toBe(true); + + const editedFile = { + ...annotatedFile, + annotations: annotatedFile.annotations.map((annotation) => ({ + ...annotation, + rationale: `${annotation.rationale ?? ""} — extended with substantially more prose so the rendered comment card definitely wraps to additional rows`, + })), + }; + const editedGeometry = measureDiffSectionGeometry( + editedFile, + "stack", + false, + theme, + buildAgentNotes(editedFile), + cardWidth, + true, + false, + ); + const inlineNoteRowAfter = editedGeometry.rowBounds.find((row) => row.kind === "inline-note"); + + expect(inlineNoteRowAfter).toBeDefined(); + expect(inlineNoteRowAfter!.height).toBeGreaterThan(inlineNoteRowBefore!.height); + + const restoredTop = resolveViewportRowAnchorTop( + [editedFile], + [editedGeometry], + noteAnchor!, + headerHeights, + ); + + expect(restoredTop).not.toBeNull(); + expect(typeof restoredTop).toBe("number"); + }); + + test("resolveViewportRowAnchorTop returns null when the anchored file is no longer present", () => { + const file = createChangedFile(); + const headerHeights = buildInStreamFileHeaderHeights([file]); + const geometry = measureDiffSectionGeometry(file, "stack", false, theme, [], 120, true, false); + const firstRowTop = geometry.rowBounds[0]?.top ?? 0; + const anchor = findViewportRowAnchor([file], [geometry], firstRowTop, headerHeights); + + expect(anchor).not.toBeNull(); + + const otherFile = createTestDiffFile({ + after: lines("const beta = 2;"), + before: lines("const beta = 1;"), + id: "other-file", + path: "other-file.ts", + }); + const otherHeaderHeights = buildInStreamFileHeaderHeights([otherFile]); + const otherGeometry = measureDiffSectionGeometry( + otherFile, + "stack", + false, + theme, + [], + 120, + true, + false, + ); + + const restoredTop = resolveViewportRowAnchorTop( + [otherFile], + [otherGeometry], + anchor!, + otherHeaderHeights, + ); + + expect(restoredTop).toBeNull(); + }); }); diff --git a/src/ui/lib/viewportAnchor.ts b/src/ui/lib/viewportAnchor.ts index 5778de7..d90f829 100644 --- a/src/ui/lib/viewportAnchor.ts +++ b/src/ui/lib/viewportAnchor.ts @@ -10,8 +10,11 @@ export interface ViewportRowAnchor { rowOffsetWithin: number; } -/** Find the measured row bounds that cover one file-relative vertical offset. */ -function binarySearchRowBounds(sectionRowBounds: DiffSectionRowBounds[], relativeTop: number) { +/** + * Find the index of the measured row whose extent covers one file-relative offset, or `-1` + * when the offset lies outside every measured row in the section. + */ +function binarySearchRowBoundsIndex(sectionRowBounds: DiffSectionRowBounds[], relativeTop: number) { let low = 0; let high = sectionRowBounds.length - 1; @@ -24,11 +27,28 @@ function binarySearchRowBounds(sectionRowBounds: DiffSectionRowBounds[], relativ } else if (relativeTop >= rowBounds.top + rowBounds.height) { low = mid + 1; } else { - return rowBounds; + return mid; + } + } + + return -1; +} + +/** Resolve the nearest neighbouring diff-row to the picked index, preferring the row above. */ +function pickNearestDiffRow(sectionRowBounds: DiffSectionRowBounds[], pickedIndex: number) { + for (let index = pickedIndex - 1; index >= 0; index -= 1) { + if (sectionRowBounds[index]!.kind === "diff-row") { + return index; + } + } + + for (let index = pickedIndex + 1; index < sectionRowBounds.length; index += 1) { + if (sectionRowBounds[index]!.kind === "diff-row") { + return index; } } - return undefined; + return pickedIndex; } /** @@ -36,6 +56,10 @@ function binarySearchRowBounds(sectionRowBounds: DiffSectionRowBounds[], relativ * * `preferredStableKey` lets callers preserve the exact logical side they were already following * when a split row can map to multiple stacked rows and vice versa. + * + * `preferDiffRows` biases the picked row toward survivable diff content when the row covering + * the viewport top is an inline comment card. That keeps the user's reading position attached to + * code that survives a comment add/edit/delete instead of disappearing with the deleted card. */ export function findViewportRowAnchor( files: DiffFile[], @@ -43,6 +67,7 @@ export function findViewportRowAnchor( scrollTop: number, headerHeights: number[], preferredStableKey?: string | null, + options?: { preferDiffRows?: boolean }, ) { const fileSectionLayouts = buildFileSectionLayouts( files, @@ -58,11 +83,18 @@ export function findViewportRowAnchor( const relativeTop = scrollTop - bodyTop; if (relativeTop >= 0 && relativeTop < bodyHeight && geometry) { - const rowBounds = binarySearchRowBounds(geometry.rowBounds, relativeTop); - if (!rowBounds) { + const pickedIndex = binarySearchRowBoundsIndex(geometry.rowBounds, relativeTop); + if (pickedIndex < 0) { continue; } + const initialRow = geometry.rowBounds[pickedIndex]!; + const chosenIndex = + options?.preferDiffRows && initialRow.kind === "inline-note" + ? pickNearestDiffRow(geometry.rowBounds, pickedIndex) + : pickedIndex; + const rowBounds = geometry.rowBounds[chosenIndex]!; + const stableKey = preferredStableKey && rowBounds.stableKeys.includes(preferredStableKey) ? preferredStableKey @@ -80,13 +112,17 @@ export function findViewportRowAnchor( return null; } -/** Resolve one captured row anchor into its next absolute scrollTop after a relayout. */ +/** + * Resolve one captured row anchor into its next absolute scrollTop after a relayout. + * Returns `null` when the file or row no longer exists, so callers can apply a sensible + * fallback rather than scrolling to the file body top. + */ export function resolveViewportRowAnchorTop( files: DiffFile[], sectionGeometry: DiffSectionGeometry[], anchor: ViewportRowAnchor, headerHeights: number[], -) { +): number | null { const fileSectionLayouts = buildFileSectionLayouts( files, sectionGeometry.map((metrics) => metrics?.bodyHeight ?? 0), @@ -105,16 +141,14 @@ export function resolveViewportRowAnchorTop( const rowBounds = geometry.rowBoundsByStableKey.get(anchor.stableKey) ?? geometry.rowBoundsByKey.get(anchor.rowKey); - if (rowBounds) { - return ( - bodyTop + - rowBounds.top + - Math.min(anchor.rowOffsetWithin, Math.max(0, rowBounds.height - 1)) - ); + if (!rowBounds) { + return null; } - return bodyTop; + return ( + bodyTop + rowBounds.top + Math.min(anchor.rowOffsetWithin, Math.max(0, rowBounds.height - 1)) + ); } - return 0; + return null; }