diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b28a34f6a..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 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/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'); diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 704473aae..ffbedb603 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -29,6 +29,9 @@ 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 { isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; +import { OversizedFileNotice } from './OversizedFileNotice'; import { ToolbarHost, type ToolbarHostHandle } from './ToolbarHost'; import { FileHeader } from './FileHeader'; import { EditSessionHud } from './EditSessionHud'; @@ -307,18 +310,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 @@ -2184,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.fullContentSwap.test.tsx b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx new file mode 100644 index 000000000..d31bcd409 --- /dev/null +++ b/packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx @@ -0,0 +1,274 @@ +/** + * 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 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'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { getSingularPatch, processFile } from '@pierre/diffs'; + +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', + // `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) {', + '- 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; +} + +/** + * 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)); + +/** + * 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(25); + }); + } + 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; + + // 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); + root = createRoot(host); + await act(async () => { + root!.render(view()); + }); + + // The gate is still closed here, so the augmented diff cannot exist yet + // no matter how slow the box is. + await fileContentRequested; + // 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 () => { + releaseFileContent!(); + await sleep(0); + }); + + // 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; 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 wall-clock bound in this test, and only a backstop: the + // assertions themselves are budgeted in scheduler turns. + 180_000, + ); +}); 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..0b8c5fa8f --- /dev/null +++ b/packages/review-editor/components/DiffViewer.oversizedStub.test.tsx @@ -0,0 +1,144 @@ +/** + * 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 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'; +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 c36fa5871..0c9f31c24 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -14,11 +14,14 @@ 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'; 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 +313,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 +376,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; } @@ -652,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 @@ -697,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/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); +} 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`, "", 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(); }); });