diff --git a/src/ui/diff/reviewRenderPlan.ts b/src/ui/diff/reviewRenderPlan.ts index 6032ec50..bc8c953d 100644 --- a/src/ui/diff/reviewRenderPlan.ts +++ b/src/ui/diff/reviewRenderPlan.ts @@ -5,18 +5,18 @@ import type { DiffRow } from "./pierre"; const EMPTY_VISIBLE_AGENT_NOTES: VisibleAgentNote[] = []; -interface SelectedInlineNote { +type DiffLineRow = Extract; + +interface PrimaryVisibleInlineNote { anchorKey: string; anchorSide?: "old" | "new"; - coveredRowKeys: Set; - endGuideAfterKey: string; + guidedRowKeys: Set; + endGuideAfterKey?: string; note: VisibleAgentNote; noteCount: number; noteIndex: number; } -type DiffLineRow = Extract; - export type PlannedReviewRow = | { kind: "diff-row"; @@ -46,6 +46,16 @@ export type PlannedReviewRow = side: "old" | "new"; }; +function hunkRows(rows: DiffRow[], hunkIndex: number) { + return rows.filter((row) => row.hunkIndex === hunkIndex); +} + +function hunkLineRows(rows: DiffRow[], hunkIndex: number) { + return hunkRows(rows, hunkIndex).filter( + (row): row is DiffLineRow => row.type === "split-line" || row.type === "stack-line", + ); +} + /** Check whether a rendered diff row visually covers the note anchor line. */ function rowMatchesNote(row: DiffLineRow, annotation: AgentAnnotation) { const anchor = annotationAnchor(annotation); @@ -92,62 +102,81 @@ function rowOverlapsAnnotation(row: DiffLineRow, annotation: AgentAnnotation) { ); } -/** Resolve the rendered diff row before which the visible inline note should appear. */ +/** + * Resolve the rendered diff row before which the inline note should appear. + * Range-less notes intentionally anchor beside the first code row in the hunk, + * not above the hunk header metadata. + */ function findInlineNoteAnchorRow( rows: DiffRow[], annotation: AgentAnnotation, selectedHunkIndex: number, ) { - const selectedHunkRows = rows.filter((row) => row.hunkIndex === selectedHunkIndex); - const lineRows = selectedHunkRows.filter( - (row): row is DiffLineRow => row.type === "split-line" || row.type === "stack-line", - ); + const selectedHunkRows = hunkRows(rows, selectedHunkIndex); + const lineRows = hunkLineRows(rows, selectedHunkIndex); const headerRow = selectedHunkRows.find((row) => row.type === "hunk-header"); return lineRows.find((row) => rowMatchesNote(row, annotation)) ?? lineRows[0] ?? headerRow; } -/** Return the one visible note, plus the diff rows that should show its guide rail. */ -function buildSelectedInlineNote( +/** + * The render plan shows at most one inline note at a time. + * The first entry in visibleAgentNotes is the primary visible note, while + * noteIndex/noteCount preserve its position within the current visible list. + */ +function selectPrimaryVisibleNote(visibleAgentNotes: VisibleAgentNote[]) { + if (visibleAgentNotes.length === 0) { + return null; + } + + return { + note: visibleAgentNotes[0]!, + noteIndex: 0, + noteCount: visibleAgentNotes.length, + }; +} + +/** Return the primary visible note, plus the diff rows that should show its guide rail. */ +function buildPrimaryVisibleInlineNote( rows: DiffRow[], visibleAgentNotes: VisibleAgentNote[], selectedHunkIndex: number, ) { - if (visibleAgentNotes.length === 0 || selectedHunkIndex < 0) { + if (selectedHunkIndex < 0) { return null; } - const note = visibleAgentNotes[0]!; - const anchorRow = findInlineNoteAnchorRow(rows, note.annotation, selectedHunkIndex); + const selectedNote = selectPrimaryVisibleNote(visibleAgentNotes); + if (!selectedNote) { + return null; + } + + const anchorRow = findInlineNoteAnchorRow(rows, selectedNote.note.annotation, selectedHunkIndex); if (!anchorRow) { return null; } - const selectedHunkLineRows = rows.filter( - (row): row is DiffLineRow => - row.hunkIndex === selectedHunkIndex && - (row.type === "split-line" || row.type === "stack-line"), - ); + const selectedHunkLineRows = hunkLineRows(rows, selectedHunkIndex); + const anchorSide = annotationAnchor(selectedNote.note.annotation)?.side; const coveredRows = selectedHunkLineRows.filter((row) => - rowOverlapsAnnotation(row, note.annotation), + rowOverlapsAnnotation(row, selectedNote.note.annotation), ); const fallbackGuideRow = - anchorRow.type === "split-line" || anchorRow.type === "stack-line" + anchorSide && (anchorRow.type === "split-line" || anchorRow.type === "stack-line") ? anchorRow - : selectedHunkLineRows[0]; + : undefined; const guideRows = coveredRows.length > 0 ? coveredRows : fallbackGuideRow ? [fallbackGuideRow] : []; - const endGuideAfterKey = guideRows.at(-1)?.key ?? anchorRow.key; return { anchorKey: anchorRow.key, - anchorSide: annotationAnchor(note.annotation)?.side, - coveredRowKeys: new Set(guideRows.map((row) => row.key)), - endGuideAfterKey, - note, - noteIndex: 0, - noteCount: visibleAgentNotes.length, - } satisfies SelectedInlineNote; + anchorSide, + guidedRowKeys: new Set(guideRows.map((row) => row.key)), + endGuideAfterKey: guideRows.at(-1)?.key, + note: selectedNote.note, + noteIndex: selectedNote.noteIndex, + noteCount: selectedNote.noteCount, + } satisfies PrimaryVisibleInlineNote; } function rowCanAnchorHunk(row: DiffRow, showHunkHeaders: boolean) { @@ -158,7 +187,11 @@ function rowCanAnchorHunk(row: DiffRow, showHunkHeaders: boolean) { return row.type !== "collapsed" && row.type !== "hunk-header"; } -/** Build the explicit presentational row plan for one file diff body. */ +/** + * Build the explicit presentational row plan for one file diff body. + * The plan always preserves diff-row order and may insert one inline note plus + * one trailing guide cap for the primary visible note. + */ export function buildReviewRenderPlan({ fileId, rows, @@ -172,7 +205,11 @@ export function buildReviewRenderPlan({ showHunkHeaders: boolean; visibleAgentNotes?: VisibleAgentNote[]; }) { - const selectedInlineNote = buildSelectedInlineNote(rows, visibleAgentNotes, selectedHunkIndex); + const primaryVisibleNote = buildPrimaryVisibleInlineNote( + rows, + visibleAgentNotes, + selectedHunkIndex, + ); const plannedRows: PlannedReviewRow[] = []; const anchoredHunks = new Set(); @@ -185,17 +222,17 @@ export function buildReviewRenderPlan({ anchoredHunks.add(row.hunkIndex); } - if (selectedInlineNote?.anchorKey === row.key) { + if (primaryVisibleNote?.anchorKey === row.key) { plannedRows.push({ kind: "inline-note", - key: `inline-note:${selectedInlineNote.note.id}:${row.key}`, + key: `inline-note:${primaryVisibleNote.note.id}:${row.key}`, fileId, hunkIndex: row.hunkIndex, - annotationId: selectedInlineNote.note.id, - annotation: selectedInlineNote.note.annotation, - anchorSide: selectedInlineNote.anchorSide, - noteCount: selectedInlineNote.noteCount, - noteIndex: selectedInlineNote.noteIndex, + annotationId: primaryVisibleNote.note.id, + annotation: primaryVisibleNote.note.annotation, + anchorSide: primaryVisibleNote.anchorSide, + noteCount: primaryVisibleNote.noteCount, + noteIndex: primaryVisibleNote.noteIndex, }); } @@ -206,18 +243,18 @@ export function buildReviewRenderPlan({ hunkIndex: row.hunkIndex, row, anchorId, - noteGuideSide: selectedInlineNote?.coveredRowKeys.has(row.key) - ? selectedInlineNote.anchorSide + noteGuideSide: primaryVisibleNote?.guidedRowKeys.has(row.key) + ? primaryVisibleNote.anchorSide : undefined, }); - if (selectedInlineNote?.endGuideAfterKey === row.key && selectedInlineNote.anchorSide) { + if (primaryVisibleNote?.anchorSide && primaryVisibleNote.endGuideAfterKey === row.key) { plannedRows.push({ kind: "note-guide-cap", - key: `note-guide-cap:${selectedInlineNote.note.id}:${row.key}`, + key: `note-guide-cap:${primaryVisibleNote.note.id}:${row.key}`, fileId, hunkIndex: row.hunkIndex, - side: selectedInlineNote.anchorSide, + side: primaryVisibleNote.anchorSide, }); } } diff --git a/test/review-render-plan.test.ts b/test/review-render-plan.test.ts index 5157cc6b..0941f079 100644 --- a/test/review-render-plan.test.ts +++ b/test/review-render-plan.test.ts @@ -1,11 +1,16 @@ import { describe, expect, test } from "bun:test"; import { parseDiffFromFile } from "@pierre/diffs"; import type { DiffFile } from "../src/core/types"; +import type { PlannedReviewRow } from "../src/ui/diff/reviewRenderPlan"; import { resolveTheme } from "../src/ui/themes"; const { buildSplitRows, buildStackRows } = await import("../src/ui/diff/pierre"); const { buildReviewRenderPlan } = await import("../src/ui/diff/reviewRenderPlan"); +function lines(...values: string[]) { + return `${values.join("\n")}\n`; +} + function createDiffFile(id: string, path: string, before: string, after: string): DiffFile { const metadata = parseDiffFromFile( { @@ -44,6 +49,25 @@ function createDiffFile(id: string, path: string, before: string, after: string) }; } +function firstInlineNote(plannedRows: PlannedReviewRow[]) { + return plannedRows.find((row) => row.kind === "inline-note"); +} + +function inlineNoteAnchorRow(plannedRows: PlannedReviewRow[]) { + const noteIndex = plannedRows.findIndex((row) => row.kind === "inline-note"); + return noteIndex >= 0 ? plannedRows[noteIndex + 1] : undefined; +} + +function guidedSplitLineNumbers(plannedRows: PlannedReviewRow[], side: "old" | "new") { + return plannedRows.flatMap((row) => { + if (row.kind !== "diff-row" || row.noteGuideSide !== side || row.row.type !== "split-line") { + return []; + } + + return [side === "new" ? row.row.right.lineNumber : row.row.left.lineNumber]; + }); +} + describe("review render plan", () => { test("inserts an inline note before the anchor row and continues the guide through the covered range", () => { const theme = resolveTheme("midnight", null); @@ -71,10 +95,15 @@ describe("review render plan", () => { ], }); - const noteIndex = plannedRows.findIndex((row) => row.kind === "inline-note"); - expect(noteIndex).toBeGreaterThan(0); + const note = firstInlineNote(plannedRows); + expect(note?.kind).toBe("inline-note"); + if (note?.kind === "inline-note") { + expect(note.anchorSide).toBe("new"); + expect(note.noteCount).toBe(1); + expect(note.noteIndex).toBe(0); + } - const anchoredRow = plannedRows[noteIndex + 1]; + const anchoredRow = inlineNoteAnchorRow(plannedRows); expect(anchoredRow?.kind).toBe("diff-row"); if (anchoredRow?.kind === "diff-row") { expect(anchoredRow.row.type).toBe("split-line"); @@ -83,28 +112,99 @@ describe("review render plan", () => { } } - const guidedRows = plannedRows.filter( - (row) => row.kind === "diff-row" && row.noteGuideSide === "new", + expect(guidedSplitLineNumbers(plannedRows, "new")).toEqual([2, 3]); + + const cap = plannedRows.find((row) => row.kind === "note-guide-cap"); + expect(cap?.kind).toBe("note-guide-cap"); + if (cap?.kind === "note-guide-cap") { + expect(cap.side).toBe("new"); + } + }); + + test("anchors deletion-only notes to old-side rows and guides the old column", () => { + const theme = resolveTheme("midnight", null); + const file = createDiffFile( + "deleted", + "deleted.ts", + lines("export const removed = true;", "export const kept = 1;"), + lines("export const kept = 1;"), ); - expect(guidedRows).toHaveLength(2); - expect( - guidedRows.map((row) => - row.kind === "diff-row" && row.row.type === "split-line" ? row.row.right.lineNumber : null, - ), - ).toEqual([2, 3]); + const rows = buildSplitRows(file, null, theme); + const plannedRows = buildReviewRenderPlan({ + fileId: file.id, + rows, + selectedHunkIndex: 0, + showHunkHeaders: true, + visibleAgentNotes: [ + { + id: "annotation:deleted:0:0", + annotation: { + oldRange: [1, 1], + summary: "Explain the removed line", + rationale: "Deletion notes should visually anchor on the old side.", + }, + }, + ], + }); + + const note = firstInlineNote(plannedRows); + expect(note?.kind).toBe("inline-note"); + if (note?.kind === "inline-note") { + expect(note.anchorSide).toBe("old"); + } + + const anchoredRow = inlineNoteAnchorRow(plannedRows); + expect(anchoredRow?.kind).toBe("diff-row"); + if (anchoredRow?.kind === "diff-row") { + expect(anchoredRow.row.type).toBe("split-line"); + if (anchoredRow.row.type === "split-line") { + expect(anchoredRow.row.left.lineNumber).toBe(1); + expect(anchoredRow.row.right.lineNumber).toBeUndefined(); + } + } + + expect(guidedSplitLineNumbers(plannedRows, "old")).toEqual([1]); - const capIndex = plannedRows.findIndex((row) => row.kind === "note-guide-cap"); - expect(capIndex).toBeGreaterThan(noteIndex); - expect(plannedRows[capIndex - 1]?.kind).toBe("diff-row"); + const cap = plannedRows.find((row) => row.kind === "note-guide-cap"); + expect(cap?.kind).toBe("note-guide-cap"); + if (cap?.kind === "note-guide-cap") { + expect(cap.side).toBe("old"); + } }); - test("assigns hunk anchor ids from the first visible row when hunk headers are hidden", () => { + test("assigns hunk anchor ids from the first visible row for every hunk when hunk headers are hidden", () => { const theme = resolveTheme("midnight", null); const file = createDiffFile( "beta", "beta.ts", - "export const beta = 1;\n", - "export const beta = 2;\nexport const gamma = true;\n", + lines( + "export const line1 = 1;", + "export const line2 = 2;", + "export const line3 = 3;", + "export const line4 = 4;", + "export const line5 = 5;", + "export const line6 = 6;", + "export const line7 = 7;", + "export const line8 = 8;", + "export const line9 = 9;", + "export const line10 = 10;", + "export const line11 = 11;", + "export const line12 = 12;", + ), + lines( + "export const line1 = 1;", + "export const line2 = 200;", + "export const line3 = 3;", + "export const line4 = 4;", + "export const line5 = 5;", + "export const line6 = 6;", + "export const line7 = 7;", + "export const line8 = 8;", + "export const line9 = 9;", + "export const line10 = 10;", + "export const line11 = 1100;", + "export const line12 = 12;", + ), ); const rows = buildSplitRows(file, null, theme); const plannedRows = buildReviewRenderPlan({ @@ -115,12 +215,17 @@ describe("review render plan", () => { visibleAgentNotes: [], }); - const anchorRow = plannedRows.find((row) => row.kind === "diff-row" && row.anchorId); - expect(anchorRow?.kind).toBe("diff-row"); - if (anchorRow?.kind === "diff-row") { - expect(anchorRow.row.type).toBe("split-line"); - expect(anchorRow.anchorId).toBe(`diff-hunk:${file.id}:0`); - } + const anchorRows = plannedRows.filter( + (row): row is Extract => + row.kind === "diff-row" && row.anchorId !== undefined, + ); + + expect(anchorRows).toHaveLength(2); + expect(anchorRows.map((row) => row.anchorId)).toEqual([ + `diff-hunk:${file.id}:0`, + `diff-hunk:${file.id}:1`, + ]); + expect(anchorRows.every((row) => row.row.type === "split-line")).toBe(true); }); test("anchors range-less notes to the first visible line row without guide rows", () => { @@ -148,17 +253,127 @@ describe("review render plan", () => { ], }); - const noteIndex = plannedRows.findIndex((row) => row.kind === "inline-note"); - expect(noteIndex).toBe(1); + const note = firstInlineNote(plannedRows); + expect(note?.kind).toBe("inline-note"); + if (note?.kind === "inline-note") { + expect(note.anchorSide).toBeUndefined(); + } + expect(plannedRows.some((row) => row.kind === "note-guide-cap")).toBe(false); expect( plannedRows.some((row) => row.kind === "diff-row" && row.noteGuideSide !== undefined), ).toBe(false); - const anchoredRow = plannedRows[noteIndex + 1]; + const anchoredRow = inlineNoteAnchorRow(plannedRows); expect(anchoredRow?.kind).toBe("diff-row"); if (anchoredRow?.kind === "diff-row") { expect(anchoredRow.row.type).toBe("stack-line"); } }); + + test("keeps note placement scoped to the selected hunk in multi-hunk diffs", () => { + const theme = resolveTheme("midnight", null); + const file = createDiffFile( + "multi", + "multi.ts", + lines( + "export const line1 = 1;", + "export const line2 = 2;", + "export const line3 = 3;", + "export const line4 = 4;", + "export const line5 = 5;", + "export const line6 = 6;", + "export const line7 = 7;", + "export const line8 = 8;", + "export const line9 = 9;", + "export const line10 = 10;", + "export const line11 = 11;", + "export const line12 = 12;", + ), + lines( + "export const line1 = 1;", + "export const line2 = 200;", + "export const line3 = 3;", + "export const line4 = 4;", + "export const line5 = 5;", + "export const line6 = 6;", + "export const line7 = 7;", + "export const line8 = 8;", + "export const line9 = 9;", + "export const line10 = 10;", + "export const line11 = 1100;", + "export const line12 = 12;", + ), + ); + const rows = buildSplitRows(file, null, theme); + const plannedRows = buildReviewRenderPlan({ + fileId: file.id, + rows, + selectedHunkIndex: 1, + showHunkHeaders: true, + visibleAgentNotes: [ + { + id: "annotation:multi:1:0", + annotation: { + newRange: [11, 11], + summary: "Explain the later change", + rationale: "The note should attach to the second hunk only.", + }, + }, + ], + }); + + const anchoredRow = inlineNoteAnchorRow(plannedRows); + expect(anchoredRow?.kind).toBe("diff-row"); + if (anchoredRow?.kind === "diff-row") { + expect(anchoredRow.hunkIndex).toBe(1); + expect(anchoredRow.row.type).toBe("split-line"); + if (anchoredRow.row.type === "split-line") { + expect(anchoredRow.row.right.lineNumber).toBe(11); + } + } + }); + + test("renders only the first visible note while preserving visible note count metadata", () => { + const theme = resolveTheme("midnight", null); + const file = createDiffFile( + "counted", + "counted.ts", + "export const value = 1;\n", + "export const value = 2;\nexport const added = true;\n", + ); + const rows = buildSplitRows(file, null, theme); + const plannedRows = buildReviewRenderPlan({ + fileId: file.id, + rows, + selectedHunkIndex: 0, + showHunkHeaders: true, + visibleAgentNotes: [ + { + id: "annotation:counted:0:0", + annotation: { + newRange: [2, 2], + summary: "First visible note", + }, + }, + { + id: "annotation:counted:0:1", + annotation: { + newRange: [1, 1], + summary: "Second visible note", + }, + }, + ], + }); + + const inlineNotes = plannedRows.filter( + (row): row is Extract => + row.kind === "inline-note", + ); + + expect(inlineNotes).toHaveLength(1); + expect(inlineNotes[0]?.annotationId).toBe("annotation:counted:0:0"); + expect(inlineNotes[0]?.noteIndex).toBe(0); + expect(inlineNotes[0]?.noteCount).toBe(2); + }); });