diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 80d7614f..d3c0e94b 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -225,6 +225,7 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon selectedFile?.id, selectedHunkIndex, delta, + hunkCursors, ); if (!nextCursor) { return; @@ -232,7 +233,7 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon selectHunk(nextCursor.fileId, nextCursor.hunkIndex, { scrollToNote: true }); }, - [annotatedHunkCursors, selectHunk, selectedFile?.id, selectedHunkIndex], + [annotatedHunkCursors, hunkCursors, selectHunk, selectedFile?.id, selectedHunkIndex], ); /** Cycle through only the currently visible files that carry annotations. */ diff --git a/src/ui/lib/hunks.test.ts b/src/ui/lib/hunks.test.ts index d01c067c..67897d28 100644 --- a/src/ui/lib/hunks.test.ts +++ b/src/ui/lib/hunks.test.ts @@ -169,4 +169,41 @@ describe("annotated hunk navigation", () => { hunkIndex: 1, }); }); + + test("uses full stream position when annotated navigation starts on an unannotated hunk", () => { + const streamCursors: HunkCursor[] = [ + { fileId: "alpha", hunkIndex: 0 }, + { fileId: "alpha", hunkIndex: 1 }, + { fileId: "beta", hunkIndex: 0 }, + { fileId: "gamma", hunkIndex: 0 }, + { fileId: "gamma", hunkIndex: 1 }, + { fileId: "omega", hunkIndex: 0 }, + ]; + const annotatedCursors: HunkCursor[] = [ + { fileId: "alpha", hunkIndex: 1 }, + { fileId: "gamma", hunkIndex: 0 }, + { fileId: "gamma", hunkIndex: 1 }, + ]; + + expect(findNextHunkCursor(annotatedCursors, "beta", 0, 1, streamCursors)).toEqual({ + fileId: "gamma", + hunkIndex: 0, + }); + expect(findNextHunkCursor(annotatedCursors, "beta", 0, -1, streamCursors)).toEqual({ + fileId: "alpha", + hunkIndex: 1, + }); + expect(findNextHunkCursor(annotatedCursors, "alpha", 0, 1, streamCursors)).toEqual({ + fileId: "alpha", + hunkIndex: 1, + }); + expect(findNextHunkCursor(annotatedCursors, "gamma", 1, 1, streamCursors)).toEqual({ + fileId: "gamma", + hunkIndex: 1, + }); + expect(findNextHunkCursor(annotatedCursors, "omega", 0, 1, streamCursors)).toEqual({ + fileId: "gamma", + hunkIndex: 1, + }); + }); }); diff --git a/src/ui/lib/hunks.ts b/src/ui/lib/hunks.ts index ba7e89fa..4ac5e8da 100644 --- a/src/ui/lib/hunks.ts +++ b/src/ui/lib/hunks.ts @@ -29,6 +29,7 @@ export function findNextHunkCursor( currentFileId: string | undefined, currentHunkIndex: number, delta: number, + streamCursors: HunkCursor[] = cursors, ): HunkCursor | null { if (cursors.length === 0) { return null; @@ -40,9 +41,56 @@ export function findNextHunkCursor( const nextIndex = currentIndex >= 0 ? Math.min(Math.max(currentIndex + delta, 0), cursors.length - 1) - : delta >= 0 - ? 0 - : cursors.length - 1; + : findNearestCursorIndex(cursors, streamCursors, currentFileId, currentHunkIndex, delta); return cursors[nextIndex] ?? null; } + +/** Resolve relative movement when the current hunk is not in the target cursor subset. */ +function findNearestCursorIndex( + cursors: HunkCursor[], + streamCursors: HunkCursor[], + currentFileId: string | undefined, + currentHunkIndex: number, + delta: number, +) { + if (!currentFileId) { + return delta >= 0 ? 0 : cursors.length - 1; + } + + const currentStreamIndex = streamCursors.findIndex( + (cursor) => cursor.fileId === currentFileId && cursor.hunkIndex === currentHunkIndex, + ); + if (currentStreamIndex < 0) { + return delta >= 0 ? 0 : cursors.length - 1; + } + + const streamIndexByCursor = new Map( + streamCursors.map((cursor, index) => [`${cursor.fileId}\0${cursor.hunkIndex}`, index] as const), + ); + const cursorStreamIndex = (cursor: HunkCursor) => + streamIndexByCursor.get(`${cursor.fileId}\0${cursor.hunkIndex}`) ?? -1; + const indexedCursors = cursors + .map((cursor, index) => ({ index, streamIndex: cursorStreamIndex(cursor) })) + .filter(({ streamIndex }) => streamIndex >= 0); + + if (indexedCursors.length === 0) { + return delta >= 0 ? 0 : cursors.length - 1; + } + + // Comment navigation is non-cyclic like normal hunk navigation, so positions outside + // the annotated span clamp to the nearest annotated edge instead of wrapping. + if (delta >= 0) { + const nextCursor = indexedCursors.find(({ streamIndex }) => streamIndex > currentStreamIndex); + return nextCursor?.index ?? indexedCursors[indexedCursors.length - 1]!.index; + } + + for (let index = indexedCursors.length - 1; index >= 0; index -= 1) { + const indexedCursor = indexedCursors[index]!; + if (indexedCursor.streamIndex < currentStreamIndex) { + return indexedCursor.index; + } + } + + return indexedCursors[0]!.index; +} diff --git a/src/ui/lib/reviewState.ts b/src/ui/lib/reviewState.ts index c5a647ba..867956e3 100644 --- a/src/ui/lib/reviewState.ts +++ b/src/ui/lib/reviewState.ts @@ -132,8 +132,15 @@ export function resolveReviewNavigationTarget({ }): ReviewNavigationTarget { if (input.commentDirection) { const delta = input.commentDirection === "next" ? 1 : -1; + const hunkCursors = buildHunkCursors(visibleFiles); const annotatedCursors = buildAnnotatedHunkCursors(visibleFiles); - const nextCursor = findNextHunkCursor(annotatedCursors, currentFileId, currentHunkIndex, delta); + const nextCursor = findNextHunkCursor( + annotatedCursors, + currentFileId, + currentHunkIndex, + delta, + hunkCursors, + ); if (!nextCursor) { throw new Error("No annotated hunks found in the current review."); diff --git a/test/pty/harness.ts b/test/pty/harness.ts index 5e841642..0709bbf5 100644 --- a/test/pty/harness.ts +++ b/test/pty/harness.ts @@ -155,6 +155,73 @@ export function createPtyHarness() { return { dir, before, after, agentContext }; } + function createAgentNavigationRepoFixture() { + const alphaBeforeLines = createNumberedExportLines(1, 80).split("\n"); + const alphaAfterLines = [...alphaBeforeLines]; + alphaAfterLines[0] = "export const line01 = 1001;"; + alphaAfterLines[59] = "export const line60 = 6000;"; + + const betaBeforeLines = createNumberedExportLines(81, 20).split("\n"); + const betaAfterLines = [...betaBeforeLines]; + betaAfterLines[0] = "export const line81 = 8100;"; + + const gammaBeforeLines = createNumberedExportLines(101, 80).split("\n"); + const gammaAfterLines = [...gammaBeforeLines]; + gammaAfterLines[0] = "export const line101 = 10100;"; + gammaAfterLines[59] = "export const line160 = 16000;"; + + const fixture = createGitRepoFixture([ + { + path: "alpha.ts", + before: `${alphaBeforeLines.join("\n")}\n`, + after: `${alphaAfterLines.join("\n")}\n`, + }, + { + path: "beta.ts", + before: `${betaBeforeLines.join("\n")}\n`, + after: `${betaAfterLines.join("\n")}\n`, + }, + { + path: "gamma.ts", + before: `${gammaBeforeLines.join("\n")}\n`, + after: `${gammaAfterLines.join("\n")}\n`, + }, + ]); + const agentContext = join(fixture.dir, "agent-context.json"); + + writeText( + agentContext, + JSON.stringify({ + version: 1, + summary: "Agent navigation notes", + files: [ + { + path: "alpha.ts", + annotations: [ + { + newRange: [60, 60], + summary: "Alpha note for navigation.", + rationale: "Used to prove comment navigation can leave an earlier note.", + }, + ], + }, + { + path: "gamma.ts", + annotations: [ + { + newRange: [60, 60], + summary: "Gamma note for navigation.", + rationale: "Used to prove comment navigation resumes after an unannotated hunk.", + }, + ], + }, + ], + }), + ); + + return { ...fixture, agentContext }; + } + function createMultiHunkFilePair() { const dir = makeTempDir("hunk-tuistory-hunks-"); const before = join(dir, "before.ts"); @@ -515,6 +582,7 @@ export function createPtyHarness() { cleanup, countMatches, createAgentFilePair, + createAgentNavigationRepoFixture, createBottomClampedRepoFixture, createCollapsedTopRepoFixture, createCrossFileHunkNavigationRepoFixture, diff --git a/test/pty/ui-integration.test.ts b/test/pty/ui-integration.test.ts index 1696b963..491e420f 100644 --- a/test/pty/ui-integration.test.ts +++ b/test/pty/ui-integration.test.ts @@ -92,6 +92,48 @@ describe("live UI integration", () => { } }); + test("comment navigation resumes from an unannotated hunk in stream order", async () => { + const fixture = harness.createAgentNavigationRepoFixture(); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "split", "--agent-context", fixture.agentContext, "--agent-notes"], + cwd: fixture.dir, + cols: 160, + rows: 14, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, { + timeout: 15_000, + }); + expect(initial).not.toContain("Maximum update depth exceeded"); + + await session.press("}"); + const alphaNote = await harness.waitForSnapshot( + session, + (text) => text.includes("Alpha note for navigation."), + 5_000, + ); + expect(alphaNote).toContain("Alpha note for navigation."); + expect(alphaNote).not.toContain("Maximum update depth exceeded"); + + await session.press("."); + await harness.waitForSnapshot(session, (text) => text.includes("line101 = 10100"), 5_000); + + await session.press("}"); + const gammaNote = await harness.waitForSnapshot( + session, + (text) => text.includes("Gamma note for navigation."), + 5_000, + ); + + expect(gammaNote).toContain("Gamma note for navigation."); + expect(gammaNote).not.toContain("Alpha note for navigation."); + expect(gammaNote).not.toContain("Maximum update depth exceeded"); + } finally { + session.close(); + } + }); + test("real hunk navigation jumps to later hunks in the review stream", async () => { const fixture = harness.createMultiHunkFilePair(); const session = await harness.launchHunk({