From 57970566b5859c641abf490b73c9123b78dae246 Mon Sep 17 00:00:00 2001 From: Michael Hackner Date: Mon, 5 Oct 2026 14:44:24 -0700 Subject: [PATCH] fix(review): stop annotated navigation skipping notes drawn outside hunks `{`/`}` and `hunk session navigate --next-comment` skipped a note whose line falls outside every hunk, such as a stored note after a reload moved the hunks. The diff pane still drew the note in its declared or gap-owner hunk, but the annotated-hunk index counted only strict range overlaps, so no navigation stop pointed at it. The index now resolves each annotation through the same anchor logic the diff pane draws with, so every drawn note's hunk is a stop. --- .changeset/annotated-nav-owner-parity.md | 5 ++ packages/hunk/src/core/review/annotations.ts | 70 ++++++++++++++++--- packages/hunk/src/core/review/types.ts | 2 +- .../hunk/src/ui/components/panes/DiffPane.tsx | 38 ++-------- .../src/ui/hooks/useTerminalReview.test.tsx | 60 ++++++++++++++++ .../hunk/src/ui/hooks/useTerminalReview.ts | 3 +- packages/hunk/src/ui/lib/agentAnnotations.ts | 29 +++----- packages/hunk/src/ui/lib/reviewNoteMapping.ts | 35 ++++++++++ packages/hunk/src/ui/lib/reviewState.test.ts | 45 ++++++++++++ 9 files changed, 221 insertions(+), 66 deletions(-) create mode 100644 .changeset/annotated-nav-owner-parity.md diff --git a/.changeset/annotated-nav-owner-parity.md b/.changeset/annotated-nav-owner-parity.md new file mode 100644 index 000000000..84d563468 --- /dev/null +++ b/.changeset/annotated-nav-owner-parity.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +`{`/`}` and `hunk session navigate --next-comment`/`--prev-comment` no longer skip notes drawn outside the diff’s changed lines, such as after a reload moves the hunks. diff --git a/packages/hunk/src/core/review/annotations.ts b/packages/hunk/src/core/review/annotations.ts index 4c2f61031..3b4bc554f 100644 --- a/packages/hunk/src/core/review/annotations.ts +++ b/packages/hunk/src/core/review/annotations.ts @@ -9,13 +9,16 @@ * *derivation* is shared, so the terminal and the producer hand the planner the same * answer instead of two that agree by coincidence. * - * File membership is broader than hunk membership: a file carrying review context but no - * note inside any hunk is still a stop on the annotated-file tour. + * A hunk counts as annotated when a note intersects it or resolves to it as its owner, + * matching the hunk where drawing places that note. File membership is broader still: a + * file carrying review context without annotations remains on the annotated-file tour. */ +import { reviewGapOwnerHunkIndex, resolveReviewNoteAnchor } from "./anchors"; import { reviewHunkRanges, reviewRangesOverlap, type ReviewHunkSpan } from "./geometry"; import type { ReviewAnnotationIndex } from "./navigation"; import type { AgentAnnotation } from "../../extension-api/types"; import type { DiffFile } from "../changeset/model"; +import type { ReviewLineAddressV1, ReviewRangeAnchorV1 } from "./types"; /** Whether one annotation lands inside a hunk's visible span on either side. */ export function reviewAnnotationOverlapsHunk(annotation: AgentAnnotation, hunk: ReviewHunkSpan) { @@ -27,25 +30,70 @@ export function reviewAnnotationOverlapsHunk(annotation: AgentAnnotation, hunk: ); } -/** Which of one file's hunks carry at least one annotation. */ -export function reviewAnnotatedHunkIndices(file: DiffFile | undefined): ReadonlySet { +/** A note target declared by the surface that knows where the note was written. */ +export interface ReviewAnnotationTarget extends ReviewLineAddressV1 { + hunkIndex: number; +} + +/** Resolve one annotation with the same preferred line and owner used by drawing. */ +export function reviewAnnotationAnchor( + hunks: readonly ReviewHunkSpan[], + annotation: AgentAnnotation, + target?: ReviewAnnotationTarget, +): ReviewRangeAnchorV1 { + const preferred: ReviewLineAddressV1 | undefined = target + ? { side: target.side, line: target.line } + : annotation.newRange + ? { side: "new", line: annotation.newRange[0] } + : annotation.oldRange + ? { side: "old", line: annotation.oldRange[0] } + : undefined; + const fallbackOwnerHunkIndex = + target?.hunkIndex ?? + (preferred ? reviewGapOwnerHunkIndex(hunks, preferred.side, preferred.line) : undefined); + + return resolveReviewNoteAnchor(hunks, { + ...(annotation.oldRange ? { oldRange: annotation.oldRange } : {}), + ...(annotation.newRange ? { newRange: annotation.newRange } : {}), + ...(preferred ? { preferred } : {}), + ...(fallbackOwnerHunkIndex !== undefined ? { fallbackOwnerHunkIndex } : {}), + }); +} + +/** Mark every intersecting hunk and resolved owner, using any declared surface target. */ +export function reviewAnnotatedHunkIndices( + file: DiffFile | undefined, + declaredTarget?: (annotation: AgentAnnotation) => ReviewAnnotationTarget | undefined, +): ReadonlySet { const annotated = new Set(); const annotations = file?.agent?.annotations; - if (!annotations) { + const hunks = file?.metadata.hunks; + if (!annotations || !hunks) { return annotated; } - file!.metadata.hunks.forEach((hunk, index) => { - if (annotations.some((annotation) => reviewAnnotationOverlapsHunk(annotation, hunk))) { - annotated.add(index); + + for (const annotation of annotations) { + const anchor = reviewAnnotationAnchor(hunks, annotation, declaredTarget?.(annotation)); + for (const hunkIndex of anchor.intersectingHunkIndices) { + annotated.add(hunkIndex); } - }); + if (anchor.ownerHunkIndex !== undefined) { + annotated.add(anchor.ownerHunkIndex); + } + } return annotated; } -/** Index the annotated files and hunks of one review, keyed by semantic file key. */ +/** + * Index annotated files and drawing-owned hunks, keyed by semantic file key. + * + * The optional target callback supplies stored-note placement; without it, geometry alone + * determines the owner, as it does for sidecar annotations. + */ export function buildReviewAnnotationIndex( files: readonly DiffFile[], keyByFileId: ReadonlyMap, + declaredTarget?: (annotation: AgentAnnotation) => ReviewAnnotationTarget | undefined, ): ReviewAnnotationIndex { const annotatedHunkIndicesByFileKey = new Map>(); const annotatedFileKeys = new Set(); @@ -58,7 +106,7 @@ export function buildReviewAnnotationIndex( if (file.agent) { annotatedFileKeys.add(fileKey); } - const annotatedHunks = reviewAnnotatedHunkIndices(file); + const annotatedHunks = reviewAnnotatedHunkIndices(file, declaredTarget); if (annotatedHunks.size > 0) { annotatedHunkIndicesByFileKey.set(fileKey, annotatedHunks); } diff --git a/packages/hunk/src/core/review/types.ts b/packages/hunk/src/core/review/types.ts index 2054f1478..99bd5076d 100644 --- a/packages/hunk/src/core/review/types.ts +++ b/packages/hunk/src/core/review/types.ts @@ -38,7 +38,7 @@ export interface ReviewRangeAnchorV1 { preferred?: ReviewLineAddressV1; /** Every hunk whose old or new range intersects the note, in file order. */ intersectingHunkIndices: number[]; - /** The one hunk that renders the note; navigation uses the intersections instead. */ + /** The one hunk that renders the note; annotated navigation includes this owner. */ ownerHunkIndex?: number; } diff --git a/packages/hunk/src/ui/components/panes/DiffPane.tsx b/packages/hunk/src/ui/components/panes/DiffPane.tsx index 45afc9f10..342c0e704 100644 --- a/packages/hunk/src/ui/components/panes/DiffPane.tsx +++ b/packages/hunk/src/ui/components/panes/DiffPane.tsx @@ -29,7 +29,11 @@ import type { FileSourceStatus } from "../../diff/expandCollapsedRows"; import type { ActiveAddNoteAffordance } from "../../diff/DiffSectionBody"; import type { CursorHighlight } from "../../diff/cursorHighlight"; import { useIntermediateRenderAfterMount } from "../../hooks/useIntermediateRenderAfterMount"; -import type { DraftReviewNote, StoredReviewNoteRenderMetadata } from "../../lib/reviewNoteMapping"; +import { + storedReviewNoteMetadata, + storedReviewNoteTarget, + type DraftReviewNote, +} from "../../lib/reviewNoteMapping"; import { createVisibleAgentNote, reviewNoteSource, @@ -122,36 +126,6 @@ type StartUserNoteAtHunk = { bivarianceHack(fileId: string, hunkIndex: number, target?: ReviewNoteTargetV1): void; }["bivarianceHack"]; -/** Read terminal-only semantic note metadata without granting it to static sidecars. */ -function storedReviewNoteMetadata( - annotation: AgentAnnotation, -): StoredReviewNoteRenderMetadata | undefined { - const candidate = annotation as AgentAnnotation & Partial; - return candidate.semanticallyStored === true && typeof candidate.reviewNoteId === "string" - ? (candidate as AgentAnnotation & StoredReviewNoteRenderMetadata) - : undefined; -} - -/** Read the semantic placement retained on stored terminal note projections. */ -function storedReviewNoteTarget( - annotation: AgentAnnotation, -): { hunkIndex: number; side: "old" | "new"; line: number } | undefined { - const candidate = annotation as AgentAnnotation & { - hunkIndex?: unknown; - side?: unknown; - line?: unknown; - }; - return Number.isInteger(candidate.hunkIndex) && - (candidate.side === "old" || candidate.side === "new") && - Number.isInteger(candidate.line) - ? { - hunkIndex: candidate.hunkIndex as number, - side: candidate.side, - line: candidate.line as number, - } - : undefined; -} - /** Grant saved-note card actions from semantic ownership rather than presentation labels. */ export function storedReviewNoteActions({ editable, @@ -643,7 +617,7 @@ export function DiffPane({ const notes: VisibleAgentNote[] = annotations.flatMap((annotation, index) => { const source = reviewNoteSource(annotation); const metadata = storedReviewNoteMetadata(annotation); - const storedTarget = metadata ? storedReviewNoteTarget(annotation) : undefined; + const storedTarget = storedReviewNoteTarget(annotation); if ( metadata && draftNote?.kind === "edit" && diff --git a/packages/hunk/src/ui/hooks/useTerminalReview.test.tsx b/packages/hunk/src/ui/hooks/useTerminalReview.test.tsx index e010b835e..f6a64cad0 100644 --- a/packages/hunk/src/ui/hooks/useTerminalReview.test.tsx +++ b/packages/hunk/src/ui/hooks/useTerminalReview.test.tsx @@ -555,6 +555,66 @@ describe("useTerminalReview", () => { } }); + test("annotated navigation follows stored note ownership after a reload moves hunks", async () => { + const before = Array.from({ length: 60 }, (_unused, index) => `line ${index + 1}`); + const initialAfter = [...before]; + initialAfter[29] = "line 30 changed"; + const reloadedAfter = [...before]; + reloadedAfter[0] = "line 1 changed"; + reloadedAfter[59] = "line 60 changed"; + const alpha = createDiffFile("alpha", "alpha.ts", "alpha old\n", "alpha new\n"); + const initialBeta = createDiffFile("beta", "beta.ts", lines(...before), lines(...initialAfter)); + const { controllerRef, setFilesRef, setup } = await renderTerminalReview([alpha, initialBeta]); + + try { + await flush(setup); + let noteId = ""; + await act(async () => { + const controller = expectValue(controllerRef.current); + controller.startUserNote("beta", 0, { side: "new", line: 30 }); + controller.updateDraftNote("Stored note"); + noteId = controller.saveDraftNote()?.id ?? ""; + }); + await flush(setup); + expect(expectValue(controllerRef.current).userNotesByFileId.beta?.[0]).toMatchObject({ + id: noteId, + hunkIndex: 0, + line: 30, + }); + + await act(async () => { + expectValue(setFilesRef.current)([ + alpha, + createDiffFile("beta", "beta.ts", lines(...before), lines(...reloadedAfter)), + ]); + }); + await flush(setup); + + const controller = expectValue(controllerRef.current); + expect( + controller.visibleFiles.find((file) => file.id === "beta")?.metadata.hunks, + ).toHaveLength(2); + expect(controller.userNotesByFileId.beta).toHaveLength(1); + expect(controller.userNotesByFileId.beta?.[0]).toMatchObject({ + id: noteId, + hunkIndex: 0, + line: 30, + }); + await act(async () => { + controller.selectHunk("alpha", 0); + controller.moveSelection("annotated-hunk", 1); + }); + await flush(setup); + + expect(expectValue(controllerRef.current).selectedFile?.id).toBe("beta"); + expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(0); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("live comments validate markup at the published live width", async () => { const noteGeometry: { current: { layout: "split" | "unified"; width: number } | null } = { current: { layout: "unified", width: 120 }, diff --git a/packages/hunk/src/ui/hooks/useTerminalReview.ts b/packages/hunk/src/ui/hooks/useTerminalReview.ts index f633d5900..78e32bd6b 100644 --- a/packages/hunk/src/ui/hooks/useTerminalReview.ts +++ b/packages/hunk/src/ui/hooks/useTerminalReview.ts @@ -113,6 +113,7 @@ import { storedDraftToDraftNote, storedNoteToLiveComment, storedNoteToUserNote, + storedReviewNoteTarget, type DraftReviewNote, type UserReviewNote, } from "../lib/reviewNoteMapping"; @@ -535,7 +536,7 @@ export function useTerminalReview({ // Which files and hunks carry notes, for the shared annotated-navigation planner. Built // from the merged stream, so a live comment that just arrived is navigable immediately. const annotations = useMemo( - () => buildReviewAnnotationIndex(allFiles, keyByFileId), + () => buildReviewAnnotationIndex(allFiles, keyByFileId, storedReviewNoteTarget), [allFiles, keyByFileId], ); // The shared normalization rule, not a terminal copy: a selected file the filter hides diff --git a/packages/hunk/src/ui/lib/agentAnnotations.ts b/packages/hunk/src/ui/lib/agentAnnotations.ts index d9fc1faff..95758a1e7 100644 --- a/packages/hunk/src/ui/lib/agentAnnotations.ts +++ b/packages/hunk/src/ui/lib/agentAnnotations.ts @@ -3,10 +3,13 @@ import type { DiffFile } from "../../core/changeset/model"; import type { ReviewNoteSource } from "../../core/run/commandInputs"; import type { AgentAnnotation } from "../../extension-api/types"; import { sanitizeTerminalLine } from "../../lib/terminalText"; -import { reviewAnnotationOverlapsHunk } from "../../core/review/annotations"; -import { resolveReviewNoteAnchor, reviewGapOwnerHunkIndex } from "../../core/review/anchors"; +import { + reviewAnnotationAnchor, + reviewAnnotationOverlapsHunk, + type ReviewAnnotationTarget, +} from "../../core/review/annotations"; import type { ReviewHunkSpan } from "../../core/review/geometry"; -import type { ReviewLineAddressV1, ReviewRangeAnchorV1 } from "../../core/review/types"; +import type { ReviewRangeAnchorV1 } from "../../core/review/types"; import { fileLabel } from "./files"; export interface VisibleAgentNote { @@ -112,9 +115,7 @@ export function annotationAnchor(annotation: AgentAnnotation): AnnotationAnchor } /** One note's declared target, from the surface that knows where the note was written. */ -export interface VisibleNoteTarget extends ReviewLineAddressV1 { - hunkIndex: number; -} +export type VisibleNoteTarget = ReviewAnnotationTarget; /** * Builds one note the review stream draws, resolving where it hangs through core. @@ -130,24 +131,10 @@ export function createVisibleAgentNote( note: Omit & { target?: VisibleNoteTarget }, ): VisibleAgentNote { const { target, ...visible } = note; - const rangeAnchor = annotationAnchor(note.annotation); - const preferred: ReviewLineAddressV1 | undefined = target - ? { side: target.side, line: target.line } - : rangeAnchor - ? { side: rangeAnchor.side, line: rangeAnchor.lineNumber } - : undefined; - const fallbackOwnerHunkIndex = - target?.hunkIndex ?? - (preferred ? reviewGapOwnerHunkIndex(hunks, preferred.side, preferred.line) : undefined); return { ...visible, - anchor: resolveReviewNoteAnchor(hunks, { - ...(note.annotation.oldRange ? { oldRange: note.annotation.oldRange } : {}), - ...(note.annotation.newRange ? { newRange: note.annotation.newRange } : {}), - ...(preferred ? { preferred } : {}), - ...(fallbackOwnerHunkIndex !== undefined ? { fallbackOwnerHunkIndex } : {}), - }), + anchor: reviewAnnotationAnchor(hunks, note.annotation, target), }; } diff --git a/packages/hunk/src/ui/lib/reviewNoteMapping.ts b/packages/hunk/src/ui/lib/reviewNoteMapping.ts index d742bb9d5..4892355d4 100644 --- a/packages/hunk/src/ui/lib/reviewNoteMapping.ts +++ b/packages/hunk/src/ui/lib/reviewNoteMapping.ts @@ -12,6 +12,7 @@ */ import type { LiveComment } from "../../core/liveComments"; import { reviewLineAnchor } from "../../core/review/anchors"; +import type { ReviewAnnotationTarget } from "../../core/review/annotations"; import type { ReviewHunkSpan } from "../../core/review/geometry"; import { isRenderableStoredReviewNote, @@ -37,6 +38,40 @@ export interface StoredReviewNoteRenderMetadata { semanticallyStored: true; } +/** Read terminal-only semantic note metadata without granting it to static sidecars. */ +export function storedReviewNoteMetadata( + annotation: AgentAnnotation, +): StoredReviewNoteRenderMetadata | undefined { + const candidate = annotation as AgentAnnotation & Partial; + return candidate.semanticallyStored === true && typeof candidate.reviewNoteId === "string" + ? (candidate as AgentAnnotation & StoredReviewNoteRenderMetadata) + : undefined; +} + +/** Read the semantic placement retained on stored terminal note projections. */ +export function storedReviewNoteTarget( + annotation: AgentAnnotation, +): ReviewAnnotationTarget | undefined { + if (!storedReviewNoteMetadata(annotation)) { + return undefined; + } + + const candidate = annotation as AgentAnnotation & { + hunkIndex?: unknown; + side?: unknown; + line?: unknown; + }; + return Number.isInteger(candidate.hunkIndex) && + (candidate.side === "old" || candidate.side === "new") && + Number.isInteger(candidate.line) + ? { + hunkIndex: candidate.hunkIndex as number, + side: candidate.side, + line: candidate.line as number, + } + : undefined; +} + /** Compatibility shape accepted by older terminal-only test projections. */ type OptionalStoredReviewNoteRenderMetadata = Partial; diff --git a/packages/hunk/src/ui/lib/reviewState.test.ts b/packages/hunk/src/ui/lib/reviewState.test.ts index 9029922e6..5ec8289d2 100644 --- a/packages/hunk/src/ui/lib/reviewState.test.ts +++ b/packages/hunk/src/ui/lib/reviewState.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { buildReviewAnnotationIndex } from "../../core/review/annotations"; +import type { AgentAnnotation } from "../../extension-api/types"; import { createTestAgentFileContext, createTestDiffFile, @@ -22,6 +23,22 @@ function createAnnotatedFile(id: string, path: string) { }); } +function createTestTwoHunkAnnotatedFile(id: string, annotations: AgentAnnotation[]) { + const before = Array.from({ length: 60 }, (_unused, index) => `line ${index + 1}`); + const after = [...before]; + after[3] = "line 4 changed"; + after[56] = "line 57 changed"; + const path = `${id}.ts`; + return createTestDiffFile({ + id, + path, + before: `${before.join("\n")}\n`, + after: `${after.join("\n")}\n`, + context: 3, + agent: createTestAgentFileContext(path, { annotations }), + }); +} + describe("review state helpers", () => { // Intent: stale selections keep their requested index without inventing ranges. test("buildSelectedHunkSummary preserves stale out-of-range selections", () => { @@ -77,6 +94,34 @@ describe("review state helpers", () => { expect([...(index.annotatedHunkIndicesByFileKey.get("key:alpha") ?? [])]).toEqual([0]); }); + test("buildReviewAnnotationIndex follows drawing ownership for gap annotations", () => { + const gapAnnotation: AgentAnnotation = { + id: "gap", + newRange: [30, 30], + summary: "Gap annotation", + }; + const gapFile = createTestTwoHunkAnnotatedFile("gap", [gapAnnotation]); + const keyByFileId = new Map([["gap", "key:gap"]]); + const geometricIndex = buildReviewAnnotationIndex([gapFile], keyByFileId); + const declaredIndex = buildReviewAnnotationIndex([gapFile], keyByFileId, (annotation) => + annotation.id === "gap" ? { hunkIndex: 0, side: "new", line: 30 } : undefined, + ); + const overlappingFile = createTestTwoHunkAnnotatedFile("overlap", [ + { id: "overlap", newRange: [57, 57], summary: "Overlapping annotation" }, + ]); + const overlappingIndex = buildReviewAnnotationIndex( + [overlappingFile], + new Map([["overlap", "key:overlap"]]), + ); + + expect([...gapFile.metadata.hunks]).toHaveLength(2); + expect([...(geometricIndex.annotatedHunkIndicesByFileKey.get("key:gap") ?? [])]).toEqual([1]); + expect([...(declaredIndex.annotatedHunkIndicesByFileKey.get("key:gap") ?? [])]).toEqual([0]); + expect([...(overlappingIndex.annotatedHunkIndicesByFileKey.get("key:overlap") ?? [])]).toEqual([ + 1, + ]); + }); + // Intent: absolute navigation supports both hunk index and side+line addressing. test("resolveReviewNavigationTarget resolves paths by explicit hunk or side and line", () => { const file = createTestDiffFile({ id: "alpha", path: "src/alpha.ts" });