From 12fd5e93cea4029809420f0ab70f7148dd1d08b0 Mon Sep 17 00:00:00 2001 From: Justin Giancola Date: Thu, 13 Aug 2026 13:51:48 -0400 Subject: [PATCH 1/7] feat(extension-api): let navigation address a single source line selectHunk was the finest target an extension had, so a match on line 211 of a 300-line hunk landed the viewport on the hunk anchor, pages above the thing the extension had just marked. revealLine names the line itself, and the guard refuses a hidden file, a side outside the two diff sides, and a line number no patch could have written. --- src/extension-api/types.ts | 16 ++++++ src/ui/lib/extensionNavigation.test.ts | 75 ++++++++++++++++++++++++++ src/ui/lib/extensionNavigation.ts | 40 ++++++++++++++ 3 files changed, 131 insertions(+) diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index ac1375a6b..e11ea29ae 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -1012,6 +1012,22 @@ export interface ExtensionReviewNavigation { selectFile(fileId: string): void; /** Jump the review stream to one hunk of one file. */ selectHunk(fileId: string, hunkIndex: number): void; + /** + * Jump the review stream to one source line, addressed by side and number. + * + * The finest navigation target there is: a hunk hundreds of lines tall no + * longer lands the viewport pages away from the line you meant. `line` is a + * 1-based number on `side` as the patch numbers it, so a context line + * answers to either side's number. The revealed line lands where every other + * Hunk reveal lands — a little below the viewport top — and becomes the + * current line, so it pairs with a mark from `registerLineHighlighter`. + * + * When the review cannot render that line (it sits inside a collapsed gap, + * or the patch never numbered it) the jump falls back to the hunk containing + * it; a line no hunk contains is refused with a warning naming the + * extension. + */ + revealLine(fileId: string, side: "old" | "new", line: number): void; } /** diff --git a/src/ui/lib/extensionNavigation.test.ts b/src/ui/lib/extensionNavigation.test.ts index 9f50fcc47..7321fc238 100644 --- a/src/ui/lib/extensionNavigation.test.ts +++ b/src/ui/lib/extensionNavigation.test.ts @@ -10,10 +10,12 @@ function createTestNavigation(options?: { files?: ReturnType[]; onSelectFile?: (fileId: string) => void; onSelectHunk?: (fileId: string, hunkIndex: number) => void; + revealResult?: "line" | "hunk" | "none"; }) { const warnings: string[] = []; const selectedFiles: string[] = []; const selectedHunks: Array<[string, number]> = []; + const revealedLines: Array<[string, string, number]> = []; let files = options?.files ?? [createTestNavigableFile("a", 3)]; const navigation = createGuardedReviewNavigation({ @@ -27,6 +29,10 @@ function createTestNavigation(options?: { onSelectFile: options?.onSelectFile ?? ((fileId) => selectedFiles.push(fileId)), onSelectHunk: options?.onSelectHunk ?? ((fileId, hunkIndex) => selectedHunks.push([fileId, hunkIndex])), + onRevealLine: (fileId, side, line) => { + revealedLines.push([fileId, side, line]); + return options?.revealResult ?? "line"; + }, }); return { @@ -34,6 +40,7 @@ function createTestNavigation(options?: { warnings, selectedFiles, selectedHunks, + revealedLines, setFiles(next: ReturnType[]) { files = next; }, @@ -116,20 +123,88 @@ describe("createGuardedReviewNavigation", () => { }, onSelectFile: (fileId) => selectedFiles.push(fileId), onSelectHunk: () => {}, + onRevealLine: () => "line", }); navigation.selectFile("a"); alive = false; navigation.selectFile("a"); navigation.selectHunk("a", 0); + navigation.revealLine("a", "new", 1); expect(selectedFiles).toEqual(["a"]); expect(warnings).toEqual([ "Extension triage selectFile ignored — the review session was reloaded", "Extension triage selectHunk ignored — the review session was reloaded", + "Extension triage revealLine ignored — the review session was reloaded", ]); }); + test("routes a line reveal on a visible file through to the host callback", () => { + const { navigation, revealedLines, warnings } = createTestNavigation(); + + navigation.revealLine("a", "old", 211); + + expect(revealedLines).toEqual([["a", "old", 211]]); + expect(warnings).toEqual([]); + }); + + test("refuses a line reveal on a file the review stream cannot show", () => { + const { navigation, revealedLines, warnings } = createTestNavigation(); + + navigation.revealLine("hidden", "new", 4); + + expect(revealedLines).toEqual([]); + expect(warnings).toEqual(['Extension triage revealLine targeted unknown file id "hidden"']); + }); + + test("refuses a side outside the two diff sides", () => { + const { navigation, revealedLines, warnings } = createTestNavigation(); + + navigation.revealLine("a", "both" as unknown as "new", 4); + + expect(revealedLines).toEqual([]); + expect(warnings).toEqual(['Extension triage revealLine received an invalid side for "a"']); + }); + + test("refuses a line number that is not a 1-based whole line", () => { + // Patches number lines from 1 upward; a fraction, a zero, or a string is a caller bug, + // not a line the review merely failed to find. + const { navigation, revealedLines, warnings } = createTestNavigation(); + + navigation.revealLine("a", "new", 0); + navigation.revealLine("a", "new", -3); + navigation.revealLine("a", "new", 2.5); + navigation.revealLine("a", "new", Number.NaN); + navigation.revealLine("a", "new", "4" as unknown as number); + + expect(revealedLines).toEqual([]); + expect(warnings).toEqual( + Array.from( + { length: 5 }, + () => 'Extension triage revealLine received an invalid line number for "a"', + ), + ); + }); + + test("stays quiet when the host falls back to the hunk containing the line", () => { + // A line inside a collapsed gap has no row to scroll to; landing on its hunk is the + // honest best effort, not a failure worth a toast. + const { navigation, warnings } = createTestNavigation({ revealResult: "hunk" }); + + navigation.revealLine("a", "new", 42); + + expect(warnings).toEqual([]); + }); + + test("warns when no hunk of the file covers the requested line", () => { + const { navigation, warnings } = createTestNavigation({ revealResult: "none" }); + + navigation.revealLine("a", "new", 9001); + + expect(warnings).toEqual(['Extension triage revealLine found no new line 9001 in "a"']); + }); + test("validates against the files visible at call time, not at creation", () => { // A command handler may await a dialog while a reload or filter changes // the review; navigation must judge the target against the current list. diff --git a/src/ui/lib/extensionNavigation.ts b/src/ui/lib/extensionNavigation.ts index 468ccc151..3a3232571 100644 --- a/src/ui/lib/extensionNavigation.ts +++ b/src/ui/lib/extensionNavigation.ts @@ -46,6 +46,13 @@ export interface CreateGuardedReviewNavigationOptions { notify: ExtensionNotifySink; onSelectFile: (fileId: string) => void; onSelectHunk: (fileId: string, hunkIndex: number) => void; + /** + * Jump to one source line, reporting which target the review could reach. + * + * `"line"` and `"hunk"` both moved the review; `"none"` means the line is nowhere in the + * file, which only this guard can report with the extension's name on it. + */ + onRevealLine: (fileId: string, side: "old" | "new", line: number) => "line" | "hunk" | "none"; } /** @@ -65,6 +72,7 @@ export function createGuardedReviewNavigation({ notify, onSelectFile, onSelectHunk, + onRevealLine, }: CreateGuardedReviewNavigationOptions): ExtensionReviewNavigation { /** Resolve a navigation target, or report one the review stream cannot show. */ const resolveVisibleFile = (method: string, fileId: string) => { @@ -124,5 +132,37 @@ export function createGuardedReviewNavigation({ onSelectHunk(fileId, Math.min(Math.max(0, Math.floor(hunkIndex)), maxHunkIndex)); }); }, + revealLine(fileId: string, side: "old" | "new", line: number) { + guard("revealLine", () => { + if (!resolveVisibleFile("revealLine", fileId)) { + return; + } + + // Line numbers are 1-based whole numbers as a patch writes them; anything else is a + // bug in the caller rather than a line the review failed to find. + if (side !== "old" && side !== "new") { + notify( + `Extension ${extensionId} revealLine received an invalid side for "${fileId}"`, + "warning", + ); + return; + } + + if (typeof line !== "number" || !Number.isInteger(line) || line < 1) { + notify( + `Extension ${extensionId} revealLine received an invalid line number for "${fileId}"`, + "warning", + ); + return; + } + + if (onRevealLine(fileId, side, line) === "none") { + notify( + `Extension ${extensionId} revealLine found no ${side} line ${line} in "${fileId}"`, + "warning", + ); + } + }); + }, }); } From 0ec9552f17ca4426d93149252a8a25fa686f948d Mon Sep 17 00:00:00 2001 From: Justin Giancola Date: Thu, 13 Aug 2026 13:51:54 -0400 Subject: [PATCH 2/7] feat(ui): reveal an extension-named line at the app's reveal position The line-cursor machinery already resolves a measured row and scrolls to it, but its stepping policy moves the minimum distance, which leaves a jump to a line that happens to be on screen exactly where it was. Give the reveal request an explicit placement so a jump lands the line where hunk and note reveals land theirs, and resolve (side, line) against the stops measured for every visible file so a cross-file jump needs no second pass. A line no measured row covers degrades to its hunk. --- src/ui/App.tsx | 20 ++- src/ui/components/panes/DiffPane.tsx | 35 +++-- .../components/panes/ExtensionPane.test.tsx | 3 + src/ui/components/panes/ExtensionPane.tsx | 5 +- src/ui/components/ui-components.test.tsx | 7 +- src/ui/diff/reviewRenderPlan.ts | 22 ++++ src/ui/hooks/useReviewController.test.tsx | 122 ++++++++++++++++-- src/ui/hooks/useReviewController.ts | 65 +++++++++- src/ui/lib/hunkScroll.ts | 10 ++ src/ui/lib/lineCursors.test.ts | 55 ++++++++ src/ui/lib/lineCursors.ts | 34 +++++ 11 files changed, 346 insertions(+), 32 deletions(-) diff --git a/src/ui/App.tsx b/src/ui/App.tsx index e198f57f7..b3e6d845f 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -84,7 +84,11 @@ import { useExtensionDialogController } from "./hooks/useExtensionDialogControll import { useExtensionNotifications } from "./hooks/useExtensionNotifications"; import { useHunkSessionBridge } from "./hooks/useHunkSessionBridge"; import { useMenuController } from "./hooks/useMenuController"; -import { useReviewController, type AgentNoteGeometrySnapshot } from "./hooks/useReviewController"; +import { + useReviewController, + type AgentNoteGeometrySnapshot, + type RevealedLineResult, +} from "./hooks/useReviewController"; import { useWatchedInput, type WatchedInputRuntime } from "./hooks/useWatchedInput"; import { agentNoteMarkupWidth } from "./lib/agentNoteGeometry"; import { @@ -491,6 +495,8 @@ export function App({ const extensionCommandNavigationRef = useRef({ onSelectFile: (_fileId: string) => {}, onSelectHunk: (_fileId: string, _hunkIndex: number) => {}, + onRevealLine: (_fileId: string, _side: "old" | "new", _line: number): RevealedLineResult => + "none", }); // A hard session reload (`resetApp`) remounts App under an in-flight async // command handler, whose `ctx.navigation` closes over *this* instance's @@ -878,6 +884,8 @@ export function App({ onSelectFile: (fileId) => extensionCommandNavigationRef.current.onSelectFile(fileId), onSelectHunk: (fileId, hunkIndex) => extensionCommandNavigationRef.current.onSelectHunk(fileId, hunkIndex), + onRevealLine: (fileId, side, line) => + extensionCommandNavigationRef.current.onRevealLine(fileId, side, line), }), }; @@ -1752,6 +1760,10 @@ export function App({ focusFiles(); review.selectHunk(fileId, hunkIndex); }, + onRevealLine: (fileId, side, line) => { + focusFiles(); + return review.revealLine(fileId, side, line); + }, }; /** Toggle keyboard focus between the file list and the file filter. */ @@ -2081,6 +2093,10 @@ export function App({ focusFiles(); review.selectHunk(fileId, hunkIndex); }} + onRevealLine={(fileId, side, line) => { + focusFiles(); + return review.revealLine(fileId, side, line); + }} onRenderFailure={ pane.key === HUNK_FILES_PANE_KEY ? undefined @@ -2215,7 +2231,7 @@ export function App({ selectedHunkRevealRequestId={review.selectedHunkRevealRequestId} cursorLine={cursorLine} lineCursor={review.lineCursor} - lineCursorRevealRequestId={review.lineCursorRevealRequestId} + lineCursorRevealRequest={review.lineCursorRevealRequest} lineCursorAlignmentRequest={lineCursorAlignmentRequest} theme={activeTheme} width={diffPaneWidth} diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 233be93fe..f40e1b1db 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -36,6 +36,7 @@ import { computeLineAlignmentScrollTop, computeLineRevealScrollTop, type CurrentLineAlignment, + type LineRevealPlacement, } from "../../lib/hunkScroll"; import { inlineNoteStableKey } from "../../diff/reviewRenderPlan"; import { @@ -218,7 +219,7 @@ export function DiffPane({ selectedHunkIndex, cursorLine = "off", lineCursor = null, - lineCursorRevealRequestId = 0, + lineCursorRevealRequest = { id: 0, placement: "nearest" }, lineCursorAlignmentRequest = { id: 0, alignment: "center" }, scrollToNote = false, draftNote = null, @@ -280,7 +281,7 @@ export function DiffPane({ selectedHunkIndex: number; cursorLine?: CursorLine; lineCursor?: LineCursor | null; - lineCursorRevealRequestId?: number; + lineCursorRevealRequest?: { id: number; placement: LineRevealPlacement }; lineCursorAlignmentRequest?: { id: number; alignment: CurrentLineAlignment }; scrollToNote?: boolean; draftNote?: DraftReviewNote | null; @@ -2110,13 +2111,13 @@ export function DiffPane({ suppressViewportSelectionSync, ]); - const previousLineCursorRevealRequestIdRef = useRef(lineCursorRevealRequestId); + const previousLineCursorRevealRequestIdRef = useRef(lineCursorRevealRequest.id); useLayoutEffect(() => { - if (previousLineCursorRevealRequestIdRef.current === lineCursorRevealRequestId) { + if (previousLineCursorRevealRequestIdRef.current === lineCursorRevealRequest.id) { return; } - previousLineCursorRevealRequestIdRef.current = lineCursorRevealRequestId; + previousLineCursorRevealRequestIdRef.current = lineCursorRevealRequest.id; const scrollBox = scrollRef.current; if (!scrollBox || !lineCursor) { @@ -2129,12 +2130,22 @@ export function DiffPane({ } const viewportHeight = scrollBox.viewport.height || scrollViewport.height; - const revealScrollTop = computeLineRevealScrollTop({ - lineTop: bounds.top, - lineHeight: bounds.height, - scrollTop: scrollBox.scrollTop, - viewportHeight, - }); + // A jump lands the line where hunk and note reveals land theirs; stepping only closes the + // gap to the viewport edge, so a held key does not drag the whole stream past the marker. + const revealScrollTop = + lineCursorRevealRequest.placement === "reveal" + ? computeHunkRevealScrollTop({ + hunkTop: bounds.top, + hunkHeight: bounds.height, + preferredTopPadding: Math.max(2, Math.floor(viewportHeight * 0.25)), + viewportHeight, + }) + : computeLineRevealScrollTop({ + lineTop: bounds.top, + lineHeight: bounds.height, + scrollTop: scrollBox.scrollTop, + viewportHeight, + }); if (revealScrollTop === scrollBox.scrollTop) { return; } @@ -2145,7 +2156,7 @@ export function DiffPane({ clampReviewScrollTop, lineCursor, lineCursorBoundsOf, - lineCursorRevealRequestId, + lineCursorRevealRequest, scrollRef, scrollViewport.height, suppressViewportSelectionSync, diff --git a/src/ui/components/panes/ExtensionPane.test.tsx b/src/ui/components/panes/ExtensionPane.test.tsx index ededa92e0..135ddb4c3 100644 --- a/src/ui/components/panes/ExtensionPane.test.tsx +++ b/src/ui/components/panes/ExtensionPane.test.tsx @@ -89,6 +89,7 @@ describe("ExtensionPaneHost actions", () => { notify={(message) => notifications.push(message)} onSelectFile={() => {}} onSelectHunk={(fileId, hunkIndex) => hunkSelections.push([fileId, hunkIndex])} + onRevealLine={() => "line"} />, async () => { if (!actions) { @@ -156,6 +157,7 @@ describe("ExtensionPaneHost failure recovery", () => { notify={(message) => notifications.push(message)} onSelectFile={() => {}} onSelectHunk={() => {}} + onRevealLine={() => "line"} />, async (setup) => { expect(setup.captureCharFrame()).toContain("Files pane unavailable"); @@ -199,6 +201,7 @@ describe("ExtensionPaneHost failure recovery", () => { notify={(message) => notifications.push(message)} onSelectFile={() => {}} onSelectHunk={() => {}} + onRevealLine={() => "line"} /> ); } diff --git a/src/ui/components/panes/ExtensionPane.tsx b/src/ui/components/panes/ExtensionPane.tsx index 21f35b48f..3689fd34c 100644 --- a/src/ui/components/panes/ExtensionPane.tsx +++ b/src/ui/components/panes/ExtensionPane.tsx @@ -66,6 +66,7 @@ export interface ExtensionPaneHostProps { notify: ExtensionNotifySink; onSelectFile: (fileId: string) => void; onSelectHunk: (fileId: string, hunkIndex: number) => void; + onRevealLine: (fileId: string, side: "old" | "new", line: number) => "line" | "hunk" | "none"; onRenderFailure?: () => void; } @@ -86,6 +87,7 @@ function ExtensionPaneHostView({ notify, onSelectFile, onSelectHunk, + onRevealLine, onRenderFailure, }: ExtensionPaneHostProps) { const { extensionId } = registered; @@ -99,12 +101,13 @@ function ExtensionPaneHostView({ notify, onSelectFile, onSelectHunk, + onRevealLine, }), notify(message: string, type: ExtensionNotifyType = "info") { notify(`${extensionId}: ${message}`, type); }, }), - [extensionId, files, notify, onSelectFile, onSelectHunk], + [extensionId, files, notify, onRevealLine, onSelectFile, onSelectHunk], ); const View = registered.pane.component as (props: ExtensionPaneProps) => ReactNode; const viewProps: ExtensionPaneProps = { diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 88ac91af2..68fca331b 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -461,7 +461,12 @@ describe("UI components", () => { theme={theme} width={30} keybindings={{ matches: () => false, getKeys: () => [] }} - actions={{ selectFile: () => {}, selectHunk: () => {}, notify: () => {} }} + actions={{ + selectFile: () => {}, + selectHunk: () => {}, + revealLine: () => {}, + notify: () => {}, + }} />, 36, 12, diff --git a/src/ui/diff/reviewRenderPlan.ts b/src/ui/diff/reviewRenderPlan.ts index 13c0e230f..b6888966c 100644 --- a/src/ui/diff/reviewRenderPlan.ts +++ b/src/ui/diff/reviewRenderPlan.ts @@ -96,6 +96,7 @@ function contextLineStableKey(hunkIndex: number, oldLineNumber?: number, newLine const SIDED_LINE_STABLE_KEY = /^line:(\d+):(old|new):(\d+)$/; const CONTEXT_LINE_STABLE_KEY = /^line:(\d+):context:\d+:(\d+)$/; +const CONTEXT_LINE_STABLE_KEY_SIDES = /^line:(\d+):context:(\d+):(\d+)$/; /** Recover the source line one single-sided stable anchor names. */ export function lineStableKeyTarget( @@ -125,6 +126,27 @@ export function contextLineStableKeyTarget( return { hunkIndex: Number(match[1]), side: "new", line: Number(match[2]) }; } +/** + * Recover both source lines one shared context anchor names. + * + * A context row shows the same text on both sides under two different numbers, so a caller + * addressing it by line number has to be able to match either one. + */ +export function contextLineStableKeySides( + stableKey: string, +): { hunkIndex: number; oldLine: number; newLine: number } | null { + const match = CONTEXT_LINE_STABLE_KEY_SIDES.exec(stableKey); + if (!match) { + return null; + } + + return { + hunkIndex: Number(match[1]), + oldLine: Number(match[2]), + newLine: Number(match[3]), + }; +} + /** Resolve the stable anchor keys for one rendered diff row across split and stack layouts. */ function diffRowStableKeys(row: DiffRow) { if (row.type === "collapsed") { diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 42152197c..944989722 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -49,7 +49,7 @@ function createTwoHunkFile() { } /** Build one file with three separated hunks for counted navigation coverage. */ -function createThreeHunkFile() { +function createThreeHunkFile(id = "alpha", path = "alpha.ts") { const beforeLines = Array.from( { length: 30 }, (_, index) => `export const line${index + 1} = ${index + 1};`, @@ -59,7 +59,7 @@ function createThreeHunkFile() { afterLines[14] = "export const line15 = 1500;"; afterLines[29] = "export const line30 = 3000;"; - return createDiffFile("alpha", "alpha.ts", lines(...beforeLines), lines(...afterLines)); + return createDiffFile(id, path, lines(...beforeLines), lines(...afterLines)); } /** Build the same file id with only one hunk so stale hunk indices must clamp. */ @@ -155,12 +155,15 @@ function expectValue(value: T): NonNullable { function ReviewControllerHarness({ initialFiles, noteGeometry, + publishLineCursors = true, stmlEnabled, onController, onSetFiles, }: { initialFiles: DiffFile[]; noteGeometry?: Parameters[0]["noteGeometry"]; + /** Publish measured stops, as the diff pane does unless the current-line marker is off. */ + publishLineCursors?: boolean; stmlEnabled?: boolean; onController: (controller: ReviewController) => void; onSetFiles?: (setFiles: (nextFiles: DiffFile[]) => void) => void; @@ -172,6 +175,10 @@ function ReviewControllerHarness({ const { expandedGapsByFileId, sourceStatusByFileId } = controller; useEffect(() => { + if (!publishLineCursors) { + return; + } + setLineCursors( buildLineCursors( visibleFiles, @@ -191,7 +198,7 @@ function ReviewControllerHarness({ ), ), ); - }, [expandedGapsByFileId, sourceStatusByFileId, visibleFiles]); + }, [expandedGapsByFileId, publishLineCursors, sourceStatusByFileId, visibleFiles]); useEffect(() => { onController(controller); @@ -210,10 +217,12 @@ async function renderReviewController( { strictMode = false, noteGeometry, + publishLineCursors, stmlEnabled, }: { strictMode?: boolean; noteGeometry?: Parameters[0]["noteGeometry"]; + publishLineCursors?: boolean; stmlEnabled?: boolean; } = {}, ) { @@ -223,6 +232,7 @@ async function renderReviewController( { controllerRef.current = nextController; @@ -1578,16 +1588,17 @@ describe("useReviewController", () => { try { await flush(setup); - const initialRequestId = expectValue(controllerRef.current).lineCursorRevealRequestId; + const initialRequestId = expectValue(controllerRef.current).lineCursorRevealRequest.id; await act(async () => { expectValue(controllerRef.current).moveLineCursor(1); }); await flush(setup); - expect(expectValue(controllerRef.current).lineCursorRevealRequestId).toBe( - initialRequestId + 1, - ); + expect(expectValue(controllerRef.current).lineCursorRevealRequest).toEqual({ + id: initialRequestId + 1, + placement: "nearest", + }); } finally { await act(async () => { setup.renderer.destroy(); @@ -1621,7 +1632,7 @@ describe("useReviewController", () => { (cursor) => cursor.stableKey === initial.stableKey && cursor.fileId === initial.fileId, ); const expected = expectValue(expectedCursors[initialIndex + 4]); - const initialRequestId = expectValue(controllerRef.current).lineCursorRevealRequestId; + const initialRequestId = expectValue(controllerRef.current).lineCursorRevealRequest.id; await act(async () => { expectValue(controllerRef.current).moveLineCursor(4); @@ -1629,9 +1640,10 @@ describe("useReviewController", () => { await flush(setup); expect(expectValue(controllerRef.current).lineCursor).toEqual(expected); - expect(expectValue(controllerRef.current).lineCursorRevealRequestId).toBe( - initialRequestId + 1, - ); + expect(expectValue(controllerRef.current).lineCursorRevealRequest).toEqual({ + id: initialRequestId + 1, + placement: "nearest", + }); } finally { await act(async () => { setup.renderer.destroy(); @@ -1797,4 +1809,92 @@ describe("useReviewController", () => { }); } }); + + test("reveals one line of another file and asks for the reveal placement", async () => { + // Cursors are measured for every visible file, so a cross-file jump resolves in the same + // pass as a local one — no pending second reveal once the target file renders. + const { controllerRef, setup } = await renderReviewController([ + createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"), + createThreeHunkFile("beta", "beta.ts"), + ]); + + try { + await flush(setup); + const before = expectValue(controllerRef.current).lineCursorRevealRequest; + expect(expectValue(expectValue(controllerRef.current).lineCursor).fileId).toBe("alpha"); + + let outcome: string | undefined; + await act(async () => { + outcome = expectValue(controllerRef.current).revealLine("beta", "new", 30); + }); + await flush(setup); + + expect(outcome).toBe("line"); + expect(expectValue(controllerRef.current).lineCursor).toMatchObject({ + fileId: "beta", + hunkIndex: 2, + target: { side: "new", line: 30 }, + }); + // Selection follows the revealed line so notes and hunk actions stay on the same target. + expect(expectValue(controllerRef.current).selectedFileId).toBe("beta"); + expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(2); + expect(expectValue(controllerRef.current).lineCursorRevealRequest).toEqual({ + id: before.id + 1, + placement: "reveal", + }); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("falls back to the containing hunk when nothing measured a row for the line", async () => { + // With the current-line marker off the pane publishes no stops at all, so there is no + // measured row to scroll to; the hunk covering the line is the closest honest landing spot. + const { controllerRef, setup } = await renderReviewController([createThreeHunkFile()], { + publishLineCursors: false, + }); + + try { + await flush(setup); + const before = expectValue(controllerRef.current).lineCursorRevealRequest; + + let outcome: string | undefined; + await act(async () => { + outcome = expectValue(controllerRef.current).revealLine("alpha", "new", 15); + }); + await flush(setup); + + expect(outcome).toBe("hunk"); + expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(1); + // A hunk selection reveal, not a line reveal: nothing measured the requested row. + expect(expectValue(controllerRef.current).lineCursorRevealRequest.id).toBe(before.id); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("reports a line no hunk of the file covers instead of moving the review", async () => { + const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]); + + try { + await flush(setup); + + let outcome: string | undefined; + await act(async () => { + outcome = expectValue(controllerRef.current).revealLine("alpha", "new", 9001); + }); + await flush(setup); + + expect(outcome).toBe("none"); + expect(expectValue(expectValue(controllerRef.current).lineCursor).hunkIndex).toBe(0); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); }); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index db838847e..ab99a84d7 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -33,6 +33,7 @@ import { } from "../../core/review/intents"; import { projectReviewDocument } from "../../core/review/document"; import { reviewExpansionSide } from "../../core/review/expansion"; +import { reviewHunkIndexForLine } from "../../core/review/geometry"; import type { ReviewSelectionScope } from "../../core/review/navigation"; import { reviewFileKeysWithRetiredContent, @@ -59,8 +60,10 @@ import type { } from "../../session/types"; import type { FileSourceStatus } from "../diff/expandCollapsedRows"; +import type { LineRevealPlacement } from "../lib/hunkScroll"; import { EMPTY_LINE_CURSORS, + findLineCursorAt, findNextLineCursor, firstLineCursorInHunk, hasLineCursor, @@ -138,6 +141,25 @@ interface SourceLoadRequest { side: "old" | "new"; } +/** + * One request to scroll the current line into view, and how far it may move the viewport. + * + * Carried as one object because the id and the placement have to change together: a consumer + * that read a bumped id against a stale placement would scroll by the previous policy. + */ +export interface LineCursorRevealRequest { + id: number; + placement: LineRevealPlacement; +} + +/** + * What a `revealLine` call could actually reach. + * + * `"line"` landed on the requested line, `"hunk"` fell back to the hunk containing it because + * the review stream draws no row for that line, and `"none"` means no hunk covers it either. + */ +export type RevealedLineResult = "line" | "hunk" | "none"; + export interface ReviewSelectionOptions { alignFileHeaderTop?: boolean; scrollToNote?: boolean; @@ -179,13 +201,14 @@ export interface ReviewController { showAgentNotes: boolean; userNotesByFileId: Record; lineCursor: LineCursor | null; - lineCursorRevealRequestId: number; + lineCursorRevealRequest: LineCursorRevealRequest; anchorLineCursor: (cursor: LineCursor) => void; /** Adopt the hunk a viewport settled on, without asking any viewport to move. */ anchorSelection: (fileId: string, hunkIndex: number) => void; moveLineCursor: (delta: number) => void; /** Step the selection through one navigable scope; the scope owns wrap and reveal. */ moveSelection: (scope: ReviewSelectionScope, delta: number) => void; + revealLine: (fileId: string, side: "old" | "new", line: number) => RevealedLineResult; scrollToNote: boolean; selectedFile: DiffFile | undefined; selectedFileId: string; @@ -315,7 +338,10 @@ export function useReviewController({ // 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 [lineCursorRevealRequest, setLineCursorRevealRequest] = useState({ + id: 0, + placement: "nearest", + }); const previousLineCursorsRef = useRef(lineCursors); const pendingLineCursorRef = useRef< | { kind: "reveal"; fileId: string; gapKey: string } @@ -488,9 +514,9 @@ export function useReviewController({ /** Move the current line to a row the reviewer just asked to see, and scroll to it. */ const revealLineCursor = useCallback( - (cursor: LineCursor) => { + (cursor: LineCursor, placement: LineRevealPlacement = "nearest") => { applyLineCursor(cursor); - setLineCursorRevealRequestId((current) => current + 1); + setLineCursorRevealRequest((current) => ({ id: current.id + 1, placement })); // The line cursor carries its own reveal request; the selection only follows it. anchorSelection(cursor.fileId, cursor.hunkIndex); }, @@ -583,6 +609,34 @@ export function useReviewController({ [annotations, runIntent], ); + /** + * Jump to one file's source line, addressed the way a patch numbers it. + * + * Cursors are measured for every visible file, not just the selected one, so a cross-file + * jump resolves in the same pass as a local one. A line the stream draws no row for — hidden + * inside a collapsed gap, or never numbered by a partial patch — has no row to scroll to, so + * the jump degrades to the hunk containing it and reports which target it reached. + */ + const revealLine = useCallback( + (fileId: string, side: "old" | "new", line: number): RevealedLineResult => { + const cursor = findLineCursorAt(lineCursors, fileId, side, line); + if (cursor) { + revealLineCursor(cursor, "reveal"); + return "line"; + } + + const file = visibleFiles.find((candidate) => candidate.id === fileId); + const hunkIndex = file ? reviewHunkIndexForLine(file.metadata.hunks, side, line) : -1; + if (hunkIndex < 0) { + return "none"; + } + + selectHunk(fileId, hunkIndex); + return "hunk"; + }, + [lineCursors, revealLineCursor, selectHunk, visibleFiles], + ); + /** Set the shared file filter. */ const setFilter = useCallback( (value: string) => { @@ -1148,7 +1202,7 @@ export function useReviewController({ liveCommentSummaries, liveCommentsByFileId, lineCursor, - lineCursorRevealRequestId, + lineCursorRevealRequest, reviewNoteCount: reviewNoteSummaries.length, reviewNoteSummaries, showAgentNotes: state.showAgentNotes, @@ -1176,6 +1230,7 @@ export function useReviewController({ navigateToLocation, removeLiveComment, removeUserNote, + revealLine, saveDraftNote, selectFile, selectHunk, diff --git a/src/ui/lib/hunkScroll.ts b/src/ui/lib/hunkScroll.ts index 60fb30e90..44f7252c9 100644 --- a/src/ui/lib/hunkScroll.ts +++ b/src/ui/lib/hunkScroll.ts @@ -57,6 +57,16 @@ export function computeLineAlignmentScrollTop({ return Math.max(0, top - Math.floor((viewport - height) / 2)); } +/** + * How far a current-line reveal may move the viewport. + * + * `"nearest"` is stepping: move only as far as it takes to bring the line on screen. + * `"reveal"` is a jump to somewhere the reviewer was not looking, so it lands the line where + * hunk and note reveals land it, a little below the viewport top, even when the line already + * happened to be visible. + */ +export type LineRevealPlacement = "nearest" | "reveal"; + /** * Pick a scroll target that brings the current line just into view. * diff --git a/src/ui/lib/lineCursors.test.ts b/src/ui/lib/lineCursors.test.ts index bc4d430ec..6a44b858e 100644 --- a/src/ui/lib/lineCursors.test.ts +++ b/src/ui/lib/lineCursors.test.ts @@ -11,6 +11,7 @@ import { resolveTheme } from "../themes"; import { buildLineCursors, clampLineCursorToViewport, + findLineCursorAt, findNextLineCursor, firstLineCursorInHunk, resolveLineCursor, @@ -194,6 +195,60 @@ describe("buildLineCursors", () => { }); }); +describe("findLineCursorAt", () => { + /** Build a file whose inserted line pushes the trailing context onto different side numbers. */ + function createShiftedContextFile() { + return createTestDiffFile({ + id: "alpha", + path: "alpha.ts", + before: lines("one", "two", "three"), + after: lines("one", "inserted", "two", "three"), + context: 3, + }); + } + + test("finds a changed line by the side the patch numbers it on", () => { + const cursors = cursorsFor([createTwoHunkFile("alpha", "alpha.ts")], "stack"); + + expect(findLineCursorAt(cursors, "alpha", "new", 10)?.target).toEqual({ + side: "new", + line: 10, + }); + expect(findLineCursorAt(cursors, "alpha", "old", 10)?.target).toEqual({ + side: "old", + line: 10, + }); + }); + + test("answers a context row to either side's number, even once they diverge", () => { + // "three" is old line 3 and new line 4 after the insertion; both address the same row. + const cursors = cursorsFor([createShiftedContextFile()], "stack"); + const byNew = findLineCursorAt(cursors, "alpha", "new", 4); + const byOld = findLineCursorAt(cursors, "alpha", "old", 3); + + expect(byNew?.stableKey).toBe("line:0:context:3:4"); + expect(byOld).toEqual(byNew); + }); + + test("stays inside the requested file when two files number the same line", () => { + const cursors = cursorsFor( + [createTwoHunkFile("alpha", "alpha.ts"), createTwoHunkFile("beta", "beta.ts")], + "stack", + ); + + expect(findLineCursorAt(cursors, "beta", "new", 1)?.fileId).toBe("beta"); + }); + + test("finds no cursor for a line the stream draws no row for", () => { + // Line 3 is inside the collapsed gap above the only hunk, so nothing measures it. + const cursors = cursorsFor([createCollapsedGapFile()], "stack"); + + expect(findLineCursorAt(cursors, "alpha", "new", 3)).toBeNull(); + expect(findLineCursorAt(cursors, "alpha", "new", 900)).toBeNull(); + expect(findLineCursorAt(cursors, "missing", "new", 6)).toBeNull(); + }); +}); + describe("findNextLineCursor", () => { const cursors = cursorsFor( [createTwoHunkFile("alpha", "alpha.ts"), createTwoHunkFile("beta", "beta.ts")], diff --git a/src/ui/lib/lineCursors.ts b/src/ui/lib/lineCursors.ts index 17c9af2f3..9329dce0a 100644 --- a/src/ui/lib/lineCursors.ts +++ b/src/ui/lib/lineCursors.ts @@ -8,6 +8,7 @@ import type { DiffFile, UserNoteLineTarget } from "../../core/types"; import type { DiffSectionGeometry, DiffSectionRowBounds } from "../diff/diffSectionGeometry"; import { + contextLineStableKeySides, contextLineStableKeyTarget, lineStableKey, lineStableKeyTarget, @@ -195,6 +196,39 @@ export function lineCursorAt( ); } +/** + * Check whether one cursor stands on the source line a caller named. + * + * A context row carries a single new-side target but renders one line under both sides' + * numbers, so its old-side number addresses the same stop. + */ +function lineCursorAddresses(cursor: LineCursor, side: "old" | "new", line: number) { + if (cursor.target.side === side && cursor.target.line === line) { + return true; + } + + const context = contextLineStableKeySides(cursor.stableKey); + return context !== null && (side === "old" ? context.oldLine : context.newLine) === line; +} + +/** + * Find the rendered stop one file's source line sits on, addressed by side and number. + * + * Answers only for lines the review stream actually draws: a line hidden inside a collapsed + * gap, or absent from a partial patch, has no cursor and no measured row to scroll to. + */ +export function findLineCursorAt( + cursors: LineCursor[], + fileId: string, + side: "old" | "new", + line: number, +): LineCursor | null { + return ( + cursors.find((cursor) => cursor.fileId === fileId && lineCursorAddresses(cursor, side, line)) ?? + null + ); +} + /** Read one cursor's measured extent in whole-stream rows. */ export type LineCursorBoundsLookup = (cursor: LineCursor) => VerticalBounds | undefined; From 5d43bf02ae4a06ede70b5fa6c354749bf21a97b3 Mon Sep 17 00:00:00 2001 From: Justin Giancola Date: Thu, 13 Aug 2026 13:53:05 -0400 Subject: [PATCH 3/7] test(pty): prove a deep line lands near the viewport top in a live session The regression this guards is spatial, so assert it where the user sees it: the marked line is off screen while the hunk anchor is selected, and one revealLine command puts it in the top half of the terminal. --- test/pty/extensions-integration.test.ts | 90 +++++++++++++++++++++++++ test/pty/harness.ts | 27 ++++++-- 2 files changed, 112 insertions(+), 5 deletions(-) diff --git a/test/pty/extensions-integration.test.ts b/test/pty/extensions-integration.test.ts index b4638ffb0..9929c618d 100644 --- a/test/pty/extensions-integration.test.ts +++ b/test/pty/extensions-integration.test.ts @@ -139,6 +139,47 @@ const LINE_HIGHLIGHT_EXTENSION_SOURCE = `export default function (hunk) { } `; +/** + * A single hunk tall enough that its anchor and its last lines cannot share a + * viewport, with one unmistakable token near the bottom. + * + * Every line differs, so git emits one hunk spanning the whole file — the shape + * `selectHunk` cannot navigate usefully. + */ +const REVEAL_LINE_TARGET = 111; +const REVEAL_LINE_TOKEN = "REVEALLINETOKEN"; +const TALL_HUNK_FILE = { + path: "tall.ts", + before: `${Array.from( + { length: 130 }, + (_, index) => `export const line${String(index + 1).padStart(3, "0")} = ${index + 1};`, + ).join("\n")}\n`, + after: `${Array.from({ length: 130 }, (_, index) => + index + 1 === REVEAL_LINE_TARGET + ? `export const needle = "${REVEAL_LINE_TOKEN}";` + : `export const line${String(index + 1).padStart(3, "0")} = ${index + 1001};`, + ).join("\n")}\n`, +}; + +/** + * An extension that jumps to one exact line of the reviewed file. + * + * The second command asks for a line no hunk covers, so the same session shows + * both halves of the contract: a reachable line scrolls, an unreachable one + * comes back as a warning naming the extension. + */ +const REVEAL_LINE_EXTENSION_SOURCE = `export default function (hunk) { + hunk.registerCommand({ id: "jump", title: "Jump to the needle", key: "f7" }, (ctx) => { + const file = ctx.selection.file; + if (file) ctx.navigation.revealLine(file.id, "new", ${REVEAL_LINE_TARGET}); + }); + hunk.registerCommand({ id: "jump-nowhere", title: "Jump past the file", key: "f8" }, (ctx) => { + const file = ctx.selection.file; + if (file) ctx.navigation.revealLine(file.id, "new", 9001); + }); +} +`; + const DIALOG_EXTENSION_SOURCE = `export default function (hunk) { hunk.registerCommand({ id: "ask", title: "Ask", key: "y" }, async (ctx) => { const proceed = await ctx.dialogs.confirm({ @@ -690,6 +731,55 @@ describe("PTY extensions", () => { } }); + test("revealLine lands a line deep inside one tall hunk near the viewport top", async () => { + const configHome = harness.createIsolatedConfigHome(); + const fixture = harness.createRepoExtensionFixture(REVEAL_LINE_EXTENSION_SOURCE, "fixture.ts", [ + TALL_HUNK_FILE, + ]); + const session = await harness.launchHunk({ + args: [ + "diff", + "--mode", + "stack", + "--extension", + join(fixture.dir, ".hunk", "extensions", "fixture.ts"), + ], + cwd: fixture.dir, + cols: 140, + rows: 24, + env: { XDG_CONFIG_HOME: configHome }, + }); + + try { + const review = await harness.waitForSnapshot( + session, + (text) => text.includes("tall.ts"), + 20_000, + ); + // This is the bug: the hunk anchor is on screen, the marked line is pages below it. + expect(review).not.toContain(REVEAL_LINE_TOKEN); + await harness.ensureKeyboardIsLive(session); + + await session.press("f7"); + const revealed = await harness.waitForSnapshot( + session, + (text) => text.includes(REVEAL_LINE_TOKEN), + 20_000, + ); + // Near the top of a 24-row terminal, where every other Hunk reveal lands: + // a little below the viewport edge, not scrolled just barely into view. + const row = lineIndexOf(revealed, REVEAL_LINE_TOKEN); + expect(row).toBeGreaterThan(0); + expect(row).toBeLessThan(12); + + await session.press("f8"); + const warned = await session.waitForText(/revealLine found no/, { timeout: 20_000 }); + expect(warned).toContain("Extension fixture revealLine found no new line 9001"); + } finally { + session.close(); + } + }); + test("a startup handler's notify renders as a toast and clears itself", async () => { const configHome = harness.createIsolatedConfigHome(); const fixture = harness.createRepoExtensionFixture(NOTIFY_EXTENSION_SOURCE); diff --git a/test/pty/harness.ts b/test/pty/harness.ts index d95f314e3..cb4fcece7 100644 --- a/test/pty/harness.ts +++ b/test/pty/harness.ts @@ -516,20 +516,37 @@ export function createPtyHarness() { * test shows only the two changed source files, keeping snapshot assertions * about the extension's effect unambiguous. */ - function createRepoExtensionFixture(source: string, entryName = "fixture.ts") { + function createRepoExtensionFixture( + source: string, + entryName = "fixture.ts", + changedFiles: ChangedFileSpec[] = [ + { + path: "alpha.ts", + before: "export const alpha = 1;\n", + after: "export const alphaValue = 2;\n", + }, + { + path: "beta.ts", + before: "export const beta = 1;\n", + after: "export const betaValue = 2;\n", + }, + ], + ) { const dir = makeTempDir("hunk-tuistory-extension-"); runGit(["init"], dir); runGit(["config", "user.name", "Pi"], dir); runGit(["config", "user.email", "pi@example.com"], dir); - writeText(join(dir, "alpha.ts"), "export const alpha = 1;\n"); - writeText(join(dir, "beta.ts"), "export const beta = 1;\n"); + for (const file of changedFiles) { + writeText(join(dir, file.path), file.before); + } writeText(join(dir, ".hunk", "extensions", entryName), source); runGit(["add", "."], dir); runGit(["commit", "-m", "initial"], dir); - writeText(join(dir, "alpha.ts"), "export const alphaValue = 2;\n"); - writeText(join(dir, "beta.ts"), "export const betaValue = 2;\n"); + for (const file of changedFiles) { + writeText(join(dir, file.path), file.after); + } return { dir }; } From 13d58839139cdc8abaf83096a387de0e44326765 Mon Sep 17 00:00:00 2001 From: Justin Giancola Date: Thu, 13 Aug 2026 13:55:14 -0400 Subject: [PATCH 4/7] docs(extensions): document revealLine and record its placement decision An extension author needs to know two things the signature does not say: where the revealed line lands (the app's own reveal position, not the caller's choice) and what happens to a line the stream cannot draw. The pack check exercises the new method so the published surface stays honest, and the proposal's companion-gap section now records the design rather than proposing it. --- .changeset/reveal-line-navigation.md | 5 ++ docs/extension-architecture.md | 11 +++ docs/extensions.md | 74 ++++++++++++++----- docs/line-highlights-proposal.md | 45 +++++++++-- scripts/check-pack.ts | 10 +++ skills/hunk-extensions/SKILL.md | 3 +- .../content/docs/docs/extend/extension-api.md | 6 +- 7 files changed, 125 insertions(+), 29 deletions(-) create mode 100644 .changeset/reveal-line-navigation.md diff --git a/.changeset/reveal-line-navigation.md b/.changeset/reveal-line-navigation.md new file mode 100644 index 000000000..559ea3c51 --- /dev/null +++ b/.changeset/reveal-line-navigation.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Extensions can jump the review to one exact source line with `ctx.navigation.revealLine(fileId, side, line)` (API v5), so a target deep inside a tall hunk lands near the top of the viewport instead of pages below its anchor. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index cda3c794b..1c35a7169 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -168,6 +168,17 @@ chord at a time and detected by probing matchers with a synthesized event `src/ui/lib/extensionSelection.ts`, derived from the same frozen file views the panes render. App reads it through a ref so the dispatch table stays stable. +`src/ui/lib/extensionNavigation.ts` mints the guarded navigation behind both +`ctx.navigation` and a pane's `actions`, so a jump from either surface is +validated, attributed, and reported the same way. It owns argument policy only +— visible-file validation, hunk clamping, `revealLine`'s side and line-number +checks — and delegates the move itself to the review controller. Where a jump +puts a line on screen stays host policy: `useReviewController` tags each +current-line reveal with a placement, and `DiffPane` reads it to choose between +stepping's minimum-distance scroll and the top-padded position hunk, note, and +`revealLine` reveals share. Extensions name a target; they never name a scroll +position. + `ctx.dialogs` is the one place extension code can interrupt the user, so its ordering and settlement live outside React in `src/ui/lib/extensionDialogs.ts` — one FIFO queue per App instance, minting a diff --git a/docs/extensions.md b/docs/extensions.md index 8bc8d72d6..7018406bb 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -279,8 +279,9 @@ new instances and run that shutdown/startup pair around the replacement. ### `hunk.apiVersion` The API generation this Hunk speaks (currently `5`). Version 5 adds line -highlighters; version 4 added keyboard modes and docked panes, with API-v3 -sidebar names remaining as deprecated aliases. +highlighters and line-granular navigation (`revealLine`); version 4 added +keyboard modes and docked panes, with API-v3 sidebar names remaining as +deprecated aliases. ### `hunk.registerTheme(theme)` @@ -648,13 +649,16 @@ The component receives fresh props as the app changes: API-v3 sidebar names remain as deprecated aliases: use `registerPane`, `ExtensionPane*`, `ctx.panes`, and `replaces: "hunk:files"` in new code. -`actions.selectFile(fileId)` and `actions.selectHunk(fileId, hunkIndex)` route -through the same review controller as the built-in files pane and the keyboard -shortcuts, so the review stream scrolls, selection updates, and the -`selection_changed` event fires exactly as if the user had clicked a built-in -row. `actions.notify(message, type?)` shows a toast attributed to your -extension. An action given a file id that is not currently visible is refused -with a warning rather than corrupting the selection. +`actions.selectFile(fileId)`, `actions.selectHunk(fileId, hunkIndex)`, and +`actions.revealLine(fileId, side, line)` route through the same review +controller as the built-in files pane and the keyboard shortcuts, so the review +stream scrolls, selection updates, and the `selection_changed` event fires +exactly as if the user had clicked a built-in row. `actions.notify(message, +type?)` shows a toast attributed to your extension. An action given a file id +that is not currently visible is refused with a warning rather than corrupting +the selection. A pane's `actions` carry the same navigation methods a command +handler's [`ctx.navigation`](#navigating-the-review) does, with the same +guarantees. The three hunk surfaces line up by design: each file's `hunks` lists public `ExtensionDiffHunk` summaries (`index`, the `@@` header, inclusive old/new @@ -1358,15 +1362,49 @@ session keyboard modes. See [Session keyboard modes](#session-keyboard-modes). `{ fileId }`-scoped. See [`hunk.registerLineHighlighter`](#hunkregisterlinehighlighterhighlighter). -`ctx.navigation` moves the review stream: `selectFile(fileId)` and -`selectHunk(fileId, hunkIndex)`, the same guarded navigation a pane's -`actions` carry, routed through the same review controller — the stream -scrolls, selection updates, and `selection_changed` fires exactly as if the -user had clicked a pane row. Unlike `selection` it is live, not a snapshot: -a call acts on the review as it is at that moment, so a handler that awaits a -dialog and then navigates still works. A file id the stream cannot currently -show is refused with a warning rather than corrupting the selection, and a -hunk index is clamped into the file's real range. +#### Navigating the review + +`ctx.navigation` moves the review stream: `selectFile(fileId)`, +`selectHunk(fileId, hunkIndex)`, and `revealLine(fileId, side, line)`, the same +guarded navigation a pane's `actions` carry, routed through the same review +controller — the stream scrolls, selection updates, and `selection_changed` +fires exactly as if the user had clicked a pane row. Unlike `selection` it is +live, not a snapshot: a call acts on the review as it is at that moment, so a +handler that awaits a dialog and then navigates still works. A file id the +stream cannot currently show is refused with a warning rather than corrupting +the selection, and a hunk index is clamped into the file's real range. + +`revealLine` is the finest target there is, and the one to reach for when your +extension knows exactly which line it means — a search hit, a lint finding, the +line a mark from [`registerLineHighlighter`](#hunkregisterlinehighlighterhighlighter) +sits on. A hunk hundreds of lines tall has one anchor, so `selectHunk` can leave +the line you meant pages below the viewport; `revealLine` scrolls to the line +itself, lands it a little below the viewport top like every other Hunk reveal, +and makes it the current line so the reverse-video marker sits on it. + +`line` is 1-based on `side` as the patch numbers it, so a context line answers +to either side's number. Two things soften the target rather than failing it: +when no rendered row carries that line — it is inside a collapsed gap, absent +from a partial patch, or the reviewer turned the current-line marker off +(`view.cursor_line = "off"`) — the jump lands on the hunk containing the line +instead. Only a line no hunk of the file covers is refused, with a warning +naming your extension, and so are a side outside `"old"`/`"new"` and a line +number that is not a positive whole number. + +```ts +hunk.registerCommand({ id: "first-todo", title: "Jump to the first TODO" }, async (ctx) => { + const file = ctx.selection.file; + if (!file) { + return; + } + + const document = await ctx.workspace.readDocument(file.id, "new"); + const index = (document ?? "").split("\n").findIndex((line) => line.includes("TODO")); + if (index >= 0) { + ctx.navigation.revealLine(file.id, "new", index + 1); + } +}); +``` A handler may be async; a failure (sync or rejected) becomes a warning naming your extension. diff --git a/docs/line-highlights-proposal.md b/docs/line-highlights-proposal.md index 380e5a68e..e64da83d9 100644 --- a/docs/line-highlights-proposal.md +++ b/docs/line-highlights-proposal.md @@ -247,11 +247,40 @@ Search is the weakest justification on this list. on a rendered line still shows nothing, and it wants its own mechanism rather than a background tint. -## Companion gap (separate PR) - -`ctx.navigation` reaches files and hunks; the finest target is `selectHunk`. A -highlight tells you which characters matched, but the review still lands on the -hunk. `revealLine(fileId, side, line)` would complete it, and the internals -already have the pieces — `firstLineCursorInHunk` and `revealLineCursor` in -`useReviewController.ts` do exactly this for the host's own line cursor. -Deliberately kept out of the highlight change to keep both reviewable. +## Companion gap — closed by `revealLine` + +`ctx.navigation` reached files and hunks; the finest target was `selectHunk`. A +highlight told you which characters matched, but the review still landed on the +hunk anchor — in a hunk hundreds of lines tall, pages above the mark. +`revealLine(fileId, side, line)` closes that, still under API v5. + +It reuses the host's own line-cursor machinery rather than adding a second +scrolling path: `(side, line)` resolves against the stops `buildLineCursors` +measures, and `revealLineCursor` sets the current line, bumps the reveal +request, and anchors the selection without moving the viewport. Two decisions +are worth recording. + +**Placement is the app's, not the caller's.** The revealed line lands at +`computeHunkRevealScrollTop` with `preferredTopPadding = max(2, floor(viewportHeight * 0.25))` +— the same position hunk and note reveals use. One landing spot app-wide, so a +reviewer never has to learn where a given extension decided to put things, and +no API option to get it wrong. That required naming the two line-reveal +policies apart: stepping keeps its minimum-distance scroll (`"nearest"`), so a +held `j` does not yank the stream, while a jump uses `"reveal"` even when the +line already happened to be on screen. + +**Cross-file needs no pending state.** The diff pane measures section geometry +for every visible file, not just the selected one, so `buildLineCursors` +already enumerates the whole review stream and a jump into another file +resolves in the same pass as a local one. The `pendingLineCursorRef` dance that +gap expansion needs — waiting for rows that do not exist yet — has no analogue +here, and adding one would have been a second mechanism for a problem that is +not there. + +What a caller gets when the line is not drawable is deliberate rather than +silent: a line with no measured row (inside a collapsed gap, absent from a +partial patch, or with the current-line marker configured off, which retires +the stop list entirely) falls back to the hunk containing it, since that is +strictly closer than where `selectHunk` would have landed anyway. Only a line +no hunk covers is refused, with a warning naming the extension — the same +attribution every other guarded navigation refusal carries. diff --git a/scripts/check-pack.ts b/scripts/check-pack.ts index c23b3a145..60b71da9f 100644 --- a/scripts/check-pack.ts +++ b/scripts/check-pack.ts @@ -195,6 +195,16 @@ export default function (hunk: HunkExtensionAPI) { modeControls.exitMode(); ctx.panes.toggle("bottom"); if (ctx.sidebars.isOpen("legacy")) ctx.sidebars.close("legacy"); + + const targetFile = ctx.selection.file; + if (targetFile) { + ctx.navigation.selectFile(targetFile.id); + ctx.navigation.selectHunk(targetFile.id, 0); + ctx.navigation.revealLine(targetFile.id, "new", 211); + ctx.navigation.revealLine(targetFile.id, "old", 1); + // @ts-expect-error Only the two diff sides address a line. + ctx.navigation.revealLine(targetFile.id, "both", 1); + } }); hunk.registerCommand({ id: "rewrite", title: "Rewrite the selection" }, async (ctx) => { diff --git a/skills/hunk-extensions/SKILL.md b/skills/hunk-extensions/SKILL.md index 958f44e79..d6a89f17f 100644 --- a/skills/hunk-extensions/SKILL.md +++ b/skills/hunk-extensions/SKILL.md @@ -123,7 +123,8 @@ transform — gets `ctx.cwd` and `ctx.notify(message, type?)`. A file view's - **Command handlers** get `ctx.panes`, `ctx.fileViews` (select/toggle/isActive/ refresh/enterMode/exitMode), `ctx.highlights` (refresh prepared line marks, whole or `{ fileId }`-scoped), `ctx.selection` (a snapshot of file + hunk index), - `ctx.navigation` (live, guarded `selectFile`/`selectHunk`), `ctx.commands` + `ctx.navigation` (live, guarded `selectFile`/`selectHunk`/`revealLine`, the + last landing one exact `(side, line)` near the viewport top), `ctx.commands` (`isEnabled`/`execute` for public semantic `hunk.*` commands), `ctx.keyboardModes` (enter/exit/probe this extension's session modes), `ctx.dialogs` (`confirm`/`select`/`input`, queued and attributed), and `ctx.workspace` diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index e55405ed2..e54b0970e 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -7,7 +7,7 @@ The extension factory receives one API object. Registration calls are only valid ## `hunk.apiVersion` -The API generation this Hunk speaks (currently `5`). Version 5 adds line highlighters; version 4 added keyboard modes and docked panes, with API-v3 sidebar names remaining as deprecated aliases. +The API generation this Hunk speaks (currently `5`). Version 5 adds line highlighters and line-granular navigation (`revealLine`); version 4 added keyboard modes and docked panes, with API-v3 sidebar names remaining as deprecated aliases. ## `hunk.registerTheme(theme)` @@ -168,7 +168,9 @@ hunk.registerCommand( `selection.file` is a frozen view, identical to a pane's `files` entries; it is `null` only when no files are visible. `selection.hunkIndex` is `null` whenever `file` is, or when the file has no hunks. The values are captured when the command fires, so an async handler keeps the selection it started from. -`ctx.navigation.selectFile(fileId)` and `selectHunk(fileId, hunkIndex)` route through the same guarded review controller as a pane's `actions` — the stream scrolls, selection updates, `selection_changed` fires. Unlike `selection` it is live: a handler that awaits a dialog and then navigates still works. +`ctx.navigation.selectFile(fileId)`, `selectHunk(fileId, hunkIndex)`, and `revealLine(fileId, side, line)` route through the same guarded review controller as a pane's `actions` — the stream scrolls, selection updates, `selection_changed` fires. Unlike `selection` it is live: a handler that awaits a dialog and then navigates still works. + +`revealLine` is the finest target: a hunk hundreds of lines tall has one anchor, so `selectHunk` can leave the line you meant pages below the viewport. `line` is 1-based on `side` as the patch numbers it, so a context line answers to either side's number. The revealed line lands a little below the viewport top — where every other Hunk reveal lands — and becomes the current line, pairing with a mark from `registerLineHighlighter`. A line no rendered row carries (inside a collapsed gap, absent from a partial patch, or with the current-line marker off) falls back to the hunk containing it; a line no hunk covers, a side outside `"old"`/`"new"`, and a line number that is not a positive whole number are refused with a warning naming the extension. All built-ins listed in the [keybindings reference](https://github.com/modem-dev/hunk/blob/main/docs/keybindings.md) are public to command handlers. This includes the unbound `hunk.review.alignCurrentLineTop`, `hunk.review.alignCurrentLineCenter`, and `hunk.review.alignCurrentLineBottom` commands. `count` defaults to `1`, is capped at `10,000`, and scales relative row, viewport, horizontal, file, hunk, and annotated navigation in one host transition. Absolute and one-shot commands run once. Unknown, disabled, non-public, extension-owned, or stale commands return `false`. `isEnabled` also returns `false` for a malformed id; malformed `execute` ids, options, and counts throw into normal extension failure containment. From 59f123181539eea6c5dd71311e55a842b414de69 Mon Sep 17 00:00:00 2001 From: Justin Giancola Date: Thu, 13 Aug 2026 14:46:04 -0400 Subject: [PATCH 5/7] fix(ui): keep a held revealLine reading the live cursor list The session's first deferred search jump landed on the hunk anchor - the exact behavior revealLine exists to fix. A memoized extension pane keeps its mount-time actions (the API documents them as valid while mounted), and the revealLine minted on that first render closed over App's cursor state before the diff pane had published any measured stops, so findLineCursorAt saw an empty list and silently degraded to the hunk fallback. selectHunk never showed this because it only dispatches through stable store callbacks; revealLine is the first navigation member that reads data. Read the cursors and visible files through refs so any held reference - a memoized pane's actions, an async handler after an await - resolves against the review as it is now. --- src/ui/hooks/useReviewController.test.tsx | 56 +++++++++++++++- src/ui/hooks/useReviewController.ts | 18 +++++- test/pty/extensions-integration.test.ts | 78 +++++++++++++++++++++++ 3 files changed, 148 insertions(+), 4 deletions(-) diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 944989722..eb6880d46 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -1,6 +1,6 @@ import { describe, expect, spyOn, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; -import { act, StrictMode, useEffect, useState } from "react"; +import { act, StrictMode, useEffect, useRef, useState } from "react"; import { SourceTextTooLargeError } from "../../core/fileSource"; import type { DiffFile } from "../../core/types"; import { @@ -158,6 +158,7 @@ function ReviewControllerHarness({ publishLineCursors = true, stmlEnabled, onController, + onFirstController, onSetFiles, }: { initialFiles: DiffFile[]; @@ -166,11 +167,20 @@ function ReviewControllerHarness({ publishLineCursors?: boolean; stmlEnabled?: boolean; onController: (controller: ReviewController) => void; + /** Receive the first render's controller, before any cursors were published. */ + onFirstController?: (controller: ReviewController) => void; onSetFiles?: (setFiles: (nextFiles: DiffFile[]) => void) => void; }) { const [files, setFiles] = useState(initialFiles); const [lineCursors, setLineCursors] = useState([]); const controller = useReviewController({ files, lineCursors, noteGeometry, stmlEnabled }); + // Capture during render, as a memoized consumer's closure would: the effects + // below have not yet published measured cursors on the first pass. + const firstControllerRef = useRef(null); + if (firstControllerRef.current === null) { + firstControllerRef.current = controller; + onFirstController?.(controller); + } const visibleFiles = controller.visibleFiles; const { expandedGapsByFileId, sourceStatusByFileId } = controller; @@ -219,11 +229,13 @@ async function renderReviewController( noteGeometry, publishLineCursors, stmlEnabled, + onFirstController, }: { strictMode?: boolean; noteGeometry?: Parameters[0]["noteGeometry"]; publishLineCursors?: boolean; stmlEnabled?: boolean; + onFirstController?: (controller: ReviewController) => void; } = {}, ) { const controllerRef: { current: ReviewController | null } = { current: null }; @@ -234,6 +246,7 @@ async function renderReviewController( noteGeometry={noteGeometry} publishLineCursors={publishLineCursors} stmlEnabled={stmlEnabled} + onFirstController={onFirstController} onController={(nextController) => { controllerRef.current = nextController; }} @@ -1877,6 +1890,47 @@ describe("useReviewController", () => { } }); + test("a revealLine reference held from before cursors were measured still lands the line", async () => { + // A memoized extension pane keeps its mount-time `actions`, whose + // `revealLine` was minted on the first render — before the diff pane's + // effect published any measured cursors. That held reference must read the + // stops live: the session's first deferred search jump used to silently + // degrade to the hunk fallback because its closure still saw the empty + // pre-measurement list. + const firstControllerRef: { current: ReviewController | null } = { current: null }; + const { controllerRef, setup } = await renderReviewController([createThreeHunkFile()], { + onFirstController: (controller) => { + firstControllerRef.current = controller; + }, + }); + + try { + await flush(setup); + // The captured instance predates the cursor publication… + const first = expectValue(firstControllerRef.current); + // …while the current instance has long since seen the measured stops. + expect(expectValue(controllerRef.current).lineCursor).not.toBeNull(); + + let outcome: string | undefined; + await act(async () => { + outcome = first.revealLine("alpha", "new", 30); + }); + await flush(setup); + + expect(outcome).toBe("line"); + expect(expectValue(controllerRef.current).lineCursor).toMatchObject({ + fileId: "alpha", + hunkIndex: 2, + target: { side: "new", line: 30 }, + }); + expect(expectValue(controllerRef.current).lineCursorRevealRequest.placement).toBe("reveal"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("reports a line no hunk of the file covers instead of moving the review", async () => { const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index ab99a84d7..9652ee7f3 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -596,6 +596,18 @@ export function useReviewController({ [lineCursors, revealLineCursor], ); + // `revealLine` is handed to surfaces that hold it across commits — a memoized + // extension pane keeps its mount-time `actions`, and an async command handler + // may navigate long after its dispatch render. Reading these through the + // callback's own closure silently downgraded such a call to the hunk + // fallback: at mount the pane captured a `revealLine` whose cursor list was + // still the pre-measurement empty state, so the first deferred jump of a + // session landed on the hunk anchor. Refs keep any held reference live. + const lineCursorsForRevealRef = useRef(lineCursors); + lineCursorsForRevealRef.current = lineCursors; + const visibleFilesRef = useRef(visibleFiles); + visibleFilesRef.current = visibleFiles; + /** * Step the selection through one navigable scope. * @@ -619,13 +631,13 @@ export function useReviewController({ */ const revealLine = useCallback( (fileId: string, side: "old" | "new", line: number): RevealedLineResult => { - const cursor = findLineCursorAt(lineCursors, fileId, side, line); + const cursor = findLineCursorAt(lineCursorsForRevealRef.current, fileId, side, line); if (cursor) { revealLineCursor(cursor, "reveal"); return "line"; } - const file = visibleFiles.find((candidate) => candidate.id === fileId); + const file = visibleFilesRef.current.find((candidate) => candidate.id === fileId); const hunkIndex = file ? reviewHunkIndexForLine(file.metadata.hunks, side, line) : -1; if (hunkIndex < 0) { return "none"; @@ -634,7 +646,7 @@ export function useReviewController({ selectHunk(fileId, hunkIndex); return "hunk"; }, - [lineCursors, revealLineCursor, selectHunk, visibleFiles], + [revealLineCursor, selectHunk], ); /** Set the shared file filter. */ diff --git a/test/pty/extensions-integration.test.ts b/test/pty/extensions-integration.test.ts index 9929c618d..f55935596 100644 --- a/test/pty/extensions-integration.test.ts +++ b/test/pty/extensions-integration.test.ts @@ -180,6 +180,38 @@ const REVEAL_LINE_EXTENSION_SOURCE = `export default function (hunk) { } `; +/** + * An extension that jumps through pane actions captured at mount. + * + * This is the shape the less-search example uses: a keyboard mode cannot + * navigate, so it leaves the jump for the mounted pane, whose \`actions\` were + * minted on the pane's first render — before the diff pane had published any + * measured line cursors — and are documented to stay valid while the pane is + * mounted. A \`revealLine\` that reads its own stale closure instead of the + * live cursor list silently degrades this exact call to the hunk fallback. + */ +const REVEAL_LINE_MOUNT_ACTIONS_EXTENSION_SOURCE = `import { createElement } from "react"; +let capturedActions = null; +export default function (hunk) { + hunk.registerPane({ + id: "capture", + placement: "bottom", + defaultOpen: true, + height: { preferred: 1, min: 1, max: 1 }, + component: (props) => { + if (capturedActions === null) capturedActions = props.actions; + return createElement("text", { content: "CAPTURE PANE", style: { fg: props.theme.text } }); + }, + }); + hunk.registerCommand({ id: "jump-held", title: "Jump via held actions", key: "f7" }, (ctx) => { + const file = ctx.selection.file; + if (file && capturedActions) { + capturedActions.revealLine(file.id, "new", ${REVEAL_LINE_TARGET}); + } + }); +} +`; + const DIALOG_EXTENSION_SOURCE = `export default function (hunk) { hunk.registerCommand({ id: "ask", title: "Ask", key: "y" }, async (ctx) => { const proceed = await ctx.dialogs.confirm({ @@ -780,6 +812,52 @@ describe("PTY extensions", () => { } }); + test("revealLine through pane actions held since mount still lands the line", async () => { + // The first deferred jump of a session runs against actions minted before + // any cursors were measured; it must land on the line, not the hunk anchor. + const configHome = harness.createIsolatedConfigHome(); + const fixture = harness.createRepoExtensionFixture( + REVEAL_LINE_MOUNT_ACTIONS_EXTENSION_SOURCE, + "fixture.ts", + [TALL_HUNK_FILE], + ); + const session = await harness.launchHunk({ + args: [ + "diff", + "--mode", + "stack", + "--extension", + join(fixture.dir, ".hunk", "extensions", "fixture.ts"), + ], + cwd: fixture.dir, + cols: 140, + rows: 24, + env: { XDG_CONFIG_HOME: configHome }, + }); + + try { + const review = await harness.waitForSnapshot( + session, + (text) => text.includes("tall.ts") && text.includes("CAPTURE PANE"), + 20_000, + ); + expect(review).not.toContain(REVEAL_LINE_TOKEN); + await harness.ensureKeyboardIsLive(session); + + await session.press("f7"); + const revealed = await harness.waitForSnapshot( + session, + (text) => text.includes(REVEAL_LINE_TOKEN), + 20_000, + ); + const row = lineIndexOf(revealed, REVEAL_LINE_TOKEN); + expect(row).toBeGreaterThan(0); + expect(row).toBeLessThan(12); + } finally { + session.close(); + } + }); + test("a startup handler's notify renders as a toast and clears itself", async () => { const configHome = harness.createIsolatedConfigHome(); const fixture = harness.createRepoExtensionFixture(NOTIFY_EXTENSION_SOURCE); From fcd86f62625841cdba58b7fbc0eaaed1ddf77a0a Mon Sep 17 00:00:00 2001 From: Justin Giancola Date: Thu, 13 Aug 2026 22:50:49 -0400 Subject: [PATCH 6/7] fix(ui): let a revealed line survive the selection reveal retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cross-file revealLine changes the selected file, and the selection reveal that schedules runs a zero-delay retry after this layout effect — dragging the viewport from the named line back to the hunk anchor, 51 rows away in the regression test. The neighbouring explicit-alignment effect already retires that work through supersedePendingSelectionReveal; a named line is just as authoritative, so it does the same. Reported by Greptile on #727. --- src/ui/components/panes/DiffPane.tsx | 10 +++ src/ui/components/ui-components.test.tsx | 99 +++++++++++++++++++++++- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index f40e1b1db..428e17080 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -2146,6 +2146,14 @@ export function DiffPane({ scrollTop: scrollBox.scrollTop, viewportHeight, }); + // A named line is the final scroll policy for this request, exactly as an + // explicit alignment is: a cross-file reveal changes the selection, and the + // selection reveal it schedules would otherwise run its zero-delay retry + // after this layout effect and drag the viewport back to the hunk anchor. + // Superseding here is what keeps a reveal into another file line-exact. + supersedePendingSelectionReveal(); + clearPendingFileTopAlign(); + if (revealScrollTop === scrollBox.scrollTop) { return; } @@ -2154,11 +2162,13 @@ export function DiffPane({ scrollBox.scrollTo(clampReviewScrollTop(revealScrollTop, viewportHeight)); }, [ clampReviewScrollTop, + clearPendingFileTopAlign, lineCursor, lineCursorBoundsOf, lineCursorRevealRequest, scrollRef, scrollViewport.height, + supersedePendingSelectionReveal, suppressViewportSelectionSync, ]); diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 68fca331b..453d4b04a 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -17,7 +17,7 @@ import { measureDiffSectionGeometry } from "../diff/diffSectionGeometry"; import { buildFileSectionLayouts, buildInStreamFileHeaderHeights } from "../lib/fileSectionLayout"; import { builtinCommandKeyDefaults, builtinCommandMatchProbes } from "../lib/appCommands"; import { resolveCommandKeys } from "../lib/keymap"; -import type { CurrentLineAlignment } from "../lib/hunkScroll"; +import type { CurrentLineAlignment, LineRevealPlacement } from "../lib/hunkScroll"; import type { LineCursor } from "../lib/lineCursors"; const { AppHost } = await import("../AppHost"); @@ -1368,6 +1368,103 @@ describe("UI components", () => { } }); + test("DiffPane keeps a cross-file line reveal against the selection reveal retry", async () => { + const theme = resolveTheme("github-dark-default", null); + // Two files, so revealing a line in the second one also changes the + // selected file — the case that schedules selection-reveal retries. The + // second file is one tall hunk, so a line near its end sits far from the + // hunk anchor the retry would scroll to, making the two outcomes + // unmistakably different rather than a row apart. + const files = [ + createWideTwoHunkDiffFile("first", "first.ts", 1), + createTallDiffFile("second", "second.ts", 60), + ]; + const scrollRef = createRef(); + let revealDeepLineInSecondFile = () => {}; + let cursorReady = false; + + function CrossFileRevealHarness() { + const [selection, setSelection] = useState({ + fileId: files[0]!.id, + hunkIndex: 0, + }); + const [selectedHunkRevealRequestId, setSelectedHunkRevealRequestId] = useState(0); + const [lineCursor, setLineCursor] = useState(null); + const [revealRequest, setRevealRequest] = useState<{ + id: number; + placement: LineRevealPlacement; + }>({ id: 0, placement: "nearest" }); + const deepCursorRef = useRef(null); + + revealDeepLineInSecondFile = () => { + const cursor = deepCursorRef.current; + if (!cursor) return; + // Exactly what a `revealLine` into another file produces: a new + // selection, a hunk reveal request, and an explicit line reveal. + setSelection({ fileId: cursor.fileId, hunkIndex: cursor.hunkIndex }); + setSelectedHunkRevealRequestId((current) => current + 1); + setLineCursor(cursor); + setRevealRequest((current) => ({ id: current.id + 1, placement: "reveal" })); + }; + + return ( + { + const deep = cursors.filter((cursor) => cursor.fileId === files[1]!.id).at(-1); + if (!deep) return; + deepCursorRef.current = deep; + cursorReady = true; + }} + /> + ); + } + + const setup = await testRender(, { width: 104, height: 14 }); + + try { + for (let attempt = 0; attempt < 10 && !cursorReady; attempt += 1) { + await settleDiffPane(setup); + } + expect(cursorReady).toBe(true); + + await act(async () => { + revealDeepLineInSecondFile(); + await setup.renderOnce(); + await setup.renderOnce(); + }); + const revealedScrollTop = scrollRef.current?.scrollTop ?? 0; + + // The selection reveal schedules a zero-delay retry plus a 120ms + // pinned-header settle window. Both must leave the exact line where the + // reveal put it; without superseding they scroll back to the hunk anchor. + await act(async () => { + await Bun.sleep(150); + await setup.renderOnce(); + }); + + expect(scrollRef.current?.scrollTop ?? 0).toBe(revealedScrollTop); + // Guard the guard: if the fixture ever put the revealed line at the hunk + // anchor, this test would pass without proving anything. + expect(revealedScrollTop).toBeGreaterThan(20); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("DiffPane viewport-follow selection does not move the scroll position", async () => { const theme = resolveTheme("github-dark-default", null); const files = [ From 026dd4548e05dda68aac65124fd52e0d616de29b Mon Sep 17 00:00:00 2001 From: Justin Giancola Date: Fri, 14 Aug 2026 11:56:19 -0400 Subject: [PATCH 7/7] docs: retire the line-highlights design proposal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proposal was pre-implementation design rationale — the kind of document that belongs in a PR description, not shipped docs — and it kept drawing rebase conflicts as the stack evolved. Everything it documented that users need survives in the real documentation: the line-highlight and revealLine API contracts in docs/extensions.md and website/src/content/docs/docs/extend/extension-api.md, the subsystem map in docs/extension-architecture.md, and the corrective notes from the post-merge review (zero-width ranges paint nothing; transparent cells resolve tints against an assumed background) in those same pages and src/extension-api/types.ts. --- docs/line-highlights-proposal.md | 286 ------------------------------- 1 file changed, 286 deletions(-) delete mode 100644 docs/line-highlights-proposal.md diff --git a/docs/line-highlights-proposal.md b/docs/line-highlights-proposal.md deleted file mode 100644 index e64da83d9..000000000 --- a/docs/line-highlights-proposal.md +++ /dev/null @@ -1,286 +0,0 @@ -# Proposal: extension-contributed line highlights - -Status: implemented. This document records the design, what a critical review -of the draft changed, and the resolution of its open questions. The authoring -contract lives in `docs/extensions.md`; ownership is mapped in -`docs/extension-architecture.md`. - -An extension can add panes, file views, commands, keyboard modes, dialogs, and -whole VCS backends — but it could not mark **a range of characters inside a -diff line**. This is the smallest API that closes that gap, and it is smaller -than it looks because Hunk already renders exactly this kind of mark for its -own word-diff emphasis. - -## The gap - -Concretely: a search extension finds `readConfig` on line 214 and can jump the -review to that hunk, but cannot show the user _which characters_ matched. The -best it could do was quote the line on its own pane and highlight it there — -one row of indirection away from the code it is talking about, which defeats -the point. - -`registerFileView` is the only nearby lever and it is the wrong one: presenting -a file as extension-supplied rows replaces Pierre's diff rendering wholesale, -trading syntax highlighting, word diff, and layout fidelity for the ability to -color a substring. - -## What already existed - -Hunk paints intra-line background ranges today. Pierre marks word-diff emphasis -spans with a `data-diff-span` attribute, and `flattenHighlightedLine` -(`src/ui/diff/pierre.ts`) turns them into `RenderSpan { text, fg?, bg? }` runs -carrying an emphasis background. - -More importantly, Hunk already solves the hard part of this problem — **a -background is invisible unless it is resolved against the background it sits -on**. `MIN_WORD_DIFF_BG_DISTANCE` / `strengthenWordDiffBg` / -`resolveWordDiffHighlightBg` in `pierre.ts` blend an anchor color into the -line's own background until it clears a minimum perceptual distance, per line -kind and per theme. An added line already has a green background; a naive -highlight background is indistinguishable on it, which is precisely what a -first attempt at this produced. That machinery is the reason this API exists at -the tone level rather than the color level. The extension-facing version is -`lineHighlightToneBg` in `src/ui/diff/rowStyle.ts`, verified against every -built-in theme and line kind by tests. - -## The shape - -```ts -hunk.registerLineHighlighter({ - id: "matches", - highlight: ({ file, signal, readDocument }) => - ExtensionLineHighlight[] | null | Promise<...>, -}); -``` - -```ts -interface ExtensionLineHighlight { - /** Which side the line belongs to; a context line may be addressed by either. */ - side: "old" | "new"; - /** 1-based source line number on that side. */ - line: number; - /** [start, end) UTF-16 code-unit offsets into the line's raw source text. */ - range: readonly [number, number]; - /** What the mark means. The host decides what that looks like. */ - tone?: "match" | "current" | "info" | "warning" | "error"; -} -``` - -Addressing by `(side, line, range)` rather than by rendered row is deliberate: -source coordinates survive split vs stack, wrapping, horizontal scroll, -collapsed context, and note insertion. Extensions never learn Hunk's row model, -which is the same boundary `registerFileView` and `DiffPane` already hold. - -Invalidation is pull-based, identical in spirit to `ctx.fileViews.refresh`: -`highlight` is a pure derivation of the file plus an invalidation epoch, so a -search that moves to the next match bumps the epoch -(`ctx.highlights.refresh("matches")`, optionally `{ fileId }`-scoped) rather -than pushing new state into the host. There is no host-held mark state to go -stale across a reload. The epoch policy is the same one file views use, -extracted to `src/ui/lib/scopedEpochs.ts` and shared. - -## Tones, not colors - -The API does not accept a color. Three reasons, in order of importance: - -1. **Visibility is not the extension's to get right.** The contrast problem is - real, theme-dependent, and per line kind — `#334455` is legible on a context - line and invisible on a green one. `tone` lets the host apply the word-diff - minimum-distance guarantee to extension marks too. -2. **Themes stay coherent.** Hunk's theme guidance keeps official palette - tokens separate from the semantic `AppTheme` mapping; a raw color from an - extension punches straight through that. -3. **It is a smaller contract.** A tone can be re-mapped later; a color cannot. - -Tinted tone anchors map to existing semantic theme tokens (`accent`, -`badgeNeutral`, `fileModified`, `removedSignColor`) with a distance floor well -above word diff's, backing off only where the code on top would stop being -readable. `current` renders as reverse video — theme text as the block, theme -background as the glyphs — the `less`/vim convention for the active hit, which -sidesteps the tint-versus-readability tug-of-war entirely. A transparent cell -has no color to blend against, so resolution falls back to the theme's -background and then to the appearance's extreme: the mark stays visible, but -its distance floor is measured against an assumed surface rather than the one -the terminal actually shows. Only a theme whose own colors cannot take a blend -(non-hex) declines the mark — the same degradation word diff uses. - -## Where it plugs in - -The instinct is to apply highlights where spans are built — `makeSplitCell` / -`makeStackCell` in `pierre.ts`. **That is the wrong layer.** -`buildDiffSectionRowPlan` is the shared plan consumed by _both_ rendering and -geometry measurement; feeding highlights into it would put them in the cache -key of the expensive, geometry-bearing artifact, so pressing `n` during a -search would re-plan whole files to change some colors. - -Highlights are applied at **paint time** instead. The review confirmed the -placement argument holds, with two wiring caveats the draft missed: - -- **Memoized rows repaint only when a prop changes.** `DiffRowView` compares - props by reference, so the paint-time index is a real prop - (`lineHighlights`), built per file in `PierreDiffView` and threaded through - `DiffPane`/`DiffSection`. Per-file mark arrays keep stable identities across - unrelated preparation runs, so an epoch bump repaints exactly the files whose - marks changed. -- **Cell spans are shared cached arrays** (`flattenHighlightedLine` memoizes - per HAST node; `aliasHighlightedContextLines` shares one array across context - sides). Application therefore copies: spans are split at column boundaries - with backgrounds overridden, before `sliceSpansWindow` (horizontal scroll) - and `wrapSpans` (wrapping), which both preserve per-span colors for free. - -The split of responsibilities: - -| Layer | File | Work | -| ------------ | -------------------------------------------------------- | --------------------------------------------------- | -| Contract | `src/extension-api/types.ts` | types (import-free), API v5 | -| Registry | `src/extensions/types.ts`, `runExtension.ts`, `apply.ts` | collect + resolve registrations | -| Preparation | `src/ui/highlights/` | bounded async, validation, caps, epochs | -| Column model | `src/ui/diff/lineHighlightPaint.ts` | offsets → terminal columns; span repaint transform | -| Tone → color | `src/ui/diff/rowStyle.ts` | `lineHighlightToneBg`, word-diff distance guarantee | -| Paint | `src/ui/diff/renderRows.tsx` | apply per rendered cell, memo-aware | - -That placement buys three properties by construction: geometry neutrality -(colors change, text never does, so measurement and wrapping cannot move), -windowed cost (only mounted rows consult the index), and no cache invalidation -(a highlight change is a repaint, never a re-plan). - -## What the review surfaced - -- **The offset mapping is derivable but not free.** Cell spans are already - sanitized and tab-expanded, so raw code-unit offsets map through one owned - conversion: sanitize-aware (control characters can shift offsets), tab-aware - (a prefix expands to the same columns the full line's expansion gives it), - and snapped outward to grapheme-cluster boundaries so a mid-surrogate or - mid-cluster offset widens to the whole glyph instead of tearing it. -- **The static pager needs nothing.** `staticDiffPager.ts` never runs extension - code, so highlights are interactive-only by construction; the docs say so - explicitly rather than leaving it to be discovered. -- **Expanded collapsed-context rows are covered.** Gap rows render source - slices, and the gap's old↔new correspondence resolves marks addressed to - either side onto the loaded source line, keyed under both line numbers. - Without loaded source those marks are silently invisible — matching the rows - themselves. -- **Marks on invisible lines are not errors.** A mark inside a collapsed gap or - absent from a partial patch is valid; the review just is not showing that - line. Only structural garbage warns. -- **Extension file views are out of scope.** A file view already owns its rows' - spans and tones; highlights apply to Hunk's own diff rendering only. -- **Theme changes re-resolve for free.** Tone resolution happens at paint time - keyed by the theme object; nothing is cached against a stale theme. - -## Validation and containment - -- Out-of-range lines: silently invisible (see above). Inverted, empty, or - non-integer ranges, bad sides, unknown tones: dropped, one warning per - extension per file. -- Caps: 2,000 ranges per file, 100 per line; beyond either the file's - highlights from that highlighter are dropped whole rather than truncated - silently. -- Overlaps resolve deterministically: ranges sort by start column and the later - range wins where they overlap. -- A throwing, rejecting, or timed-out `highlight` costs that file's marks and - nothing else, bounded by the same timeout and concurrency discipline as file - views. -- Precedence: extension marks override word-diff emphasis where they overlap - (the more specific statement); cursor-line and copy-selection blends compose - on top, since they are `bg => blend(bg)` functions over whatever background - is present. - -## Resolved questions - -1. **Offset units** — UTF-16 code units into the raw line text. That is what - `indexOf`/`RegExp.exec` return against `file.patch` lines, so it is the only - contract extensions can satisfy without reimplementing Hunk's measurement - stack. The host clamps to cluster boundaries, widening outward. -2. **Context lines** — one mark, mirrored to both halves in split view, found - through either side's line number in stack view. A context line is one - occurrence that split view happens to draw twice; changed lines are separate - addresses with separate marks, which is what makes `n` step from an old-side - hit across to a new-side hit. -3. **`current` as a tone** — kept as a tone, rendered as reverse video. A - separate concept would be more API for the same pixels, and inversion is - both unmistakable and readable by construction where a stronger tint kept - trading against the text on top of it. -4. **Interaction with word diff** — the extension mark wins where they overlap. - It is the more specific statement, and it is stated in the docs. -5. **`matches` pre-filter** — dropped. File views need `matches` because view - availability is user-visible (the per-file View menu); highlights have no - visible selection state, so a cheap `highlight` returning `null` makes the - pre-filter pure optimization with no observable role. - -## What else this unlocks - -Search is the weakest justification on this list. - -- **Diagnostics inline.** `tsc`, `eslint`, `clippy`, `ruff` output mapped onto - the exact columns of the changed lines — "this PR introduces this error, - here" during review rather than in CI later. -- **Secret scanning.** Mark the exact token that tripped a rule, instead of - naming the line in a pane. -- **Homoglyph marking.** Marking the confusable identifier characters that read - as ASCII but are not. (Trojan-source defense more broadly does _not_ work - yet, and listing it here was a mistake: a bidi control or zero-width - character occupies no terminal column, so a range covering only those - characters resolves to zero columns and paints nothing. Making invisible - characters visible needs a separate mechanism — a gutter or margin marker, or - rendering a visible stand-in glyph — that this API does not have.) -- **Coverage.** Uncovered added ranges, straight from an lcov file. -- **Provenance.** Which ranges an agent wrote versus a human — squarely in - Hunk's stated purpose of understanding coding-agent changesets. -- **Occurrence highlighting.** Every use of the identifier under the cursor, - the way an editor does. -- **Repo conventions.** A `.hunk/extensions/` extension marking banned APIs, - `any`, stray `console.log`, or missing license headers for every reviewer on - the team. -- **Anchored agent notes.** Notes attach to lines today; a range would tie a - note to the exact expression it discusses. -- **Agent-driven marks.** With the session daemon already brokering agent - commands into live sessions, an agent answering "where does this change - behavior?" could light up the ranges in the user's terminal as it explains. - Nothing else in the ecosystem can do that. - -## Follow-ups - -- **Zero-width ranges are silently invisible.** A mark covering only characters - that occupy no terminal column (bidi controls, ZWSP, ZWJ) resolves to an - empty column range and paints nothing. It is the one case where a valid mark - on a rendered line still shows nothing, and it wants its own mechanism rather - than a background tint. - -## Companion gap — closed by `revealLine` - -`ctx.navigation` reached files and hunks; the finest target was `selectHunk`. A -highlight told you which characters matched, but the review still landed on the -hunk anchor — in a hunk hundreds of lines tall, pages above the mark. -`revealLine(fileId, side, line)` closes that, still under API v5. - -It reuses the host's own line-cursor machinery rather than adding a second -scrolling path: `(side, line)` resolves against the stops `buildLineCursors` -measures, and `revealLineCursor` sets the current line, bumps the reveal -request, and anchors the selection without moving the viewport. Two decisions -are worth recording. - -**Placement is the app's, not the caller's.** The revealed line lands at -`computeHunkRevealScrollTop` with `preferredTopPadding = max(2, floor(viewportHeight * 0.25))` -— the same position hunk and note reveals use. One landing spot app-wide, so a -reviewer never has to learn where a given extension decided to put things, and -no API option to get it wrong. That required naming the two line-reveal -policies apart: stepping keeps its minimum-distance scroll (`"nearest"`), so a -held `j` does not yank the stream, while a jump uses `"reveal"` even when the -line already happened to be on screen. - -**Cross-file needs no pending state.** The diff pane measures section geometry -for every visible file, not just the selected one, so `buildLineCursors` -already enumerates the whole review stream and a jump into another file -resolves in the same pass as a local one. The `pendingLineCursorRef` dance that -gap expansion needs — waiting for rows that do not exist yet — has no analogue -here, and adding one would have been a second mechanism for a problem that is -not there. - -What a caller gets when the line is not drawable is deliberate rather than -silent: a line with no measured row (inside a collapsed gap, absent from a -partial patch, or with the current-line marker configured off, which retires -the stop list entirely) falls back to the hunk containing it, since that is -strictly closer than where `selectHunk` would have landed anyway. Only a line -no hunk covers is refused, with a warning naming the extension — the same -attribution every other guarded navigation refusal carries.