diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index b535c4f45e97..08a46872b697 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/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 = (
@@ -1276,6 +1280,30 @@ function PullRequestCodeTab({
+ + { + setIgnoreWhitespace(Boolean(pressed)); + setDraft(null); + setSelectedLines(null); + }} + /> + } + > + + + + {ignoreWhitespace ? "Show whitespace changes" : "Hide whitespace changes"} + + {fileKeys.length > 0 ? ( { }); 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 @@", + ' ', + "- ", + '-

{name}

', + "-
", + "+ {showName && (", + "+ ", + '+

{name}

', + "+
", + "+ )}", + "
", + "@@ -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"], diff --git a/apps/web/src/lib/diffRendering.ts b/apps/web/src/lib/diffRendering.ts index 3a6c6e20969a..c99349a7dafd 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/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): 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 {