fix(review): keep cross-file hunk navigation anchored (#222)

Author: aliou

CHANGELOG.md

@@ -12,6 +12,8 @@ All notable user-visible changes to Hunk are documented in this file.
 
 ### Fixed
 
+- Fixed cross-file hunk navigation so near-boundary jumps keep the selected file pinned and backward jumps reveal the target hunk instead of the file top.
+
 ## [0.10.0] - 2026-04-21
 
 ### Added

src/ui/AppHost.interactions.test.tsx

@@ -238,6 +238,51 @@ function createTwoFileHunkBootstrap(): AppBootstrap {
   });
 }
 
+/** Build the cross-file hunk-navigation shape that used to flash the previous pinned header. */
+function createCrossFileHunkNavigationBootstrap(): AppBootstrap {
+  const longBeforeLines = Array.from(
+    { length: 342 },
+    (_, index) => `line ${String(index + 1).padStart(3, "0")}`,
+  );
+  const longAfterLines = [...longBeforeLines];
+  for (const lineNumber of [
+    2, 21, 41, 61, 81, 101, 121, 141, 161, 181, 201, 221, 241, 261, 281, 301, 321, 341,
+  ]) {
+    longAfterLines[lineNumber - 1] = `line ${String(lineNumber).padStart(3, "0")} changed`;
+  }
+
+  const shortBeforeLines = [
+    "// hunk 0 - at the very top of the file",
+    "export const top = 1;",
+    "",
+    "",
+    ...Array.from({ length: 25 }, (_, index) => `// filler ${index + 1}`),
+    "// hunk 1 - mid-file",
+    "export const mid = 3;",
+  ];
+  const shortAfterLines = [...shortBeforeLines];
+  shortAfterLines[1] = "export const top = 2;";
+  shortAfterLines[30] = "export const mid = 4;";
+
+  return createTestGitAppBootstrap({
+    changesetId: "changeset:cross-file-hunk-navigation",
+    files: [
+      createTestDiffFile(
+        "long-file",
+        "long-file.txt",
+        lines(...longBeforeLines),
+        lines(...longAfterLines),
+      ),
+      createTestDiffFile(
+        "short-file",
+        "short-file.ts",
+        lines(...shortBeforeLines),
+        lines(...shortAfterLines),
+      ),
+    ],
+  });
+}
+
 function createMouseScrollSelectionBootstrap(): AppBootstrap {
   const firstBeforeLines = createNumberedAssignmentLines(1, 12);
   const secondBeforeLines = Array.from(
@@ -339,6 +384,28 @@ async function waitForFrame(
   return frame;
 }
 
+async function pressHunkNavigationKey(
+  setup: Awaited<ReturnType<typeof testRender>>,
+  key: "]" | "[",
+  count: number,
+) {
+  for (let index = 0; index < count; index += 1) {
+    await act(async () => {
+      await setup.mockInput.typeText(key);
+    });
+    await flush(setup);
+  }
+}
+
+function firstCrossFileHunkNavigationHeader(frame: string) {
+  return (
+    frame
+      .split("\n")
+      .map((line) => line.trim())
+      .find((line) => line.startsWith("long-file.txt") || line.startsWith("short-file.ts")) ?? ""
+  );
+}
+
 async function waitForSnapshot(
   setup: Awaited<ReturnType<typeof testRender>>,
   getSnapshot: () => HunkSessionSnapshot["state"] | null,
@@ -1837,6 +1904,74 @@ describe("App interactions", () => {
     }
   });
 
+  test("forward cross-file hunk navigation keeps the destination file owning the review pane", async () => {
+    const setup = await testRender(
+      <AppHost bootstrap={createCrossFileHunkNavigationBootstrap()} />,
+      {
+        width: 120,
+        height: 16,
+      },
+    );
+
+    try {
+      await flush(setup);
+      await pressHunkNavigationKey(setup, "]", 18);
+
+      let frame = await waitForFrame(
+        setup,
+        (nextFrame) =>
+          nextFrame.includes("short-file.ts") && nextFrame.includes("export const top = 2;"),
+        24,
+      );
+      expect(firstCrossFileHunkNavigationHeader(frame)).toContain("short-file.ts");
+
+      await pressHunkNavigationKey(setup, "]", 1);
+      frame = await waitForFrame(
+        setup,
+        (nextFrame) => nextFrame.includes("export const mid = 4;"),
+        24,
+      );
+
+      expect(firstCrossFileHunkNavigationHeader(frame)).toContain("short-file.ts");
+      expect(frame).not.toContain("line 341 changed");
+    } finally {
+      await act(async () => {
+        setup.renderer.destroy();
+      });
+    }
+  });
+
+  test("backward cross-file hunk navigation reveals the target hunk instead of the file top", async () => {
+    const setup = await testRender(
+      <AppHost bootstrap={createCrossFileHunkNavigationBootstrap()} />,
+      {
+        width: 120,
+        height: 16,
+      },
+    );
+
+    try {
+      await flush(setup);
+      await pressHunkNavigationKey(setup, "]", 19);
+      await waitForFrame(setup, (nextFrame) => nextFrame.includes("export const mid = 4;"), 24);
+
+      await pressHunkNavigationKey(setup, "[", 2);
+      const frame = await waitForFrame(
+        setup,
+        (nextFrame) =>
+          nextFrame.includes("line 341 changed") || nextFrame.includes("line 002 changed"),
+        24,
+      );
+
+      expect(frame).toContain("line 341 changed");
+      expect(frame).not.toContain("line 002 changed");
+    } finally {
+      await act(async () => {
+        setup.renderer.destroy();
+      });
+    }
+  });
+
   test("mouse wheel scrolling updates the active file and hunk to the viewport center", async () => {
     const { getLatestSnapshot, hostClient } = createMockHostClient();
     const setup = await testRender(

src/ui/components/panes/DiffPane.tsx

@@ -664,6 +664,10 @@ export function DiffPane({
   const selectedNoteTop = selectedNoteBounds?.top ?? null;
   const selectedNoteHeight = selectedNoteBounds?.height ?? null;
 
+  /** The bodyTop of the currently selected file's section layout, used to floor hunk reveal scroll targets so they never cross above the owning file boundary. */
+  const selectedFileBodyTop =
+    selectedFileIndex >= 0 ? (fileSectionLayouts[selectedFileIndex]?.bodyTop ?? 0) : 0;
+
   // Track the previous selected anchor to detect actual selection changes.
   const prevSelectedAnchorIdRef = useRef<string | null>(null);
   const prevPinnedHeaderFileIdRef = useRef<string | null>(null);
@@ -902,17 +906,16 @@ export function DiffPane({
       // hunk can request a top offset that is no longer reachable once the viewport hits EOF.
       // Using the reachable value keeps the reveal logic from fighting later manual scrolling.
       if (selectedNoteBounds) {
-        scrollBox.scrollTo(
-          clampReviewScrollTop(
-            computeHunkRevealScrollTop({
-              hunkTop: selectedNoteBounds.top,
-              hunkHeight: selectedNoteBounds.height,
-              preferredTopPadding,
-              viewportHeight,
-            }),
-            viewportHeight,
-          ),
-        );
+        const revealScrollTop = computeHunkRevealScrollTop({
+          hunkTop: selectedNoteBounds.top,
+          hunkHeight: selectedNoteBounds.height,
+          preferredTopPadding,
+          viewportHeight,
+        });
+        // Floor against the owning file's body boundary so the viewport never crosses above it
+        // and triggers a pinned-header flash.
+        const flooredScrollTop = Math.max(revealScrollTop, selectedFileBodyTop);
+        scrollBox.scrollTo(clampReviewScrollTop(flooredScrollTop, viewportHeight));
         return;
       }
 
@@ -938,17 +941,16 @@ export function DiffPane({
           ? Math.max(0, renderedBottom - renderedTop)
           : selectedEstimatedHunkBounds.height;
 
-        scrollBox.scrollTo(
-          clampReviewScrollTop(
-            computeHunkRevealScrollTop({
-              hunkTop,
-              hunkHeight,
-              preferredTopPadding,
-              viewportHeight,
-            }),
-            viewportHeight,
-          ),
-        );
+        const revealScrollTop = computeHunkRevealScrollTop({
+          hunkTop,
+          hunkHeight,
+          preferredTopPadding,
+          viewportHeight,
+        });
+        // Floor against the owning file's body boundary so the viewport never crosses above it
+        // and triggers a pinned-header flash.
+        const flooredScrollTop = Math.max(revealScrollTop, selectedFileBodyTop);
+        scrollBox.scrollTo(clampReviewScrollTop(flooredScrollTop, viewportHeight));
         return;
       }
 
@@ -988,6 +990,7 @@ export function DiffPane({
     selectedFileIndex,
     selectedHunkIndex,
     selectedHunkRevealRequestId,
+    selectedFileBodyTop,
     selectedNoteHeight,
     selectedNoteTop,
     suppressViewportSelectionSync,

src/ui/hooks/useReviewController.ts

@@ -204,8 +204,13 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon
         return;
       }
 
+      const crossingFileBoundary = nextCursor.fileId !== selectedFile?.id;
       selectHunk(nextCursor.fileId, nextCursor.hunkIndex, {
-        alignFileHeaderTop: nextCursor.fileId !== selectedFile?.id,
+        // Align the file header to top only for forward cross-file jumps so the new file
+        // starts at its header. Backward jumps should reveal the target hunk directly,
+        // since the target is often near the bottom of the previous file and the file-top
+        // align would require an extra navigation press to reach it.
+        alignFileHeaderTop: crossingFileBoundary && delta > 0,
       });
     },
     [hunkCursors, selectHunk, selectedFile?.id, selectedHunkIndex],

test/pty/harness.ts

@@ -332,6 +332,46 @@ export function createPtyHarness() {
     ]);
   }
 
+  /** Build the cross-file hunk-navigation shape that used to jump backward to the file top. */
+  function createCrossFileHunkNavigationRepoFixture() {
+    const longBeforeLines = Array.from(
+      { length: 342 },
+      (_, index) => `line ${String(index + 1).padStart(3, "0")}`,
+    );
+    const longAfterLines = [...longBeforeLines];
+    for (const lineNumber of [
+      2, 21, 41, 61, 81, 101, 121, 141, 161, 181, 201, 221, 241, 261, 281, 301, 321, 341,
+    ]) {
+      longAfterLines[lineNumber - 1] = `line ${String(lineNumber).padStart(3, "0")} changed`;
+    }
+
+    const shortBeforeLines = [
+      "// hunk 0 - at the very top of the file",
+      "export const top = 1;",
+      "",
+      "",
+      ...Array.from({ length: 25 }, (_, index) => `// filler ${index + 1}`),
+      "// hunk 1 - mid-file",
+      "export const mid = 3;",
+    ];
+    const shortAfterLines = [...shortBeforeLines];
+    shortAfterLines[1] = "export const top = 2;";
+    shortAfterLines[30] = "export const mid = 4;";
+
+    return createGitRepoFixture([
+      {
+        path: "long-file.txt",
+        before: `${longBeforeLines.join("\n")}\n`,
+        after: `${longAfterLines.join("\n")}\n`,
+      },
+      {
+        path: "short-file.ts",
+        before: `${shortBeforeLines.join("\n")}\n`,
+        after: `${shortAfterLines.join("\n")}\n`,
+      },
+    ]);
+  }
+
   function createPagerPatchFixture(lines = 40) {
     const dir = makeTempDir("hunk-tuistory-pager-");
     const beforeDir = join(dir, "before");
@@ -477,6 +517,7 @@ export function createPtyHarness() {
     createAgentFilePair,
     createBottomClampedRepoFixture,
     createCollapsedTopRepoFixture,
+    createCrossFileHunkNavigationRepoFixture,
     createLongWrapFilePair,
     createMultiHunkFilePair,
     createPagerPatchFixture,

test/pty/ui-integration.test.ts

@@ -122,6 +122,47 @@ describe("live UI integration", () => {
     }
   });
 
+  test("backward cross-file hunk navigation reveals the target hunk in a real PTY", async () => {
+    const fixture = harness.createCrossFileHunkNavigationRepoFixture();
+    const session = await harness.launchHunk({
+      args: ["diff", "--mode", "split"],
+      cwd: fixture.dir,
+      cols: 120,
+      rows: 16,
+    });
+
+    try {
+      await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, {
+        timeout: 15_000,
+      });
+
+      for (let index = 0; index < 19; index += 1) {
+        await session.press("]");
+        await session.waitIdle({ timeout: 40 });
+      }
+
+      await harness.waitForSnapshot(
+        session,
+        (text) => text.includes("export const mid = 4;"),
+        5_000,
+      );
+
+      await session.press("[");
+      await session.waitIdle({ timeout: 80 });
+      await session.press("[");
+      const backward = await harness.waitForSnapshot(
+        session,
+        (text) => text.includes("line 341 changed") || text.includes("line 002 changed"),
+        5_000,
+      );
+
+      expect(backward).toContain("line 341 changed");
+      expect(backward).not.toContain("line 002 changed");
+    } finally {
+      session.close();
+    }
+  });
+
   test("a short last file does not trap upward scrolling at the bottom edge", async () => {
     const fixture = harness.createBottomClampedRepoFixture();
     const session = await harness.launchHunk({