fix(review): mint content-derived diff cache keys so single-file tabs render fully by backnotprop · Pull Request #1219 · backnotprop/plannotator

backnotprop · 2026-08-06T06:03:10Z

TLDR: single-file diff tabs stopped rendering their full-content diff in v0.26.0. Click a file in the sidebar and the expansion gap bars have no chevrons and clicks do nothing, at every file size. Cause: the @pierre/diffs 1.2.8 to 1.3.2 bump added name-based cacheKey defaulting, and DiffViewer hands two different diffs of the same file to one surviving FileDiff instance, so Pierre treated them as identical and served the stale first render forever. Fix is one class of one-liners: mint content-derived cache keys for both diffs. Resolves the "expand file does nothing" report.

The upstream mechanism

FileDiff.render in 1.3.2 defaults an unset key to the file's name:

if (fileDiff != null && fileDiff.cacheKey === void 0)
  fileDiff.cacheKey = fileDiff.prevName != null ? fileDiff.prevName + ":" + fileDiff.name : fileDiff.name;

and areDiffTargetsEqual, the only identity check its render and highlight caches make, compares nothing else:

function areDiffTargetsEqual(diffA, diffB) {
  return diffA === diffB || (diffA?.cacheKey != null && diffA.cacheKey === diffB?.cacheKey);
}

DiffViewer renders every file twice on one instance (key={filePath}):

the PARTIAL diff parsed from the raw patch (getSingularPatch), which Pierre marks non-expandable, and
the AUGMENTED full-content diff (processFile) once /api/file-content resolves, which is the only one that can be expanded.

Neither set a cacheKey, so both defaulted to calc.ts and step 2 was served step 1's cached render. The all-files view was never affected because AllFilesCodeView already mints content-derived keys for its items.

The partial diff needs a key of its own too: with key={filePath} the instance also survives diff-type, base and whitespace switches, where a same-named new patch would otherwise land on the same name-keyed stale cache.

Content hash, not patch.length: Pierre's worker highlight cache is a singleton that outlives remounts, so a same-length different-content patch must not collide either. hashString moved out of AllFilesCodeView into utils/hashString.ts so both surfaces mint keys the same way.

Affected versions

Version	`@pierre/diffs`	Single-file tabs
v0.25.1 and earlier	1.2.x	fine (no cacheKey defaulting)
v0.26.0	1.3.2	broken
v0.26.1	1.3.2	broken
this PR	1.3.2	fixed

The 1.3.1 bump landed in #1190 and 1.3.2 in #1191, both after the v0.25.1 tag.

Failing-then-passing proof

packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx mounts DiffViewer against the REAL @pierre/diffs renderer (no diff mocks, since the defect lives entirely inside its render cache), holds the /api/file-content response until the non-expandable partial baseline has been asserted, then requires the expansion affordances to reach the pixels.

Against the unfixed tree:

195 |       const swapped = await waitFor(() => countExpandButtons(host!) > 0, 15_000);
196 |       expect(swapped).toBe(true);
                            ^
error: expect(received).toBe(expected)
Expected: true
Received: false

(fail) DiffViewer full-content swap (DOM) > expansion affordances appear once /api/file-content lands [15838.64ms]
 0 pass
 1 fail

With the fix: 1 pass, 0 fail.

Live-verified against a real review server on a scratch repo: a single-file tab now shows chevrons on its gap bars, and clicking one expands 197 unmodified lines down to 97 with the real file content in between. All-files expansion is unchanged.

Also in this PR: explaining the oversized stub

Second commit, separate concern, same area. Files over the 5 MB review limit are replaced server-side by a contents-free stub, which rendered as a header-only card with no counts and no explanation. Users read that as broken too.

The stub now carries an explicit marker line in its extended header, and both review surfaces render one line under the file header saying the file is over the limit and only a stub is shown. A marker rather than a client heuristic because the only other signal, Binary files ... differ, is exactly what a genuine binary file emits, so a heuristic would put a false size-cap explanation on every image in a diff. The marker lives in shared/diff-paths so the browser bundle can detect it without pulling in the node-facing review core; both server runtimes get it from review-core, which vendor.sh already copies to Pi. Git ignores unknown extended-header lines and @pierre/diffs parses the stub identically with or without it. Which files get stubbed is unchanged.

Covered by DiffViewer.oversizedStub.test.tsx (stub explains itself; a genuine binary file and an ordinary text diff never show the line) plus review-core tests for the marker's presence, its absence from ordinary and genuinely binary diffs, and label-versus-byte-cap drift.

Relation to #1208

Same family, different hole. #1208 is Pierre's renderDiff refusing to swap a HIGHLIGHTED render cache for new content, worked around in writeRestore. This one is upstream of that check entirely: the two diffs were never recognised as different content in the first place, because their cache-key identity collided. Both are consequences of the same invariant, that every diff object handed to a surviving FileDiff must carry a content-derived cacheKey, which is now written down at both mint sites.

Verification

bun test: 2928 pass, 0 fail
DOM_TESTS=1 bun test packages/review-editor packages/ui: 958 pass, 0 fail
bun run typecheck: clean
build chain: review, hook, opencode all clean
both new DOM test files registered in .github/workflows/test.yml

This change was made with AI assistance. The root cause was established against the unpatched @pierre/diffs dist and confirmed with the failing-then-passing DOM test above plus live browser verification.

Addendum: the CI failures on this branch, and what they turned out to be

This PR's first CI runs failed the new DOM test even though it passed locally. It was never a timing problem, and the fix is unrelated to the product change above.

AllFilesCodeView.lifecycle.test.tsx calls mock.module('@pierre/diffs', ...) with a hunk-less getSingularPatch and processFile: () => null. Bun's module mocks are process global and are not unwound at file boundaries, and that file sits immediately before DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux runner the stub reached this file; on macOS it did not, which is why 13 local runs of the exact list, in both orders with every core saturated, stayed green.

That single cause explains both symptoms, including the pair that looked contradictory:

Symptom on CI	Which stub produced it
chevrons never appear, separators do	`processFile: () => null`, so the augmented diff never exists
nothing paints at all	the stub `getSingularPatch` returns `hunks: []`

The WorkerPoolManager: operation canceled because the pool terminated line in the log was a red herring. It sits inside discardRestoreRender's own group, about 0.3s before this file's group opens, is that file's provider unmounting the pool it created, and prints on every local run too.

A precondition assertion added while diagnosing is what proved it, turning a 10.3s mystery into a 0.45ms verdict:

228 |       expect(expected?.isPartial).toBe(false);
error: expect(received).toBe(expected)
Expected: false
Received: undefined

Fixed at both ends:

Source: the mocking file now captures the real modules before stubbing them and restores both library specifiers in afterAll, so no later file in any run inherits its stubs. This is the actual leak, and fixing it there covers every future DOM test that needs the real renderer.
Consumer: the two tests that render against the real @pierre/diffs get their own CI step, the same isolation this workflow already gives useFileBrowser.test.tsx, and are removed from the shared list so that step is their single source of truth. The restore above should make this unnecessary; it is not something to bet a green build on from a machine that cannot reproduce the platform behaviour.

Two smaller things were fixed along the way:

App.archiveReadOnly.test.tsx compared a fenced block's innerHTML before and after a click. Since perf(ui): single Shiki highlighter, palette-matched code blocks, drop highlight.js #1218 applyHighlight writes plain text first and swaps in Shiki markup when the grammar attaches, so that markup changes on its own schedule and the assertion was racing the swap. It now compares textContent, which is what "the click opened no mutation entry point" actually means and which no highlight swap can perturb. Latent on main; this branch's run happened to lose the race.
codeHighlight.test.ts asserted a global precondition ("no grammar attached yet") that any earlier file rendering a typescript fence invalidates, so DOM_TESTS=1 bun test packages/ui packages/editor failed on file order alone. It now resets the attachment cache through the existing __resetCodeHighlightCacheForTests seam.

The swap test's waits are also budgeted in scheduler turns rather than milliseconds now, so a slower runner needs no more of them and the budget never has to be retuned for CI hardware.

Verification for all of it: a CI-faithful harness running one bun process per step with the exact lists from test.yml (isolated, shared forward, shared reverse), 10 iterations with every core saturated, then 6 more after the final split. All green, plus the full suite and typecheck.

… render fully Single-file diff tabs have not rendered their full-content diff since v0.26.0: the expansion gap bars show no chevrons and clicking them does nothing, at every file size. @pierre/diffs 1.3.2 (the 1.2.8 -> 1.3.2 bump, upstream "Fix diff rerender in edit mode (#878)") added name-based cacheKey defaulting in FileDiff.render: an unset `fileDiff.cacheKey` becomes the file's name. `areDiffTargetsEqual` — the only identity check its render and highlight caches make — compares nothing but that key. DiffViewer renders each file twice on one surviving FileDiff instance (key={filePath}): first the PARTIAL diff from getSingularPatch, then the AUGMENTED full-content diff from processFile once /api/file-content lands. Neither set a cacheKey, so both defaulted to the filename and Pierre served the stale partial render forever. Only the augmented diff is expandable, hence the dead gap bars. Both diffs now mint content-derived keys (`<path>#<hash>` and `<path>#full#<hash>`), matching how AllFilesCodeView already keys its items — which is why the all-files view was never affected. The hash (not patch.length) matters because Pierre's worker highlight cache is a singleton that outlives remounts. The partial diff needs its own key too: with key={filePath} the instance also survives diff-type and base switches, where a same-named new patch would otherwise hit the same name-keyed stale cache. hashString moves from AllFilesCodeView to utils/hashString.ts so both surfaces mint keys the same way. Covered by a new DOM test that mounts DiffViewer against the real @pierre/diffs renderer, holds the /api/file-content response until the non-expandable partial baseline is asserted, then requires the expansion affordances to reach the pixels. It fails against the unfixed tree.

Files over the 5 MB review limit are replaced by a contents-free stub (buildOversizedTrackedStub, plus the untracked equivalent), which renders as a header-only card with no counts and no explanation. Users read that as a broken diff. The stub now carries an explicit marker line in its extended header (OVERSIZED_REVIEW_STUB_MARKER). A marker rather than a client heuristic because the only other signal, `Binary files ... differ`, is exactly what a genuine binary file emits, so a heuristic would put a false size-cap explanation on every image in the diff. The marker lives in shared/diff-paths so the browser bundle can detect it without pulling in the node-facing review core; both server runtimes pick it up from review-core, which vendor.sh already copies to Pi. Git ignores unknown extended-header lines and @pierre/diffs parses the stub identically with or without it, so nothing else moves. Which files get stubbed is unchanged. Both review surfaces now render one line under the file header saying the file is over the limit and only a stub is shown.

…atch based CI failed two tests that pass locally. Both were timing races, neither was an app bug. 1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal wall-clock budget, which a cold, contended CI runner blows and a warm laptop clears. Two changes, both aimed at the clock rather than the symptom: - The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A slower box spends longer inside each turn but needs no more of them, so the budget never has to be retuned for CI hardware. - Pierre's shared Shiki highlighter is preloaded before mounting. It is a module singleton, and building it was the entire multi-second cost the old budget was accidentally measuring; warming it moves that work into an unbounded await OUTSIDE the observed window. Disposed in afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the pre-attachment behaviour of that same singleton. Verified against an artificially stalled clock: forcing 20s of dead time into every wait (41s total, far past the old 15s budget) still passes, and with the cacheKey fix removed it still fails on the assertion (not as an opaque timeout) in ~12s. A 20-turn budget with the preload removed and every core saturated also passed 10/10, so 400 turns is a wide margin rather than a guess. 2. App.archiveReadOnly compared the fenced block's innerHTML before and after a click. Since #1218, applyHighlight writes plain text first and swaps in Shiki markup when the grammar attaches, so that MARKUP changes on its own schedule and the assertion was racing the swap. The test is checking that the click opened no mutation entry point, which textContent plus the absence of an annotation <mark> says exactly, and which no highlight swap can perturb. Latent on main; the branch's run happened to lose the race. Also fixed while confirming the above: codeHighlight.test.ts asserted a GLOBAL precondition ("no grammar attached yet") that any earlier file attaching a typescript fence invalidates, so `DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order alone. It now resets the attachment cache through the existing __resetCodeHighlightCacheForTests seam and asserts the contract instead of the run order. Not currently reachable from CI (that file is not in the DOM list), but one list edit away.

…port why a paint is missing The preload added in the previous commit made CI strictly worse, so it is gone. Before it, CI's partial diff painted and only the swap was missing; with it, CI never painted at all. It was an optimization for a theory the evidence has since killed, and it mutated a process-wide singleton to buy it, so it is not worth keeping while the real failure is unexplained. The afterAll dispose that existed only to undo the preload goes with it. What the CI log actually shows: - The "WorkerPoolManager: operation canceled because the pool terminated" error is inside discardRestoreRender.test.tsx's own group, ~0.3s BEFORE this file's group opens. It is that file's provider unmounting and terminating the pool singleton it created: end-of-file teardown, the same benign noise documented on #1209. It also prints on every local run, where the whole list passes. It is not a mid-test terminator, and nothing in this file uses the worker pool (no WorkerPoolContextProvider is mounted, so useWorkerPool() is undefined and rendering takes the main-thread path). - This file's group prints NOTHING for its whole 10.3s: no console.warn from the stale-content guard, no error. Pierre simply painted nothing. Not reproducible locally: the exact DOM list from test.yml, one bun process, forward and reverse order, 13 runs with every core saturated, all green. So the remaining difference is the environment, which cannot be reasoned out from here. Three changes make the next CI run answer it instead of costing another guess: - renderDiagnostics() dumps what Pierre actually painted (container / separator / chevron / line-number counts plus a markup fragment) when a wait gives up. Prints only on failure, so it is worth keeping. - The precondition is asserted rather than assumed: the REAL getSingularPatch and processFile must produce partial-then-full on these fixtures. Bun's mock.module is process global and an earlier file in this very list mocks '@pierre/diffs', so a leaked mock now fails in milliseconds with a clear message instead of as a render that never arrives. - The first paint is now REPORTED, not asserted. The verdict belongs to the swap; gating on the partial paint let a slow or absent first paint mask the result the test exists for. Removing the cacheKey fix still fails it (verified), because that tree paints no chevrons at any point. Also fixed a real trap in the fixture: the hunk header said @@ -61 while its context lines start at line 59 of both contents. Pierre realigns a misaligned header rather than rejecting it, so it was silently tolerated.

… diff tests Root cause, and it was never a timing problem. AllFilesCodeView.lifecycle.test.tsx calls `mock.module('@pierre/diffs', ...)` with a hunk-less `getSingularPatch` and `processFile: () => null`. Bun's module mocks are process global and are not unwound at file boundaries, and that file sits immediately before DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux runner the stub reached this file; on macOS it did not, which is why 13 local runs of the exact list, both orders, cores saturated, stayed green. It explains both CI symptoms exactly, including the one that looked like a contradiction: `processFile: () => null` means the augmented diff never exists, so no chevrons ever (the failure before the preload); the stub `getSingularPatch` has `hunks: []`, so nothing paints at all (the failure after it). The "WorkerPoolManager: operation canceled because the pool terminated" line was a red herring throughout: it is inside discardRestoreRender's own group, ~0.3s BEFORE this file's group opens, is that file's provider unmounting the pool it created, and prints on every local run too. The precondition assertion added in the previous commit is what proved it, turning a 10.3s mystery into a 0.45ms verdict: 228 | expect(expected?.isPartial).toBe(false); error: expect(received).toBe(expected) Expected: false Received: undefined Fixed at both ends: - Source: the mocking file now captures the real modules before it stubs them and restores both library specifiers in afterAll, so no later file in any run inherits its stubs. This fixes the class for every future DOM test that needs the real renderer, which was the actual leak. - Consumer: the two tests that render against the real @pierre/diffs get their own CI step, the same isolation (and for the same kind of reason) this workflow already gives useFileBrowser.test.tsx. They are removed from the shared list so that step is their single source of truth. The restore above should make this unnecessary; it is not something to bet a green build on from a machine that cannot reproduce the platform behaviour. Verified with a CI-faithful harness: one bun process per step, the exact lists from test.yml, isolated + shared-forward + shared-reverse, 10 iterations with every core saturated, then 6 more after the final split. All green, plus the full suite and typecheck.

…ually runs them

Harmonizes with #1219's oversized-file notice at the three points they overlap. diff-paths.ts keeps both detectors, ordered specific then general: isOversizedReviewStubPatch (marker-based, from #1219) stays first, and isContentlessBinaryPatch follows as the general fallback with a doc note that callers ask the marker first. Both mount points render exactly one notice. A marker-carrying stub gets OversizedFileNotice, which knows the file is over the size cap; anything else hunkless and binary (a genuine binary, or a stub shape the marker does not cover) gets BinaryFileNotice. The fallback is gated on the marker so the two can never stack on one card. The new DOM test moves into #1219's isolated real-renderer CI step rather than the shared list, for the same reason that step exists.

backnotprop added 6 commits August 5, 2026 22:49

docs(test): point the diff-renderer DOM tests at the CI step that act…

e377f54

…ually runs them

backnotprop force-pushed the fix/single-file-diff-cache-key branch from 560b4e5 to e377f54 Compare August 6, 2026 07:23

backnotprop merged commit 7ba4e3b into main Aug 6, 2026
15 checks passed

backnotprop mentioned this pull request Aug 6, 2026

fix(review): stop stubbing files whose worktree content the size probe cannot find #1220

Merged

Navigation Menu

fix(review): mint content-derived diff cache keys so single-file tabs render fully - #1219

fix(review): mint content-derived diff cache keys so single-file tabs render fully#1219
backnotprop merged 6 commits into
mainfrom
fix/single-file-diff-cache-key

backnotprop commented Aug 6, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

backnotprop commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The upstream mechanism

Affected versions

Failing-then-passing proof

Also in this PR: explaining the oversized stub

Relation to #1208

Verification

Addendum: the CI failures on this branch, and what they turned out to be

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

backnotprop commented Aug 6, 2026 •

edited

Loading