From 0d885acedea2ef77a7b3702201c2d9fcca969b26 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 5 Aug 2026 22:49:37 -0700 Subject: [PATCH 1/6] fix(review): mint content-derived diff cache keys so single-file tabs render fully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (`#` and `#full#`), 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. --- .github/workflows/test.yml | 1 + .../components/AllFilesCodeView.tsx | 13 +- .../DiffViewer.fullContentSwap.test.tsx | 204 ++++++++++++++++++ .../review-editor/components/DiffViewer.tsx | 34 ++- packages/review-editor/utils/hashString.ts | 17 ++ 5 files changed, 254 insertions(+), 15 deletions(-) create mode 100644 packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx create mode 100644 packages/review-editor/utils/hashString.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b28a34f6a..628750964 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -60,6 +60,7 @@ jobs: packages/review-editor/edit/discardRestoreRender.test.tsx packages/review-editor/edit/selectionActionPopover.test.ts packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx + packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx packages/review-editor/components/guide/GuideSectionCard.test.tsx packages/review-editor/components/guide/GuideView.test.tsx packages/review-editor/hooks/useReviewSearch.test.tsx diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 704473aae..3dd41c84f 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -29,6 +29,7 @@ import { buildFileTree, getVisualFileOrder } from '../utils/buildFileTree'; import { buildCodeNavRequest } from '../utils/buildCodeNavRequest'; import { getDiffSelection, getLineNumberFromNode, getSideFromNode } from '../utils/diffSelection'; import { isContentConsistentWithPatch } from '../utils/patchConsistency'; +import { hashString } from '../utils/hashString'; import { ToolbarHost, type ToolbarHostHandle } from './ToolbarHost'; import { FileHeader } from './FileHeader'; import { EditSessionHud } from './EditSessionHud'; @@ -307,18 +308,6 @@ interface ItemIdentity { itemIdToFile: Map; } -// Cheap content hash (djb2 xor variant) for diff-change detection. Replaces -// patch-LENGTH proxies: a same-length different-content patch set must still -// remount CodeView (fileSetKey) and must not collide in highlight caches -// (cacheKey). Not cryptographic — collision odds for this purpose are fine. -function hashString(value: string): string { - let hash = 5381; - for (let i = 0; i < value.length; i++) { - hash = ((hash * 33) ^ value.charCodeAt(i)) >>> 0; - } - return hash.toString(36); -} - // The first rendered line of a file's diff, used to anchor file-scoped comments. // Pierre suppresses the header-prefix slot whenever a custom header is present // (renderDiffChildren makes them mutually exclusive), so file comments can't diff --git a/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx new file mode 100644 index 000000000..226777daa --- /dev/null +++ b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx @@ -0,0 +1,204 @@ +/** + * The single-file diff tab must actually REPAINT when the full-content diff + * arrives. + * + * DiffViewer renders twice for every file it shows: first the PARTIAL diff + * parsed from the raw patch (`getSingularPatch`), then, once + * `/api/file-content` resolves, an AUGMENTED full-content diff + * (`processFile`) swapped onto the SAME surviving FileDiff instance + * (`key={filePath}`). Only the augmented diff can be expanded, so the + * gutter's expansion chevrons appear only after the swap lands. + * + * @pierre/diffs 1.3.2 defaults `fileDiff.cacheKey` to the file NAME when the + * caller leaves it unset, and `areDiffTargetsEqual` compares nothing but that + * key. Two diffs of the same file therefore look identical to the render + * cache, so the augmented diff is served the stale partial render forever: + * gap bars with no chevrons, dead clicks, at every file size. The fix mints + * content-derived cache keys for BOTH diffs. + * + * Real @pierre/diffs (no diff mocks) — the defect lives entirely inside its + * DiffHunksRenderer cache, so a mocked renderer would prove nothing. Only + * DiffViewer's Vite-only worker-pool module and the theme/toolbar chrome are + * stubbed. + * + * DOM-gated (DOM_TESTS=1) and registered in .github/workflows/test.yml's + * "Run UI seam-contract + DOM tests" step. + */ +import { afterEach, describe, expect, mock, test } from 'bun:test'; +import React from 'react'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; + +mock.module('../workerPool', () => ({ + useIsWorkerPoolReadyOrDisabled: () => true, + useWorkerPoolThemeSync: () => {}, +})); + +mock.module('../hooks/usePierreTheme', () => ({ + usePierreTheme: () => ({ type: 'light', css: '' }), +})); + +mock.module('./ToolbarHost', () => ({ + ToolbarHost: React.forwardRef(function MockToolbarHost() { + return null; + }), +})); + +const { DiffViewer } = await import('./DiffViewer'); + +const hasDom = typeof document !== 'undefined'; + +// A realistically sized file so there is genuine collapsed context above and +// below the hunk — the gaps whose chevrons are the user-visible symptom. +const filler = (n: number, name: string) => + Array.from({ length: n }, (_, i) => `const ${name}${i} = ${i};`).join('\n'); +const HEAD = filler(60, 'head'); +const TAIL = filler(50, 'tail'); +const NEW_CONTENTS = `${HEAD}\nexport function add(a: number, b: number) {\n return a + b;\n}\n${TAIL}\n`; +const OLD_CONTENTS = `${HEAD}\nexport function add(a: number, b: number) {\n return a + b; // old\n}\n${TAIL}\n`; + +const PATCH = [ + 'diff --git a/calc.ts b/calc.ts', + 'index 0000000..1111111 100644', + '--- a/calc.ts', + '+++ b/calc.ts', + '@@ -61,7 +61,7 @@', + ' const head58 = 58;', + ' const head59 = 59;', + ' export function add(a: number, b: number) {', + '- return a + b; // old', + '+ return a + b;', + ' }', + ' const tail0 = 0;', + ' const tail1 = 1;', + '', +].join('\n'); + +/** All markup including shadow roots (Pierre renders into shadow DOM). */ +function shadowHTML(host: HTMLElement): string { + let out = host.innerHTML ?? ''; + const visit = (root: ParentNode) => { + for (const el of root.querySelectorAll('*')) { + const shadow = (el as { shadowRoot?: ShadowRoot | null }).shadowRoot; + if (shadow) { + out += shadow.innerHTML ?? ''; + visit(shadow); + } + } + }; + visit(host); + return out; +} + +function countExpandButtons(host: HTMLElement): number { + return (shadowHTML(host).match(/data-expand-button/g) ?? []).length; +} + +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +async function waitFor(predicate: () => boolean, timeoutMs: number, stepMs = 25): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (predicate()) return true; + await act(async () => { + await sleep(stepMs); + }); + } + return predicate(); +} + +function view(overrides: Partial> = {}) { + return ( + {}} + onAddAnnotation={() => {}} + onAddFileComment={() => {}} + onEditAnnotation={() => {}} + onSelectAnnotation={() => {}} + onDeleteAnnotation={() => {}} + {...overrides} + /> + ); +} + +describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { + let root: Root | null = null; + let host: HTMLDivElement | null = null; + const originalFetch = globalThis.fetch; + + afterEach(async () => { + if (root) { + await act(async () => root!.unmount()); + root = null; + } + host?.remove(); + host = null; + globalThis.fetch = originalFetch; + }); + + test( + 'expansion affordances appear once /api/file-content lands', + async () => { + // The full-content response is held until the partial baseline has been + // asserted — with the fix the swap lands within a frame, so an + // unthrottled response would race the baseline check. + let markRequested: (() => void) | null = null; + const fileContentRequested = new Promise((resolve) => { + markRequested = resolve; + }); + let releaseFileContent: (() => void) | null = null; + const fileContentGate = new Promise((resolve) => { + releaseFileContent = resolve; + }); + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = String(input); + if (url.startsWith('/api/file-content')) { + markRequested?.(); + await fileContentGate; + return new Response( + JSON.stringify({ oldContent: OLD_CONTENTS, newContent: NEW_CONTENTS }), + { headers: { 'content-type': 'application/json' } }, + ); + } + return new Response('{}', { headers: { 'content-type': 'application/json' } }); + }) as typeof fetch; + + host = document.createElement('div'); + document.body.appendChild(host); + root = createRoot(host); + await act(async () => { + root!.render(view()); + }); + + // The partial diff paints first, and a partial diff is NOT expandable — + // Pierre renders the gap bars without chevrons. This is the baseline the + // stale render cache would freeze forever. + await fileContentRequested; + expect(await waitFor(() => shadowHTML(host!).includes('data-separator'), 15_000)).toBe(true); + expect(countExpandButtons(host!)).toBe(0); + + await act(async () => { + releaseFileContent!(); + await sleep(0); + }); + + // The augmented full-content diff must reach the PIXELS, not just the + // React tree: expansion chevrons in the gap bars. + const swapped = await waitFor(() => countExpandButtons(host!) > 0, 15_000); + expect(swapped).toBe(true); + + // And the separator now advertises a real expand target, which is what + // makes the click live rather than dead. + expect(shadowHTML(host!)).toContain('data-expand-index'); + }, + 60_000, + ); +}); diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index c36fa5871..e6b96c59c 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -19,6 +19,7 @@ import { lineAnnotationMetadata } from '../utils/annotationDisplay'; import type { AnnotationScrollTarget } from '../types'; import { getLineNumberFromNode, getSideFromNode, getDiffSelection } from '../utils/diffSelection'; import { isContentConsistentWithPatch } from '../utils/patchConsistency'; +import { hashString } from '../utils/hashString'; import { InlineAnnotation } from './InlineAnnotation'; import { InlineAIMarker } from './InlineAIMarker'; import type { AIChatEntry } from '../hooks/useAIChat'; @@ -310,8 +311,28 @@ export const DiffViewer: React.FC = ({ const toolbarHostRef = useRef(null); - // Parse patch into FileDiffMetadata for @pierre/diffs FileDiff component - const fileDiff = useMemo(() => getSingularPatch(patch), [patch]); + // Parse patch into FileDiffMetadata for @pierre/diffs FileDiff component. + // + // Pinned to @pierre/diffs 1.3.2: `FileDiff.render` DEFAULTS an unset + // `fileDiff.cacheKey` to the file's NAME (`prevName:name` for renames), and + // `areDiffTargetsEqual` — the only identity check its render/highlight + // caches make — compares nothing but that key. Two different diffs of the + // same path therefore look IDENTICAL to Pierre, and the second one is + // silently served the first one's cached render. + // + // This FileDiff instance survives (`key={filePath}`) across both the + // partial -> full-content swap below AND diff-type / base / whitespace + // switches, so every diff object handed to it must mint its own + // content-derived key. Hash, not `patch.length`: the worker highlight cache + // is a singleton that outlives remounts, so a same-length different-content + // patch must not collide either. See AllFilesCodeView, which mints the same + // shape of key for the all-files surface (which is why that surface was + // never affected by this bug). + const fileDiff = useMemo(() => { + const parsed = getSingularPatch(patch); + parsed.cacheKey = `${filePath}#${hashString(patch)}`; + return parsed; + }, [patch, filePath]); // Fetch full file contents for expandable context const [fileContents, setFileContents] = useState<{ forPath: string; old: string | null; new: string | null } | null>(null); @@ -353,7 +374,14 @@ export const DiffViewer: React.FC = ({ oldFile: fileContents.old != null ? { name: oldPath || filePath, contents: fileContents.old } : undefined, newFile: fileContents.new != null ? { name: filePath, contents: fileContents.new } : undefined, }); - return result && !result.isPartial ? result : fileDiff; + if (!result || result.isPartial) return fileDiff; + // A DIFFERENT key from the partial diff above (`#full`), still derived + // from the patch content so it also changes across diff-type / base + // switches. Without it Pierre keeps painting the partial render forever: + // gap bars with no chevrons and dead expansion clicks, at every file + // size. (See the cacheKey note on `fileDiff`.) + result.cacheKey = `${filePath}#full#${hashString(patch)}`; + return result; } catch { return fileDiff; } diff --git a/packages/review-editor/utils/hashString.ts b/packages/review-editor/utils/hashString.ts new file mode 100644 index 000000000..260aa7514 --- /dev/null +++ b/packages/review-editor/utils/hashString.ts @@ -0,0 +1,17 @@ +/** + * Cheap content hash (djb2 xor variant) for diff-change detection. Replaces + * patch-LENGTH proxies: a same-length different-content patch must still + * remount the all-files list (fileSetKey) and must not collide in Pierre's + * highlight / render caches (cacheKey). Not cryptographic — collision odds + * for this purpose are fine. + * + * Shared by both review surfaces (AllFilesCodeView, DiffViewer) so their + * cache keys are minted the same way. + */ +export function hashString(value: string): string { + let hash = 5381; + for (let i = 0; i < value.length; i++) { + hash = ((hash * 33) ^ value.charCodeAt(i)) >>> 0; + } + return hash.toString(36); +} From 6d23f02f18c366d92be8e17ca5f1c78b0bb153bd Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 5 Aug 2026 23:01:48 -0700 Subject: [PATCH 2/6] 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. --- .github/workflows/test.yml | 1 + .../components/AllFilesCodeView.tsx | 8 + .../DiffViewer.oversizedStub.test.tsx | 142 ++++++++++++++++++ .../review-editor/components/DiffViewer.tsx | 7 + .../components/OversizedFileNotice.tsx | 33 ++++ packages/shared/diff-paths.ts | 30 ++++ packages/shared/review-core.test.ts | 27 ++++ packages/shared/review-core.ts | 7 + 8 files changed, 255 insertions(+) create mode 100644 packages/review-editor/components/DiffViewer.oversizedStub.test.tsx create mode 100644 packages/review-editor/components/OversizedFileNotice.tsx diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 628750964..1ebf6dd8c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -61,6 +61,7 @@ jobs: packages/review-editor/edit/selectionActionPopover.test.ts packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx + packages/review-editor/components/DiffViewer.oversizedStub.test.tsx packages/review-editor/components/guide/GuideSectionCard.test.tsx packages/review-editor/components/guide/GuideView.test.tsx packages/review-editor/hooks/useReviewSearch.test.tsx diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 3dd41c84f..ffbedb603 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -30,6 +30,8 @@ import { buildCodeNavRequest } from '../utils/buildCodeNavRequest'; import { getDiffSelection, getLineNumberFromNode, getSideFromNode } from '../utils/diffSelection'; import { isContentConsistentWithPatch } from '../utils/patchConsistency'; import { hashString } from '../utils/hashString'; +import { isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; +import { OversizedFileNotice } from './OversizedFileNotice'; import { ToolbarHost, type ToolbarHostHandle } from './ToolbarHost'; import { FileHeader } from './FileHeader'; import { EditSessionHud } from './EditSessionHud'; @@ -2173,6 +2175,12 @@ export const AllFilesCodeView: React.FC = ({ } onCollapseToggle={() => toggleItemCollapsed(item.id)} /> + {/* Files over the review size cap arrive as a contents-free stub, so + Pierre renders nothing below the header. Explain why rather than + leaving a bare header that reads as a broken diff. */} + {!collapsed && isOversizedReviewStubPatch(file.patch) && ( + refreshItem(item.id)} /> + )} {/* EXPERIMENTAL edit-session HUD: session controls + state in a slim strip below the header, above the file content. Appears/disappears with session start/end, which both go through a version-bumped diff --git a/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx b/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx new file mode 100644 index 000000000..f16fcf624 --- /dev/null +++ b/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx @@ -0,0 +1,142 @@ +/** + * A file over the review size cap must SAY why its card is empty. + * + * The review core replaces such files with a contents-free stub patch + * (`buildOversizedTrackedStub`), which @pierre/diffs renders as a body with no + * lines. Before this, the card was a bare header with no counts and no reason, + * and users read it as a broken diff. The stub carries an explicit marker line + * (`OVERSIZED_REVIEW_STUB_MARKER`) so the UI can tell it apart from a genuine + * binary file, which must keep rendering exactly as it always did. + * + * DOM-gated (DOM_TESTS=1) and registered in .github/workflows/test.yml's + * "Run UI seam-contract + DOM tests" step. + */ +import { afterEach, describe, expect, mock, test } from 'bun:test'; +import React from 'react'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { OVERSIZED_REVIEW_STUB_MARKER } from '@plannotator/shared/diff-paths'; + +mock.module('../workerPool', () => ({ + useIsWorkerPoolReadyOrDisabled: () => true, + useWorkerPoolThemeSync: () => {}, +})); + +mock.module('../hooks/usePierreTheme', () => ({ + usePierreTheme: () => ({ type: 'light', css: '' }), +})); + +mock.module('./ToolbarHost', () => ({ + ToolbarHost: React.forwardRef(function MockToolbarHost() { + return null; + }), +})); + +const { DiffViewer } = await import('./DiffViewer'); + +const hasDom = typeof document !== 'undefined'; + +const OVERSIZED_STUB = [ + 'diff --git a/assets/blob.pack b/assets/blob.pack', + OVERSIZED_REVIEW_STUB_MARKER, + 'index 1111111111aa..2222222222bb 100644', + 'Binary files a/assets/blob.pack and b/assets/blob.pack differ', + '', +].join('\n'); + +// A genuine binary file: same shape MINUS the marker. It must not pick up the +// size-cap explanation, which would be a lie about why it has no diff. +const REAL_BINARY = [ + 'diff --git a/assets/logo.png b/assets/logo.png', + 'index 1111111111aa..2222222222bb 100644', + 'Binary files a/assets/logo.png and b/assets/logo.png differ', + '', +].join('\n'); + +const TEXT_PATCH = [ + 'diff --git a/calc.ts b/calc.ts', + 'index 0000000..1111111 100644', + '--- a/calc.ts', + '+++ b/calc.ts', + '@@ -1,3 +1,3 @@', + ' const a = 1;', + '-const b = 1;', + '+const b = 2;', + ' const c = 3;', + '', +].join('\n'); + +const NOTICE_SELECTOR = '[data-oversized-file-notice]'; +const NOTICE_COPY = 'review limit'; + +function view(patch: string, filePath: string) { + return ( + {}} + onAddAnnotation={() => {}} + onAddFileComment={() => {}} + onEditAnnotation={() => {}} + onSelectAnnotation={() => {}} + onDeleteAnnotation={() => {}} + /> + ); +} + +describe.if(hasDom)('oversized-file stub presentation (DOM)', () => { + let root: Root | null = null; + let host: HTMLDivElement | null = null; + const originalFetch = globalThis.fetch; + + async function render(patch: string, filePath: string) { + // No expandable content for a stub; keep the lookup inert either way. + globalThis.fetch = (async () => + new Response(JSON.stringify({ oldContent: null, newContent: null }), { + headers: { 'content-type': 'application/json' }, + })) as typeof fetch; + + host = document.createElement('div'); + document.body.appendChild(host); + root = createRoot(host); + await act(async () => { + root!.render(view(patch, filePath)); + await new Promise((resolve) => setTimeout(resolve, 25)); + }); + return host; + } + + afterEach(async () => { + if (root) { + await act(async () => root!.unmount()); + root = null; + } + host?.remove(); + host = null; + globalThis.fetch = originalFetch; + }); + + test('an oversized stub explains itself', async () => { + const el = await render(OVERSIZED_STUB, 'assets/blob.pack'); + const notice = el.querySelector(NOTICE_SELECTOR); + expect(notice).not.toBeNull(); + expect(notice!.textContent).toContain(NOTICE_COPY); + // The raw marker is plumbing, never user-facing copy. + expect(el.textContent).not.toContain(OVERSIZED_REVIEW_STUB_MARKER); + }); + + test('a genuine binary file is left alone', async () => { + const el = await render(REAL_BINARY, 'assets/logo.png'); + expect(el.querySelector(NOTICE_SELECTOR)).toBeNull(); + }); + + test('an ordinary text diff is left alone', async () => { + const el = await render(TEXT_PATCH, 'calc.ts'); + expect(el.querySelector(NOTICE_SELECTOR)).toBeNull(); + }); +}); diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index e6b96c59c..0c9f31c24 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -14,6 +14,8 @@ import { OverlayScrollArea } from '@plannotator/ui/components/OverlayScrollArea' import { useOverlayViewport } from '@plannotator/ui/hooks/useOverlayViewport'; import { FileHeader } from './FileHeader'; import { FileCommentBanner } from './FileCommentBanner'; +import { OversizedFileNotice } from './OversizedFileNotice'; +import { isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; import { isFileScopedAnnotation, lineRangeForAnnotation } from '../utils/annotationScope'; import { lineAnnotationMetadata } from '../utils/annotationDisplay'; import type { AnnotationScrollTarget } from '../types'; @@ -680,6 +682,10 @@ export const DiffViewer: React.FC = ({ [annotations], ); + // Files over the review size cap arrive as a contents-free stub, which + // renders as an empty body. Say so instead of showing a bare header. + const isOversizedStub = useMemo(() => isOversizedReviewStubPatch(patch), [patch]); + // Replay a selected line/range comment's anchor as the controlled highlight so // clicking it (inline card or sidebar) lights up its lines. A live compose // selection (pendingSelection) wins while the toolbar is open; file-scoped @@ -725,6 +731,7 @@ export const DiffViewer: React.FC = ({ overflowX="scroll" onViewportReady={onViewportReady} > + {isOversizedStub && } void; +}> = ({ onHeightChange }) => { + // Layout effect (before paint) so the host re-measures without a one-frame + // overlap with the content below. + useLayoutEffect(() => { + onHeightChange?.(); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + return ( +
+ This file is over the {OVERSIZED_REVIEW_STUB_LIMIT_LABEL} review limit, so + its contents were not diffed. Only this stub is shown. +
+ ); +}; diff --git a/packages/shared/diff-paths.ts b/packages/shared/diff-paths.ts index e1db18fc1..b704229f5 100644 --- a/packages/shared/diff-paths.ts +++ b/packages/shared/diff-paths.ts @@ -180,6 +180,36 @@ export function parseDiffFilePathLines(lines: string[]): DiffPathPair { return { oldPath, newPath }; } +/** + * Extended-header line the review core injects into the display-only stub it + * emits for a file whose bytes exceed the review size cap. Without it the stub + * is indistinguishable from a genuine binary file, and the UI could only show + * a header-only card with no counts and no explanation — which reads as broken. + * + * Lives here (rather than in review-core) so the browser bundle can detect the + * shape without pulling the whole node-facing review core in. Both server + * runtimes get it from review-core, which is vendored to Pi alongside this file. + * + * The `#` prefix is what makes detection unambiguous: diff CONTENT lines are + * always prefixed with `+`, `-`, or a space, so a bare match on this exact line + * can only come from the extended header we wrote. Git ignores unknown + * extended-header lines, and @pierre/diffs parses the stub identically with or + * without it. + */ +export const OVERSIZED_REVIEW_STUB_MARKER = "#plannotator-oversized-file"; + +/** + * Human-readable form of the cap for UI copy. The authoritative byte value is + * `MAX_REVIEW_FILE_CONTENT_BYTES` in review-core, which is node-facing; a + * review-core test asserts the two never drift. + */ +export const OVERSIZED_REVIEW_STUB_LIMIT_LABEL = "5 MB"; + +/** True when `patch` is one of our oversized-file stubs (see the marker above). */ +export function isOversizedReviewStubPatch(patch: string): boolean { + return patch.split("\n").some((line) => line === OVERSIZED_REVIEW_STUB_MARKER); +} + export function parseDiffMetadataPathLines(lines: string[]): DiffPathPair { let oldPath: string | undefined; let newPath: string | undefined; diff --git a/packages/shared/review-core.test.ts b/packages/shared/review-core.test.ts index ac2b11d49..bac20cbd0 100644 --- a/packages/shared/review-core.test.ts +++ b/packages/shared/review-core.test.ts @@ -1,4 +1,8 @@ import { afterEach, describe, expect, test } from "bun:test"; +import { + isOversizedReviewStubPatch, + OVERSIZED_REVIEW_STUB_LIMIT_LABEL, +} from "./diff-paths"; import { spawnSync } from "node:child_process"; import { chmodSync, @@ -390,6 +394,29 @@ describe("review-core", () => { deletions: 0, }); expect(isBinaryPatchFile(result.patch, "large build.bin")).toBe(true); + // Marked so the UI can say WHY the card is empty. A genuine binary file + // produces the same `Binary files ... differ` line, so the marker is the + // only thing that tells the two apart. + expect(isOversizedReviewStubPatch(result.patch)).toBe(true); + }); + + test("the oversized-stub marker is absent from ordinary and genuinely binary diffs", async () => { + const repoDir = initRepo(); + const runtime = makeRuntime(repoDir); + writeFileSync(join(repoDir, "notes.txt"), "hello\n", "utf-8"); + // NUL bytes, well under the cap: git calls it binary on its own merits. + writeFileSync(join(repoDir, "logo.png"), Buffer.from([0, 1, 2, 0, 3])); + + const result = await runGitDiff(runtime, "uncommitted", "main"); + + expect(result.patch).toContain("Binary files"); + expect(isOversizedReviewStubPatch(result.patch)).toBe(false); + }); + + test("the UI's size-cap label matches the enforced byte cap", () => { + expect(OVERSIZED_REVIEW_STUB_LIMIT_LABEL).toBe( + `${MAX_REVIEW_FILE_CONTENT_BYTES / (1024 * 1024)} MB`, + ); }); test("large tracked text files render as binary in staged and working-tree diffs (#1120)", async () => { diff --git a/packages/shared/review-core.ts b/packages/shared/review-core.ts index 2141f3550..b1e726619 100644 --- a/packages/shared/review-core.ts +++ b/packages/shared/review-core.ts @@ -13,6 +13,7 @@ import { parseDiffFilePathLines, parseDiffGitHeader, parseDiffMetadataPathLines, + OVERSIZED_REVIEW_STUB_MARKER, } from "./diff-paths"; export const JJ_TRUNK_REVSET = "trunk()"; @@ -911,8 +912,11 @@ function buildOversizedTrackedStub(entry: OversizedTrackedDiffEntry): string { const newToken = entry.newPath ? formatPatchPathToken("b", entry.newPath) : "/dev/null"; const oldId = isNullObjectId(entry.oldObjectId) ? "000000000000" : entry.oldObjectId.slice(0, 12); const newId = isNullObjectId(entry.newObjectId) ? "000000000000" : entry.newObjectId.slice(0, 12); + // The marker tells the UI this is OUR size-cap stub rather than a real + // binary file, so the card can say why it has no contents. const lines = [ `diff --git ${headerOldToken} ${headerNewToken}`, + OVERSIZED_REVIEW_STUB_MARKER, ]; if (!entry.oldPath) lines.push(`new file mode ${entry.newMode}`); @@ -1179,6 +1183,9 @@ async function getUntrackedFileDiffs( const newToken = formatPatchPathToken("b", file); return [ `diff --git ${oldToken} ${newToken}`, + // Same size-cap marker the tracked stub carries (see + // buildOversizedTrackedStub) so the UI explains both the same way. + OVERSIZED_REVIEW_STUB_MARKER, `new file mode ${mode}`, `Binary files /dev/null and ${newToken} differ`, "", From ccdb9971c411707c9a56b79a97cc1e76ff06fe71 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 5 Aug 2026 23:35:59 -0700 Subject: [PATCH 3/6] 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 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. --- packages/editor/App.archiveReadOnly.test.tsx | 11 ++- .../DiffViewer.fullContentSwap.test.tsx | 69 ++++++++++++++++--- packages/ui/utils/codeHighlight.test.ts | 13 +++- 3 files changed, 80 insertions(+), 13 deletions(-) diff --git a/packages/editor/App.archiveReadOnly.test.tsx b/packages/editor/App.archiveReadOnly.test.tsx index 2af4e7fe4..2705e64e4 100644 --- a/packages/editor/App.archiveReadOnly.test.tsx +++ b/packages/editor/App.archiveReadOnly.test.tsx @@ -180,7 +180,14 @@ describe.if(hasDom)("App document permissions", () => { const codeBlock = document.querySelector('pre')?.closest('[data-block-id]'); const code = codeBlock?.querySelector('code'); if (!codeBlock || !code) throw new Error("Archived fenced code block did not render"); - const renderedCode = code.innerHTML; + // Text, NOT innerHTML: `applyHighlight` writes plain text first and swaps in + // Shiki markup once the grammar attaches, so the fence's MARKUP legitimately + // changes on its own schedule and an innerHTML comparison races that swap. + // What this test is actually asserting is that the click/hover opened no + // mutation entry point, which is exactly what the text plus the absence of an + // annotation `` says (a code-block annotation is one whole-fence + // ``, per codeBlockMark.ts). + const renderedCodeText = code.textContent; await act(async () => { codeBlock.dispatchEvent(new MouseEvent("mouseover", { bubbles: true })); codeBlock.dispatchEvent(new MouseEvent("click", { bubbles: true })); @@ -189,7 +196,7 @@ describe.if(hasDom)("App document permissions", () => { expect(document.querySelector('textarea')).toBeNull(); expect(document.querySelector('[data-quick-label-picker]')).toBeNull(); expect(code.querySelector('mark')).toBeNull(); - expect(code.innerHTML).toBe(renderedCode); + expect(code.textContent).toBe(renderedCodeText); const optionsButton = document.querySelector('button[title="Options"]'); if (!optionsButton) throw new Error("Options menu trigger did not render"); diff --git a/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx index 226777daa..2c73ca3bd 100644 --- a/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx +++ b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx @@ -24,10 +24,17 @@ * DOM-gated (DOM_TESTS=1) and registered in .github/workflows/test.yml's * "Run UI seam-contract + DOM tests" step. */ -import { afterEach, describe, expect, mock, test } from 'bun:test'; +import { afterAll, afterEach, describe, expect, mock, test } from 'bun:test'; import React from 'react'; import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; +import { + DEFAULT_THEMES, + disposeHighlighter, + getFiletypeFromFileName, + getHighlighterOptions, + preloadHighlighter, +} from '@pierre/diffs'; mock.module('../workerPool', () => ({ useIsWorkerPoolReadyOrDisabled: () => true, @@ -96,12 +103,30 @@ function countExpandButtons(host: HTMLElement): number { const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); -async function waitFor(predicate: () => boolean, timeoutMs: number, stepMs = 25): Promise { - const deadline = Date.now() + timeoutMs; - while (Date.now() < deadline) { +/** + * Wait for a rendered state, budgeted in SCHEDULER TURNS rather than + * milliseconds. + * + * A wall-clock budget makes the test a stopwatch race against the runner: a + * cold, contended CI box blows a deadline that a warm laptop clears, which is + * exactly how the first version of this test flaked. Turns are machine + * independent — a slower box simply spends longer inside each turn — so this + * budget never has to be retuned for CI hardware. Every turn is an `act` + * flushed macrotask boundary, so it also drains microtasks; the states waited + * on here are at most a two step async chain past a synchronous render, which + * makes 400 turns a ~100x margin rather than a guess. + * + * What is waited on is the completion signal itself, not a proxy for one: + * Pierre renders expansion chevrons only from a NON-PARTIAL diff, so their + * presence IS the augmented diff having reached paint. + */ +const WAIT_TURNS = 400; + +async function waitUntil(predicate: () => boolean, turns = WAIT_TURNS): Promise { + for (let i = 0; i < turns; i += 1) { if (predicate()) return true; await act(async () => { - await sleep(stepMs); + await sleep(25); }); } return predicate(); @@ -133,6 +158,14 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { let host: HTMLDivElement | null = null; const originalFetch = globalThis.fetch; + // The highlighter warmed below is a MODULE SINGLETON shared by every test + // file in this bun process, and packages/ui/utils/codeHighlight.test.ts + // asserts the pre-attachment behaviour of exactly that singleton. Put it back + // the way we found it so warming it here cannot decide another file's result. + afterAll(async () => { + await disposeHighlighter(); + }); + afterEach(async () => { if (root) { await act(async () => root!.unmount()); @@ -171,6 +204,16 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { return new Response('{}', { headers: { 'content-type': 'application/json' } }); }) as typeof fetch; + // Warm Pierre's SHARED (module-singleton) Shiki highlighter before + // mounting. Otherwise the first render has to build it, and every paint + // this test observes queues behind a multi-second Shiki/grammar + // initialization whose cost is entirely the runner's CPU. Warming it here + // moves that cost outside the observed window on fast and slow machines + // alike, so the swap is measured, not the highlighter's startup. + await preloadHighlighter( + getHighlighterOptions(getFiletypeFromFileName('calc.ts'), { theme: DEFAULT_THEMES }), + ); + host = document.createElement('div'); document.body.appendChild(host); root = createRoot(host); @@ -180,9 +223,10 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { // The partial diff paints first, and a partial diff is NOT expandable — // Pierre renders the gap bars without chevrons. This is the baseline the - // stale render cache would freeze forever. + // stale render cache would freeze forever. The gate is still closed here, + // so the augmented diff cannot exist yet no matter how slow the box is. await fileContentRequested; - expect(await waitFor(() => shadowHTML(host!).includes('data-separator'), 15_000)).toBe(true); + expect(await waitUntil(() => shadowHTML(host!).includes('data-separator'))).toBe(true); expect(countExpandButtons(host!)).toBe(0); await act(async () => { @@ -191,14 +235,19 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { }); // The augmented full-content diff must reach the PIXELS, not just the - // React tree: expansion chevrons in the gap bars. - const swapped = await waitFor(() => countExpandButtons(host!) > 0, 15_000); + // React tree: expansion chevrons in the gap bars. With the fix these + // arrive in the first turn or two (the highlighter is warm, so the swap + // render is synchronous); without it they never arrive at all. + const swapped = await waitUntil(() => countExpandButtons(host!) > 0); expect(swapped).toBe(true); // And the separator now advertises a real expand target, which is what // makes the click live rather than dead. expect(shadowHTML(host!)).toContain('data-expand-index'); }, - 60_000, + // The only bound in this test. Generous because it has to cover the + // highlighter warm-up on a cold, contended CI runner; the assertions + // themselves carry no wall-clock budget. + 180_000, ); }); diff --git a/packages/ui/utils/codeHighlight.test.ts b/packages/ui/utils/codeHighlight.test.ts index 229222328..d4f3345f5 100644 --- a/packages/ui/utils/codeHighlight.test.ts +++ b/packages/ui/utils/codeHighlight.test.ts @@ -1,6 +1,12 @@ import { describe, expect, test } from 'bun:test'; -import { codeBlockClassName, CODE_BLOCK_CLASS, applyHighlight, highlightToHtml } from './codeHighlight'; +import { + codeBlockClassName, + CODE_BLOCK_CLASS, + applyHighlight, + highlightToHtml, + __resetCodeHighlightCacheForTests, +} from './codeHighlight'; import { resolveFenceTheme, resolveSyntaxTheme, DEFAULT_SYNTAX_THEME, SHIKI_THEME_MAP } from './syntaxTheme'; const hasDom = typeof document !== 'undefined'; @@ -48,6 +54,11 @@ describe('fence theme resolution', () => { describe('highlightToHtml', () => { test('returns null until a grammar is attached, so callers render plain', () => { + // The attachment cache is MODULE state shared with every other test file in + // this bun process, and any file that renders a typescript fence attaches + // that grammar for good. Reset it so this test asserts the pre-attachment + // CONTRACT rather than whichever files happened to run first. + __resetCodeHighlightCacheForTests(); expect(highlightToHtml('const x = 1', 'typescript', 'pierre-dark')).toBeNull(); }); }); From cead24baadb209c1f6a78bd4c17888e7243d7fc2 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 5 Aug 2026 23:59:19 -0700 Subject: [PATCH 4/6] 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. --- .../DiffViewer.fullContentSwap.test.tsx | 91 +++++++++++-------- 1 file changed, 55 insertions(+), 36 deletions(-) diff --git a/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx index 2c73ca3bd..a146b739e 100644 --- a/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx +++ b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx @@ -24,17 +24,11 @@ * DOM-gated (DOM_TESTS=1) and registered in .github/workflows/test.yml's * "Run UI seam-contract + DOM tests" step. */ -import { afterAll, afterEach, describe, expect, mock, test } from 'bun:test'; +import { afterEach, describe, expect, mock, test } from 'bun:test'; import React from 'react'; import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; -import { - DEFAULT_THEMES, - disposeHighlighter, - getFiletypeFromFileName, - getHighlighterOptions, - preloadHighlighter, -} from '@pierre/diffs'; +import { getSingularPatch, processFile } from '@pierre/diffs'; mock.module('../workerPool', () => ({ useIsWorkerPoolReadyOrDisabled: () => true, @@ -69,7 +63,10 @@ const PATCH = [ 'index 0000000..1111111 100644', '--- a/calc.ts', '+++ b/calc.ts', - '@@ -61,7 +61,7 @@', + // `const head58 = 58;` really is line 59 of both contents, so the header, + // the context lines and the files agree. Pierre realigns a misaligned header + // rather than rejecting it, which makes a wrong one a silent trap. + '@@ -59,7 +59,7 @@', ' const head58 = 58;', ' const head59 = 59;', ' export function add(a: number, b: number) {', @@ -101,6 +98,27 @@ function countExpandButtons(host: HTMLElement): number { return (shadowHTML(host).match(/data-expand-button/g) ?? []).length; } +/** + * What Pierre actually painted, for when a wait gives up. + * + * A bare "expected true, received false" from a render wait says nothing about + * WHICH of the several things upstream of the pixels went wrong, and this test + * has already cost one CI round trip to a mystery. Printed only on failure. + */ +function renderDiagnostics(host: HTMLElement | null, label: string): string { + if (host == null) return `${label}: no host`; + const html = shadowHTML(host); + const count = (needle: string) => (html.match(new RegExp(needle, 'g')) ?? []).length; + return [ + `${label}: shadowHTML ${html.length} chars`, + ` diffs-container=${count('diffs-container')}`, + ` data-separator=${count('data-separator')}`, + ` data-expand-button=${count('data-expand-button')}`, + ` data-line-number=${count('data-line-number')}`, + ` head-fragment: ${html.slice(0, 600).replace(/\s+/g, ' ')}`, + ].join('\n'); +} + const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); /** @@ -158,14 +176,6 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { let host: HTMLDivElement | null = null; const originalFetch = globalThis.fetch; - // The highlighter warmed below is a MODULE SINGLETON shared by every test - // file in this bun process, and packages/ui/utils/codeHighlight.test.ts - // asserts the pre-attachment behaviour of exactly that singleton. Put it back - // the way we found it so warming it here cannot decide another file's result. - afterAll(async () => { - await disposeHighlighter(); - }); - afterEach(async () => { if (root) { await act(async () => root!.unmount()); @@ -204,15 +214,18 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { return new Response('{}', { headers: { 'content-type': 'application/json' } }); }) as typeof fetch; - // Warm Pierre's SHARED (module-singleton) Shiki highlighter before - // mounting. Otherwise the first render has to build it, and every paint - // this test observes queues behind a multi-second Shiki/grammar - // initialization whose cost is entirely the runner's CPU. Warming it here - // moves that cost outside the observed window on fast and slow machines - // alike, so the swap is measured, not the highlighter's startup. - await preloadHighlighter( - getHighlighterOptions(getFiletypeFromFileName('calc.ts'), { theme: DEFAULT_THEMES }), - ); + // Precondition, asserted rather than assumed: the REAL parser and the + // REAL augmenter, on these exact fixtures, must produce a non-partial + // diff. Bun's `mock.module` is process global and earlier files in the + // DOM suite mock this very specifier, so this also fails loudly (and in + // milliseconds) if a stale module mock ever reaches this file, instead of + // presenting as an inexplicable render that never arrives. + expect(getSingularPatch(PATCH).isPartial).toBe(true); + const expected = processFile(PATCH, { + oldFile: { name: 'calc.ts', contents: OLD_CONTENTS }, + newFile: { name: 'calc.ts', contents: NEW_CONTENTS }, + }); + expect(expected?.isPartial).toBe(false); host = document.createElement('div'); document.body.appendChild(host); @@ -221,12 +234,19 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { root!.render(view()); }); - // The partial diff paints first, and a partial diff is NOT expandable — - // Pierre renders the gap bars without chevrons. This is the baseline the - // stale render cache would freeze forever. The gate is still closed here, - // so the augmented diff cannot exist yet no matter how slow the box is. + // The gate is still closed here, so the augmented diff cannot exist yet + // no matter how slow the box is. await fileContentRequested; - expect(await waitUntil(() => shadowHTML(host!).includes('data-separator'))).toBe(true); + // Reported, deliberately NOT asserted. The verdict belongs to the swap + // below: whether the partial diff had painted first only affects how + // strong the "no chevrons yet" observation is, and making it a hard gate + // would let a slow or absent FIRST paint mask the result we actually + // came for. A tree without the fix still fails, because it never paints + // chevrons at any point. + const painted = await waitUntil(() => shadowHTML(host!).includes('data-separator')); + if (!painted) console.error(renderDiagnostics(host, 'partial diff never painted')); + // A partial diff is not expandable, so Pierre draws its gap bars without + // chevrons. This is the state the stale render cache freezes forever. expect(countExpandButtons(host!)).toBe(0); await act(async () => { @@ -236,18 +256,17 @@ describe.if(hasDom)('DiffViewer full-content swap (DOM)', () => { // The augmented full-content diff must reach the PIXELS, not just the // React tree: expansion chevrons in the gap bars. With the fix these - // arrive in the first turn or two (the highlighter is warm, so the swap - // render is synchronous); without it they never arrive at all. + // arrive in the first turn or two; without it they never arrive at all. const swapped = await waitUntil(() => countExpandButtons(host!) > 0); + if (!swapped) console.error(renderDiagnostics(host, 'augmented diff never painted')); expect(swapped).toBe(true); // And the separator now advertises a real expand target, which is what // makes the click live rather than dead. expect(shadowHTML(host!)).toContain('data-expand-index'); }, - // The only bound in this test. Generous because it has to cover the - // highlighter warm-up on a cold, contended CI runner; the assertions - // themselves carry no wall-clock budget. + // The only wall-clock bound in this test, and only a backstop: the + // assertions themselves are budgeted in scheduler turns. 180_000, ); }); From e1be381ba0e4d0b7483db9940b8d282b2e3a9ea5 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 6 Aug 2026 00:18:36 -0700 Subject: [PATCH 5/6] 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. --- .github/workflows/test.yml | 19 ++++++++++++++++-- .../AllFilesCodeView.lifecycle.test.tsx | 20 ++++++++++++++++++- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1ebf6dd8c..b73a45c8d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -39,6 +39,23 @@ jobs: - name: Run file-browser DOM test (isolated) run: DOM_TESTS=1 bun test packages/ui/hooks/useFileBrowser.test.tsx + # These two render against the REAL @pierre/diffs, because the defects + # they cover (a stale render cache served across a cacheKey collision; + # an empty stub card) only exist inside its renderer. A sibling in the + # list below, AllFilesCodeView.lifecycle.test.tsx, calls + # `mock.module('@pierre/diffs', ...)` with a hunk-less getSingularPatch + # and `processFile: () => null` — and bun's module mocks are process + # global with no restore, so in one shared process the real renderer is + # not guaranteed to still be there by the time these load. That leak + # reproduced on the Linux runner and not on macOS. Their own process + # removes the coupling outright, same reasoning as the isolated step + # above. + - name: Run diff-renderer DOM tests (isolated, real @pierre/diffs) + run: >- + DOM_TESTS=1 bun test + packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx + packages/review-editor/components/DiffViewer.oversizedStub.test.tsx + # Seam contracts + the remaining DOM-gated tests. Scoped to the DOM files # (not the whole ui suite) to keep this process light. - name: Run UI seam-contract + DOM tests @@ -60,8 +77,6 @@ jobs: packages/review-editor/edit/discardRestoreRender.test.tsx packages/review-editor/edit/selectionActionPopover.test.ts packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx - packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx - packages/review-editor/components/DiffViewer.oversizedStub.test.tsx packages/review-editor/components/guide/GuideSectionCard.test.tsx packages/review-editor/components/guide/GuideView.test.tsx packages/review-editor/hooks/useReviewSearch.test.tsx diff --git a/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx b/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx index d0c2bd8f6..7f62046ce 100644 --- a/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx +++ b/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, mock, test } from 'bun:test'; +import { afterAll, afterEach, describe, expect, mock, test } from 'bun:test'; import React, { act, useCallback, useEffect, useImperativeHandle, useRef } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import type { DiffFile } from '../types'; @@ -7,6 +7,16 @@ let codeViewMounts = 0; let codeViewUnmounts = 0; let scrollTargets: Array> = []; +// Captured BEFORE the mocks below replace the specifiers, so this file can put +// the real modules back when it is done. `mock.module` is process global and +// bun does not unwind it at file boundaries: without this, every later file in +// the same run sees this file's stubs — a `getSingularPatch` with no hunks and +// a `processFile` that returns null. That leaked into +// DiffViewer.fullContentSwap.test.tsx on the Linux runner (and not on macOS), +// where it presented as a diff that silently never rendered. +const realPierreDiffs = await import('@pierre/diffs'); +const realPierreDiffsReact = await import('@pierre/diffs/react'); + mock.module('../workerPool', () => ({ useIsWorkerPoolReadyOrDisabled: () => true, useWorkerPoolThemeSync: () => {}, @@ -138,6 +148,14 @@ afterEach(async () => { scrollTargets = []; }); +// Hand the real @pierre/diffs back to the process. Only the two library +// specifiers are restored: the sibling-module mocks above name paths relative +// to THIS file, so they cannot be reached by another file's imports. +afterAll(() => { + mock.module('@pierre/diffs', () => realPierreDiffs); + mock.module('@pierre/diffs/react', () => realPierreDiffsReact); +}); + describe('AllFilesCodeView guide mount state', () => { test.skipIf(!hasDom)('does not remount when the live shell collapse value changes', async () => { host = document.createElement('div'); From e377f54af1e75f74b35655c29f09dfdc0473f6ae Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 6 Aug 2026 00:19:31 -0700 Subject: [PATCH 6/6] docs(test): point the diff-renderer DOM tests at the CI step that actually runs them --- .../components/DiffViewer.fullContentSwap.test.tsx | 6 ++++-- .../components/DiffViewer.oversizedStub.test.tsx | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx index a146b739e..d31bcd409 100644 --- a/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx +++ b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx @@ -21,8 +21,10 @@ * DiffViewer's Vite-only worker-pool module and the theme/toolbar chrome are * stubbed. * - * DOM-gated (DOM_TESTS=1) and registered in .github/workflows/test.yml's - * "Run UI seam-contract + DOM tests" step. + * DOM-gated (DOM_TESTS=1) and run by .github/workflows/test.yml's + * "Run diff-renderer DOM tests (isolated, real @pierre/diffs)" step. Its own + * process on purpose: a file in the shared DOM step mocks '@pierre/diffs' + * process-wide, and this test is only meaningful against the real renderer. */ import { afterEach, describe, expect, mock, test } from 'bun:test'; import React from 'react'; diff --git a/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx b/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx index f16fcf624..0b8c5fa8f 100644 --- a/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx +++ b/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx @@ -8,8 +8,10 @@ * (`OVERSIZED_REVIEW_STUB_MARKER`) so the UI can tell it apart from a genuine * binary file, which must keep rendering exactly as it always did. * - * DOM-gated (DOM_TESTS=1) and registered in .github/workflows/test.yml's - * "Run UI seam-contract + DOM tests" step. + * DOM-gated (DOM_TESTS=1) and run by .github/workflows/test.yml's + * "Run diff-renderer DOM tests (isolated, real @pierre/diffs)" step — its own + * process, because a file in the shared DOM step mocks '@pierre/diffs' + * process-wide and this renders against the real renderer. */ import { afterEach, describe, expect, mock, test } from 'bun:test'; import React from 'react';