fix(ui): navigate comments by stream position (#315)

Author: benvinegar

src/ui/hooks/useReviewController.ts

@@ -225,14 +225,15 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon
         selectedFile?.id,
         selectedHunkIndex,
         delta,
+        hunkCursors,
       );
       if (!nextCursor) {
         return;
       }
 
       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. */

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,
+    });
+  });
 });

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;
+}

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

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,

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({