From 6c96eaedc747ffce0b5cabcdfaa641fd92ffcdba Mon Sep 17 00:00:00 2001 From: loganthomas Date: Mon, 3 Aug 2026 08:43:02 -0500 Subject: [PATCH 01/20] feat(review): enumerate line-granular navigation targets `hunks.ts` flattens the review stream into hunks for `[` and `]`. This does the same one level down, so a current-line cursor and the line a note anchors to can come from one list. Targets are ordered to match the rows the active layout draws: split pairs a change block's two columns per row, stack renders one column then the other. --- src/ui/lib/lineCursors.test.ts | 225 +++++++++++++++++++++++++++++++++ src/ui/lib/lineCursors.ts | 176 ++++++++++++++++++++++++++ 2 files changed, 401 insertions(+) create mode 100644 src/ui/lib/lineCursors.test.ts create mode 100644 src/ui/lib/lineCursors.ts diff --git a/src/ui/lib/lineCursors.test.ts b/src/ui/lib/lineCursors.test.ts new file mode 100644 index 000000000..f78def30c --- /dev/null +++ b/src/ui/lib/lineCursors.test.ts @@ -0,0 +1,225 @@ +import { describe, expect, test } from "bun:test"; +import { createTestDiffFile, lines } from "../../../test/helpers/diff-helpers"; +import { + buildLineCursors, + findNextLineCursor, + firstLineCursorInHunk, + resolveLineCursor, + type LineCursor, +} from "./lineCursors"; + +/** Build a file whose single hunk wraps one changed line in context. */ +function createContextWrappedFile(id: string, path: string) { + return createTestDiffFile({ + id, + path, + before: lines("one", "two", "three", "four"), + after: lines("one", "TWO", "three", "four"), + context: 1, + }); +} + +/** Build a file with two separated single-line changes and no context. */ +function createTwoHunkFile(id: string, path: string) { + return createTestDiffFile({ + id, + path, + before: lines("a", "b", "c", "d", "e", "f", "g", "h", "i", "j"), + after: lines("A", "b", "c", "d", "e", "f", "g", "h", "i", "J"), + context: 0, + }); +} + +describe("buildLineCursors", () => { + test("walks context and changed lines in rendered order", () => { + const cursors = buildLineCursors([createContextWrappedFile("alpha", "alpha.ts")], "stack"); + + expect(cursors).toEqual([ + { fileId: "alpha", hunkIndex: 0, target: { side: "new", line: 1 } }, + { fileId: "alpha", hunkIndex: 0, target: { side: "old", line: 2 } }, + { fileId: "alpha", hunkIndex: 0, target: { side: "new", line: 2 } }, + { fileId: "alpha", hunkIndex: 0, target: { side: "new", line: 3 } }, + ]); + }); + + test("keeps old and new line numbering independent across hunks", () => { + const cursors = buildLineCursors([createTwoHunkFile("alpha", "alpha.ts")], "stack"); + + expect(cursors).toEqual([ + { fileId: "alpha", hunkIndex: 0, target: { side: "old", line: 1 } }, + { fileId: "alpha", hunkIndex: 0, target: { side: "new", line: 1 } }, + { fileId: "alpha", hunkIndex: 1, target: { side: "old", line: 10 } }, + { fileId: "alpha", hunkIndex: 1, target: { side: "new", line: 10 } }, + ]); + }); + + test("pairs a change block's two sides per row in split layout", () => { + const file = createTestDiffFile({ + id: "alpha", + path: "alpha.ts", + before: lines("a", "b", "c"), + after: lines("A", "B", "C"), + context: 0, + }); + + expect(buildLineCursors([file], "split").map((cursor) => cursor.target)).toEqual([ + { side: "old", line: 1 }, + { side: "new", line: 1 }, + { side: "old", line: 2 }, + { side: "new", line: 2 }, + { side: "old", line: 3 }, + { side: "new", line: 3 }, + ]); + }); + + test("walks a change block one column at a time in stack layout", () => { + const file = createTestDiffFile({ + id: "alpha", + path: "alpha.ts", + before: lines("a", "b", "c"), + after: lines("A", "B", "C"), + context: 0, + }); + + expect(buildLineCursors([file], "stack").map((cursor) => cursor.target)).toEqual([ + { side: "old", line: 1 }, + { side: "old", line: 2 }, + { side: "old", line: 3 }, + { side: "new", line: 1 }, + { side: "new", line: 2 }, + { side: "new", line: 3 }, + ]); + }); + + test("flattens every visible file into one review stream", () => { + const cursors = buildLineCursors( + [createTwoHunkFile("alpha", "alpha.ts"), createTwoHunkFile("beta", "beta.ts")], + "stack", + ); + + expect(cursors).toHaveLength(8); + expect(cursors.slice(0, 4).every((cursor) => cursor.fileId === "alpha")).toBe(true); + expect(cursors.slice(4).every((cursor) => cursor.fileId === "beta")).toBe(true); + }); + + test("returns nothing when no files are visible", () => { + expect(buildLineCursors([], "stack")).toEqual([]); + }); +}); + +describe("findNextLineCursor", () => { + const cursors = buildLineCursors( + [createTwoHunkFile("alpha", "alpha.ts"), createTwoHunkFile("beta", "beta.ts")], + "stack", + ); + + test("steps forward and backward one line at a time", () => { + expect(findNextLineCursor(cursors, cursors[0]!, 1)).toEqual(cursors[1]!); + expect(findNextLineCursor(cursors, cursors[1]!, -1)).toEqual(cursors[0]!); + }); + + test("rolls across hunk and file boundaries", () => { + expect(findNextLineCursor(cursors, cursors[1]!, 1)).toEqual(cursors[2]!); + expect(findNextLineCursor(cursors, cursors[3]!, 1)).toEqual(cursors[4]!); + expect(findNextLineCursor(cursors, cursors[4]!, -1)).toEqual(cursors[3]!); + }); + + test("clamps at both ends instead of wrapping", () => { + expect(findNextLineCursor(cursors, cursors[0]!, -1)).toEqual(cursors[0]!); + expect(findNextLineCursor(cursors, cursors[cursors.length - 1]!, 1)).toEqual( + cursors[cursors.length - 1]!, + ); + }); + + test("starts at the top of the stream when no cursor is set", () => { + expect(findNextLineCursor(cursors, null, 1)).toEqual(cursors[0]!); + expect(findNextLineCursor(cursors, null, -1)).toEqual(cursors[0]!); + }); + + test("recovers to the top when the current line left the stream", () => { + const retired: LineCursor = { + fileId: "gamma", + hunkIndex: 4, + target: { side: "new", line: 99 }, + }; + + expect(findNextLineCursor(cursors, retired, 1)).toEqual(cursors[0]!); + }); + + test("returns nothing when the stream is empty", () => { + expect(findNextLineCursor([], null, 1)).toBeNull(); + }); +}); + +describe("firstLineCursorInHunk", () => { + const cursors = buildLineCursors( + [createTwoHunkFile("alpha", "alpha.ts"), createTwoHunkFile("beta", "beta.ts")], + "stack", + ); + + test("seeds at the first line of the requested hunk", () => { + expect(firstLineCursorInHunk(cursors, "beta", 1)).toEqual({ + fileId: "beta", + hunkIndex: 1, + target: { side: "old", line: 10 }, + }); + }); + + test("falls back within the file when the hunk is gone", () => { + expect(firstLineCursorInHunk(cursors, "beta", 7)?.fileId).toBe("beta"); + }); + + test("falls back to the top of the stream without a selected file", () => { + expect(firstLineCursorInHunk(cursors, undefined, 0)).toEqual(cursors[0]!); + }); + + test("returns nothing when the stream is empty", () => { + expect(firstLineCursorInHunk([], "alpha", 0)).toBeNull(); + }); +}); + +describe("resolveLineCursor", () => { + const cursors = buildLineCursors([createTwoHunkFile("alpha", "alpha.ts")], "stack"); + + test("keeps a cursor that still points at a real line", () => { + expect(resolveLineCursor(cursors, cursors[2]!)).toEqual(cursors[2]!); + }); + + test("falls back to the same hunk when the line is gone", () => { + const movedLine: LineCursor = { + fileId: "alpha", + hunkIndex: 1, + target: { side: "new", line: 42 }, + }; + + expect(resolveLineCursor(cursors, movedLine)).toEqual({ + fileId: "alpha", + hunkIndex: 1, + target: { side: "old", line: 10 }, + }); + }); + + test("falls back to the same file when the hunk is gone", () => { + const retiredHunk: LineCursor = { + fileId: "alpha", + hunkIndex: 9, + target: { side: "new", line: 1 }, + }; + + expect(resolveLineCursor(cursors, retiredHunk)?.fileId).toBe("alpha"); + }); + + test("gives up when the file left the review stream", () => { + const filteredOut: LineCursor = { + fileId: "gamma", + hunkIndex: 0, + target: { side: "new", line: 1 }, + }; + + expect(resolveLineCursor(cursors, filteredOut)).toBeNull(); + }); + + test("gives up when there is no cursor to resolve", () => { + expect(resolveLineCursor(cursors, null)).toBeNull(); + }); +}); diff --git a/src/ui/lib/lineCursors.ts b/src/ui/lib/lineCursors.ts new file mode 100644 index 000000000..e1ec9ad0d --- /dev/null +++ b/src/ui/lib/lineCursors.ts @@ -0,0 +1,176 @@ +/** + * Line-granular navigation targets for the review stream. + * + * `hunks.ts` flattens the stream into hunks for `[` and `]`; this does the same one level down. + * Targets come from `hunkContent` rather than the render plan, so they stay cheap to hold, but + * they are ordered to match the rows the active layout actually draws. Lines revealed by + * expanding a collapsed gap are still out of reach. + */ + +import type { Hunk } from "@pierre/diffs"; +import type { DiffFile, LayoutMode, UserNoteLineTarget } from "../../core/types"; + +type ResolvedLayout = Exclude; + +export interface LineCursor { + fileId: string; + hunkIndex: number; + target: UserNoteLineTarget; +} + +const cursorsByFileMetadata = new WeakMap< + DiffFile["metadata"], + Map +>(); + +/** Enumerate the source lines one hunk renders, top to bottom. */ +function hunkLineTargets(hunk: Hunk, layout: ResolvedLayout): UserNoteLineTarget[] { + const targets: UserNoteLineTarget[] = []; + let deletionLineNumber = hunk.deletionStart; + let additionLineNumber = hunk.additionStart; + + for (const content of hunk.hunkContent) { + if (content.type === "context") { + for (let offset = 0; offset < content.lines; offset += 1) { + // Both sides exist here; the new side is what the mouse affordance already anchors to. + targets.push({ side: "new", line: additionLineNumber + offset }); + } + + deletionLineNumber += content.lines; + additionLineNumber += content.lines; + continue; + } + + if (layout === "split") { + // Split draws one row per pair, padding the shorter side, so the marker has to cross each + // row before moving down instead of walking the whole old column first. + const pairedLines = Math.max(content.deletions, content.additions); + for (let offset = 0; offset < pairedLines; offset += 1) { + if (offset < content.deletions) { + targets.push({ side: "old", line: deletionLineNumber + offset }); + } + + if (offset < content.additions) { + targets.push({ side: "new", line: additionLineNumber + offset }); + } + } + } else { + for (let offset = 0; offset < content.deletions; offset += 1) { + targets.push({ side: "old", line: deletionLineNumber + offset }); + } + + for (let offset = 0; offset < content.additions; offset += 1) { + targets.push({ side: "new", line: additionLineNumber + offset }); + } + } + + deletionLineNumber += content.deletions; + additionLineNumber += content.additions; + } + + return targets; +} + +/** + * List one file's cursors, reusing the last result while its parsed diff is unchanged. + * + * Selection changes rebuild the visible file array without reparsing, so this keeps a keypress + * from reallocating one object per source line across the whole changeset. + */ +function fileLineCursors(file: DiffFile, layout: ResolvedLayout): LineCursor[] { + let byLayout = cursorsByFileMetadata.get(file.metadata); + if (!byLayout) { + byLayout = new Map(); + cursorsByFileMetadata.set(file.metadata, byLayout); + } + + const cached = byLayout.get(layout); + if (cached) { + return cached; + } + + const cursors = file.metadata.hunks.flatMap((hunk, hunkIndex) => + hunkLineTargets(hunk, layout).map((target) => ({ fileId: file.id, hunkIndex, target })), + ); + byLayout.set(layout, cursors); + return cursors; +} + +/** Flatten the visible files into one review-stream line cursor list. */ +export function buildLineCursors(files: DiffFile[], layout: ResolvedLayout): LineCursor[] { + return files.flatMap((file) => fileLineCursors(file, layout)); +} + +/** Check whether two cursors name the same review-stream line. */ +function sameLineCursor(left: LineCursor, right: LineCursor) { + return ( + left.fileId === right.fileId && + left.hunkIndex === right.hunkIndex && + left.target.side === right.target.side && + left.target.line === right.target.line + ); +} + +/** Find the first cursor in one hunk, then anywhere in its file. */ +function nearestCursorInFile(cursors: LineCursor[], fileId: string, hunkIndex: number) { + return ( + cursors.find((cursor) => cursor.fileId === fileId && cursor.hunkIndex === hunkIndex) ?? + cursors.find((cursor) => cursor.fileId === fileId) + ); +} + +/** + * Find the first navigable line inside one hunk. + * + * Stays inside the requested file: falling back to the top of the stream would move the marker, + * and with it the selection, off the file the reviewer just picked. + */ +export function firstLineCursorInHunk( + cursors: LineCursor[], + fileId: string | undefined, + hunkIndex: number, +): LineCursor | null { + if (!fileId) { + return cursors[0] ?? null; + } + + return nearestCursorInFile(cursors, fileId, hunkIndex) ?? null; +} + +/** Move forward or backward through the review-stream line cursor list. */ +export function findNextLineCursor( + cursors: LineCursor[], + current: LineCursor | null, + delta: number, +): LineCursor | null { + const currentIndex = current + ? cursors.findIndex((cursor) => sameLineCursor(cursor, current)) + : -1; + if (currentIndex < 0) { + return cursors[0] ?? null; + } + + // Line navigation is non-cyclic like hunk navigation, so both ends of the stream clamp. + const nextIndex = Math.min(Math.max(currentIndex + delta, 0), cursors.length - 1); + return cursors[nextIndex] ?? null; +} + +/** + * Keep a cursor pointing at a real line after filtering or a reload retires the one it was on. + * + * Falls back toward the same hunk and then the same file, mirroring how file selection recovers. + */ +export function resolveLineCursor( + cursors: LineCursor[], + current: LineCursor | null, +): LineCursor | null { + if (!current) { + return null; + } + + if (cursors.some((cursor) => sameLineCursor(cursor, current))) { + return current; + } + + return nearestCursorInFile(cursors, current.fileId, current.hunkIndex) ?? null; +} From 2cb096fe9e4e85a92e3950dfa73ee10dde7bf8de Mon Sep 17 00:00:00 2001 From: loganthomas Date: Mon, 3 Aug 2026 08:43:03 -0500 Subject: [PATCH 02/20] feat(review): add a minimum-move reveal scroll target Hunk reveal biases its target a quarter screen from the top, which is right for a jump but yanks the viewport when it runs on every step key. This moves the shortest distance that puts a line on screen, and stays put when it already is. --- src/ui/lib/hunkScroll.test.ts | 34 +++++++++++++++++++++++++++++++++- src/ui/lib/hunkScroll.ts | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/ui/lib/hunkScroll.test.ts b/src/ui/lib/hunkScroll.test.ts index 8f00bb5a0..aa641d16d 100644 --- a/src/ui/lib/hunkScroll.test.ts +++ b/src/ui/lib/hunkScroll.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { computeHunkRevealScrollTop } from "./hunkScroll"; +import { computeHunkRevealScrollTop, computeLineRevealScrollTop } from "./hunkScroll"; describe("computeHunkRevealScrollTop", () => { test("keeps a fitting hunk fully visible when the preferred padding would clip the end", () => { @@ -57,3 +57,35 @@ describe("computeHunkRevealScrollTop", () => { ).toBe(19); }); }); + +describe("computeLineRevealScrollTop", () => { + test("leaves the viewport alone when the line is already visible", () => { + expect( + computeLineRevealScrollTop({ lineTop: 12, lineHeight: 1, scrollTop: 10, viewportHeight: 20 }), + ).toBe(10); + }); + + test("scrolls up just far enough to reach a line above the viewport", () => { + expect( + computeLineRevealScrollTop({ lineTop: 4, lineHeight: 1, scrollTop: 10, viewportHeight: 20 }), + ).toBe(4); + }); + + test("scrolls down just far enough to reach a line below the viewport", () => { + expect( + computeLineRevealScrollTop({ lineTop: 40, lineHeight: 1, scrollTop: 10, viewportHeight: 20 }), + ).toBe(21); + }); + + test("keeps a tall wrapped line's end on screen", () => { + expect( + computeLineRevealScrollTop({ lineTop: 28, lineHeight: 3, scrollTop: 10, viewportHeight: 20 }), + ).toBe(11); + }); + + test("never scrolls above the top of the stream", () => { + expect( + computeLineRevealScrollTop({ lineTop: 0, lineHeight: 40, scrollTop: 5, viewportHeight: 20 }), + ).toBe(0); + }); +}); diff --git a/src/ui/lib/hunkScroll.ts b/src/ui/lib/hunkScroll.ts index 7057cd27f..9de6c9cd7 100644 --- a/src/ui/lib/hunkScroll.ts +++ b/src/ui/lib/hunkScroll.ts @@ -33,3 +33,37 @@ export function computeHunkRevealScrollTop({ return desiredTop; } + +/** + * Pick a scroll target that brings the current line just into view. + * + * This runs on every step key, so it moves the minimum distance and stays put while the line is + * already on screen; hunk reveal's top bias would yank the viewport on each keystroke. + */ +export function computeLineRevealScrollTop({ + lineTop, + lineHeight, + scrollTop, + viewportHeight, +}: { + lineTop: number; + lineHeight: number; + scrollTop: number; + viewportHeight: number; +}) { + const clampedTop = Math.max(0, lineTop); + const clampedHeight = Math.max(1, lineHeight); + const clampedViewportHeight = Math.max(0, viewportHeight); + + if (clampedTop < scrollTop) { + return clampedTop; + } + + const lineBottom = clampedTop + clampedHeight; + const viewportBottom = scrollTop + clampedViewportHeight; + if (lineBottom > viewportBottom) { + return Math.max(0, lineBottom - clampedViewportHeight); + } + + return scrollTop; +} From 4a8e5f15d3c617a30dea16c36f77ee0c76b11451 Mon Sep 17 00:00:00 2001 From: loganthomas Date: Mon, 3 Aug 2026 08:43:20 -0500 Subject: [PATCH 03/20] feat(diff): mark the current line in the review stream The marker shifts luminance rather than hue. Blending toward one fixed highlight color barely moves a background that already shares its hue, which left the row invisible on additions, and reading the transparent sentinel as a color painted an opaque band on light themes. Cells now take one resolved highlight instead of a `selected` flag, a column range and a blender that had to be kept in agreement by hand. Copy selection and the current line resolve through the same path, so a drag keeps its exact extent and the cursor falls back to the row. --- src/ui/components/panes/DiffPane.tsx | 71 ++++++++- src/ui/components/panes/DiffSection.tsx | 10 +- src/ui/diff/PierreDiffView.tsx | 33 ++++- src/ui/diff/renderRows.tsx | 182 ++++++++++++++---------- src/ui/diff/reviewRenderPlan.ts | 9 +- src/ui/diff/rowStyle.test.ts | 45 ++++++ src/ui/diff/rowStyle.ts | 33 ++++- 7 files changed, 304 insertions(+), 79 deletions(-) create mode 100644 src/ui/diff/rowStyle.test.ts diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 301d42043..6395cccbd 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -16,10 +16,12 @@ import { import { DEFAULT_TAB_WIDTH } from "../../../core/tabWidth"; import type { AgentAnnotation, + CursorLine, DiffFile, LayoutMode, UserNoteLineTarget, } from "../../../core/types"; +import type { LineCursor } from "../../lib/lineCursors"; import type { FileSourceStatus } from "../../diff/expandCollapsedRows"; import type { ActiveAddNoteAffordance } from "../../diff/PierreDiffView"; import type { DraftReviewNote } from "../../hooks/useReviewController"; @@ -32,7 +34,8 @@ import { computeRapidScrollOverscanRows, RAPID_SCROLL_OVERSCAN_IDLE_MS, } from "../../lib/adaptiveScrollOverscan"; -import { computeHunkRevealScrollTop } from "../../lib/hunkScroll"; +import { computeHunkRevealScrollTop, computeLineRevealScrollTop } from "../../lib/hunkScroll"; +import { lineStableKey } from "../../diff/reviewRenderPlan"; import { measureDiffSectionGeometry, type DiffSectionGeometry, @@ -195,6 +198,9 @@ export function DiffPane({ scrollRef, selectedFileId, selectedHunkIndex, + cursorLine = "off", + lineCursor = null, + lineCursorRevealRequestId = 0, scrollToNote = false, draftNote = null, draftNoteFocused = false, @@ -246,6 +252,9 @@ export function DiffPane({ scrollRef: RefObject; selectedFileId?: string; selectedHunkIndex: number; + cursorLine?: CursorLine; + lineCursor?: LineCursor | null; + lineCursorRevealRequestId?: number; scrollToNote?: boolean; draftNote?: DraftReviewNote | null; draftNoteFocused?: boolean; @@ -866,6 +875,13 @@ export function DiffPane({ return resolveCopySelectionSide(copySelectionDrag.anchor.column, layout, diffContentWidth); }, [copySelectionDrag, diffContentWidth, layout]); + // One object per cursor move, so the section and row memos below only see a new reference when + // the current line actually moves. + const cursorLineTarget = useMemo( + () => (lineCursor ? { hunkIndex: lineCursor.hunkIndex, ...lineCursor.target } : undefined), + [lineCursor], + ); + const copySelectedRowKeysByFile = useMemo( () => buildCopySelectedRowKeys({ @@ -1863,6 +1879,55 @@ export function DiffPane({ suppressViewportSelectionSync, ]); + const previousLineCursorRevealRequestIdRef = useRef(lineCursorRevealRequestId); + + // Measured bounds rather than a row count, so wrapped rows taller than one row still fit. + useLayoutEffect(() => { + if (previousLineCursorRevealRequestIdRef.current === lineCursorRevealRequestId) { + return; + } + previousLineCursorRevealRequestIdRef.current = lineCursorRevealRequestId; + + const scrollBox = scrollRef.current; + if (!scrollBox || !lineCursor || !cursorLineTarget) { + return; + } + + const sectionIndex = files.findIndex((file) => file.id === lineCursor.fileId); + const section = fileSectionLayouts[sectionIndex]; + const bounds = sectionGeometry[sectionIndex]?.rowBoundsByStableKey.get( + lineStableKey(cursorLineTarget.hunkIndex, cursorLineTarget.side, cursorLineTarget.line), + ); + if (!section || !bounds) { + return; + } + + const viewportHeight = scrollBox.viewport.height || scrollViewport.height; + const revealScrollTop = computeLineRevealScrollTop({ + lineTop: section.bodyTop + bounds.top, + lineHeight: bounds.height, + scrollTop: scrollBox.scrollTop, + viewportHeight, + }); + if (revealScrollTop === scrollBox.scrollTop) { + return; + } + + suppressViewportSelectionSync(); + scrollBox.scrollTo(clampReviewScrollTop(revealScrollTop, viewportHeight)); + }, [ + clampReviewScrollTop, + cursorLineTarget, + fileSectionLayouts, + files, + lineCursor, + lineCursorRevealRequestId, + scrollRef, + scrollViewport.height, + sectionGeometry, + suppressViewportSelectionSync, + ]); + // Keep keyboard step scrolling at exactly one row while wheel scrolling uses its own multiplier. useEffect(() => { const scrollBox = scrollRef.current; @@ -1963,6 +2028,10 @@ export function DiffPane({ selectedHunkIndex={file.id === selectedFileId ? selectedHunkIndex : -1} copySelectedRowRanges={copySelectedRowKeysByFile.get(file.id)} copySelectedSide={copySelectionSide} + cursorLine={cursorLine} + cursorLineTarget={ + file.id === lineCursor?.fileId ? cursorLineTarget : undefined + } shouldLoadHighlight={ (!wrapLines || initialWrappedRenderWindowWarmed) && highlightPrefetchFileIds.has(file.id) diff --git a/src/ui/components/panes/DiffSection.tsx b/src/ui/components/panes/DiffSection.tsx index b3846d800..bd0a9d75c 100644 --- a/src/ui/components/panes/DiffSection.tsx +++ b/src/ui/components/panes/DiffSection.tsx @@ -1,5 +1,5 @@ import { memo } from "react"; -import type { DiffFile, LayoutMode, UserNoteLineTarget } from "../../../core/types"; +import type { CursorLine, DiffFile, LayoutMode, UserNoteLineTarget } from "../../../core/types"; import type { FileSourceStatus } from "../../diff/expandCollapsedRows"; import { PierreDiffView, type ActiveAddNoteAffordance } from "../../diff/PierreDiffView"; import type { VisibleBodyBounds } from "../../diff/rowWindowing"; @@ -24,6 +24,8 @@ interface DiffSectionProps { selectedHunkIndex: number; copySelectedRowRanges?: Map; copySelectedSide?: "left" | "right"; + cursorLine?: CursorLine; + cursorLineTarget?: UserNoteLineTarget & { hunkIndex: number }; shouldLoadHighlight: boolean; sectionGeometry?: DiffSectionGeometry; separatorWidth: number; @@ -61,6 +63,8 @@ function DiffSectionComponent({ selectedHunkIndex, copySelectedRowRanges, copySelectedSide, + cursorLine, + cursorLineTarget, shouldLoadHighlight, sectionGeometry, separatorWidth, @@ -156,6 +160,8 @@ function DiffSectionComponent({ codeHorizontalOffset={codeHorizontalOffset} copySelectedRowRanges={copySelectedRowRanges} copySelectedSide={copySelectedSide} + cursorLine={cursorLine} + cursorLineTarget={cursorLineTarget} theme={theme} width={viewWidth} visibleAgentNotes={visibleAgentNotes} @@ -193,6 +199,8 @@ export const DiffSection = memo(DiffSectionComponent, (previous, next) => { previous.selectedHunkIndex === next.selectedHunkIndex && previous.copySelectedRowRanges === next.copySelectedRowRanges && previous.copySelectedSide === next.copySelectedSide && + previous.cursorLine === next.cursorLine && + previous.cursorLineTarget === next.cursorLineTarget && previous.shouldLoadHighlight === next.shouldLoadHighlight && previous.sectionGeometry === next.sectionGeometry && previous.separatorWidth === next.separatorWidth && diff --git a/src/ui/diff/PierreDiffView.tsx b/src/ui/diff/PierreDiffView.tsx index ec7df7d7c..556717de5 100644 --- a/src/ui/diff/PierreDiffView.tsx +++ b/src/ui/diff/PierreDiffView.tsx @@ -1,7 +1,7 @@ import { useRenderer } from "@opentui/react"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { DEFAULT_TAB_WIDTH } from "../../core/tabWidth"; -import type { DiffFile, LayoutMode, UserNoteLineTarget } from "../../core/types"; +import type { CursorLine, DiffFile, LayoutMode, UserNoteLineTarget } from "../../core/types"; import { AgentInlineNote } from "../components/panes/AgentInlineNote"; import type { VisibleAgentNote } from "../lib/agentAnnotations"; import type { CopySelectedRowRange } from "../components/panes/copySelection"; @@ -13,7 +13,8 @@ import { spansForHighlightedSourceLine, type DiffRow } from "./pierre"; import { plannedReviewRowVisible } from "./plannedReviewRows"; import { buildDiffSectionRowPlan } from "./diffSectionRowPlan"; import { resolveVisiblePlannedRowWindow, type VisibleBodyBounds } from "./rowWindowing"; -import { diffMessage, DiffRowView, fitText } from "./renderRows"; +import { diffMessage, DiffRowView, fitText, type CursorHighlight } from "./renderRows"; +import { lineStableKey } from "./reviewRenderPlan"; import { useHighlightedDiff } from "./useHighlightedDiff"; import { useHighlightedSource } from "./useHighlightedSource"; @@ -63,6 +64,8 @@ export function PierreDiffView({ codeHorizontalOffset = 0, copySelectedRowRanges, copySelectedSide, + cursorLine = "off", + cursorLineTarget, expandedGapKeys = EMPTY_EXPANDED_GAP_KEYS, file, layout, @@ -89,6 +92,9 @@ export function PierreDiffView({ codeHorizontalOffset?: number; copySelectedRowRanges?: Map; copySelectedSide?: "left" | "right"; + cursorLine?: CursorLine; + /** The current line within this file, when the review-stream cursor sits in it. */ + cursorLineTarget?: UserNoteLineTarget & { hunkIndex: number }; expandedGapKeys?: ReadonlySet; file: DiffFile | undefined; layout: Exclude; @@ -261,6 +267,23 @@ export function PierreDiffView({ return next; }, [plannedRows]); + // Matching on the render plan's own line anchor lets split and stack share one lookup. One + // object per cursor position keeps DiffRowView's memo effective for every other row. + const cursor = useMemo( + () => + cursorLine === "off" || !cursorLineTarget + ? null + : { + stableKey: lineStableKey( + cursorLineTarget.hunkIndex, + cursorLineTarget.side, + cursorLineTarget.line, + ), + highlight: { style: cursorLine, side: cursorLineTarget.side } satisfies CursorHighlight, + }, + [cursorLine, cursorLineTarget], + ); + /** One shared hover handler for every diff row; DiffRowView passes the hovered row's key. */ const handleHoverRow = useCallback( (rowKey: string) => { @@ -371,6 +394,11 @@ export function PierreDiffView({ ); } + const isCursorRow = + cursor !== null && + (plannedRow.stableKey === cursor.stableKey || + plannedRow.stableAliasKeys?.includes(cursor.stableKey) === true); + return ( ; + /** Which half of a split row the cursor sits on, and where a note would anchor. */ + side: "old" | "new"; +} + +interface RowHighlight { + bg: (baseBg: string) => string; + /** Global columns to blend; absent blends the gutter alone. */ + colRange?: CopySelectedRowRange; +} + +/** Column span covering a row's whole content column, in the global columns selection uses. */ +const FULL_ROW_COL_RANGE: CopySelectedRowRange = { startCol: 0, endCol: Number.MAX_SAFE_INTEGER }; /** Clamp a label to one terminal row with an ellipsis. */ export function fitText(text: string, width: number) { @@ -327,7 +344,7 @@ function renderInlineSpans( fallbackBg: string, keyPrefix: string, horizontalOffset = 0, - selectionTheme?: AppTheme, + highlightBg?: (baseBg: string) => string, selectionColRange?: { start: number; end: number }, spansAreSanitized = false, ) { @@ -336,7 +353,7 @@ function renderInlineSpans( horizontalOffset, width, ); - const needsBlending = selectionTheme && selectionColRange; + const needsBlending = highlightBg && selectionColRange; const paddingAmount = Math.max(0, width - usedWidth); let paddingMerged = false; const lastSpan = trimmed.at(-1); @@ -430,7 +447,7 @@ function renderInlineSpans( {selected} , @@ -473,11 +490,7 @@ function renderInlineSpans( } if (inSel > 0) { elements.push( - + {" ".repeat(inSel)} , ); @@ -1126,27 +1139,30 @@ function resolvePlainContentWidth(totalWidth: number, prefixWidth: number, gutte } /** - * Apply the selection-highlight blend to a cell palette's gutter bg only. + * Apply a highlight blend to a cell palette's gutter bg only. * * The content bg is intentionally left untouched here so renderInlineSpans can apply the same * blend uniformly across every rendered span (including syntax-emphasis spans that supply their * own bg). Pre-blending contentBg would cause the fallback path to double-blend. */ -function applySelectionPalette

( +function applyHighlightPalette

( palette: P, - theme: AppTheme, + highlightBg: (baseBg: string) => string, ): P { return { ...palette, - gutterBg: selectionHighlightBg(palette.gutterBg, theme), + gutterBg: highlightBg(palette.gutterBg), }; } -/** Apply the selection-highlight blend to a prefix descriptor. */ -function applySelectionPrefix

(prefix: P, theme: AppTheme): P { +/** Apply a highlight blend to a prefix descriptor. */ +function applyHighlightPrefix

( + prefix: P, + highlightBg: (baseBg: string) => string, +): P { return { ...prefix, - bg: selectionHighlightBg(prefix.bg, theme), + bg: highlightBg(prefix.bg), }; } @@ -1164,13 +1180,12 @@ function renderSplitCell( fg: string; bg: string; }, - selected = false, - selectionColRange?: CopySelectedRowRange, + highlight?: RowHighlight, paneOffset = 0, ) { const basePalette = splitCellPalette(cell.kind, theme, cell.moveKind); - const palette = selected ? applySelectionPalette(basePalette, theme) : basePalette; - const resolvedPrefix = selected && prefix ? applySelectionPrefix(prefix, theme) : prefix; + const palette = highlight ? applyHighlightPalette(basePalette, highlight.bg) : basePalette; + const resolvedPrefix = highlight && prefix ? applyHighlightPrefix(prefix, highlight.bg) : prefix; const prefixWidth = resolvedPrefix?.text.length ?? 0; const { gutterWidth, contentWidth } = resolveSplitCellGeometry( width, @@ -1182,14 +1197,12 @@ function renderSplitCell( // Convert global selection column range to content-local range. const globalContentStart = paneOffset + prefixWidth + gutterWidth; + const colRange = highlight?.colRange; const localColRange = - selectionColRange && globalContentStart < selectionColRange.endCol + colRange && globalContentStart < colRange.endCol ? { - start: Math.max(0, selectionColRange.startCol - globalContentStart), - end: Math.min( - contentWidth, - Math.max(0, selectionColRange.endCol - globalContentStart + 1), - ), + start: Math.max(0, colRange.startCol - globalContentStart), + end: Math.min(contentWidth, Math.max(0, colRange.endCol - globalContentStart + 1)), } : undefined; @@ -1210,7 +1223,7 @@ function renderSplitCell( palette.contentBg, `${keyPrefix}:content`, contentOffset, - selected ? theme : undefined, + highlight?.bg, localColRange, )} @@ -1231,12 +1244,11 @@ function renderStackCell( fg: string; bg: string; }, - selected = false, - selectionColRange?: CopySelectedRowRange, + highlight?: RowHighlight, ) { const basePalette = stackCellPalette(cell.kind, theme, cell.moveKind); - const palette = selected ? applySelectionPalette(basePalette, theme) : basePalette; - const resolvedPrefix = selected && prefix ? applySelectionPrefix(prefix, theme) : prefix; + const palette = highlight ? applyHighlightPalette(basePalette, highlight.bg) : basePalette; + const resolvedPrefix = highlight && prefix ? applyHighlightPrefix(prefix, highlight.bg) : prefix; const prefixWidth = resolvedPrefix?.text.length ?? 0; const { gutterWidth, contentWidth } = resolveStackCellGeometry( width, @@ -1247,14 +1259,12 @@ function renderStackCell( // Convert global selection column range to content-local range. const globalContentStart = prefixWidth + gutterWidth; + const colRange = highlight?.colRange; const localColRange = - selectionColRange && globalContentStart < selectionColRange.endCol + colRange && globalContentStart < colRange.endCol ? { - start: Math.max(0, selectionColRange.startCol - globalContentStart), - end: Math.min( - contentWidth, - Math.max(0, selectionColRange.endCol - globalContentStart + 1), - ), + start: Math.max(0, colRange.startCol - globalContentStart), + end: Math.min(contentWidth, Math.max(0, colRange.endCol - globalContentStart + 1)), } : undefined; @@ -1275,7 +1285,7 @@ function renderStackCell( palette.contentBg, `${keyPrefix}:content`, contentOffset, - selected ? theme : undefined, + highlight?.bg, localColRange, )} @@ -1294,24 +1304,21 @@ function renderWrappedSplitCellLine( fg: string; bg: string; }, - selected = false, - selectionColRange?: CopySelectedRowRange, + highlight?: RowHighlight, paneOffset = 0, ) { - const resolvedPalette = selected ? applySelectionPalette(palette, theme) : palette; - const resolvedPrefix = selected ? applySelectionPrefix(prefix, theme) : prefix; + const resolvedPalette = highlight ? applyHighlightPalette(palette, highlight.bg) : palette; + const resolvedPrefix = highlight ? applyHighlightPrefix(prefix, highlight.bg) : prefix; const prefixWidth = prefix.text.length; const gutterWidth = line.gutterText.length; const globalContentStart = paneOffset + prefixWidth + gutterWidth; + const colRange = highlight?.colRange; const localColRange = - selectionColRange && globalContentStart < selectionColRange.endCol + colRange && globalContentStart < colRange.endCol ? { - start: Math.max(0, selectionColRange.startCol - globalContentStart), - end: Math.min( - contentWidth, - Math.max(0, selectionColRange.endCol - globalContentStart + 1), - ), + start: Math.max(0, colRange.startCol - globalContentStart), + end: Math.min(contentWidth, Math.max(0, colRange.endCol - globalContentStart + 1)), } : undefined; @@ -1334,7 +1341,7 @@ function renderWrappedSplitCellLine( resolvedPalette.contentBg, `${keyPrefix}:content`, 0, - selected ? theme : undefined, + highlight?.bg, localColRange, true, )} @@ -1354,23 +1361,20 @@ function renderWrappedStackCellLine( fg: string; bg: string; }, - selected = false, - selectionColRange?: CopySelectedRowRange, + highlight?: RowHighlight, ) { - const resolvedPalette = selected ? applySelectionPalette(palette, theme) : palette; - const resolvedPrefix = selected ? applySelectionPrefix(prefix, theme) : prefix; + const resolvedPalette = highlight ? applyHighlightPalette(palette, highlight.bg) : palette; + const resolvedPrefix = highlight ? applyHighlightPrefix(prefix, highlight.bg) : prefix; const prefixWidth = prefix.text.length; const gutterWidth = line.gutterText.length; const globalContentStart = prefixWidth + gutterWidth; + const colRange = highlight?.colRange; const localColRange = - selectionColRange && globalContentStart < selectionColRange.endCol + colRange && globalContentStart < colRange.endCol ? { - start: Math.max(0, selectionColRange.startCol - globalContentStart), - end: Math.min( - contentWidth, - Math.max(0, selectionColRange.endCol - globalContentStart + 1), - ), + start: Math.max(0, colRange.startCol - globalContentStart), + end: Math.min(contentWidth, Math.max(0, colRange.endCol - globalContentStart + 1)), } : undefined; @@ -1393,7 +1397,7 @@ function renderWrappedStackCellLine( resolvedPalette.contentBg, `${keyPrefix}:content`, 0, - selected ? theme : undefined, + highlight?.bg, localColRange, true, )} @@ -1667,6 +1671,7 @@ function renderRow( selected: boolean, copySelectedRowRange: CopySelectedRowRange | undefined, copySelectedSide: "left" | "right" | undefined, + cursorHighlight: CursorHighlight | undefined, anchorId?: string, noteGuideSide?: "old" | "new", showAddNoteBadge = false, @@ -1682,6 +1687,41 @@ function renderRow( // selection represents. const hasLeftSelection = hasCopySelection && copySelectedSide !== "right"; const hasRightSelection = hasCopySelection && copySelectedSide !== "left"; + + // An active drag outranks the resting cursor, so copy selection keeps its exact extent. On a + // split change row the two sides are different note targets, so each resolves separately. + const resolveHighlight = (hasSelection: boolean, onCursor: boolean): RowHighlight | undefined => { + if (hasSelection) { + return { + bg: (baseBg) => selectionHighlightBg(baseBg, theme), + colRange: copySelectedRowRange, + }; + } + + if (!onCursor) { + return undefined; + } + + return { + bg: (baseBg) => cursorLineHighlightBg(baseBg, theme), + colRange: cursorHighlight?.style === "row" ? FULL_ROW_COL_RANGE : undefined, + }; + }; + + // A split context row shows the same source line on both halves, so marking one of them would + // read as half a row. Change rows keep the split, since the halves are different note targets. + const splitContextRow = + row.type === "split-line" && row.left.kind === "context" && row.right.kind === "context"; + const onCursorRow = cursorHighlight !== undefined; + const leftHighlight = resolveHighlight( + hasLeftSelection, + onCursorRow && (splitContextRow || cursorHighlight.side === "old"), + ); + const rightHighlight = resolveHighlight( + hasRightSelection, + onCursorRow && (splitContextRow || cursorHighlight.side === "new"), + ); + const cellHighlight = resolveHighlight(hasCopySelection, cursorHighlight !== undefined); let baseRow: ReactNode; if (row.type === "collapsed") { @@ -1760,8 +1800,7 @@ function renderRow( `${row.key}:left`, codeHorizontalOffset, leftPrefix, - hasLeftSelection, - hasLeftSelection ? copySelectedRowRange : undefined, + leftHighlight, 0, )} {renderSplitCell( @@ -1773,8 +1812,7 @@ function renderRow( `${row.key}:right`, codeHorizontalOffset, rightPrefix, - hasRightSelection, - hasRightSelection ? copySelectedRowRange : undefined, + rightHighlight, leftWidth, )} {guideOnNewSide ? ( @@ -1836,7 +1874,7 @@ function renderRow( const showBadgeOnLine = showAddNoteBadge && index === 0; let styledRow: StyledText; - if (hasCopySelection) { + if (leftHighlight || rightHighlight) { styledRow = styledTextFromSpanNodes([ renderWrappedSplitCellLine( leftLine, @@ -1845,8 +1883,7 @@ function renderRow( theme, `${row.key}:left:${index}`, leftPrefix, - hasLeftSelection, - copySelectedRowRange, + leftHighlight, 0, ), renderWrappedSplitCellLine( @@ -1856,8 +1893,7 @@ function renderRow( theme, `${row.key}:right:${index}`, rightPrefix, - hasRightSelection, - copySelectedRowRange, + rightHighlight, leftWidth, ), guideOnNewSide ? ( @@ -1975,8 +2011,7 @@ function renderRow( `${row.key}:stack`, codeHorizontalOffset, prefix, - hasCopySelection, - hasCopySelection ? copySelectedRowRange : undefined, + cellHighlight, )} {guideOnNewSide ? ( @@ -2022,8 +2057,7 @@ function renderRow( theme, `${row.key}:stack:${index}`, prefix, - hasCopySelection, - hasCopySelection ? copySelectedRowRange : undefined, + cellHighlight, ), guideOnNewSide ? ( @@ -2088,6 +2122,7 @@ interface DiffRowViewProps { selected: boolean; copySelectedRowRange?: CopySelectedRowRange; copySelectedSide?: "left" | "right"; + cursorHighlight?: CursorHighlight; anchorId?: string; noteGuideSide?: "old" | "new"; showAddNoteBadge?: boolean; @@ -2116,6 +2151,7 @@ export const DiffRowView = memo( selected, copySelectedRowRange, copySelectedSide, + cursorHighlight, anchorId, noteGuideSide, showAddNoteBadge, @@ -2135,6 +2171,7 @@ export const DiffRowView = memo( selected, copySelectedRowRange, copySelectedSide, + cursorHighlight, anchorId, noteGuideSide, showAddNoteBadge, @@ -2156,6 +2193,7 @@ export const DiffRowView = memo( previous.selected === next.selected && previous.copySelectedRowRange === next.copySelectedRowRange && previous.copySelectedSide === next.copySelectedSide && + previous.cursorHighlight === next.cursorHighlight && previous.anchorId === next.anchorId && previous.noteGuideSide === next.noteGuideSide && previous.showAddNoteBadge === next.showAddNoteBadge && diff --git a/src/ui/diff/reviewRenderPlan.ts b/src/ui/diff/reviewRenderPlan.ts index f3bfc76d7..b10adc232 100644 --- a/src/ui/diff/reviewRenderPlan.ts +++ b/src/ui/diff/reviewRenderPlan.ts @@ -67,14 +67,19 @@ function uniqueStableKeys(keys: Array) { return next; } +/** Build the file-scoped stable anchor for one source line on either diff side. */ +export function lineStableKey(hunkIndex: number, side: "old" | "new", lineNumber: number) { + return `line:${hunkIndex}:${side}:${lineNumber}`; +} + /** Build the file-scoped stable anchor for one old-side source line. */ function oldLineStableKey(hunkIndex: number, lineNumber?: number) { - return lineNumber === undefined ? undefined : `line:${hunkIndex}:old:${lineNumber}`; + return lineNumber === undefined ? undefined : lineStableKey(hunkIndex, "old", lineNumber); } /** Build the file-scoped stable anchor for one new-side source line. */ function newLineStableKey(hunkIndex: number, lineNumber?: number) { - return lineNumber === undefined ? undefined : `line:${hunkIndex}:new:${lineNumber}`; + return lineNumber === undefined ? undefined : lineStableKey(hunkIndex, "new", lineNumber); } /** Build the file-scoped stable anchor for one context row shared by both sides. */ diff --git a/src/ui/diff/rowStyle.test.ts b/src/ui/diff/rowStyle.test.ts new file mode 100644 index 000000000..08e5c682e --- /dev/null +++ b/src/ui/diff/rowStyle.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, test } from "bun:test"; +import { contrastRatio } from "../lib/color"; +import { THEMES, TRANSPARENT_BACKGROUND, withTransparentSurfaces } from "../themes"; +import { cursorLineHighlightBg, stackCellPalette } from "./rowStyle"; + +const DARK = THEMES.find((theme) => theme.id === "github-dark-dimmed")!; +const LIGHT = THEMES.find((theme) => theme.id === "github-light-default")!; + +describe("cursorLineHighlightBg", () => { + test("marks context rows on transparent surfaces", () => { + for (const base of [DARK, LIGHT]) { + const theme = withTransparentSurfaces(base); + const context = stackCellPalette("context", theme); + + expect(context.contentBg).toBe(TRANSPARENT_BACKGROUND); + expect(cursorLineHighlightBg(context.contentBg, theme)).not.toBe(TRANSPARENT_BACKGROUND); + } + }); + + test("keeps the marked row readable on every built-in theme", () => { + for (const base of THEMES) { + for (const theme of [base, withTransparentSurfaces(base)]) { + for (const kind of ["context", "addition", "deletion"] as const) { + const marked = cursorLineHighlightBg(stackCellPalette(kind, theme).contentBg, theme); + expect(contrastRatio(theme.text, marked)).toBeGreaterThan(3); + } + } + } + }); + + test("moves added and removed rows as far as it moves context rows", () => { + const context = stackCellPalette("context", DARK).contentBg; + const added = stackCellPalette("addition", DARK).contentBg; + + // Blending toward one fixed highlight color barely moved a background already sharing its + // hue, which left the marker invisible on added rows. + const shift = (from: string) => { + const to = cursorLineHighlightBg(from, DARK); + return contrastRatio(to, from); + }; + + expect(shift(added)).toBeGreaterThan(1.2); + expect(shift(context)).toBeGreaterThan(1.2); + }); +}); diff --git a/src/ui/diff/rowStyle.ts b/src/ui/diff/rowStyle.ts index df5274b50..351408fea 100644 --- a/src/ui/diff/rowStyle.ts +++ b/src/ui/diff/rowStyle.ts @@ -1,10 +1,12 @@ -import type { AppTheme } from "../themes"; +import { TRANSPARENT_BACKGROUND, type AppTheme } from "../themes"; import { blendHex } from "../lib/color"; import type { SplitLineCell, StackLineCell } from "./pierre"; const INACTIVE_RAIL_BLEND = 0.35; const SELECTION_BG_BLEND = 0.75; +const CURSOR_LINE_BG_BLEND = 0.2; const selectionBackgroundCache = new WeakMap>(); +const cursorLineBackgroundCache = new WeakMap>(); /** The diff rail marker is always visible in Hunk stack and split rows. */ export function diffRailMarker() { @@ -32,6 +34,35 @@ export function selectionHighlightBg(baseBg: string, theme: AppTheme) { return background; } +/** + * Lift a cell background toward the theme text color to mark the current line. + * + * Shifts luminance rather than hue: blending toward one fixed color barely moves a background + * already sharing that hue, which left the marker invisible on added rows. + */ +export function cursorLineHighlightBg(baseBg: string, theme: AppTheme) { + // Marking a row means painting it, so a transparent surface still gets a band. Blend from the + // appearance's own extreme, since reading the sentinel as a color yields black on light themes. + const source = + baseBg === TRANSPARENT_BACKGROUND + ? theme.appearance === "dark" + ? "#000000" + : "#ffffff" + : baseBg; + + let backgrounds = cursorLineBackgroundCache.get(theme); + if (!backgrounds) { + backgrounds = new Map(); + cursorLineBackgroundCache.set(theme, backgrounds); + } + let background = backgrounds.get(baseBg); + if (!background) { + background = blendHex(theme.text, source, CURSOR_LINE_BG_BLEND); + backgrounds.set(baseBg, background); + } + return background; +} + /** Return the neutral active-hunk rail color for the current theme. */ export function neutralRailColor(theme: AppTheme) { return theme.lineNumberFg; From 52924a519ecafcc680ef74a6973f2970fdb14151 Mon Sep 17 00:00:00 2001 From: loganthomas Date: Mon, 3 Aug 2026 08:43:21 -0500 Subject: [PATCH 04/20] feat(review): track the current line beside the selection Line and hunk are two granularities of one review position, so the cursor lives next to `selectedHunkIndex` and the two follow each other: moving the line carries the selection, and `]` or the sidebar re-seeds the line. Reads go through a ref because a held key drains as one stdin chunk, and batched state would leave every press in the burst seeing the same row. --- src/ui/hooks/useReviewController.test.tsx | 199 +++++++++++++++++++++- src/ui/hooks/useReviewController.ts | 74 +++++++- 2 files changed, 270 insertions(+), 3 deletions(-) diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 18d9d0079..0eda96e04 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -98,7 +98,7 @@ function ReviewControllerHarness({ onSetFiles?: (setFiles: (nextFiles: DiffFile[]) => void) => void; }) { const [files, setFiles] = useState(initialFiles); - const controller = useReviewController({ files, noteGeometry, stmlEnabled }); + const controller = useReviewController({ files, layout: "stack", noteGeometry, stmlEnabled }); useEffect(() => { onController(controller); @@ -1304,4 +1304,201 @@ describe("useReviewController", () => { }); } }); + + test("seeds the current line at the selected hunk so the indicator is visible on launch", async () => { + const { controllerRef, setup } = await renderReviewController([createAlphaFile()]); + + try { + await flush(setup); + + const cursor = expectValue(expectValue(controllerRef.current).lineCursor); + expect(cursor.fileId).toBe("alpha"); + expect(cursor.hunkIndex).toBe(0); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("moves the current line one row at a time and clamps at the top of the stream", async () => { + const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]); + + try { + await flush(setup); + const first = expectValue(expectValue(controllerRef.current).lineCursor); + + await act(async () => { + expectValue(controllerRef.current).moveLineCursor(1); + }); + await flush(setup); + const second = expectValue(expectValue(controllerRef.current).lineCursor); + expect(second).not.toEqual(first); + + await act(async () => { + expectValue(controllerRef.current).moveLineCursor(-1); + }); + await flush(setup); + expect(expectValue(controllerRef.current).lineCursor).toEqual(first); + + await act(async () => { + expectValue(controllerRef.current).moveLineCursor(-1); + }); + await flush(setup); + expect(expectValue(controllerRef.current).lineCursor).toEqual(first); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("requests a reveal every time the current line moves", async () => { + const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]); + + try { + await flush(setup); + const initialRequestId = expectValue(controllerRef.current).lineCursorRevealRequestId; + + await act(async () => { + expectValue(controllerRef.current).moveLineCursor(1); + }); + await flush(setup); + + expect(expectValue(controllerRef.current).lineCursorRevealRequestId).toBe( + initialRequestId + 1, + ); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("carries hunk selection along as the current line crosses a hunk boundary", async () => { + const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]); + + try { + await flush(setup); + expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(0); + + // Walk far enough that the stream has to roll into the file's second hunk. + for (let step = 0; step < 40; step += 1) { + await act(async () => { + expectValue(controllerRef.current).moveLineCursor(1); + }); + } + await flush(setup); + + const cursor = expectValue(expectValue(controllerRef.current).lineCursor); + expect(cursor.hunkIndex).toBe(1); + expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("moves the current line to the row a note is started on", async () => { + const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]); + + try { + await flush(setup); + + await act(async () => { + expectValue(controllerRef.current).startUserNote("alpha", 1, { side: "new", line: 12 }); + }); + await flush(setup); + + expect(expectValue(controllerRef.current).lineCursor).toEqual({ + fileId: "alpha", + hunkIndex: 1, + target: { side: "new", line: 12 }, + }); + expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("carries the current line along when hunk navigation moves the selection", async () => { + const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]); + + try { + await flush(setup); + expect(expectValue(expectValue(controllerRef.current).lineCursor).hunkIndex).toBe(0); + + await act(async () => { + expectValue(controllerRef.current).moveToHunk(1); + }); + await flush(setup); + + expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(1); + expect(expectValue(expectValue(controllerRef.current).lineCursor).hunkIndex).toBe(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("recovers the current line when a reload retires the hunk it was on", async () => { + const { controllerRef, setFilesRef, setup } = await renderReviewController([ + createTwoHunkFile(), + ]); + + try { + await flush(setup); + + await act(async () => { + expectValue(controllerRef.current).selectHunk("alpha", 1); + }); + await flush(setup); + expect(expectValue(expectValue(controllerRef.current).lineCursor).hunkIndex).toBe(1); + + await act(async () => { + expectValue(setFilesRef.current)([createSingleHunkFile()]); + }); + await flush(setup); + + const cursor = expectValue(expectValue(controllerRef.current).lineCursor); + expect(cursor.fileId).toBe("alpha"); + expect(cursor.hunkIndex).toBe(0); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("re-seeds the current line into the file a filter leaves visible", async () => { + const { controllerRef, setup } = await renderReviewController([ + createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"), + createDiffFile( + "beta", + "beta.ts", + "export const beta = 1;\n", + "export const betaValue = 2;\n", + ), + ]); + + try { + await flush(setup); + expect(expectValue(expectValue(controllerRef.current).lineCursor).fileId).toBe("alpha"); + + await act(async () => { + expectValue(controllerRef.current).setFilter("beta"); + }); + await flush(setup); + + expect(expectValue(expectValue(controllerRef.current).lineCursor).fileId).toBe("beta"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); }); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 9f211807c..864b89ebe 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -41,6 +41,13 @@ import type { FileSourceStatus } from "../diff/expandCollapsedRows"; import { selectGapForKeyboardToggle } from "../diff/expandCollapsedRows"; import { trailingCollapsedLines } from "../diff/pierre"; import { findNextHunkCursor } from "../lib/hunks"; +import { + buildLineCursors, + findNextLineCursor, + firstLineCursorInHunk, + resolveLineCursor, + type LineCursor, +} from "../lib/lineCursors"; import { agentNoteMarkupWidth } from "../lib/agentNoteGeometry"; import { reviewNoteSource } from "../lib/agentAnnotations"; import { STML_REFERENCE_WIDTH, validateStmlMarkup } from "../lib/stml/layout"; @@ -138,6 +145,9 @@ export interface ReviewController { reviewNoteCount: number; reviewNoteSummaries: SessionReviewNoteSummary[]; userNotesByFileId: Record; + lineCursor: LineCursor | null; + lineCursorRevealRequestId: number; + moveLineCursor: (delta: number) => void; moveToAnnotatedFile: (delta: number) => void; moveToAnnotatedHunk: (delta: number) => void; moveToFile: (delta: number) => void; @@ -179,6 +189,7 @@ export interface ReviewController { fileId?: string, hunkIndex?: number, target?: UserNoteLineTarget, + options?: { preserveViewport?: boolean }, ) => DraftReviewNote | null; setFilter: (value: string) => void; updateDraftNote: (body: string) => void; @@ -194,10 +205,13 @@ export interface AgentNoteGeometrySnapshot { export function useReviewController({ files, + layout, noteGeometry, stmlEnabled = false, }: { files: DiffFile[]; + /** Resolved layout, so line navigation follows the order rows are actually rendered in. */ + layout: Exclude; /** Allow STML bodies for live comments in this explicitly opted-in session. */ stmlEnabled?: boolean; /** @@ -212,6 +226,11 @@ export function useReviewController({ const [selectedHunkIndex, setSelectedHunkIndex] = useState(0); const [selectedFileTopAlignRequestId, setSelectedFileTopAlignRequestId] = useState(0); const [selectedHunkRevealRequestId, setSelectedHunkRevealRequestId] = useState(0); + const [lineCursor, setLineCursor] = useState(null); + // A held key drains as one stdin chunk, so every press in the burst would otherwise read the + // same pre-batch state and the cursor would advance a single row. + const lineCursorRef = useRef(null); + const [lineCursorRevealRequestId, setLineCursorRevealRequestId] = useState(0); const [scrollToNote, setScrollToNote] = useState(false); const [liveCommentsByFileId, setLiveCommentsByFileId] = useState>( {}, @@ -357,6 +376,52 @@ export function useReviewController({ reconcileSelectedHunkIndex(); }, [reconcileSelectedHunkIndex]); + const lineCursors = useMemo(() => buildLineCursors(visibleFiles, layout), [layout, visibleFiles]); + + /** + * Keep the current line on a row the review stream still renders. + * + * Seeding from the selected hunk makes the marker visible from launch, not just after the + * first keypress. + */ + const applyLineCursor = useCallback((next: LineCursor | null) => { + lineCursorRef.current = next; + setLineCursor(next); + }, []); + + const reconcileLineCursor = useCallback(() => { + const resolved = resolveLineCursor(lineCursors, lineCursorRef.current); + if (resolved?.fileId === selectedFileId && resolved?.hunkIndex === selectedHunkIndex) { + applyLineCursor(resolved); + return; + } + + // Selection moved without the cursor, so `]` and the sidebar leave one review position + // rather than stranding the marker in the hunk the reviewer just left. + applyLineCursor(firstLineCursorInHunk(lineCursors, selectedFileId, selectedHunkIndex)); + }, [applyLineCursor, lineCursors, selectedFileId, selectedHunkIndex]); + + useEffect(() => { + reconcileLineCursor(); + }, [reconcileLineCursor]); + + /** Move the current line one row through the visible review stream. */ + const moveLineCursor = useCallback( + (delta: number) => { + const nextCursor = findNextLineCursor(lineCursors, lineCursorRef.current, delta); + if (!nextCursor) { + return; + } + + applyLineCursor(nextCursor); + setLineCursorRevealRequestId((current) => current + 1); + // Selection follows the line so notes and `[`/`]` agree on where the reviewer is; the + // reveal above already owns scrolling, so this must not scroll too. + selectHunk(nextCursor.fileId, nextCursor.hunkIndex, { preserveViewport: true }); + }, + [applyLineCursor, lineCursors, selectHunk], + ); + /** Move through the full visible review stream one hunk at a time. */ const moveToHunk = useCallback( (delta: number) => { @@ -861,6 +926,7 @@ export function useReviewController({ fileId = selectedFile?.id, hunkIndex = selectedHunkIndex, requestedTarget?: UserNoteLineTarget, + options?: { preserveViewport?: boolean }, ): DraftReviewNote | null => { const file = allFiles.find((candidate) => candidate.id === fileId); const hunk = file?.metadata.hunks[hunkIndex]; @@ -869,6 +935,7 @@ export function useReviewController({ } const target = requestedTarget ?? firstCommentTargetForHunk(hunk); + applyLineCursor({ fileId: file.id, hunkIndex, target }); const draft: DraftReviewNote = { id: `draft:${file.id}:${hunkIndex}:${Date.now()}`, fileId: file.id, @@ -885,11 +952,11 @@ export function useReviewController({ selectHunk( file.id, hunkIndex, - requestedTarget ? { preserveViewport: true } : { scrollToNote: true }, + options?.preserveViewport ? { preserveViewport: true } : { scrollToNote: true }, ); return draft; }, - [allFiles, selectHunk, selectedFile?.id, selectedHunkIndex], + [allFiles, applyLineCursor, selectHunk, selectedFile?.id, selectedHunkIndex], ); /** Update the body of the active draft note. */ @@ -1056,6 +1123,8 @@ export function useReviewController({ liveCommentCount, liveCommentSummaries, liveCommentsByFileId, + lineCursor, + lineCursorRevealRequestId, reviewNoteCount, reviewNoteSummaries, userNotesByFileId, @@ -1075,6 +1144,7 @@ export function useReviewController({ clearFilter, cancelDraftNote, clearLiveComments, + moveLineCursor, moveToAnnotatedFile, moveToAnnotatedHunk, moveToFile, From 444646bca6341a13f7581df45b356a040fe40073 Mon Sep 17 00:00:00 2001 From: loganthomas Date: Mon, 3 Aug 2026 08:43:34 -0500 Subject: [PATCH 05/20] feat(config): add the cursor_line setting `row` marks the whole row, `number` marks only the line number, and `off` removes the marker. Saved with the other view preferences, and settable per run with `--cursor-line`. --- src/core/cli.test.ts | 13 ++++++ src/core/cli.ts | 21 +++++++++- src/core/config.test.ts | 40 +++++++++++++++++++ src/core/config.ts | 20 ++++++++++ src/core/loaders.ts | 1 + src/core/types.ts | 4 ++ .../src/content/docs/docs/reference/cli.md | 1 + .../src/content/docs/docs/reference/config.md | 10 +++++ 8 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/core/cli.test.ts b/src/core/cli.test.ts index 33cf1bc81..0ac924770 100644 --- a/src/core/cli.test.ts +++ b/src/core/cli.test.ts @@ -151,6 +151,19 @@ describe("parseCli", () => { }); }); + test("parses the current-line style and rejects an unknown one", async () => { + const parsed = await parseCli(["bun", "hunk", "diff", "--cursor-line", "number"]); + + expect(parsed).toMatchObject({ + kind: "vcs", + options: { cursorLine: "number" }, + }); + + await expect(parseCli(["bun", "hunk", "diff", "--cursor-line", "sparkles"])).rejects.toThrow( + "Invalid cursor line style: sparkles", + ); + }); + test("accepts --experimental before the review command", async () => { const parsed = await parseCli(["bun", "hunk", "--experimental", "diff"]); diff --git a/src/core/cli.ts b/src/core/cli.ts index 38efebbb9..f95b66d09 100644 --- a/src/core/cli.ts +++ b/src/core/cli.ts @@ -4,6 +4,7 @@ import { Command, Option } from "commander"; import type { CliInput, CommonOptions, + CursorLine, HelpCommandInput, LayoutMode, PagerCommandInput, @@ -38,7 +39,7 @@ import { resolveCliVersion } from "./version"; export interface CliReferenceOption { readonly flag: string; readonly description: string; - readonly parse?: "layout" | "positiveInt" | "tabWidth" | "collect"; + readonly parse?: "layout" | "cursorLine" | "positiveInt" | "tabWidth" | "collect"; readonly defaultValue?: string; /** Default applied directly by Commander (as opposed to a config-resolved default). */ readonly commanderDefault?: string; @@ -59,6 +60,11 @@ export interface CliReferenceCommand { /** Review flags registered on every full-screen review command. */ export const COMMON_REVIEW_OPTIONS = [ { flag: "--mode ", description: "layout mode: auto, split, stack", parse: "layout" }, + { + flag: "--cursor-line