fix(review): repaint pristine diff immediately on edit-mode Discard - #1209
Merged
Conversation
writeRestore's combined item write leaves upstream's highlighted render cache serving the stale edited content; clear the live instance's render cache and rerender so the pristine diff paints immediately (plaintext first, then the normal async highlight). Covers Discard, Suggest, finishIfEditing, and the deferred external-teardown restore. Adds a DOM-gated integration test driving the real @pierre/diffs CodeView + EditProvider + worker pool through startEdit -> real Editor.applyEdits -> cancelEdit, asserting the pristine content is back in the shadow DOM within a bounded settle.
backnotprop
added a commit
that referenced
this pull request
Aug 6, 2026
…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.
backnotprop
added a commit
that referenced
this pull request
Aug 6, 2026
… render fully (#1219) * fix(review): mint content-derived diff cache keys so single-file tabs 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. * fix(review): explain why an oversized file's card has no diff 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. * test(review): make the diff-swap proof machine independent, not stopwatch 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. * test(review): drop the highlighter preload, harden the swap proof, report 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. * test(review): stop a leaked module mock from silently unrendering the 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. * docs(test): point the diff-renderer DOM tests at the CI step that actually runs them
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR: After editing code in flag-gated Edit Mode (#1193) and clicking Discard, the diff kept displaying the edited content even though no annotation was created and item state was fully pristine (QA finding #2 from the v0.26.0 manual QA).
writeRestorenow clears the live instance's render cache and rerenders right after the restore write, so the pristine diff paints immediately. Adds a real-CodeView integration test that fails without the fix.Root cause
writeRestorewas already correct at the state level: pristinefileDiffclone with a freshcacheKey,edit = false, version bump, one combinedupdateItem. The stale pixels come from upstream:@pierre/diffs@1.3.2DiffHunksRenderer.renderDiffonly swaps its render cache for new content while the cache is UNhighlighted. An ended edit session leavesrenderCache.highlighted === true, so the teardown repaint paints the stale editedrenderCache.resultand merely queues an async worker highlight of the pristine content. That heal takes 30ms to seconds, and never lands at all if the task is invalidated first (for example by the worker-pool theme sync callinginvalidateRenderTasks), leaving the edited content on screen permanently. Tracked for upstream reporting in #1208.Fix
After the restore
updateItem,writeRestoreobtains the liveFileDiffinstance via the React handle (getInstance()thengetRenderedItems()) and, when the item is currently rendered, callsclearRenderCache()plusrerender()in a try/catch. The next paint takes the cold-render path and shows pristine immediately (plaintext first, then the normal async highlight, the same UX as any diff switch). Virtualized-away items are unaffected: they repaint pristine fromitem.fileDiffon remount.writeRestoreis shared by Suggest, Discard,finishIfEditing, and the deferred external-teardown restore, and the fix intentionally applies to all of them. Suggest previously only looked correct because its annotations-syncupdateItemrepainted after the heal; it could still flash edited content briefly, which this also removes.The access deliberately reaches past the edit adapter wall into the protected
hunksRenderer; a version-pinned comment inwriteRestoredocuments why, and the now-incorrect "one combined write is sufficient" comment onendSessionwas corrected.Test
packages/review-editor/edit/discardRestoreRender.test.tsx(DOM-gated, registered in the workflow's DOM_TESTS list): mounts the real@pierre/diffsReact CodeView + EditProvider + worker pool (real Bun workers under happy-dom), drivesstartEdit-> realEditor.applyEdits->cancelEdit, and asserts the shadow-DOM buffer contains the pristine text and not the edited text within one bounded 16ms settle, never waiting out the async heal.Proven both ways: against unfixed
useEditSession.tsthe test fails atexpect(text).not.toContain("edited-marker")(the edited buffer is still painted after discard); with the fix it passes.DOM_TESTS=1 bun test packages/review-editor: 260 pass, 0 failbun test packages/review-editor: 218 pass, 43 skip, 0 failbun run typecheck: cleanbun run --cwd apps/review build && bun run build:hook: cleanThis PR was written with AI assistance; the root cause was established with an empirical repro harness against the real @pierre/diffs dist.