From 1d7798abc51431aa8b84757b0fbf3f1aa5276b88 Mon Sep 17 00:00:00 2001 From: Oleh Omelchenko Date: Sun, 7 Jun 2026 23:08:56 +0300 Subject: [PATCH] Serialize live-preview renders to prevent concurrent embeds (M3) --- src/app/components/LivePreview.test.tsx | 101 ++++++++++++++++- src/app/components/LivePreview.tsx | 143 +++++++++++++++--------- 2 files changed, 188 insertions(+), 56 deletions(-) diff --git a/src/app/components/LivePreview.test.tsx b/src/app/components/LivePreview.test.tsx index 71d8182..aa1d866 100644 --- a/src/app/components/LivePreview.test.tsx +++ b/src/app/components/LivePreview.test.tsx @@ -15,9 +15,35 @@ import { useSnippetStore } from '../stores/SnippetStore'; import { useDatasetStore } from '../stores/DatasetStore'; import { LivePreview } from './LivePreview'; -// Heavy services and sub-components the LivePreview wires — not under test here. +// Controllable renderSpec: each call gets an id and parks a resolver in `H.pending` +// so a test can decide exactly when (and in what order) embeds settle — the only +// way to exercise the overlapping-render serialization deterministically. `destroy` +// records its id and clears the node, mirroring the real handle (which calls +// node.replaceChildren()), so a wrongful blank would be observable. +const H = vi.hoisted(() => ({ + calls: 0, + pending: [] as Array<() => void>, + destroyed: [] as number[], +})); vi.mock('../services/chart-renderer', () => ({ - renderSpec: () => Promise.resolve({ destroy() {}, resize() {} }), + renderSpec: (node: HTMLElement) => { + const id = ++H.calls; + return new Promise((resolve) => { + H.pending.push(() => { + node.replaceChildren(); // a real embed wipes then rebuilds the host + const mark = document.createElement('div'); + mark.dataset.render = String(id); + node.appendChild(mark); + resolve({ + destroy() { + node.replaceChildren(); + H.destroyed.push(id); + }, + resize() {}, + }); + }); + }); + }, })); vi.mock('./SettingsPopover', () => ({ SettingsPopover: () => null, @@ -31,6 +57,9 @@ let container: HTMLDivElement; let root: Root; beforeEach(() => { + H.calls = 0; + H.pending.length = 0; + H.destroyed.length = 0; usePreviewStore.setState({ error: null, busy: false }); useSnippetStore.getState().reset(); useDatasetStore.getState().reset(); @@ -85,3 +114,71 @@ describe('LivePreview busy overlay', () => { expect(container.querySelector('[aria-hidden="true"]')).toBeNull(); }); }); + +describe('LivePreview render serialization', () => { + /** Drive the debounced render effect to completion under fake timers. */ + const tick = (ms = 6000) => act(async () => void (await vi.advanceTimersByTimeAsync(ms))); + + test('a second render waits for the in-flight one, which is finalized when superseded', async () => { + vi.useFakeTimers(); + try { + // Render A: valid, ref-less spec so prepareSpecForRender passes and we reach + // renderSpec (which now parks, awaiting our resolver). + act(() => { + useSnippetStore.setState({ draftText: '{"data":{"values":[]},"mark":"point"}' }); + }); + await tick(); + expect(H.calls).toBe(1); // A invoked renderSpec and is in flight + + // Render B arrives while A is still embedding. It must NOT start a second + // embed on the shared node — it queues behind A's lock. + act(() => { + useSnippetStore.setState({ draftText: '{"data":{"values":[]},"mark":"bar"}' }); + }); + await tick(); + expect(H.calls).toBe(1); // still only A's embed; B is serialized behind it + + // A settles. It was superseded by B, so its handle is finalized (destroy), + // and only now does B acquire the lock and start its embed. + act(() => H.pending[0]()); + await tick(0); + expect(H.destroyed).toContain(1); // superseded A finalized + expect(H.calls).toBe(2); // B now embeds + + // B settles and becomes the live chart — never destroyed, and its node + // content survives (A's finalize ran before B drew, so nothing blanked it). + act(() => H.pending[1]()); + await tick(0); + expect(H.destroyed).not.toContain(2); + expect(container.querySelector('[data-render="2"]')).not.toBeNull(); + } finally { + vi.useRealTimers(); + } + }); + + test('clearing the editor invalidates a pending render (no stale redraw)', async () => { + vi.useFakeTimers(); + try { + act(() => { + useSnippetStore.setState({ draftText: '{"data":{"values":[]},"mark":"point"}' }); + }); + await tick(); + expect(H.calls).toBe(1); // render A in flight + + // Editor cleared while A embeds. The clear advances the generation token, so + // when A finally resolves it knows it is stale and finalizes instead of + // committing — the pane ends up empty, not showing A's chart. + act(() => { + useSnippetStore.setState({ draftText: '' }); + }); + await tick(); + act(() => H.pending[0]()); + await tick(0); + + expect(H.destroyed).toContain(1); // stale A finalized, not committed + expect(container.querySelector('[data-render="1"]')).toBeNull(); + } finally { + vi.useRealTimers(); + } + }); +}); diff --git a/src/app/components/LivePreview.tsx b/src/app/components/LivePreview.tsx index 1aa2367..9fea174 100644 --- a/src/app/components/LivePreview.tsx +++ b/src/app/components/LivePreview.tsx @@ -92,6 +92,10 @@ export function LivePreview() { const hostRef = useRef(null); const handleRef = useRef(null); const generationRef = useRef(0); + // Serializes node mutations across overlapping renders: each render chains onto + // the previous one's promise so only one vega-embed ever touches the shared host + // node at a time (see the render effect for why this matters). + const renderChainRef = useRef>(Promise.resolve()); const shownText = useSnippetStore(selectShownText); const fitMode = useAppStore((s) => s.previewFitMode); const uiTheme = useAppStore((s) => s.uiTheme); @@ -133,32 +137,23 @@ export function LivePreview() { // returns void (it handles its own errors internally — nothing awaits it). const timer = setTimeout(() => { void (async () => { - // Empty/blank is not an error — clean, empty pane (spec §04). - if (text === '') { - handleRef.current?.destroy(); - handleRef.current = null; - setError(null); - return; - } - - let parsed: unknown; - try { - parsed = JSON.parse(text); - } catch (e) { - setError(`Invalid JSON: ${(e as Error).message}`); - return; - } - + // Every render — including the empty/clear path — advances the generation + // token, so a slow in-flight render that resolves later knows it's stale. + // (Clearing must invalidate a pending render too, or it would redraw the + // chart the user just cleared.) const mine = ++generationRef.current; + const isEmpty = text === ''; - // Arm the busy indicator: if the render hasn't settled within ~1s, flip the - // flag. Sub-1s renders (the common case) never show the overlay — no flicker - // (arch §10.2; NN/g ≤1s = uninterrupted thought, >1s = noticeably waiting). - if (busyTimerRef.current !== null) clearTimeout(busyTimerRef.current); - busyTimerRef.current = setTimeout(() => { - // Only arm for the current generation; a superseded render doesn't own busy. - if (mine === generationRef.current) setBusy(true); - }, 1000); + // Parse before touching the node so a syntax error leaves the chart up. + let parsed: unknown; + if (!isEmpty) { + try { + parsed = JSON.parse(text); + } catch (e) { + if (mine === generationRef.current) setError(`Invalid JSON: ${(e as Error).message}`); + return; + } + } /** Clear busy and the timer unconditionally — called on settle or error. */ const clearBusy = () => { @@ -169,43 +164,83 @@ export function LivePreview() { setBusy(false); }; + // Arm the busy indicator: if a (non-empty) render hasn't settled within ~1s, + // flip the flag. Sub-1s renders (the common case) never show the overlay — no + // flicker (arch §10.2; NN/g ≤1s = uninterrupted thought, >1s = noticeably waiting). + if (busyTimerRef.current !== null) clearTimeout(busyTimerRef.current); + if (!isEmpty) { + busyTimerRef.current = setTimeout(() => { + // Only arm for the current generation; a superseded render doesn't own busy. + if (mine === generationRef.current) setBusy(true); + }, 1000); + } + + // Serialize node mutations: wait for any in-flight render to settle before + // we touch the shared host node. Two concurrent vega-embed calls on one node + // interleave, and a superseded render's destroy() (which clears the node) + // could blank the chart a newer render is showing. Holding this lock makes + // both impossible. After the wait, bail if a newer render already superseded + // us — no wasted embed, and the clear/destroy below only runs while we hold + // the lock, so it can never wipe a chart another render is currently showing. + const prior = renderChainRef.current; + let release!: () => void; + renderChainRef.current = new Promise((r) => { + release = r; + }); try { - const prepared = prepareSpecForRender(parsed, { fitMode, datasets }); - const config = chartConfigFor(uiTheme); - handleRef.current?.destroy(); - handleRef.current = null; - const handle = await renderSpec(node, prepared as VisualizationSpec, config); + await prior; if (mine !== generationRef.current) { - // TODO: a superseded render's destroy() calls node.replaceChildren(), - // which can blank the live chart if two embeds on the same node are - // ever in flight at once (heavy spec whose embed outlasts the 300ms - // debounce). The debounce makes this rare; when dataset work (M3) - // lands, serialize renders or finalize the stale view without - // clearing the shared node. - handle.destroy(); // a newer render superseded this one + clearBusy(); return; } - handleRef.current = handle; - setError(null); - clearBusy(); - } catch (e) { - if (mine === generationRef.current) { + + // Empty/blank is not an error — clean, empty pane (spec §04). + if (isEmpty) { + handleRef.current?.destroy(); + handleRef.current = null; + setError(null); clearBusy(); - // A missing dataset reference is not a JSON/spec problem, so it gets a - // tailored, fixable message instead of the generic syntax hint (council: - // GOV.UK error-message + NN/g #9 — name the problem, give the real fix). - if (e instanceof DatasetNotFoundError) { - setError( - `Dataset "${e.datasetName}" not found. Create it from Datasets ` + - `(⌘/Ctrl+K), or check the dataset name in your spec.`, - ); - } else { - setError( - `Rendering error: ${(e as Error).message}. ` + - `Check your JSON syntax and that the spec is valid Vega-Lite.`, - ); + return; + } + + try { + const prepared = prepareSpecForRender(parsed, { fitMode, datasets }); + const config = chartConfigFor(uiTheme); + handleRef.current?.destroy(); + handleRef.current = null; + const handle = await renderSpec(node, prepared as VisualizationSpec, config); + if (mine !== generationRef.current) { + // A newer render superseded this one. We still hold the lock, so the + // newer render hasn't drawn yet (it's queued behind us) — finalizing + // this handle and clearing the node is safe; the newer render redraws + // on a clean node when it acquires the lock. + handle.destroy(); + return; + } + handleRef.current = handle; + setError(null); + clearBusy(); + } catch (e) { + if (mine === generationRef.current) { + clearBusy(); + // A missing dataset reference is not a JSON/spec problem, so it gets a + // tailored, fixable message instead of the generic syntax hint (council: + // GOV.UK error-message + NN/g #9 — name the problem, give the real fix). + if (e instanceof DatasetNotFoundError) { + setError( + `Dataset "${e.datasetName}" not found. Create it from Datasets ` + + `(⌘/Ctrl+K), or check the dataset name in your spec.`, + ); + } else { + setError( + `Rendering error: ${(e as Error).message}. ` + + `Check your JSON syntax and that the spec is valid Vega-Lite.`, + ); + } } } + } finally { + release(); } })(); }, delay);