From 60663800e50d64d830c9b0e931119bb68e087183 Mon Sep 17 00:00:00 2001
From: Johannes du Plessis <johannes@langchain.dev>
Date: Tue, 23 Jun 2026 12:21:20 -0700
Subject: [PATCH] fix: prevent reviews page crash on binary/large file diffs
 (#1597)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 .../components/agents/utils/diffUtils.test.ts | 29 +++++++++++++++++++
 ui/src/components/agents/utils/diffUtils.ts   |  9 ++++--
 2 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 ui/src/components/agents/utils/diffUtils.test.ts

diff --git a/ui/src/components/agents/utils/diffUtils.test.ts b/ui/src/components/agents/utils/diffUtils.test.ts
new file mode 100644
index 00000000..86430873
--- /dev/null
+++ b/ui/src/components/agents/utils/diffUtils.test.ts
@@ -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", "")
+    )
+  })
+})
diff --git a/ui/src/components/agents/utils/diffUtils.ts b/ui/src/components/agents/utils/diffUtils.ts
index 5c9c8fd0..c502f86e 100644
--- a/ui/src/components/agents/utils/diffUtils.ts
+++ b/ui/src/components/agents/utils/diffUtils.ts
@@ -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