Skip to content

Commit 52c6fc0

Browse files
voltcrashvoltcrash
committed
fix(web): explain binary files in PR code diffs
1 parent 20c3dd3 commit 52c6fc0

3 files changed

Lines changed: 103 additions & 8 deletions

File tree

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

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
TriangleAlertIcon,
3131
} from "lucide-react";
3232
import { ChevronDown, ChevronRight, ChevronsDownUp, ChevronsUpDown } from "lucide";
33+
import { sha256 } from "@noble/hashes/sha2";
3334
import { useAtomRefresh } from "@effect/atom-react";
3435
import * as Schema from "effect/Schema";
3536
import {
@@ -59,6 +60,7 @@ import {
5960
import { isTestChangedFile } from "@t3tools/shared/changedFileKind";
6061
import { orderDiffFiles } from "./pullRequestFileOrder.logic";
6162
import {
63+
buildFileDiffIdentityKey,
6264
buildFileDiffRenderKey,
6365
fnv1a32,
6466
getDiffLineStat,
@@ -184,6 +186,7 @@ function parseSlice(slice: DiffSlice, cacheKey: string, ignoreWhitespace: boolea
184186
type DiffModel = {
185187
readonly parsedSlices: ReadonlyArray<RenderablePatch | null>;
186188
readonly loadedFiles: ReadonlyArray<Extract<RenderablePatch, { kind: "files" }>["files"][number]>;
189+
readonly binaryFileKeys: ReadonlySet<string>;
187190
readonly sourceFileStats: ReadonlyMap<string, ReturnType<typeof getDiffLineStat>>;
188191
};
189192
const diffModels = new WeakMap<ReadonlyArray<DiffSlice>, Map<string, DiffModel>>();
@@ -214,6 +217,11 @@ function diffModel(
214217
parsed?.kind === "files" ? orderDiffFiles(parsed.files) : [],
215218
),
216219
),
220+
binaryFileKeys: new Set(
221+
parsedSlices.flatMap((parsed) =>
222+
parsed?.kind === "files" ? [...parsed.binaryFileKeys] : [],
223+
),
224+
),
217225
sourceFileStats: new Map(
218226
parsedSlices.flatMap((parsed) =>
219227
parsed?.kind === "files"
@@ -570,7 +578,7 @@ function PullRequestCodeTab({
570578
// Agent context is local, so it does not require host comment permission.
571579
const canSelectLines =
572580
commit === null && (review.inlineComment || onAddToAgentSelection !== undefined);
573-
const { parsedSlices, loadedFiles, sourceFileStats } = diffModel(
581+
const { parsedSlices, loadedFiles, binaryFileKeys, sourceFileStats } = diffModel(
574582
loadedSlices,
575583
`pull-request:${scopeKey}:${resolvedTheme}:${ignoreWhitespace}`,
576584
ignoreWhitespace,
@@ -911,16 +919,35 @@ function PullRequestCodeTab({
911919
const withheld = omittedFileStats.get(path);
912920
if (withheld) ({ additions, deletions } = withheld);
913921
}
922+
const binary = binaryFileKeys.has(buildFileDiffIdentityKey(item.fileDiff));
923+
const host = sourceControlClients.get(detail.provider);
924+
const fileUrl =
925+
binary && detail.provider === "github"
926+
? `${detail.url}/files#diff-${Array.from(sha256(new TextEncoder().encode(path)), (byte) => byte.toString(16).padStart(2, "0")).join("")}`
927+
: detail.url;
914928
return (
915929
<FileViewedToggle
916930
fileKey={item.id}
917931
path={path}
918932
stat={
919-
<PullRequestDiffStat
920-
additions={additions}
921-
deletions={deletions}
922-
className="font-mono text-2xs"
923-
/>
933+
binary ? (
934+
<span className="flex items-center gap-2 text-xs text-muted-foreground">
935+
<span>Binary file</span>
936+
<Button
937+
size="micro"
938+
variant="link"
939+
render={<a href={fileUrl} target="_blank" rel="noopener noreferrer" />}
940+
>
941+
View on {host.label}
942+
</Button>
943+
</span>
944+
) : (
945+
<PullRequestDiffStat
946+
additions={additions}
947+
deletions={deletions}
948+
className="font-mono text-2xs"
949+
/>
950+
)
924951
}
925952
/>
926953
);

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,64 @@ import {
1212
} from "./diffRendering";
1313

1414
describe("getRenderablePatch", () => {
15+
it("retains explicit binary markers without classifying other hunkless files as binary", () => {
16+
const patch = [
17+
"diff --git a/added.png b/added.png",
18+
"new file mode 100644",
19+
"index 0000000..1234567",
20+
"Binary files /dev/null and b/added.png differ",
21+
'diff --git "a/old image.png" "b/new image.png"',
22+
"similarity index 50%",
23+
"rename from old image.png",
24+
"rename to new image.png",
25+
'Binary files "a/old image.png" and "b/new image.png" differ',
26+
"diff --git a/deleted.png b/deleted.png",
27+
"deleted file mode 100644",
28+
"Binary files a/deleted.png and /dev/null differ",
29+
"diff --git a/payload.bin b/payload.bin",
30+
"GIT binary patch",
31+
"literal 3",
32+
"KcmZQzWC8#H2mk;8",
33+
"diff --git a/empty.txt b/empty.txt",
34+
"new file mode 100644",
35+
"index 0000000..e69de29",
36+
"diff --git a/old.txt b/new.txt",
37+
"similarity index 100%",
38+
"rename from old.txt",
39+
"rename to new.txt",
40+
"diff --git a/omitted.txt b/omitted.txt",
41+
"index 1111111..2222222 100644",
42+
"diff --git a/lfs.png b/lfs.png",
43+
"new file mode 100644",
44+
"--- /dev/null",
45+
"+++ b/lfs.png",
46+
"@@ -0,0 +1,3 @@",
47+
"+version https://git-lfs.github.com/spec/v1",
48+
"+oid sha256:1234567890",
49+
"+size 123",
50+
"diff --git a/marker.txt b/marker.txt",
51+
"--- a/marker.txt",
52+
"+++ b/marker.txt",
53+
"@@ -1 +1 @@",
54+
"-Binary files a/fake.png and b/fake.png differ",
55+
"+GIT binary patch",
56+
"",
57+
].join("\n");
58+
for (const ignoreWhitespace of [false, true]) {
59+
const parsed = getRenderablePatch(patch, "pr-page-1", {
60+
ignoreWhitespace,
61+
compactPartialHunkOffsets: true,
62+
});
63+
expect(parsed?.kind).toBe("files");
64+
if (parsed?.kind !== "files") return;
65+
expect(parsed.files).toHaveLength(9);
66+
expect(parsed.files.slice(0, 7).every((file) => file.hunks.length === 0)).toBe(true);
67+
expect(parsed.binaryFileKeys).toEqual(
68+
new Set(parsed.files.slice(0, 4).map(buildFileDiffIdentityKey)),
69+
);
70+
}
71+
});
72+
1573
it("hides indentation changes around inserted JSX without moving review lines", () => {
1674
const patch = [
1775
"diff --git a/item.tsx b/item.tsx",

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles";
1+
import { parsePatchFiles, processFile } from "@pierre/diffs/utils/parsePatchFiles";
22
import { parseDiffFromFile } from "@pierre/diffs";
33
import type { FileDiffMetadata } from "@pierre/diffs/types";
44
import { READABLE_LIGHT_THEME_NAME } from "./readableSyntaxTheme";
@@ -44,6 +44,7 @@ export type RenderablePatch =
4444
kind: "files";
4545
files: FileDiffMetadata[];
4646
sourceFiles: FileDiffMetadata[];
47+
binaryFileKeys: ReadonlySet<string>;
4748
}
4849
| {
4950
kind: "raw";
@@ -171,7 +172,16 @@ export function getRenderablePatch(
171172
return options.compactPartialHunkOffsets ? compactPartialHunkOffsets(filtered) : filtered;
172173
});
173174
if (files.length > 0) {
174-
return { kind: "files", files, sourceFiles };
175+
const binaryFileKeys = new Set<string>();
176+
// Pierre discards binary markers, so retain them before handing its metadata to the viewer.
177+
for (const section of patch.split(/(?=^diff --git )/m)) {
178+
if (!section.startsWith("diff --git ")) continue;
179+
const header = section.split("\n@@ ", 1)[0]!;
180+
if (!/^(?:Binary files .+ and .+ differ|GIT binary patch)\r?$/m.test(header)) continue;
181+
const file = processFile(header);
182+
if (file) binaryFileKeys.add(buildFileDiffIdentityKey(file));
183+
}
184+
return { kind: "files", files, sourceFiles, binaryFileKeys };
175185
}
176186

177187
return {

0 commit comments

Comments
 (0)