refactor(review): tighten expansion geometry polish

Author: adit-chandra

CHANGELOG.md

@@ -6,7 +6,7 @@ All notable user-visible changes to Hunk are documented in this file.
 
 ### Added
 
-- Added inline expansion for collapsed unchanged file content. Click an unchanged-context row (`▾ N unchanged lines` when expandable, otherwise the static `··· N unchanged lines ···` form) or press `e` while a hunk is selected to reveal surrounding and trailing file lines without leaving the review. The affordance is shown only for input modes that have reachable source content (`hunk diff`, `show`, `stash show`, file-pair `diff` and `difftool`, untracked files); raw `hunk patch` input still renders as before. Failed and in-flight loads surface a one-line status ("Loading…", "Could not load N unchanged lines") on the gap row. Expanded context rows are plain text in this release; syntax highlighting will follow.
+- Added inline expansion for collapsed unchanged file content. Click an unchanged-context row (`▾ N unchanged lines` when expandable, otherwise the static `··· N unchanged lines ···` form) or press `e` while a hunk is selected to reveal surrounding and trailing file lines without leaving the review. The affordance is shown only for input modes that have reachable source content (`hunk diff`, `show`, `stash show`, file-pair `diff` and `difftool`, untracked files); raw `hunk patch` input still renders as before. Failed and in-flight loads surface a one-line status ("Loading…", "Could not load N unchanged lines") on the gap row. Expanded context rows use the same syntax highlighting as the surrounding diff.
 
 ### Changed
 

src/core/fileSource.ts

@@ -17,9 +17,9 @@ export type FileSourceSide = "old" | "new";
 export interface FileSourceFetcher {
   /**
    * Returns the file's full source text on the requested side, or `null` when
-   * the side is not reachable (deleted side, missing path, git error). Never
-   * rejects: callers may safely fire-and-forget the promise without handling
-   * rejection.
+   * the side is not reachable (deleted side, missing path, git error). Built-in
+   * fetchers resolve `null` instead of rejecting, but UI callers still handle
+   * custom fetcher rejection defensively.
    */
   getFullText(side: FileSourceSide): Promise<string | null>;
 }

src/ui/diff/plannedReviewRows.ts

@@ -25,10 +25,25 @@ export interface PlannedHunkBounds extends VerticalBounds {
 export type PlannedSectionGeometry = SectionGeometry<PlannedHunkBounds>;
 
 /** Return whether this planned row should count toward a hunk's own visible extent. */
-function rowContributesToHunkBounds(row: PlannedReviewRow) {
+export function plannedReviewRowContributesToHunkBounds(row: PlannedReviewRow) {
+  if (row.kind !== "diff-row") {
+    return true;
+  }
+
   // Collapsed gap rows sit outside hunk bodies, so they affect total section height but not a
-  // hunk's own visible extent.
-  return !(row.kind === "diff-row" && row.row.type === "collapsed");
+  // hunk's own visible extent. Expanded rows share that property: they fill a gap, not a hunk.
+  if (row.row.type === "collapsed") {
+    return false;
+  }
+
+  if (
+    (row.row.type === "split-line" || row.row.type === "stack-line") &&
+    row.row.isExpansionRow === true
+  ) {
+    return false;
+  }
+
+  return true;
 }
 
 /** Measure how many terminal rows one planned review row will occupy once rendered. */
@@ -86,7 +101,7 @@ export function measurePlannedSectionGeometry(
 
     const rowHeight = plannedReviewRowHeight(row, options);
 
-    if (rowHeight > 0 && rowContributesToHunkBounds(row)) {
+    if (rowHeight > 0 && plannedReviewRowContributesToHunkBounds(row)) {
       const rowId = reviewRowId(row.key);
       const existingBounds = hunkBounds.get(row.hunkIndex);
 

src/ui/hooks/useReviewController.test.tsx

@@ -128,29 +128,15 @@ async function renderReviewController(initialFiles: DiffFile[]) {
 
 describe("useReviewController", () => {
   test("reselects the first visible file when filtering hides the current selection", async () => {
-    const controllerRef: { current: ReviewController | null } = { current: null };
-    const setup = await testRender(
-      <ReviewControllerHarness
-        initialFiles={[
-          createDiffFile(
-            "alpha",
-            "alpha.ts",
-            "export const alpha = 1;\n",
-            "export const alpha = 2;\n",
-          ),
-          createDiffFile(
-            "beta",
-            "beta.ts",
-            "export const beta = 1;\n",
-            "export const betaValue = 2;\n",
-          ),
-        ]}
-        onController={(nextController) => {
-          controllerRef.current = nextController;
-        }}
-      />,
-      { width: 80, height: 4 },
-    );
+    const { controllerRef, setup } = await renderReviewController([
+      createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"),
+      createDiffFile(
+        "beta",
+        "beta.ts",
+        "export const beta = 1;\n",
+        "export const betaValue = 2;\n",
+      ),
+    ]);
 
     try {
       await flush(setup);
@@ -175,20 +161,9 @@ describe("useReviewController", () => {
   });
 
   test("clamps the selected hunk index when files update under a soft reload", async () => {
-    const controllerRef: { current: ReviewController | null } = { current: null };
-    const setFilesRef: { current: ((nextFiles: DiffFile[]) => void) | null } = { current: null };
-    const setup = await testRender(
-      <ReviewControllerHarness
-        initialFiles={[createTwoHunkFile()]}
-        onController={(nextController) => {
-          controllerRef.current = nextController;
-        }}
-        onSetFiles={(nextSetFiles) => {
-          setFilesRef.current = nextSetFiles;
-        }}
-      />,
-      { width: 80, height: 4 },
-    );
+    const { controllerRef, setFilesRef, setup } = await renderReviewController([
+      createTwoHunkFile(),
+    ]);
 
     try {
       await flush(setup);
@@ -215,24 +190,10 @@ describe("useReviewController", () => {
   });
 
   test("live comment mutations update annotated navigation without remounting the app", async () => {
-    const controllerRef: { current: ReviewController | null } = { current: null };
-    const setup = await testRender(
-      <ReviewControllerHarness
-        initialFiles={[
-          createDiffFile(
-            "alpha",
-            "alpha.ts",
-            "export const alpha = 1;\n",
-            "export const alpha = 2;\n",
-          ),
-          createDiffFile("beta", "beta.ts", "export const beta = 1;\n", "export const beta = 2;\n"),
-        ]}
-        onController={(nextController) => {
-          controllerRef.current = nextController;
-        }}
-      />,
-      { width: 80, height: 4 },
-    );
+    const { controllerRef, setup } = await renderReviewController([
+      createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"),
+      createDiffFile("beta", "beta.ts", "export const beta = 1;\n", "export const beta = 2;\n"),
+    ]);
 
     try {
       await flush(setup);
@@ -287,16 +248,7 @@ describe("useReviewController", () => {
   });
 
   test("batch live comments validate together and reveal the first applied hunk", async () => {
-    const controllerRef: { current: ReviewController | null } = { current: null };
-    const setup = await testRender(
-      <ReviewControllerHarness
-        initialFiles={[createTwoHunkFile()]}
-        onController={(nextController) => {
-          controllerRef.current = nextController;
-        }}
-      />,
-      { width: 80, height: 4 },
-    );
+    const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]);
 
     try {
       await flush(setup);
@@ -336,16 +288,7 @@ describe("useReviewController", () => {
   });
 
   test("batch live comments do not mutate state when any target is invalid", async () => {
-    const controllerRef: { current: ReviewController | null } = { current: null };
-    const setup = await testRender(
-      <ReviewControllerHarness
-        initialFiles={[createTwoHunkFile()]}
-        onController={(nextController) => {
-          controllerRef.current = nextController;
-        }}
-      />,
-      { width: 80, height: 4 },
-    );
+    const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]);
 
     try {
       await flush(setup);

src/ui/lib/diffSectionGeometry.ts

@@ -4,7 +4,10 @@ import { findMaxLineNumber, findMaxLineNumberInRows } from "../diff/codeColumns"
 import { expandCollapsedRows, type FileSourceStatus } from "../diff/expandCollapsedRows";
 import { buildSplitRows, buildStackRows } from "../diff/pierre";
 import { measureRenderedRowHeight } from "../diff/renderRows";
-import type { PlannedHunkBounds } from "../diff/plannedReviewRows";
+import {
+  plannedReviewRowContributesToHunkBounds,
+  type PlannedHunkBounds,
+} from "../diff/plannedReviewRows";
 import { buildReviewRenderPlan, type PlannedReviewRow } from "../diff/reviewRenderPlan";
 import type { SectionGeometry, VerticalBounds } from "./diffSpatial";
 import { reviewRowId } from "./ids";
@@ -117,26 +120,6 @@ function plannedRowHeight(
   );
 }
 
-/** Return whether a measured review row should count toward the visible extent of its hunk. */
-function rowContributesToHunkBounds(row: PlannedReviewRow) {
-  if (row.kind !== "diff-row") {
-    return true;
-  }
-  // Collapsed gap rows sit outside hunk bodies, so they affect total section height but should not
-  // make a selected hunk look taller than the rows that actually belong to it. Expanded rows share
-  // that property: they fill the gap but still belong to the gap, not to either neighbor.
-  if (row.row.type === "collapsed") {
-    return false;
-  }
-  if (
-    (row.row.type === "split-line" || row.row.type === "stack-line") &&
-    row.row.isExpansionRow === true
-  ) {
-    return false;
-  }
-  return true;
-}
-
 /** Measure one file section from the same render plan used by PierreDiffView. */
 export function measureDiffSectionGeometry(
   file: DiffFile,
@@ -226,7 +209,7 @@ export function measureDiffSectionGeometry(
       }
     }
 
-    if (height > 0 && rowContributesToHunkBounds(row)) {
+    if (height > 0 && plannedReviewRowContributesToHunkBounds(row)) {
       const rowId = reviewRowId(row.key);
       const existingBounds = hunkBounds.get(row.hunkIndex);