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" });