feat: Mosaic chart builders - #473

packages/mosaic/src/editor/useMosaicChartEditor.ts (2)

143-146: useEffect without dependency array updates ref on every render.

This useEffect runs on every render to keep onSpecChangeRef current. While functional, consider omitting the useEffect entirely and assigning directly since refs don't trigger re-renders:

♻️ Simpler pattern

 const onSpecChangeRef = useRef(onSpecChange);
-useEffect(() => {
-  onSpecChangeRef.current = onSpecChange;
-});
+onSpecChangeRef.current = onSpecChange;

Note: Both patterns work; the direct assignment is slightly more concise.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/editor/useMosaicChartEditor.ts` around lines 143 - 146,
The current useEffect updates onSpecChangeRef.current on every render; replace
that pattern by removing the useEffect and directly assigning the prop into the
ref (keep the existing const onSpecChangeRef = useRef(onSpecChange)) by setting
onSpecChangeRef.current = onSpecChange in the component body so the ref stays
current without an effect—refer to onSpecChangeRef, useRef and onSpecChange to
locate the change.

---

152-187: Complex nested setState in applyChanges.

The applyChanges callback has nested setDebouncedParsed and setEditorState calls, which can be difficult to reason about. The logic is correct, but consider extracting the dirty-check into a separate utility for clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/editor/useMosaicChartEditor.ts` around lines 152 - 187,
The applyChanges function contains a nested setDebouncedParsed -> setEditorState
with an inline IIFE dirty-check; extract that dirty-check into a small pure
utility (e.g., isSpecDirty(editedSpecString, appliedSpecString)) and replace the
inline try/catch IIFE with a call to that utility inside setEditorState, keeping
the same behavior for calling onSpecChangeRef.current and updating
appliedSpecString/prevInitialSpecString; ensure the utility handles JSON.parse
errors the same way and update imports/refs so applyChanges, setDebouncedParsed,
setEditorState and onSpecChangeRef use the new function.

packages/mosaic/src/editor/mosaicSchema.ts (1)

26-48: Consider adding a timeout to the fetch request.

The fetch has no timeout configured, which could cause indefinite hangs if the CDN is slow or unresponsive. Consider using AbortSignal.timeout() for resilience.

♻️ Proposed fix

 loadingPromise = (async () => {
   try {
-    const response = await fetch(MOSAIC_SCHEMA_URL);
+    const response = await fetch(MOSAIC_SCHEMA_URL, {
+      signal: AbortSignal.timeout(10000), // 10 second timeout
+    });
     if (!response.ok) {
       throw new Error(`HTTP ${response.status}: ${response.statusText}`);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/editor/mosaicSchema.ts` around lines 26 - 48, The fetch
of MOSAIC_SCHEMA_URL inside the async IIFE that assigns loadingPromise should
use an AbortSignal timeout to avoid hanging: create an AbortController (or use
AbortSignal.timeout if available), pass its signal to fetch(MOSAIC_SCHEMA_URL, {
signal }), and set a timeout to call controller.abort() (or rely on
AbortSignal.timeout) before awaiting the response; ensure you catch the abort
error in the existing catch block and still set cachedSchema fallback and clear
loadingPromise in the finally block—update references in this function to use
the controller/signal and avoid changing the existing cachedSchema/error
handling logic.

packages/mosaic/src/editor/MosaicCodeMirrorEditor.tsx (1)

33-41: Schema state is not cleared when validation is disabled.

If enableSchemaValidation transitions from true to false, the previously loaded schema remains in state but is unused. Consider clearing it to avoid confusion and potential memory overhead.

♻️ Proposed fix

 useEffect(() => {
   if (enableSchemaValidation) {
     loadMosaicSchema().then((loadedSchema) => {
       if (loadedSchema) {
         setSchema(loadedSchema);
       }
     });
+  } else {
+    setSchema(null);
   }
 }, [enableSchemaValidation]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/editor/MosaicCodeMirrorEditor.tsx` around lines 33 - 41,
When enableSchemaValidation toggles off the existing schema in state should be
cleared to avoid keeping unused data; update the useEffect that depends on
enableSchemaValidation (the one calling loadMosaicSchema) so that when
enableSchemaValidation is false you call setSchema(null or undefined) (and keep
the existing branch that loads and sets the schema when true), and ensure
loadMosaicSchema and setSchema usage remain consistent with the chosen empty
value.

packages/mosaic/src/VgPlotChart.tsx (1)

36-53: Async effect lacks cleanup handling for unmount during render.

If the component unmounts while parseSpec or astToDOM is in progress, containerRef.current may be null when replaceChildren is called (line 50 guards with ?., which is good). However, the rendered element is still created unnecessarily. Consider using an AbortSignal or cleanup flag for better resource management.

♻️ Proposed fix with cleanup

 useEffect(() => {
+  let cancelled = false;
   (async () => {
     if (containerRef.current) {
       let element: HTMLElement | SVGSVGElement;
       if (isPlotProps(props)) {
         element = props.plot;
       } else if (isSpecProps(props)) {
         const ast = await parseSpec(props.spec);
+        if (cancelled) return;
         const options = props.params ? {params: props.params} : undefined;
         element = (await astToDOM(ast, options)).element;
+        if (cancelled) return;
       } else {
         element = document.createElement('div');
         element.innerHTML = 'Error: Invalid props provided to VgPlotChart';
       }
       containerRef.current?.replaceChildren(element);
     }
   })();
+  return () => { cancelled = true; };
 }, [props]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/VgPlotChart.tsx` around lines 36 - 53, The async effect
in VgPlotChart should avoid doing work after unmount: inside the useEffect that
calls parseSpec and astToDOM, add a cleanup flag (e.g., let cancelled = false)
and, if available, pass an AbortSignal to parseSpec/astToDOM; after each await
check the flag (and signal) before creating the element or calling
containerRef.current.replaceChildren. Ensure the cleanup function sets cancelled
= true (and aborts the signal if used) so no DOM creation or expensive work
proceeds after unmount; reference the useEffect block, parseSpec, astToDOM,
containerRef, and replaceChildren when applying this change.

examples/deckgl-mosaic/src/components/filters/filterPlots.ts (2)

73-73: Unusual negative left margin.

left: -15 is atypical. If intentional (e.g., to compensate for hidden y-axis), a brief comment would help future maintainers understand the reasoning.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/deckgl-mosaic/src/components/filters/filterPlots.ts` at line 73, The
left margin in the margins object is set to an unusual negative value (margins:
{left: -15, right: 15, top: 10, bottom: 30}); if this was intentional (e.g., to
offset a hidden y-axis or align tiles), add a short inline comment next to the
left property describing the reason and the expected visual effect, otherwise
change left to a non-negative value (e.g., 0 or a small positive number) and
verify layout in the component that uses margins (the margins object in
filterPlots.ts).

---

6-10: Consider using Zod schema for ChartConfig if this becomes a reusable pattern.

The ChartConfig interface is clean for this example. If chart configurations become user-editable or serialized, consider adding a Zod schema for runtime validation. As per coding guidelines for **/*.{ts,tsx}: "Use Zod schemas for all configuration objects."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/deckgl-mosaic/src/components/filters/filterPlots.ts` around lines 6
- 10, ChartConfig is currently a TypeScript interface only; add a Zod runtime
schema to validate ChartConfig when configurations become user-editable or
serialized. Create and export a Zod schema named ChartConfigSchema (e.g., using
z.object with id: z.string(), title: z.string(), spec: appropriate schema or
z.any() if Spec can't be expressed) and use it where configs are parsed/loaded
to parse/validate inputs; update any factory/loader code that consumes
ChartConfig to call ChartConfigSchema.parse() (or safeParse) so runtime
validation is enforced.

packages/mosaic/src/chart-builders/types.ts (1)

7-16: Consider adding Zod schema for runtime validation.

Per coding guidelines, Zod schemas are recommended for configuration objects. While these interfaces work well for compile-time type checking, adding corresponding Zod schemas would enable runtime validation when processing user-provided or serialized builder configurations.

♻️ Example Zod schema addition

import {z} from 'zod';

export const MosaicChartBuilderFieldSchema = z.object({
  key: z.string(),
  label: z.string(),
  required: z.boolean().optional(),
  types: z.array(z.string()).optional(),
});

export type MosaicChartBuilderField = z.infer<typeof MosaicChartBuilderFieldSchema>;

As per coding guidelines: "Use Zod schemas for all configuration objects and infer types with z.infer<typeof ConfigSchema>"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/chart-builders/types.ts` around lines 7 - 16, Add a Zod
schema for the MosaicChartBuilderField interface so configurations can be
runtime-validated: create a MosaicChartBuilderFieldSchema using z.object(...)
that requires key and label as strings and marks required as optional boolean
and types as optional array of strings, then export type MosaicChartBuilderField
= z.infer<typeof MosaicChartBuilderFieldSchema>; update any code that imports
the interface to use the inferred type or accept the schema for validation where
builder configs are parsed.

packages/mosaic/src/chart-builders/ChartBuilderDialog.tsx (3)

60-64: Hardcoded animation delay is fragile.

The setTimeout(..., 200) assumes the dialog close animation takes exactly 200ms. If the animation duration changes, the reset could happen too early (visual glitch) or too late (wasted delay).

Consider using the dialog's onAnimationEnd callback or onOpenChange with a flag, or extracting 200 as a named constant.

♻️ Extract as named constant

+const DIALOG_CLOSE_ANIMATION_MS = 200;
+
 const handleClose = useCallback(() => {
   onOpenChange(false);
   // Reset after animation
-  setTimeout(handleReset, 200);
+  setTimeout(handleReset, DIALOG_CLOSE_ANIMATION_MS);
 }, [onOpenChange, handleReset]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/chart-builders/ChartBuilderDialog.tsx` around lines 60 -
64, The hardcoded 200ms delay in handleClose (setTimeout(handleReset, 200)) is
fragile; replace it by either listening for the dialog's animation end event or
by extracting the delay into a clearly named constant and referencing that
constant, and then call handleReset only after the animation completes; update
the handler around onOpenChange and handleReset in ChartBuilderDialog.tsx
(function handleClose) to trigger reset via an onAnimationEnd/onTransitionEnd
callback on the dialog component or use a named constant like
DIALOG_CLOSE_ANIMATION_MS instead of magic number 200 so the timing remains
correct and maintainable.

---

75-80: Implicit required field behavior.

The condition f.required !== false treats required: undefined as required. This is likely intentional (default to required), but consider documenting this behavior in the MosaicChartBuilderField interface or using an explicit default.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/chart-builders/ChartBuilderDialog.tsx` around lines 75 -
80, The current canCreate logic treats undefined required as required
implicitly; update the check to make that explicit and/or document the default:
replace the filter predicate with (f.required ?? true) so the code reads
selectedBuilder.fields.filter(f => (f.required ?? true)).every(f =>
fieldValues[f.key]); also add a short comment or JSDoc on the
MosaicChartBuilderField interface indicating that required defaults to true to
make the behavior explicit (referencing selectedBuilder, canCreate, and
MosaicChartBuilderField).

---

156-167: Consider adding a Cancel button in step 2.

When in field configuration (step 2), users can only go "Back" or "Create". A "Cancel" button would provide a clearer way to close the dialog entirely without completing the flow.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/chart-builders/ChartBuilderDialog.tsx` around lines 156 -
167, Add a Cancel button in the DialogFooter when in step 2 (i.e., when
selectedBuilder is truthy) that closes the dialog without creating anything;
place it alongside the existing Back and Create buttons and wire it to the
dialog close handler (use the component's existing onClose/close handler or add
one if missing), ensuring it does not affect canCreate logic and that
handleReset remains attached to the Back button and handleCreate to Create.

packages/mosaic/src/chart-builders/FieldSelectorInput.tsx (1)

36-42: Type filtering uses partial matching.

The includes() check will match partial type names (e.g., a filter for 'INT' would match 'INTEGER', 'BIGINT', and 'SMALLINT'). This is likely intentional for flexibility, but if exact matching is needed, consider using equality or a more precise pattern.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/chart-builders/FieldSelectorInput.tsx` around lines 36 -
42, The current filtering for filteredColumns uses partial matching via
col.type.toUpperCase().includes(t.toUpperCase()), which allows e.g. "INT" to
match "INTEGER"; change the comparison in the FieldSelectorInput filtering logic
to perform an exact case-insensitive match (e.g., compare col.type.toUpperCase()
=== t.toUpperCase()) or switch to a stricter pattern (anchored regex) so
field.types items only match exact column types; update the filter expression
around field.types, columns, and col.type accordingly.

packages/mosaic/src/chart-builders/builders.ts (1)

20-93: Consider consolidating similar builders.

countPlotBuilder and histogramBuilder are nearly identical—the only differences are maxbins (25 vs 40), yLabel (null vs 'Count'), and left margin (0 vs 40). Consider a shared factory function to reduce duplication.

♻️ Example refactor using a shared factory

const createHistogramBuilder = (
  id: string,
  description: string,
  maxbins: number,
  yLabel: string | null,
  leftMargin: number,
): MosaicChartBuilder => ({
  id,
  icon: id === 'count-plot' ? BarChartHorizontal : BarChart3,
  description,
  fields: [{key: 'field', label: 'Field', required: true}],
  createSpec: (tableName, {field}): Spec => ({
    // ... shared implementation with parameters
  }) as Spec,
});

const countPlotBuilder = createHistogramBuilder('count-plot', 'Create a count plot of a field', 25, null, 0);
const histogramBuilder = createHistogramBuilder('histogram', 'Create a histogram of a field', 40, 'Count', 40);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/chart-builders/builders.ts` around lines 20 - 93, Both
builders (countPlotBuilder and histogramBuilder) duplicate the same createSpec
logic; extract a shared factory (e.g., createHistogramBuilder) that accepts
parameters for id, icon, description, maxbins, yLabel, and leftMargin and
returns a MosaicChartBuilder; inside the factory implement the common plot array
(reusing BG_COLOR, FG_COLOR, inset, params, and select names) and use it to
construct countPlotBuilder and histogramBuilder by passing (25, null, 0) and
(40, 'Count', 40) respectively, keeping fields and createSpec signatures
identical.

examples/deckgl-mosaic/src/components/filters/FiltersPanel.tsx (1)

33-34: Use memoized selector references for all useRoomStore calls in this file.

This file has four inline selector lambdas that should be replaced with stable memoized selectors. Define them at module level (outside components) for reuse.

♻️ Proposed refactor

+const selectMosaicConnection = (state: ReturnType<typeof useRoomStore.getState>) =>
+  state.mosaic.connection;
+const selectIsTableReady = (state: ReturnType<typeof useRoomStore.getState>) =>
+  state.db.tables.find((t) => t.tableName === 'earthquakes');
+const selectBrush = (state: ReturnType<typeof useRoomStore.getState>) =>
+  state.mosaic.getSelection('brush');
+const selectTables = (state: ReturnType<typeof useRoomStore.getState>) =>
+  state.db.tables;
+
 export default function FiltersPanel({className}: {className?: string}) {
-  const mosaicConn = useRoomStore((state) => state.mosaic.connection);
-  const isTableReady = useRoomStore((state) =>
-    state.db.tables.find((t) => t.tableName === 'earthquakes'),
-  );
+  const mosaicConn = useRoomStore(selectMosaicConnection);
+  const isTableReady = useRoomStore(selectIsTableReady);
   // ...
 }

 const FiltersPanelContent = ({className}: {className?: string}) => {
-  const brush = useRoomStore((state) => state.mosaic.getSelection('brush'));
-  const tables = useRoomStore((state) => state.db.tables);
+  const brush = useRoomStore(selectBrush);
+  const tables = useRoomStore(selectTables);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/deckgl-mosaic/src/components/filters/FiltersPanel.tsx` around lines
33 - 34, The inline selector lambdas passed to useRoomStore (e.g., the ones that
produce brush via state.mosaic.getSelection('brush') and tables via
state.db.tables) should be replaced by stable, module-level memoized selector
functions; define named selectors (for example selectBrush, selectTables and the
other two selectors used in this file) outside the FiltersPanel component and
reuse them in the four useRoomStore calls so the selector references are stable
across renders and avoid unnecessary re-subscriptions.

packages/mosaic/src/editor/useMosaicChartEditor.ts (1)

153-188: ⚠️ Potential issue | 🟠 Major

applyChanges can commit stale spec if invoked before debounce settles.

When the user edits JSON and clicks Apply within 300ms (before the debounce timer fires), applyChanges reads from debouncedParsed which still holds the previous parsed spec. This can cause the applied spec to differ from what the user sees in the editor.

Consider parsing directly from editedSpecString in applyChanges to ensure the latest editor content is applied.

🐛 Proposed fix

-  const applyChanges = useCallback(() => {
-    setDebouncedParsed((currentParsed) => {
-      if (!currentParsed.parsedSpec) return currentParsed;
-
-      const newAppliedSpecString = JSON.stringify(currentParsed.parsedSpec);
+  const applyChanges = useCallback(() => {
+    let parsed: Spec;
+    try {
+      parsed = JSON.parse(editorState.editedSpecString) as Spec;
+    } catch {
+      return; // Can't apply invalid JSON
+    }
+    const newAppliedSpecString = JSON.stringify(parsed);

       setEditorState((prev) => {
         const dirty = (() => {
           try {
             return (
               JSON.stringify(JSON.parse(prev.editedSpecString)) !==
               prev.appliedSpecString
             );
           } catch {
             return true;
           }
         })();

         if (dirty) {
-          onSpecChangeRef.current?.(currentParsed.parsedSpec!);
+          onSpecChangeRef.current?.(parsed);
         }

         return {
           ...prev,
           appliedSpecString: dirty
             ? newAppliedSpecString
             : prev.appliedSpecString,
           prevInitialSpecString: dirty
             ? newAppliedSpecString
             : prev.prevInitialSpecString,
         };
       });
-
-      return currentParsed;
-    });
-  }, []);
+  }, [editorState.editedSpecString]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/editor/useMosaicChartEditor.ts` around lines 153 - 188,
The applyChanges function currently reads from debouncedParsed and can apply a
stale spec; instead, synchronously parse the latest editedSpecString inside
applyChanges (use editorState.editedSpecString or a ref to it), handle JSON
parse errors (abort/apply nothing on parse failure), then compute
newAppliedSpecString = JSON.stringify(parsed) and call
onSpecChangeRef.current(parsed) if dirty, and update setEditorState to set
appliedSpecString and prevInitialSpecString to newAppliedSpecString; keep the
existing dirty-detection logic but compare against prev.appliedSpecString using
the freshly parsed string so the exact latest editor content is applied even if
debounce hasn't settled.

packages/mosaic/src/editor/useMosaicChartEditor.ts (1)

127-129: State update during render may cause extra re-renders.

Calling setEditorState during render (outside of an effect) can lead to cascading re-renders. While React handles this gracefully for derived state patterns, it's cleaner to use useEffect for this synchronization.

♻️ Consider moving to useEffect

+  useEffect(() => {
+    if (parsedSpec && parsedSpec !== editorState.lastValidSpec) {
+      setEditorState((prev) => ({...prev, lastValidSpec: parsedSpec}));
+    }
+  }, [parsedSpec, editorState.lastValidSpec]);
-
-  if (parsedSpec && parsedSpec !== editorState.lastValidSpec) {
-    setEditorState((prev) => ({...prev, lastValidSpec: parsedSpec}));
-  }
   const lastValidSpec = editorState.lastValidSpec;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mosaic/src/editor/useMosaicChartEditor.ts` around lines 127 - 129,
The code updates editorState.lastValidSpec during render when parsedSpec
differs, which can cause extra re-renders; move this synchronization into a
useEffect: create an effect that runs when parsedSpec (and optionally
editorState.lastValidSpec) changes, compare parsedSpec to
editorState.lastValidSpec inside the effect and call setEditorState(prev =>
({...prev, lastValidSpec: parsedSpec})) only when they differ; ensure the effect
dependency list includes parsedSpec (and editorState.lastValidSpec or use a
functional updater to avoid stale reads) so the update happens after render
rather than during it.