Serialize live-preview renders to prevent concurrent embeds (M3)

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();
+    }
+  });
+});

src/app/components/LivePreview.tsx

@@ -92,6 +92,10 @@ export function LivePreview() {
   const hostRef = useRef<HTMLDivElement>(null);
   const handleRef = useRef<RenderHandle | null>(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<void>>(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<void>((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);