fix(web): honor whitespace settings in pull request diffs

apps/web/src/components/pullRequest/PullRequestCodeTab.tsx

@@ -20,6 +20,7 @@ import {
   InfoIcon,
   MessageSquareIcon,
   MessageSquareOffIcon,
+  PilcrowIcon,
   Rows3Icon,
   TextWrapIcon,
   TriangleAlertIcon,
@@ -233,6 +234,7 @@ function PullRequestCodeTab({
   const diffLayout = settings.diffLayout;
   const updateClientSettings = useUpdateClientSettings();
   const [wordWrap, setWordWrap] = useState(settings.wordWrap);
+  const [ignoreWhitespace, setIgnoreWhitespace] = useState(settings.diffIgnoreWhitespace);
   const [fileTreeOpen, setFileTreeOpen] = useLocalStorage(
     PULL_REQUEST_FILE_TREE_STORAGE_KEY,
     false,
@@ -390,16 +392,17 @@ function PullRequestCodeTab({
       loadedSlices.map((slice) => {
         // The patch's own hash is part of the key: a refreshed page reuses its cursor, and a
         // key of position alone would keep handing back the parse of the patch it replaced.
-        const cacheKey = `pull-request:${scopeKey}:${resolvedTheme}:${slice.cursor ?? "first"}:${fnv1a32(slice.patch)}`;
+        const cacheKey = `pull-request:${scopeKey}:${resolvedTheme}:${ignoreWhitespace}:${slice.cursor ?? "first"}:${fnv1a32(slice.patch)}`;
         const cached = parseCache.current.get(cacheKey);
         if (cached) return cached;
         const parsed = getRenderablePatch(slice.patch, cacheKey, {
           compactPartialHunkOffsets: true,
+          ignoreWhitespace,
         });
         if (parsed) parseCache.current.set(cacheKey, parsed);
         return parsed;
       }),
-    [loadedSlices, resolvedTheme, scopeKey],
+    [loadedSlices, resolvedTheme, scopeKey, ignoreWhitespace],
   );
   // Ordered within a slice rather than across them: ordering the accumulated set would let a late
   // slice push a file the reader is part way through further down the page.
@@ -702,7 +705,13 @@ function PullRequestCodeTab({
       // that silently lost its first line on the other hosts would be worse than one line.
       const path = resolveFileDiffPath(file);
       const previousPath = resolveFileDiffPreviousPath(file);
-      const position = resolveDiffReviewPosition(file, range.end, range.endSide ?? range.side);
+      const sourceFile = parsedSlices
+        .flatMap((slice) => (slice?.kind === "files" ? slice.sourceFiles : []))
+        .find((candidate) => resolveFileDiffPath(candidate) === path);
+      const side = range.endSide ?? range.side;
+      const position =
+        (sourceFile && resolveDiffReviewPosition(sourceFile, range.end, side)) ??
+        resolveDiffReviewPosition(file, range.end, side);
       if (position === null) return;
       setDraft({
         fileKey: item.id,
@@ -712,7 +721,7 @@ function PullRequestCodeTab({
         range,
       });
     },
-    [canCommentOnLines, files],
+    [canCommentOnLines, files, parsedSlices],
   );
 
   // Built here because the parsed diff only lives here, and built by the same function the
@@ -1123,11 +1132,6 @@ function PullRequestCodeTab({
     }
   }, [commit, onSelectedCommitChange, selectedCommit]);
   const scopeLabel = selectedCommit ? selectedCommit.messageHeadline : "All commits";
-  /**
-   * The same controls the thread diff panel carries, in the same order, minus the
-   * ignore-whitespace toggle: that is `git diff -w` on the server, and no host's pull request
-   * diff API offers it.
-   */
   const toolbar = (
     <div className="flex h-10 min-h-10 shrink-0 items-center justify-between gap-2 border-b border-border/60 bg-background px-4 text-xs text-muted-foreground">
       <div className="flex min-w-0 flex-1 items-center gap-3">
@@ -1276,6 +1280,30 @@ function PullRequestCodeTab({
         </PullRequestMetaLine>
       </div>
       <div className="flex shrink-0 items-center gap-1">
+        <Tooltip>
+          <TooltipTrigger
+            render={
+              <Toggle
+                aria-label={
+                  ignoreWhitespace ? "Show whitespace changes" : "Hide whitespace changes"
+                }
+                variant="ghost"
+                size="sm"
+                pressed={ignoreWhitespace}
+                onPressedChange={(pressed) => {
+                  setIgnoreWhitespace(Boolean(pressed));
+                  setDraft(null);
+                  setSelectedLines(null);
+                }}
+              />
+            }
+          >
+            <PilcrowIcon className="size-3.5" />
+          </TooltipTrigger>
+          <TooltipPopup side="top">
+            {ignoreWhitespace ? "Show whitespace changes" : "Hide whitespace changes"}
+          </TooltipPopup>
+        </Tooltip>
         {fileKeys.length > 0 ? (
           <Tooltip>
             <TooltipTrigger

apps/web/src/lib/diffRendering.test.ts

@@ -1,4 +1,6 @@
+import { hydratePartialDiff } from "@pierre/diffs";
 import { describe, expect, it } from "vite-plus/test";
+import { resolveDiffReviewPosition } from "../reviewCommentContext";
 import {
   buildFileDiffContentVersion,
   buildFileDiffIdentityKey,
@@ -34,6 +36,135 @@ describe("buildPatchCacheKey", () => {
 });
 
 describe("getRenderablePatch", () => {
+  it("hides indentation changes around inserted JSX without moving review lines", () => {
+    const patch = [
+      "diff --git a/item.tsx b/item.tsx",
+      "--- a/item.tsx",
+      "+++ b/item.tsx",
+      "@@ -40,5 +40,7 @@",
+      ' <ItemContent className="min-w-0">',
+      "-  <ItemTitle>",
+      '-    <h4 className="wrap-break-word">{name}</h4>',
+      "-  </ItemTitle>",
+      "+  {showName && (",
+      "+    <ItemTitle>",
+      '+      <h4 className="wrap-break-word">{name}</h4>',
+      "+    </ItemTitle>",
+      "+  )}",
+      " </ItemContent>",
+      "@@ -80 +82 @@",
+      "-const value = 1;",
+      "+const value = 2;",
+    ].join("\n");
+    const shown = getRenderablePatch(patch, "pr", { compactPartialHunkOffsets: true });
+    const hidden = getRenderablePatch(patch, "pr", {
+      compactPartialHunkOffsets: true,
+      ignoreWhitespace: true,
+    });
+    expect(shown?.kind).toBe("files");
+    expect(hidden?.kind).toBe("files");
+    if (shown?.kind !== "files" || hidden?.kind !== "files") return;
+    expect(getDiffLineStat(shown.files)).toEqual({ additions: 6, deletions: 4 });
+    expect(getDiffLineStat(hidden.files)).toEqual({ additions: 3, deletions: 1 });
+    const file = hidden.files[0]!;
+    expect(file.additionLines).toEqual(shown.files[0]!.additionLines);
+    expect(file.deletionLines).toEqual(shown.files[0]!.deletionLines);
+    expect(file.cacheKey).not.toBe(shown.files[0]!.cacheKey);
+    expect(resolveDiffReviewPosition(hidden.sourceFiles[0]!, 43, "additions")).toEqual({
+      kind: "added",
+      newLine: 43,
+    });
+    expect(resolveDiffReviewPosition(hidden.sourceFiles[0]!, 42, "deletions")).toEqual({
+      kind: "deleted",
+      oldLine: 42,
+    });
+    expect(file.hunks[0]?.hunkContent).toContainEqual({
+      type: "context",
+      lines: 3,
+      additionLineIndex: 2,
+      deletionLineIndex: 1,
+    });
+    expect(resolveDiffReviewPosition(file, 41, "additions")).toEqual({
+      kind: "added",
+      newLine: 41,
+    });
+    expect(resolveDiffReviewPosition(file, 43, "additions")).toEqual({
+      kind: "context",
+      oldLine: 42,
+      newLine: 43,
+      side: "right",
+    });
+    expect(resolveDiffReviewPosition(file, 42, "deletions")).toEqual({
+      kind: "context",
+      oldLine: 42,
+      newLine: 43,
+      side: "left",
+    });
+    expect(file.hunks[1]).toMatchObject({
+      additionStart: 82,
+      deletionStart: 80,
+      splitLineStart: 7,
+      unifiedLineStart: 7,
+    });
+    const prefix = "unchanged\n".repeat(39);
+    const gap = "unchanged\n".repeat(35);
+    const hydrated = hydratePartialDiff("clone", file, {
+      oldFile: {
+        name: file.name,
+        contents: prefix + file.deletionLines.slice(0, 5).join("") + gap + "const value = 1;\n",
+      },
+      newFile: {
+        name: file.name,
+        contents: prefix + file.additionLines.slice(0, 7).join("") + gap + "const value = 2;\n",
+      },
+    });
+    expect(getDiffLineStat([hydrated])).toEqual({ additions: 3, deletions: 1 });
+    expect(resolveDiffReviewPosition(hydrated, 43, "additions")).toEqual(
+      resolveDiffReviewPosition(file, 43, "additions"),
+    );
+  });
+
+  it.each([
+    ["  const x = 1;\t", "\tconst x=1;", 0, 0],
+    ["const x = 1;", "const x = 2;", 1, 1],
+    ["const x = 1;", "const x = 1;\n", 1, 0],
+  ])("filters whitespace in %j to %j", (before, after, additions, deletions) => {
+    const patch = [
+      "diff --git a/example.ts b/example.ts",
+      "--- a/example.ts",
+      "+++ b/example.ts",
+      `@@ -1 +1,${after.split("\n").length} @@`,
+      `-${before}`,
+      ...after.split("\n").map((line) => `+${line}`),
+      "",
+    ].join("\n");
+    const filtered = getRenderablePatch(patch, "pr", { ignoreWhitespace: true });
+    expect(filtered?.kind).toBe("files");
+    if (filtered?.kind !== "files") return;
+    expect(getDiffLineStat(filtered.files)).toEqual({ additions, deletions });
+  });
+
+  it.each(["+", "-"])("keeps %s blank lines without a final newline", (sign) => {
+    const parsed = getRenderablePatch(
+      [
+        "diff --git a/blank.txt b/blank.txt",
+        "--- a/blank.txt",
+        "+++ b/blank.txt",
+        sign === "+" ? "@@ -0,0 +1 @@" : "@@ -1 +0,0 @@",
+        `${sign}  `,
+        "\\ No newline at end of file",
+      ].join("\n"),
+      "pr",
+      { ignoreWhitespace: true },
+    );
+    expect(parsed?.kind).toBe("files");
+    if (parsed?.kind !== "files") return;
+    expect(getDiffLineStat(parsed.files)).toEqual({
+      additions: sign === "+" ? 1 : 0,
+      deletions: sign === "-" ? 1 : 0,
+    });
+  });
+
   it.each([
     ["a/example.ts", "a/example.ts", "change"],
     ["b/example.ts", "b/example.ts", "change"],

apps/web/src/lib/diffRendering.ts

@@ -1,4 +1,5 @@
 import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles";
+import { parseDiffFromFile } from "@pierre/diffs";
 import type { FileDiffMetadata } from "@pierre/diffs/types";
 import { unquoteGitPatchPath } from "@t3tools/shared/gitPatchPath";
 
@@ -46,6 +47,7 @@ export type RenderablePatch =
   | {
       kind: "files";
       files: FileDiffMetadata[];
+      sourceFiles: FileDiffMetadata[];
     }
   | {
       kind: "raw";
@@ -73,6 +75,7 @@ export function getDiffLineStat(files: ReadonlyArray<FileDiffMetadata>): DiffLin
 }
 
 interface RenderablePatchOptions {
+  ignoreWhitespace?: boolean;
   /**
    * Pierre's partial-patch parser keeps hunk render starts in source-file
    * coordinates. Its virtualizer iterates partial patches as compact rows, so
@@ -82,6 +85,59 @@ interface RenderablePatchOptions {
   compactPartialHunkOffsets?: boolean;
 }
 
+function hideWhitespaceChanges(file: FileDiffMetadata): FileDiffMetadata {
+  let splitDelta = 0;
+  let unifiedDelta = 0;
+  const hunks = file.hunks.map((hunk) => {
+    const oldContents = file.deletionLines
+      .slice(hunk.deletionLineIndex, hunk.deletionLineIndex + hunk.deletionCount)
+      .map((line) => `${line.replace(/\s/g, "")}\n`)
+      .join("");
+    const newContents = file.additionLines
+      .slice(hunk.additionLineIndex, hunk.additionLineIndex + hunk.additionCount)
+      .map((line) => `${line.replace(/\s/g, "")}\n`)
+      .join("");
+    const filtered = parseDiffFromFile(
+      { name: file.name, contents: oldContents },
+      { name: file.name, contents: newContents },
+      { context: Infinity },
+    ).hunks[0];
+    const next = {
+      ...hunk,
+      additionLines: filtered?.additionLines ?? 0,
+      deletionLines: filtered?.deletionLines ?? 0,
+      hunkContent: filtered
+        ? filtered.hunkContent.map((content) => ({
+            ...content,
+            additionLineIndex: content.additionLineIndex + hunk.additionLineIndex,
+            deletionLineIndex: content.deletionLineIndex + hunk.deletionLineIndex,
+          }))
+        : [
+            {
+              type: "context" as const,
+              lines: hunk.additionCount,
+              additionLineIndex: hunk.additionLineIndex,
+              deletionLineIndex: hunk.deletionLineIndex,
+            },
+          ],
+      splitLineStart: hunk.splitLineStart + splitDelta,
+      unifiedLineStart: hunk.unifiedLineStart + unifiedDelta,
+      splitLineCount: filtered?.splitLineCount ?? hunk.additionCount,
+      unifiedLineCount: filtered?.unifiedLineCount ?? hunk.additionCount,
+    };
+    splitDelta += next.splitLineCount - hunk.splitLineCount;
+    unifiedDelta += next.unifiedLineCount - hunk.unifiedLineCount;
+    return next;
+  });
+  return {
+    ...file,
+    hunks,
+    splitLineCount: file.splitLineCount + splitDelta,
+    unifiedLineCount: file.unifiedLineCount + unifiedDelta,
+    ...(file.cacheKey ? { cacheKey: `${file.cacheKey}:ignore-whitespace` } : {}),
+  };
+}
+
 function compactPartialHunkOffsets(file: FileDiffMetadata): FileDiffMetadata {
   if (!file.isPartial) return file;
 
@@ -121,13 +177,13 @@ export function getRenderablePatch(
       normalizedPatch,
       buildPatchCacheKey(normalizedPatch, cacheScope),
     );
-    const files = parsedPatches.flatMap((parsedPatch) =>
-      options.compactPartialHunkOffsets
-        ? parsedPatch.files.map(compactPartialHunkOffsets)
-        : parsedPatch.files,
-    );
+    const sourceFiles = parsedPatches.flatMap((parsedPatch) => parsedPatch.files);
+    const files = sourceFiles.map((file) => {
+      const filtered = options.ignoreWhitespace ? hideWhitespaceChanges(file) : file;
+      return options.compactPartialHunkOffsets ? compactPartialHunkOffsets(filtered) : filtered;
+    });
     if (files.length > 0) {
-      return { kind: "files", files };
+      return { kind: "files", files, sourceFiles };
     }
 
     return {