Image export: surface rasterize/serialize failures instead of reporting 'not ready'

2026-08-08 02:02:33 +00:00 · 2026-06-14 20:39:10 +03:00
parent 91b8b1e7fe
commit 6fc8c29b50
3 changed files with 114 additions and 13 deletions
@@ -0,0 +1,87 @@
+/**
+ * ChartExport — image-export failure vs not-ready (arch 05 §7 fail-loud).
+ *
+ * `getImageUrl` (injected by LivePreview) returns `null` ONLY when no view is
+ * live — the not-ready case — and rejects on a real rasterize/serialize failure.
+ * These two outcomes must land as DIFFERENT toasts: the not-ready one tells the
+ * user to wait (no `detail`), the failure one carries diagnostic `detail` so the
+ * problem can be reported. Burying the rejection behind the not-ready copy is the
+ * regression this guards.
+ */
+
+import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest';
+import { act } from 'react';
+import { createRoot, type Root } from 'react-dom/client';
+import { useNotificationStore } from '../stores/NotificationStore';
+import { usePopoverStore } from '../stores/PopoverStore';
+import { useSnippetStore } from '../stores/SnippetStore';
+import { useDatasetStore } from '../stores/DatasetStore';
+import { ChartExport, type ChartExportProps } from './ChartExport';
+
+(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
+let container: HTMLDivElement;
+let root: Root;
+
+beforeEach(() => {
+  useNotificationStore.getState().clear();
+  usePopoverStore.setState({ openId: null });
+  useSnippetStore.getState().reset();
+  useDatasetStore.getState().reset();
+  // A non-empty draft makes `hasSpec` true so the export trigger is enabled.
+  useSnippetStore.setState({ draftText: '{"mark":"point"}' });
+
+  container = document.createElement('div');
+  document.body.appendChild(container);
+  root = createRoot(container);
+});
+
+afterEach(() => {
+  act(() => root.unmount());
+  container.remove();
+  useNotificationStore.getState().clear();
+  usePopoverStore.setState({ openId: null });
+  useSnippetStore.getState().reset();
+  useDatasetStore.getState().reset();
+});
+
+/** Mount, open the export popover (portaled to <body>), and click "Download PNG".
+ *  `downloadImage` awaits `getImageUrl`, so the caller flushes microtasks after. */
+async function mountAndDownloadPng(getImageUrl: ChartExportProps['getImageUrl']) {
+  act(() => {
+    root.render(<ChartExport chartReady getImageUrl={getImageUrl} />);
+  });
+  act(() => usePopoverStore.getState().show('chart-export'));
+  const pngBtn = Array.from(document.body.querySelectorAll('button')).find(
+    (b) => b.textContent?.trim() === 'Download PNG',
+  );
+  if (!pngBtn) throw new Error('Download PNG button not found — popover did not render');
+  await act(async () => {
+    pngBtn.dispatchEvent(new MouseEvent('click', { bubbles: true }));
+    await Promise.resolve();
+  });
+}
+
+describe('ChartExport image-export error handling', () => {
+  test('a rejecting getImageUrl surfaces the failure with diagnostic detail', async () => {
+    await mountAndDownloadPng(vi.fn().mockRejectedValue(new TypeError('tainted canvas')));
+
+    const toasts = useNotificationStore.getState().notifications;
+    expect(toasts).toHaveLength(1);
+    expect(toasts[0].kind).toBe('error');
+    // A real failure carries its diagnostic detail (not the "wait, it's not ready" copy).
+    expect(toasts[0].detail).toBe('TypeError: tainted canvas');
+    expect(toasts[0].message).not.toMatch(/ready yet/i);
+  });
+
+  test('a null getImageUrl reports not-ready, with no diagnostic detail', async () => {
+    await mountAndDownloadPng(vi.fn().mockResolvedValue(null));
+
+    const toasts = useNotificationStore.getState().notifications;
+    expect(toasts).toHaveLength(1);
+    expect(toasts[0].kind).toBe('error');
+    expect(toasts[0].message).toMatch(/ready yet/i);
+    // Nothing for the user to report — the not-ready case omits detail.
+    expect(toasts[0].detail).toBeUndefined();
+  });
+});
@@ -89,8 +89,9 @@ export interface ChartExportProps {
  /** Whether a chart is currently rendered — gates the image (PNG/SVG) actions.
   *  The spec actions only need text, so they ignore this. */
  chartReady: boolean;
-  /** Rasterize/serialize the live view to a downloadable URL, or null if the view
-   *  isn't available (caller owns the Vega view). */
+  /** Rasterize/serialize the live view to a downloadable URL, or null if no view
+   *  is live (the not-ready case). Rejects if rasterizing/serializing fails — a real
+   *  error the caller reports, not a not-ready state (caller owns the Vega view). */
  getImageUrl: (
    format: 'png' | 'svg',
    options: { scale: number; background: string | null },
@@ -165,10 +166,24 @@ export function ChartExport({ chartReady, getImageUrl }: ChartExportProps) {

  const downloadImage = async (format: 'png' | 'svg') => {
    close();
-    const url = await getImageUrl(format, {
-      scale: Number(scale),
-      background: resolveBackground(background),
-    });
+    let url: string | null;
+    try {
+      url = await getImageUrl(format, {
+        scale: Number(scale),
+        background: resolveBackground(background),
+      });
+    } catch (err) {
+      // The view is live but rasterizing/serializing it threw — a real failure
+      // (e.g. a tainted canvas or an SVG the browser won't serialize), not a
+      // not-ready state. Surface it with its detail rather than burying it.
+      notify({
+        kind: 'error',
+        title: 'Couldn’t export image',
+        message: `Astrolabe couldn’t turn the chart into ${format.toUpperCase()}. This can happen with very large charts or content the browser won’t serialize.`,
+        detail: err instanceof Error ? `${err.name}: ${err.message}` : String(err),
+      });
+      return;
+    }
    if (!url) {
      notify({
        kind: 'error',
@@ -342,8 +342,11 @@ export function LivePreview() {
  ]);

  // Rasterize/serialize the live view for the per-chart export (spec §08). Reads
-  // `handleRef` (the view LivePreview owns); returns null when no view is live or
-  // the export fails, so the export UI can report it. Stable identity (no deps).
+  // `handleRef` (the view LivePreview owns); returns null only when no view is live
+  // (the "not ready" case). A rasterize/serialize *failure* is a real error, not a
+  // not-ready state, so it propagates for the export UI to report with its detail —
+  // burying it here would misreport a tainted canvas or a serialization bug as
+  // "not ready yet" (arch 02 fail-loud). Stable identity (no deps).
  const getImageUrl = useCallback(
    async (
      format: 'png' | 'svg',
@@ -351,11 +354,7 @@ export function LivePreview() {
    ): Promise<string | null> => {
      const handle = handleRef.current;
      if (!handle) return null;
-      try {
-        return await handle.toImageURL(format, options);
-      } catch {
-        return null;
-      }
+      return await handle.toImageURL(format, options);
    },
    [],
  );