mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
7ba4e3b3e4
* fix(review): mint content-derived diff cache keys so single-file tabs render fully Single-file diff tabs have not rendered their full-content diff since v0.26.0: the expansion gap bars show no chevrons and clicking them does nothing, at every file size. @pierre/diffs 1.3.2 (the 1.2.8 -> 1.3.2 bump, upstream "Fix diff rerender in edit mode (#878)") added name-based cacheKey defaulting in FileDiff.render: an unset `fileDiff.cacheKey` becomes the file's name. `areDiffTargetsEqual` — the only identity check its render and highlight caches make — compares nothing but that key. DiffViewer renders each file twice on one surviving FileDiff instance (key={filePath}): first the PARTIAL diff from getSingularPatch, then the AUGMENTED full-content diff from processFile once /api/file-content lands. Neither set a cacheKey, so both defaulted to the filename and Pierre served the stale partial render forever. Only the augmented diff is expandable, hence the dead gap bars. Both diffs now mint content-derived keys (`<path>#<hash>` and `<path>#full#<hash>`), matching how AllFilesCodeView already keys its items — which is why the all-files view was never affected. The hash (not patch.length) matters because Pierre's worker highlight cache is a singleton that outlives remounts. The partial diff needs its own key too: with key={filePath} the instance also survives diff-type and base switches, where a same-named new patch would otherwise hit the same name-keyed stale cache. hashString moves from AllFilesCodeView to utils/hashString.ts so both surfaces mint keys the same way. Covered by a new DOM test that mounts DiffViewer against the real @pierre/diffs renderer, holds the /api/file-content response until the non-expandable partial baseline is asserted, then requires the expansion affordances to reach the pixels. It fails against the unfixed tree. * fix(review): explain why an oversized file's card has no diff Files over the 5 MB review limit are replaced by a contents-free stub (buildOversizedTrackedStub, plus the untracked equivalent), which renders as a header-only card with no counts and no explanation. Users read that as a broken diff. The stub now carries an explicit marker line in its extended header (OVERSIZED_REVIEW_STUB_MARKER). A marker rather than a client heuristic because the only other signal, `Binary files ... differ`, is exactly what a genuine binary file emits, so a heuristic would put a false size-cap explanation on every image in the diff. The marker lives in shared/diff-paths so the browser bundle can detect it without pulling in the node-facing review core; both server runtimes pick it up from review-core, which vendor.sh already copies to Pi. Git ignores unknown extended-header lines and @pierre/diffs parses the stub identically with or without it, so nothing else moves. Which files get stubbed is unchanged. Both review surfaces now render one line under the file header saying the file is over the limit and only a stub is shown. * test(review): make the diff-swap proof machine independent, not stopwatch based CI failed two tests that pass locally. Both were timing races, neither was an app bug. 1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal wall-clock budget, which a cold, contended CI runner blows and a warm laptop clears. Two changes, both aimed at the clock rather than the symptom: - The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A slower box spends longer inside each turn but needs no more of them, so the budget never has to be retuned for CI hardware. - Pierre's shared Shiki highlighter is preloaded before mounting. It is a module singleton, and building it was the entire multi-second cost the old budget was accidentally measuring; warming it moves that work into an unbounded await OUTSIDE the observed window. Disposed in afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the pre-attachment behaviour of that same singleton. Verified against an artificially stalled clock: forcing 20s of dead time into every wait (41s total, far past the old 15s budget) still passes, and with the cacheKey fix removed it still fails on the assertion (not as an opaque timeout) in ~12s. A 20-turn budget with the preload removed and every core saturated also passed 10/10, so 400 turns is a wide margin rather than a guess. 2. App.archiveReadOnly compared the fenced block's innerHTML before and after a click. Since #1218, applyHighlight writes plain text first and swaps in Shiki markup when the grammar attaches, so that MARKUP changes on its own schedule and the assertion was racing the swap. The test is checking that the click opened no mutation entry point, which textContent plus the absence of an annotation <mark> says exactly, and which no highlight swap can perturb. Latent on main; the branch's run happened to lose the race. Also fixed while confirming the above: codeHighlight.test.ts asserted a GLOBAL precondition ("no grammar attached yet") that any earlier file attaching a typescript fence invalidates, so `DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order alone. It now resets the attachment cache through the existing __resetCodeHighlightCacheForTests seam and asserts the contract instead of the run order. Not currently reachable from CI (that file is not in the DOM list), but one list edit away. * test(review): drop the highlighter preload, harden the swap proof, report why a paint is missing The preload added in the previous commit made CI strictly worse, so it is gone. Before it, CI's partial diff painted and only the swap was missing; with it, CI never painted at all. It was an optimization for a theory the evidence has since killed, and it mutated a process-wide singleton to buy it, so it is not worth keeping while the real failure is unexplained. The afterAll dispose that existed only to undo the preload goes with it. What the CI log actually shows: - The "WorkerPoolManager: operation canceled because the pool terminated" error is inside discardRestoreRender.test.tsx's own group, ~0.3s BEFORE this file's group opens. It is that file's provider unmounting and terminating the pool singleton it created: end-of-file teardown, the same benign noise documented on #1209. It also prints on every local run, where the whole list passes. It is not a mid-test terminator, and nothing in this file uses the worker pool (no WorkerPoolContextProvider is mounted, so useWorkerPool() is undefined and rendering takes the main-thread path). - This file's group prints NOTHING for its whole 10.3s: no console.warn from the stale-content guard, no error. Pierre simply painted nothing. Not reproducible locally: the exact DOM list from test.yml, one bun process, forward and reverse order, 13 runs with every core saturated, all green. So the remaining difference is the environment, which cannot be reasoned out from here. Three changes make the next CI run answer it instead of costing another guess: - renderDiagnostics() dumps what Pierre actually painted (container / separator / chevron / line-number counts plus a markup fragment) when a wait gives up. Prints only on failure, so it is worth keeping. - The precondition is asserted rather than assumed: the REAL getSingularPatch and processFile must produce partial-then-full on these fixtures. Bun's mock.module is process global and an earlier file in this very list mocks '@pierre/diffs', so a leaked mock now fails in milliseconds with a clear message instead of as a render that never arrives. - The first paint is now REPORTED, not asserted. The verdict belongs to the swap; gating on the partial paint let a slow or absent first paint mask the result the test exists for. Removing the cacheKey fix still fails it (verified), because that tree paints no chevrons at any point. Also fixed a real trap in the fixture: the hunk header said @@ -61 while its context lines start at line 59 of both contents. Pierre realigns a misaligned header rather than rejecting it, so it was silently tolerated. * test(review): stop a leaked module mock from silently unrendering the diff tests Root cause, and it was never a timing problem. AllFilesCodeView.lifecycle.test.tsx calls `mock.module('@pierre/diffs', ...)` with a hunk-less `getSingularPatch` and `processFile: () => null`. Bun's module mocks are process global and are not unwound at file boundaries, and that file sits immediately before DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux runner the stub reached this file; on macOS it did not, which is why 13 local runs of the exact list, both orders, cores saturated, stayed green. It explains both CI symptoms exactly, including the one that looked like a contradiction: `processFile: () => null` means the augmented diff never exists, so no chevrons ever (the failure before the preload); the stub `getSingularPatch` has `hunks: []`, so nothing paints at all (the failure after it). The "WorkerPoolManager: operation canceled because the pool terminated" line was a red herring throughout: it is inside discardRestoreRender's own group, ~0.3s BEFORE this file's group opens, is that file's provider unmounting the pool it created, and prints on every local run too. The precondition assertion added in the previous commit is what proved it, turning a 10.3s mystery into a 0.45ms verdict: 228 | expect(expected?.isPartial).toBe(false); error: expect(received).toBe(expected) Expected: false Received: undefined Fixed at both ends: - Source: the mocking file now captures the real modules before it stubs them and restores both library specifiers in afterAll, so no later file in any run inherits its stubs. This fixes the class for every future DOM test that needs the real renderer, which was the actual leak. - Consumer: the two tests that render against the real @pierre/diffs get their own CI step, the same isolation (and for the same kind of reason) this workflow already gives useFileBrowser.test.tsx. They are removed from the shared list so that step is their single source of truth. The restore above should make this unnecessary; it is not something to bet a green build on from a machine that cannot reproduce the platform behaviour. Verified with a CI-faithful harness: one bun process per step, the exact lists from test.yml, isolated + shared-forward + shared-reverse, 10 iterations with every core saturated, then 6 more after the final split. All green, plus the full suite and typecheck. * docs(test): point the diff-renderer DOM tests at the CI step that actually runs them
85 lines
3.7 KiB
TypeScript
85 lines
3.7 KiB
TypeScript
import { describe, expect, test } from 'bun:test';
|
|
|
|
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';
|
|
|
|
describe('code block class', () => {
|
|
test('carries the structural class and the language hook', () => {
|
|
expect(codeBlockClassName('rust')).toBe(`${CODE_BLOCK_CLASS} font-mono language-rust`);
|
|
});
|
|
|
|
test('omits the language hook for language-less fences', () => {
|
|
expect(codeBlockClassName()).toBe(`${CODE_BLOCK_CLASS} font-mono`);
|
|
expect(codeBlockClassName(undefined)).not.toContain('language-');
|
|
});
|
|
});
|
|
|
|
describe('fence theme resolution', () => {
|
|
test('matches the theme the diff pane resolves, per mode', () => {
|
|
expect(resolveFenceTheme('kanagawa-wave', 'dark')).toBe('kanagawa-wave');
|
|
expect(resolveFenceTheme('github', 'light')).toBe('github-light');
|
|
expect(resolveFenceTheme('colorblind', 'dark')).toBe('pierre-dark-protanopia-deuteranopia');
|
|
expect(resolveFenceTheme('colorblind', 'light')).toBe('pierre-light-protanopia-deuteranopia');
|
|
});
|
|
|
|
test('falls back to the Pierre defaults for unmapped palettes', () => {
|
|
// The default Plannotator palette has no Shiki counterpart, so it renders
|
|
// in exactly what @pierre/diffs uses when handed no theme at all.
|
|
expect(resolveSyntaxTheme('plannotator', 'dark')).toBeUndefined();
|
|
expect(resolveFenceTheme('plannotator', 'dark')).toBe(DEFAULT_SYNTAX_THEME.dark);
|
|
expect(resolveFenceTheme('plannotator', 'light')).toBe(DEFAULT_SYNTAX_THEME.light);
|
|
});
|
|
|
|
test('falls back per mode when a palette only defines one side', () => {
|
|
// dracula is dark-only; its light mode must still resolve to something.
|
|
expect(SHIKI_THEME_MAP['dracula']?.light).toBeNull();
|
|
expect(resolveFenceTheme('dracula', 'dark')).toBe('dracula');
|
|
expect(resolveFenceTheme('dracula', 'light')).toBe(DEFAULT_SYNTAX_THEME.light);
|
|
});
|
|
|
|
test('every mapped theme name is non-empty', () => {
|
|
for (const [palette, pair] of Object.entries(SHIKI_THEME_MAP)) {
|
|
expect(pair.dark ?? pair.light, `${palette} maps to nothing`).toBeTruthy();
|
|
}
|
|
});
|
|
});
|
|
|
|
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();
|
|
});
|
|
});
|
|
|
|
describe.if(hasDom)('applyHighlight', () => {
|
|
test('language-less fences render as plain text, never guessed (#1212)', () => {
|
|
const el = document.createElement('code');
|
|
applyHighlight(el, 'plain <b>text</b> & more', undefined, 'pierre-dark');
|
|
expect(el.textContent).toBe('plain <b>text</b> & more');
|
|
// Escaped into text nodes, not parsed as markup.
|
|
expect(el.querySelector('b')).toBeNull();
|
|
expect(el.children.length).toBe(0);
|
|
});
|
|
|
|
test('writes the exact source immediately so there is no layout shift', () => {
|
|
const el = document.createElement('code');
|
|
const code = 'fn main() {\n println!("hi");\n}';
|
|
applyHighlight(el, code, 'rust', 'pierre-dark');
|
|
// The highlighter is cold here, so the synchronous result is the plain
|
|
// source at its final size; the highlighted swap lands later.
|
|
expect(el.textContent).toBe(code);
|
|
});
|
|
});
|