fix: prevent reviews page crash on binary/large file diffs (#1597)

The PR reviews page builds a Pierre FileContents cache key for every
file in an unconditional useMemo. Binary/oversized/added/removed blobs
arrive with null originalContent/modifiedContent (pr_diff.py flags them
unrenderable), so fileContentsCacheKey dereferenced null via
contents.length and crashed the whole route — escaping the markdown
error boundary, which was unrelated. Coerce null/undefined contents to
"" in the cache-key helper so it can never throw.

Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
Johannes du Plessis 2026-06-23 12:21:20 -07:00 • committed by GitHub
parent 860aee48ee
commit 60663800e5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 35 additions and 3 deletions

View file

@ -0,0 +1,29 @@
import { describe, expect, it } from "vitest"
import { fileContentsCacheKey } from "./diffUtils"
describe("fileContentsCacheKey", () => {
it("builds a stable key from string contents", () => {
const key = fileContentsCacheKey("src/a.ts", "new", "hello")
expect(key.startsWith("src/a.ts:new:5:")).toBe(true)
expect(fileContentsCacheKey("src/a.ts", "new", "hello")).toBe(key)
})
it("treats null contents as empty instead of crashing", () => {
// Binary/oversized/added/removed blobs arrive as null from the backend; the
// reviews page still computes this key in an unconditional useMemo.
expect(() => fileContentsCacheKey("bin.png", "old", null)).not.toThrow()
expect(fileContentsCacheKey("bin.png", "old", null)).toBe(
fileContentsCacheKey("bin.png", "old", "")
)
})
it("treats undefined contents as empty", () => {
expect(() =>
fileContentsCacheKey("bin.png", "new", undefined)
).not.toThrow()
expect(fileContentsCacheKey("bin.png", "new", undefined)).toBe(
fileContentsCacheKey("bin.png", "new", "")
)
})
})

View file

@ -137,13 +137,16 @@ function hashFileContents(contents: string): string {
}
// Stable per-file content key so the worker pool dedupes highlight work across
// re-renders instead of re-tokenizing identical content.
// re-renders instead of re-tokenizing identical content. Added/removed/binary/
// oversized blobs arrive as null (see pr_diff.py); coerce to "" so the key never
// dereferences null — these files don't render a diff, so the exact key is moot.
export function fileContentsCacheKey(
path: string,
side: "old" | "new",
contents: string
contents: string | null | undefined
): string {
return `${path}:${side}:${contents.length}:${hashFileContents(contents)}`
const text = contents ?? ""
return `${path}:${side}:${text.length}:${hashFileContents(text)}`
}
let highlighterWarmup: Promise<void> | null = null