diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 4258842d04be..fe6e1aacce7b 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -1,3 +1,5 @@ +import { WHITESPACE_MODES } from "./pullRequestWhitespace"; +import { usePullRequestWhitespace } from "./usePullRequestWhitespace"; import type { CodeViewItem, DiffLineAnnotation, SelectedLineRange } from "@pierre/diffs"; import type { CodeViewDiffItem, CodeViewHandle } from "@pierre/diffs/react"; import type { @@ -70,6 +72,8 @@ import { DropdownMenu, DropdownMenuContent, DropdownMenuItem, + DropdownMenuRadioGroup, + DropdownMenuRadioItem, DropdownMenuTrigger, } from "../ui/menu"; import { toastManager } from "../ui/toast"; @@ -103,6 +107,8 @@ type ReviewAnnotation = DiffLineAnnotation; /** Commits per press of "Show more" in the scope menu. */ const COMMIT_PAGE_SIZE = 10; +const WhitespaceModeSchema = Schema.Literals(["all", "ignore-all", "ignore-amount", "ignore-eol"]); + const PULL_REQUEST_FILE_TREE_STORAGE_KEY = "t3code.pullRequestFileTreeOpen"; /** One answer from the host: a whole number of files, and where the next one carries on. */ @@ -228,6 +234,11 @@ function PullRequestCodeTab({ const diffLayout = settings.diffLayout; const updateClientSettings = useUpdateClientSettings(); const [wordWrap, setWordWrap] = useState(settings.wordWrap); + const [whitespaceMode, setWhitespaceMode] = useLocalStorage( + "t3code.pullRequestWhitespace", + "all", + WhitespaceModeSchema, + ); const [fileTreeOpen, setFileTreeOpen] = useLocalStorage( PULL_REQUEST_FILE_TREE_STORAGE_KEY, false, @@ -419,7 +430,9 @@ function PullRequestCodeTab({ const loadThreadComments = useAtomCommand(pullRequestEnvironment.threadComments, { reportFailure: false, }); - const getDiffFileContents = useAtomCommand(pullRequestEnvironment.diffFileContents); + const getDiffFileContents = useAtomCommand(pullRequestEnvironment.diffFileContents, { + reportFailure: false, + }); const loadDiffFiles = useMemo( () => createPullRequestDiffFileContentsLoader(getDiffFileContents, { @@ -452,9 +465,6 @@ function PullRequestCodeTab({ verdicts: hostReview.verdicts.filter((verdict) => viewer.verdicts.includes(verdict)), }; }, [detail.capabilities.review, detail.viewerPermissions]); - // A comment is posted against the pull request's head diff, so a line number taken from one - // commit's own diff would land somewhere else entirely. Commenting waits for the whole change. - const canCommentOnLines = review.inlineComment && commit === null; // Every slice is parsed on its own and the result held, so a slice arriving costs one parse // rather than one per slice already on screen. Its cache key carries the theme, which is what // the tokenizer caches against, so a theme change is still a fresh parse. @@ -476,13 +486,18 @@ function PullRequestCodeTab({ ); // 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. - const files = useMemo( + const originalFiles = useMemo( () => parsedSlices.flatMap((parsed) => parsed?.kind === "files" ? orderDiffFiles(parsed.files) : [], ), [parsedSlices], ); + const whitespace = usePullRequestWhitespace(originalFiles, whitespaceMode, loadDiffFiles); + const files = whitespace.files; + // A comment is posted against the pull request's head diff, so a line number taken from one + // commit's own diff would land somewhere else entirely. Commenting waits for the whole change. + const canCommentOnLines = review.inlineComment && commit === null && !whitespace.pending; const nextCursor = loadedSlices.at(-1)?.nextCursor ?? null; // What a slice withheld: the host declining to inline part of it, or a patch the viewer could // not structure and so dropped. Neither says anything about there being more to fetch. @@ -515,6 +530,21 @@ function PullRequestCodeTab({ return placed; }, [commit, detail.reviewThreads, files]); + const placedPendingIds = useMemo(() => { + const placed = new Set(); + if (commit !== null) return placed; + for (const file of files) { + const path = resolveFileDiffPath(file); + for (const comment of pendingComments) { + const anchor = getReviewPositionAnchor(comment.position); + if (comment.path === path && isLineInFileDiff(file, anchor.side, anchor.line)) { + placed.add(comment.id); + } + } + } + return placed; + }, [commit, files, pendingComments]); + const items = useMemo[]>( () => files.map((fileDiff) => { @@ -547,7 +577,7 @@ function PullRequestCodeTab({ // commit's diff must not place them either — the same line means other code there. if (commit === null) { for (const comment of pendingComments) { - if (comment.path !== path) continue; + if (comment.path !== path || !placedPendingIds.has(comment.id)) continue; const anchor = getReviewPositionAnchor(comment.position); groupAt(anchor.side, anchor.line).pending.push(comment); } @@ -609,6 +639,7 @@ function PullRequestCodeTab({ files, foldOverride, pendingComments, + placedPendingIds, placedThreadIds, toggledFiles, ], @@ -716,7 +747,12 @@ 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 original = originalFiles.find((candidate) => resolveFileDiffPath(candidate) === path); + const position = resolveDiffReviewPosition( + original ?? file, + range.end, + range.endSide ?? range.side, + ); if (position === null) return; setDraft({ fileKey: item.id, @@ -726,7 +762,7 @@ function PullRequestCodeTab({ range, }); }, - [canCommentOnLines, files], + [canCommentOnLines, files, originalFiles], ); // Built here because the parsed diff only lives here, and built by the same function the @@ -829,6 +865,11 @@ function PullRequestCodeTab({ } return (
+ {whitespaceMode !== "all" && + item.fileDiff.cacheKey?.includes(":whitespace:") && + item.fileDiff.hunks.length === 0 ? ( + Only whitespace changes + ) : null}
@@ -1229,6 +1275,51 @@ function PullRequestCodeTab({
+ {whitespace.pending ? Filtering whitespace... : null} + {whitespace.error ? ( + + }> + + + {whitespace.error} + + ) : null} + + + } + > + {whitespaceMode === "all" ? "Whitespace" : "Ignoring whitespace"} + + + + { + const mode = WHITESPACE_MODES.find((entry) => entry.value === value); + if (!mode) return; + setDraft(null); + setSelectedLines(null); + setWhitespaceMode(mode.value); + }} + > + {WHITESPACE_MODES.map((mode) => ( + + {mode.label} + + ))} + + + !placedThreadIds.has(thread.id)); + const orphanPending = pendingComments.filter((comment) => !placedPendingIds.has(comment.id)); // A file carrying five stranded conversations should read as that file once rather than as // five copies of its path. - const orphanFiles = new Map(); + const orphanFiles = new Map< + string, + { threads: PullRequestReviewThread[]; pending: PendingReviewComment[] } + >(); for (const thread of orphanThreads) { const existing = orphanFiles.get(thread.path); - if (existing) existing.push(thread); - else orphanFiles.set(thread.path, [thread]); + if (existing) existing.threads.push(thread); + else orphanFiles.set(thread.path, { threads: [thread], pending: [] }); + } + for (const comment of orphanPending) { + const existing = orphanFiles.get(comment.path); + if (existing) existing.pending.push(comment); + else orphanFiles.set(comment.path, { threads: [], pending: [comment] }); } const unstructured = @@ -1424,20 +1524,21 @@ function PullRequestCodeTab({ that has not landed yet, which is not the same as being off the diff. */} {nextCursor === null - ? "Conversations not on the current diff" - : "Conversations not on the diff loaded so far"} + ? "Comments not on the current diff" + : "Comments not on the diff loaded so far"} - {orphanThreads.length} + {orphanThreads.length + orphanPending.length} {orphanThreads.length === 1 ? "1 conversation" : `${orphanThreads.length} conversations`} + {orphanPending.length > 0 ? ` and ${orphanPending.length} pending comments` : null} @@ -1445,7 +1546,7 @@ function PullRequestCodeTab({ {/* Capped: opened on a change with dozens of them, this would otherwise leave no room for the diff it sits above. */}
- {[...orphanFiles].map(([path, threads]) => ( + {[...orphanFiles].map(([path, { threads, pending }]) => (
))} + {pending.map((comment) => ( +
+

+ Line {getReviewPositionAnchor(comment.position).line} +

+ removeComment(reviewKey, comment.id)} + /> +
+ ))}
))} diff --git a/apps/web/src/components/pullRequest/pullRequestWhitespace.test.ts b/apps/web/src/components/pullRequest/pullRequestWhitespace.test.ts new file mode 100644 index 000000000000..bc714fecc160 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestWhitespace.test.ts @@ -0,0 +1,104 @@ +import { parseDiffFromFile, parsePatchFiles } from "@pierre/diffs"; +import { describe, expect, it } from "vite-plus/test"; +import { getDiffLineStat } from "~/lib/diffRendering"; +import { resolveDiffReviewPosition } from "~/reviewCommentContext"; +import { filterDiffWhitespace } from "./pullRequestWhitespace"; + +const file = (before: string, after: string) => + parseDiffFromFile( + { name: "view.vue", contents: before }, + { name: "view.vue", contents: after }, + { context: 3 }, + ); + +describe("PR whitespace comparison", () => { + it("shows the wrapper without marking its indented children as changed", () => { + const children = Array.from({ length: 200 }, (_, i) => `

Item ${i}

\n`); + const original = file( + `\n`, + `\n`, + ); + const filtered = filterDiffWhitespace(original, "ignore-all"); + expect(getDiffLineStat([filtered])).toEqual({ additions: 2, deletions: 0 }); + expect(filtered.hunks).toHaveLength(2); + expect(filtered.additionLines).toEqual(original.additionLines); + expect(filtered.deletionLines).toEqual(original.deletionLines); + expect(resolveDiffReviewPosition(filtered, 203, "additions")).toEqual({ + kind: "added", + newLine: 203, + }); + expect(resolveDiffReviewPosition(filtered, 3, "additions")?.kind).toBe("context"); + // Submission must use the source diff: the host still calls the reindented line an addition. + expect(resolveDiffReviewPosition(original, 3, "additions")).toEqual({ + kind: "added", + newLine: 3, + }); + expect(filterDiffWhitespace(original, "all")).toBe(original); + }); + + it("keeps file coordinates and text when a partial patch starts far into a file", () => { + const original = parsePatchFiles( + "diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -100,3 +110,4 @@\n- before();\n- keep();\n- end();\n+ before();\n+ added();\n+ keep();\n+ end();\n", + )[0]!.files[0]!; + const filtered = filterDiffWhitespace(original, "ignore-all"); + expect(getDiffLineStat([filtered])).toEqual({ additions: 1, deletions: 0 }); + expect(filtered.hunks[0]).toMatchObject({ additionStart: 110, deletionStart: 100 }); + expect(resolveDiffReviewPosition(filtered, 111, "additions")).toEqual({ + kind: "added", + newLine: 111, + }); + expect(filtered.additionLines[filtered.hunks[0]!.additionLineIndex]).toBe(" before();\n"); + }); + + it("distinguishes all whitespace, whitespace amount, and trailing whitespace", () => { + const spacing = file("const x = a + b;\n", "const x = a + b; \n"); + expect(getDiffLineStat([filterDiffWhitespace(spacing, "ignore-amount")])).toEqual({ + additions: 0, + deletions: 0, + }); + expect(getDiffLineStat([filterDiffWhitespace(spacing, "ignore-eol")])).toEqual({ + additions: 1, + deletions: 1, + }); + const removal = file("a + b\n", "a+b\n"); + expect(filterDiffWhitespace(removal, "ignore-all").hunks).toHaveLength(0); + expect(filterDiffWhitespace(removal, "ignore-amount").hunks).toHaveLength(1); + expect(filterDiffWhitespace(file("x\n", "x \t\n"), "ignore-eol").hunks).toHaveLength(0); + }); + + it("compares across old hunk boundaries using full revisions and rejects changed revisions", () => { + const before = Array.from({ length: 30 }, (_, i) => `line ${i}\n`).join(""); + const after = before.replace("line 1\n", " line 1\n").replace("line 28\n", "new line 28\n"); + const original = parsePatchFiles( + "diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -1,3 +1,3 @@\n line 0\n-line 1\n+ line 1\n line 2\n@@ -28,3 +28,3 @@\n line 27\n-line 28\n+new line 28\n line 29\n", + )[0]!.files[0]!; + const contents = { + oldFile: { name: "a.ts", contents: before }, + newFile: { name: "a.ts", contents: after }, + }; + const filtered = filterDiffWhitespace(original, "ignore-all", contents); + expect(getDiffLineStat([filtered])).toEqual({ additions: 1, deletions: 1 }); + expect(filtered.hunks).toHaveLength(1); + expect(resolveDiffReviewPosition(filtered, 29, "additions")).toEqual({ + kind: "added", + newLine: 29, + }); + expect(() => filterDiffWhitespace(original, "ignore-all")).toThrow("Full file contents"); + expect(() => + filterDiffWhitespace(original, "ignore-all", { + ...contents, + newFile: { name: "a.ts", contents: `another line\n${after}` }, + }), + ).toThrow("file changed"); + }); + + it("preserves real edits, blank-line insertions, and missing final newlines", () => { + expect( + getDiffLineStat([filterDiffWhitespace(file(" old();\n", " new();\n"), "ignore-all")]), + ).toEqual({ additions: 1, deletions: 1 }); + expect( + getDiffLineStat([filterDiffWhitespace(file("x\ny\n", "x\n\ny\n"), "ignore-all")]), + ).toEqual({ additions: 1, deletions: 0 }); + expect(filterDiffWhitespace(file("x", "x\n"), "ignore-all").hunks).toHaveLength(1); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestWhitespace.ts b/apps/web/src/components/pullRequest/pullRequestWhitespace.ts new file mode 100644 index 000000000000..3cd985386c48 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestWhitespace.ts @@ -0,0 +1,115 @@ +import { hydratePartialDiff, parseDiffFromFile } from "@pierre/diffs"; +import type { FileDiffLoadedFiles, FileDiffMetadata } from "@pierre/diffs"; + +export const WHITESPACE_MODES = [ + { value: "all", label: "Show all changes" }, + { value: "ignore-all", label: "Ignore whitespace when comparing lines" }, + { value: "ignore-amount", label: "Ignore changes in amount of whitespace" }, + { value: "ignore-eol", label: "Ignore changes in whitespace at EOL" }, +] as const; +export type WhitespaceMode = (typeof WHITESPACE_MODES)[number]["value"]; + +function normalize(contents: string, mode: WhitespaceMode) { + switch (mode) { + case "ignore-all": + return contents.replace(/[^\S\n]+/g, ""); + case "ignore-amount": + return contents.replace(/[^\S\n]+(?=\n|$)/g, "").replace(/[^\S\n]+/g, " "); + case "ignore-eol": + return contents.replace(/[^\S\n]+(?=\n|$)/g, ""); + case "all": + return contents; + } +} + +/** Recompare only supplied hunks, keeping the real text and both source line coordinates. */ +export function filterDiffWhitespace( + file: FileDiffMetadata, + mode: WhitespaceMode, + contents?: FileDiffLoadedFiles, +): FileDiffMetadata { + if (mode === "all" || file.type === "new" || file.type === "deleted" || file.hunks.length === 0) + return file; + if (contents) { + const hydrated = hydratePartialDiff("clone", file, contents); + for (const hunk of file.hunks) { + for (const side of ["addition", "deletion"] as const) { + const actual = hydrated[`${side}Lines`].slice( + Math.max(0, hunk[`${side}Start`] - 1), + Math.max(0, hunk[`${side}Start`] - 1) + hunk[`${side}Count`], + ); + const expected = file[`${side}Lines`].slice( + hunk[`${side}LineIndex`], + hunk[`${side}LineIndex`] + hunk[`${side}Count`], + ); + if (actual.join("") !== expected.join("")) + throw new Error("The file changed since this diff was loaded"); + } + } + return filterDiffWhitespace(hydrated, mode); + } + if (!file.isPartial) { + const compared = parseDiffFromFile( + { name: file.prevName ?? file.name, contents: normalize(file.deletionLines.join(""), mode) }, + { name: file.name, contents: normalize(file.additionLines.join(""), mode) }, + { context: 3 }, + ); + return { + ...file, + hunks: compared.hunks, + splitLineCount: compared.splitLineCount, + unifiedLineCount: compared.unifiedLineCount, + cacheKey: `${file.cacheKey}:whitespace:${mode}`, + }; + } + // Separate hunks can hide lines that should match across the old boundaries after filtering. + if (file.hunks.length > 1) + throw new Error("Full file contents are required to compare separate hunks"); + const hunks: FileDiffMetadata["hunks"] = []; + let splitLineCount = 0; + let unifiedLineCount = 0; + let previousAdditionEnd = 0; + for (const original of file.hunks) { + const oldContents = file.deletionLines + .slice(original.deletionLineIndex, original.deletionLineIndex + original.deletionCount) + .join(""); + const newContents = file.additionLines + .slice(original.additionLineIndex, original.additionLineIndex + original.additionCount) + .join(""); + const compared = parseDiffFromFile( + { name: file.prevName ?? file.name, contents: normalize(oldContents, mode) }, + { name: file.name, contents: normalize(newContents, mode) }, + { context: 3 }, + ); + for (const hunk of compared.hunks) { + const additionStart = hunk.additionStart + Math.max(0, original.additionStart - 1); + const deletionStart = hunk.deletionStart + Math.max(0, original.deletionStart - 1); + hunks.push({ + ...hunk, + additionStart, + deletionStart, + additionLineIndex: hunk.additionLineIndex + original.additionLineIndex, + deletionLineIndex: hunk.deletionLineIndex + original.deletionLineIndex, + collapsedBefore: Math.max(0, additionStart - 1 - previousAdditionEnd), + splitLineStart: splitLineCount, + unifiedLineStart: unifiedLineCount, + hunkContent: hunk.hunkContent.map((content) => ({ + ...content, + additionLineIndex: content.additionLineIndex + original.additionLineIndex, + deletionLineIndex: content.deletionLineIndex + original.deletionLineIndex, + })), + }); + splitLineCount += hunk.splitLineCount; + unifiedLineCount += hunk.unifiedLineCount; + previousAdditionEnd = additionStart + hunk.additionCount - 1; + } + } + return { + ...file, + cacheKey: `${file.cacheKey}:whitespace:${mode}`, + isPartial: true, + hunks, + splitLineCount, + unifiedLineCount, + }; +} diff --git a/apps/web/src/components/pullRequest/pullRequestWhitespace.worker.ts b/apps/web/src/components/pullRequest/pullRequestWhitespace.worker.ts new file mode 100644 index 000000000000..6ea2a7b057e8 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestWhitespace.worker.ts @@ -0,0 +1,23 @@ +import type { FileDiffLoadedFiles, FileDiffMetadata } from "@pierre/diffs"; +import { filterDiffWhitespace, type WhitespaceMode } from "./pullRequestWhitespace"; + +self.addEventListener( + "message", + ( + event: MessageEvent<{ + files: { file: FileDiffMetadata; contents?: FileDiffLoadedFiles }[]; + mode: WhitespaceMode; + }>, + ) => { + const failures: string[] = []; + const files = event.data.files.map(({ file, contents }) => { + try { + return filterDiffWhitespace(file, event.data.mode, contents); + } catch { + failures.push(file.name); + return file; + } + }); + self.postMessage({ files, failures }, { transfer: [] }); + }, +); diff --git a/apps/web/src/components/pullRequest/usePullRequestWhitespace.test.tsx b/apps/web/src/components/pullRequest/usePullRequestWhitespace.test.tsx new file mode 100644 index 000000000000..de9450b67dc6 --- /dev/null +++ b/apps/web/src/components/pullRequest/usePullRequestWhitespace.test.tsx @@ -0,0 +1,175 @@ +import { act, useLayoutEffect, useState } from "react"; +import { create, type ReactTestRenderer } from "react-test-renderer"; +import { afterEach, expect, it, vi } from "vite-plus/test"; +import { parsePatchFiles, type FileDiffLoadedFiles, type FileDiffMetadata } from "@pierre/diffs"; +import { buildFileDiffRenderKey } from "~/lib/diffRendering"; +import { usePullRequestWhitespace } from "./usePullRequestWhitespace"; +import type { WhitespaceMode } from "./pullRequestWhitespace"; + +const files = parsePatchFiles( + "diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n@@ -20 +20 @@\n-old\n+new\n", +)[0]!.files; +const workers: TestWorker[] = []; +class TestWorker { + listeners = new Map void>(); + postMessage = + vi.fn<(input: { files: { file: FileDiffMetadata }[]; mode: WhitespaceMode }) => void>(); + terminate = vi.fn(); + constructor() { + workers.push(this); + } + addEventListener(name: string, listener: (event: unknown) => void) { + this.listeners.set(name, listener); + } + reply(failures: string[] = []) { + const input = this.postMessage.mock.lastCall![0]; + this.listeners.get("message")?.({ + data: { + files: input.files.map(({ file }) => + failures.includes(file.name) + ? file + : { ...file, cacheKey: `${file.name}:whitespace:${input.mode}` }, + ), + failures, + }, + }); + } +} +let renderer: ReactTestRenderer; +let state: ReturnType; +const rendered: (typeof state)[] = []; +function Reply() { + const [text, setText] = useState(""); + return