feat: dashboard charts settings - #581

packages/mosaic/src/dashboard/chart-settings/ChartSettingsContext.tsx (1)

41-47: ⚡ Quick win

Inline value object creates a new reference on every render, causing all context consumers to re-render unnecessarily.

Stabilize the context value with useMemo so consumers only re-render when the inputs actually change.

♻️ Proposed refactor

-import {createContext, type ReactNode, useContext} from 'react';
+import {createContext, type ReactNode, useContext, useMemo} from 'react';

 export function ChartSettingsProvider({
   tableName,
   config,
   columns,
   onChange,
   children,
 }: ChartSettingsProviderProps) {
+  const value = useMemo(
+    () => ({tableName, config, columns, onChange}),
+    [tableName, config, columns, onChange],
+  );
   return (
-    <ChartSettingsContext.Provider
-      value={{tableName, config, columns, onChange}}
-    >
+    <ChartSettingsContext.Provider value={value}>
       {children}
     </ChartSettingsContext.Provider>
   );
 }

As per coding guidelines, "Keep the compound context/provider layer thin and stable."

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/dashboard/chart-settings/ChartSettingsContext.tsx` around
lines 41 - 47, The Provider is passing an inline object to
ChartSettingsContext.Provider causing a new reference each render; wrap the
value in React.useMemo and return {tableName, config, columns, onChange} inside
it so the object identity is stable, and use [tableName, config, columns,
onChange] (or the minimal real dependencies) as the dependency array; update the
ChartSettingsContext.Provider to pass this memoizedValue instead of the inline
object so consumers only re-render when those inputs actually change.

packages/mosaic/src/dashboard/MosaicDashboardPanelLayout.tsx (1)

31-41: 💤 Low value

onResize should be wrapped in useCallback for stable reference.

A new function is allocated every render and passed as the onResize prop, triggering unnecessary reconciliation inside ResizablePanel.

♻️ Proposed refactor

-  const onResize = () => {
+  const onResize = React.useCallback(() => {
     const isCollapsed = panelRef.current?.isCollapsed();
 
     if (isCollapsed && isOpen) {
       onIsOpenChange?.(false);
     }
 
     if (!isCollapsed && !isOpen) {
       onIsOpenChange?.(true);
     }
-  };
+  }, [isOpen, onIsOpenChange]);

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/dashboard/MosaicDashboardPanelLayout.tsx` around lines 31
- 41, The onResize function is recreated each render causing unstable props to
ResizablePanel; wrap the handler in React.useCallback to memoize it (referencing
panelRef.current, isOpen, and onIsOpenChange) so its identity remains stable
across renders. Update the onResize declaration (used as prop to ResizablePanel)
to use useCallback and include only the necessary dependencies (panelRef,
isOpen, onIsOpenChange) to avoid stale closures while preventing unnecessary
re-renders.

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

9-14: ⚡ Quick win

Extract FG_COLOR and titleFromDescription to chartTypeUtils.ts — duplicated across every chart-type definition.

titleFromDescription (lines 11-14) and FG_COLOR (line 9) are copy-pasted verbatim into ecdf/definition.ts, box-plot/definition.ts, heatmap/definition.ts, and (based on context) histogram/definition.ts. Moving them to chartTypeUtils.ts eliminates the risk of diverging quietly.

♻️ Proposed refactor

In chartTypeUtils.ts:

+export const FG_COLOR = 'var(--color-chart-1)';
+
+export function titleFromDescription(description: string) {
+  return (fieldValues: Record<string, string>) =>
+    buildDefaultChartTitle(description, fieldValues);
+}

Then in each definition file:

 import {
   buildDefaultChartTitle,
+  FG_COLOR,
+  titleFromDescription,
   QUANTITATIVE_COLUMN_TYPES,
 } from '../../chart-builders/chartTypeUtils';
-
-const FG_COLOR = 'var(--color-chart-1)';
-
-function titleFromDescription(description: string) {
-  return (fieldValues: Record<string, string>) =>
-    buildDefaultChartTitle(description, fieldValues);
-}

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/chart-types/ecdf/definition.ts` around lines 9 - 14, The
FG_COLOR constant and titleFromDescription function are duplicated across
chart-type definitions; extract them into a new exported helpers module
(chartTypeUtils.ts), export FG_COLOR and titleFromDescription (which should call
buildDefaultChartTitle as before), then replace the local declarations in each
chart-type definition (ecdf, box-plot, heatmap, histogram) with imports from
chartTypeUtils.ts so all files use the single shared FG_COLOR and
titleFromDescription.

packages/mosaic/src/chart-types/custom-spec/definition.ts (1)

6-9: ⚡ Quick win

titleFromDescription is the same function defined in line-chart/definition.ts — consolidate into chartTypeUtils.

See the extraction suggestion on line-chart/definition.ts.

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/chart-types/custom-spec/definition.ts` around lines 6 -
9, The titleFromDescription function is duplicated; extract it into the shared
utility module chartTypeUtils (create and export a function
titleFromDescription(description: string): (fieldValues: Record<string,string>)
=> string that delegates to buildDefaultChartTitle), update both usages in
custom-spec/definition.ts and line-chart/definition.ts to import the shared
titleFromDescription from chartTypeUtils, and remove the duplicate definition
from each file so only the exported utility remains.

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

15-20: ⚡ Quick win

useCallback([onChange]) provides no stability when callers pass an inline onChange.

Both current call sites create onChange as an inline arrow on every render (e.g., (key, value) => setFieldValue(key, value as string) in ChartBuilderFields.tsx). A single value that is "always new" is enough to break memoization for an entire component. Since onChange is unstable at both call sites, handleFieldChange will be a new reference on every render regardless of the useCallback.

Either require callers to stabilize onChange before passing it in (document this contract), or drop the useCallback wrapper since it adds overhead without benefit in the current usage pattern.

♻️ Option A — drop the wrapper (honest, no false stability promise)

 export function useChartFieldForm({
   fields,
   values,
   onChange,
 }: UseChartFieldFormProps) {
-  const handleFieldChange = useCallback(
-    (fieldKey: string, value: unknown) => {
-      onChange(fieldKey, value);
-    },
-    [onChange],
-  );
-
   return {
     fields,
     values,
-    handleFieldChange,
+    handleFieldChange: onChange,
   };
 }

♻️ Option B — callers stabilize onChange (keeps the hook's contract)

In ChartBuilderFields.tsx, wrap with useCallback:

+const handleChange = useCallback(
+  (key: string, value: unknown) => setFieldValue(key, value as string),
+  [setFieldValue],
+);
 const {fields, handleFieldChange} = useChartFieldForm({
   fields: selectedTemplate?.fields || [],
   values: fieldValues,
-  onChange: (key: string, value: unknown) =>
-    setFieldValue(key, value as string),
+  onChange: handleChange,
 });

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/chart-builders/hooks/useChartFieldForm.ts` around lines
15 - 20, The useCallback around handleFieldChange in useChartFieldForm is
ineffective because callers pass an inline unstable onChange; remove the
misleading memoization by replacing the useCallback wrapper with a plain
function export (i.e., change handleFieldChange in useChartFieldForm to a direct
function that calls onChange), or alternatively stabilize the caller-side
onChange (wrap the inline (key, value) => setFieldValue(...) in
ChartBuilderFields.tsx with useCallback) if you want to preserve the hook's
memoization contract—pick one approach and apply consistently to
handleFieldChange and its caller(s).

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

12-15: ⚡ Quick win

titleFromDescription is duplicated across chart-type definitions — extract it to chartTypeUtils.ts.

The same helper is defined verbatim in custom-spec/definition.ts (and possibly other chart-type definitions). Since both files already import buildDefaultChartTitle from chartTypeUtils, the natural home is to export titleFromDescription from there.

♻️ Suggested extraction

In packages/mosaic/src/chart-builders/chartTypeUtils.ts, add:

+export function titleFromDescription(description: string) {
+  return (fieldValues: Record<string, string>) =>
+    buildDefaultChartTitle(description, fieldValues);
+}

Then in each definition file:

 import {
   buildDefaultChartTitle,
+  titleFromDescription,
   QUANTITATIVE_COLUMN_TYPES,
   NUMERIC_COLUMN_TYPES,
 } from '../../chart-builders/chartTypeUtils';

-function titleFromDescription(description: string) {
-  return (fieldValues: Record<string, string>) =>
-    buildDefaultChartTitle(description, fieldValues);
-}

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/chart-types/line-chart/definition.ts` around lines 12 -
15, The helper titleFromDescription is duplicated across chart-type definitions;
extract and export it from the shared chartTypeUtils module where
buildDefaultChartTitle already lives, then remove the duplicate definitions.
Specifically, add an exported function titleFromDescription(description: string)
that returns (fieldValues) => buildDefaultChartTitle(description, fieldValues)
in the chartTypeUtils module, and in each chart definition (e.g., the line-chart
definition that currently declares titleFromDescription) replace the local
function with an import of the shared titleFromDescription and remove the
redundant code.

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

9-15: ⚡ Quick win

Extract shared chart-definition helpers to chartTypeUtils.ts.

BG_COLOR, FG_COLOR, and titleFromDescription are identically defined in both histogram/definition.ts (lines 9–15) and count-plot/definition.ts (lines 9–15), and presumably in all 8 chart-type definition files. With buildDefaultChartTitle already living in chartTypeUtils.ts, the curried wrapper belongs there too.

♻️ Suggested extraction

In packages/mosaic/src/chart-builders/chartTypeUtils.ts, add:

+export const CHART_BG_COLOR = 'var(--color-chart-overlay)';
+export const CHART_FG_COLOR = 'var(--color-chart-1)';
+
+export function titleFromDescription(description: string) {
+  return (fieldValues: Record<string, string>) =>
+    buildDefaultChartTitle(description, fieldValues);
+}

Then in each definition file:

-const BG_COLOR = 'var(--color-chart-overlay)';
-const FG_COLOR = 'var(--color-chart-1)';
-
-function titleFromDescription(description: string) {
-  return (fieldValues: Record<string, string>) =>
-    buildDefaultChartTitle(description, fieldValues);
-}
 import {
   buildDefaultChartTitle,
+  CHART_BG_COLOR as BG_COLOR,
+  CHART_FG_COLOR as FG_COLOR,
+  titleFromDescription,
   QUANTITATIVE_COLUMN_TYPES,
 } from '../../chart-builders/chartTypeUtils';

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/chart-types/histogram/definition.ts` around lines 9 - 15,
BG_COLOR, FG_COLOR, and titleFromDescription are duplicated across multiple
chart definition files; move these shared helpers into chartTypeUtils.ts and
import them from there. Specifically, add BG_COLOR and FG_COLOR constants and a
titleFromDescription(description: string) curried wrapper that calls
buildDefaultChartTitle in packages/mosaic/src/chart-builders/chartTypeUtils.ts,
then remove the duplicated definitions from histogram/definition.ts (and other
chart definition files) and replace them with imports of BG_COLOR, FG_COLOR, and
titleFromDescription from chartTypeUtils.ts.

packages/mosaic/src/chart-types/count-plot/definition.ts (1)

9-15: ⚡ Quick win

Same duplication as histogram/definition.ts.

BG_COLOR, FG_COLOR, and titleFromDescription are byte-for-byte identical to lines 9–15 in histogram/definition.ts. See the suggested extraction comment on that file.

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/chart-types/count-plot/definition.ts` around lines 9 -
15, BG_COLOR, FG_COLOR and titleFromDescription are duplicated; extract them
into a shared module (e.g., export constants BG_COLOR, FG_COLOR and a function
titleFromDescription from a new/shared helper like chartConstants or
chartTitleUtils) and replace the local definitions in both count-plot and
histogram definitions with imports of those symbols; update usages to reference
the imported BG_COLOR, FG_COLOR and titleFromDescription so both files reuse the
single implementation.

packages/mosaic/src/dashboard/MosaicDashboard.tsx (1)

106-124: ⚡ Quick win

Narrow handleAddDefaultChart deps to dashboard?.selectedTable to avoid context churn.

The callback only reads dashboard?.selectedTable, but dashboard (the full object) is in the deps array. Since addPanel mutates dashboard, every panel addition recreates handleAddDefaultChart, which then re-evaluates contextValue (line 148), re-rendering all context consumers. Compare with handleCreateChart at line 103, which correctly scopes to dashboard?.selectedTable.

🔧 Proposed fix

-  const handleAddDefaultChart = useCallback(() => {
-    if (!dashboard?.selectedTable) {
-      return;
-    }
-
-    // Create empty chart panel - user will select type and fields
-    const panel = createMosaicDashboardVgPlotPanelConfig(
-      'New Chart',
-      {
-        chartType: 'histogram', // Default type, but with empty settings
-        settings: {},
-        vgplot: null,
-        settingsOpen: true, // Open settings by default
-      },
-      {tableName: dashboard.selectedTable},
-    );
-
-    addPanel(dashboardId, panel);
-  }, [addPanel, dashboard, dashboardId]);
+  const handleAddDefaultChart = useCallback(() => {
+    if (!dashboard?.selectedTable) {
+      return;
+    }
+
+    // Create empty chart panel - user will select type and fields
+    const panel = createMosaicDashboardVgPlotPanelConfig(
+      'New Chart',
+      {
+        chartType: 'histogram',
+        settings: {},
+        vgplot: null,
+        settingsOpen: true,
+      },
+      {tableName: dashboard?.selectedTable},
+    );
+
+    addPanel(dashboardId, panel);
+  }, [addPanel, dashboard?.selectedTable, dashboardId]);

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/dashboard/MosaicDashboard.tsx` around lines 106 - 124,
The handleAddDefaultChart callback closes over the whole dashboard object
causing unnecessary re-creations; change its dependency array to only include
the specific values it reads (dashboard?.selectedTable, addPanel, dashboardId)
by replacing "dashboard" with "dashboard?.selectedTable" so that
handleAddDefaultChart only re-runs when the selected table changes; keep the
rest of the implementation the same and ensure references to
createMosaicDashboardVgPlotPanelConfig and addPanel remain unchanged.

packages/mosaic/__tests__/generateMosaicChartSpec.test.ts (1)

101-126: ⚡ Quick win

Consider using jest.spyOn instead of direct registry mutation.

Direct mutation of mosaicChartTypes.histogram is fragile—if an assertion fails before line 124, the restore never runs, potentially breaking subsequent tests. Using jest.spyOn with mockRestore() in a finally block or afterEach is safer.

♻️ Proposed safer pattern

   it('handles createSpec throwing an error', () => {
     const consoleSpy = jest.spyOn(console, 'error').mockImplementation();
+    const createSpecSpy = jest.spyOn(mosaicChartTypes.histogram, 'createSpec')
+      .mockImplementation(() => {
+        throw new Error('Test error');
+      });

-    // Mock a chart type that throws
-    const originalHistogram = mosaicChartTypes.histogram;
-    mosaicChartTypes.histogram = {
-      ...originalHistogram,
-      createSpec: () => {
-        throw new Error('Test error');
-      },
-    };
-
     const result = generateMosaicChartSpec('sales', 'histogram', {
       field: 'amount',
     });

     expect(result).toBeNull();
     expect(consoleSpy).toHaveBeenCalledWith(
       expect.stringContaining('Failed to generate spec'),
       expect.any(Error),
     );

-    // Restore
-    mosaicChartTypes.histogram = originalHistogram;
+    createSpecSpy.mockRestore();
     consoleSpy.mockRestore();
   });

🤖 Prompt for AI Agents

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

In `@packages/mosaic/__tests__/generateMosaicChartSpec.test.ts` around lines 101 -
126, The test directly mutates mosaicChartTypes.histogram which is fragile;
instead spy on the registry entry and restore it reliably. Replace the direct
assignment to mosaicChartTypes.histogram with jest.spyOn(mosaicChartTypes,
'histogram') (or spy on the createSpec method) to mock createSpec to throw, call
generateMosaicChartSpec('sales','histogram', {...}) as before, and ensure you
call mockRestore() in a finally block or in afterEach so the original
mosaicChartTypes.histogram is always restored; reference the
mosaicChartTypes.histogram symbol, the createSpec method,
generateMosaicChartSpec, jest.spyOn, and mockRestore when making the change.

packages/mosaic/src/dashboard/chart-settings/DynamicChartSettings.tsx (1)

16-48: ⚡ Quick win

Inline callback inside memo causes unnecessary re-renders.

The onChange callback on line 21-23 creates a new function reference on every render, which propagates through useChartFieldForm and triggers child re-renders, partially defeating memo(). Wrap it with useCallback.

♻️ Proposed fix

-export const DynamicChartSettings: FC<DynamicChartSettingsProps> = memo(
-  ({chartTypeDefinition, columns, values, onChange}) => {
+export const DynamicChartSettings: FC<DynamicChartSettingsProps> = memo(
+  ({chartTypeDefinition, columns, values, onChange}) => {
+    const handleChange = useCallback(
+      (key: string, value: unknown) => {
+        onChange({...values, [key]: value});
+      },
+      [onChange, values],
+    );
+
     const {fields, handleFieldChange} = useChartFieldForm({
       fields: chartTypeDefinition.fields,
       values,
-      onChange: (key, value) => {
-        onChange({...values, [key]: value});
-      },
+      onChange: handleChange,
     });

Note: You'll need to add useCallback to the import on line 1.

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/dashboard/chart-settings/DynamicChartSettings.tsx` around
lines 16 - 48, The inline onChange passed into useChartFieldForm inside
DynamicChartSettings creates a new function each render and defeats memoization;
wrap that callback in useCallback (and add useCallback to the imports) so the
function reference is stable before calling useChartFieldForm — e.g. create a
memoized const onFieldChange = useCallback((key, value) => onChange({...values,
[key]: value}), [onChange, values]) and pass onFieldChange to useChartFieldForm;
keep the rest (fields, handleFieldChange, FieldSelectorInput usage) unchanged.

packages/mosaic/src/dashboard/chart-settings/ChartSettings.tsx (1)

55-76: 💤 Low value

Consider wrapping handleChartTypeChange in useCallback.

The handler creates a new function reference on every render. If ChartTypeSelector is memoized or passes this to memoized children, this could cause unnecessary re-renders.

♻️ Proposed fix

 const ChartSettingsTypeSelector: FC = () => {
   const {config, columns, onChange} = useChartSettingsContext();

-  const handleChartTypeChange = (newChartType: VgPlotChartType) => {
+  const handleChartTypeChange = useCallback((newChartType: VgPlotChartType) => {
     // When changing chart type, clear settings and don't show chart
     // until user selects all required fields
     onChange({
       chartType: newChartType,
       settings: {},
       vgplot: null,
       settingsOpen: config.settingsOpen,
     });
-  };
+  }, [config.settingsOpen, onChange]);

   return (

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/dashboard/chart-settings/ChartSettings.tsx` around lines
55 - 76, The handler handleChartTypeChange in ChartSettingsTypeSelector should
be memoized with React's useCallback to avoid creating a new function reference
each render; wrap the existing function in useCallback and include the correct
dependency array (at minimum onChange and config.settingsOpen) so it only
changes when those values change, ensure useCallback is imported from React, and
keep the function body unchanged (still calling onChange to reset chartType,
settings, vgplot, and settingsOpen) so ChartTypeSelector receives a stable
onChange prop.

packages/mosaic/src/dashboard/MosaicDashboardVgPlotPanelRenderer.tsx (1)

18-34: 💤 Low value

Simplify toRenderableMosaicSpec by removing unnecessary try-catch.

The only operations inside the try block (type checking, destructuring, returning) cannot throw. The defensive try-catch adds complexity without catching real errors.

♻️ Simplified version

 function toRenderableMosaicSpec(vgplot: unknown): Spec | null {
-  try {
-    if (!vgplot || typeof vgplot !== 'object' || Array.isArray(vgplot)) {
-      return null;
-    }
-
-    const vgplotRecord = vgplot as Spec;
-
-    // eslint-disable-next-line `@typescript-eslint/no-unused-vars`
-    const {$schema, ...mosaicSpec} = vgplotRecord;
-
-    return mosaicSpec;
-  } catch (error) {
-    console.error('[toRenderableMosaicSpec] Failed to parse spec:', error);
+  if (!vgplot || typeof vgplot !== 'object' || Array.isArray(vgplot)) {
     return null;
   }
+
+  // eslint-disable-next-line `@typescript-eslint/no-unused-vars`
+  const {$schema, ...mosaicSpec} = vgplot as Spec;
+
+  return mosaicSpec;
 }

🤖 Prompt for AI Agents

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

In `@packages/mosaic/src/dashboard/MosaicDashboardVgPlotPanelRenderer.tsx` around
lines 18 - 34, Remove the unnecessary try-catch in toRenderableMosaicSpec: keep
the input validation (check for falsy, non-object, or array) and the
destructuring that strips $schema from the vgplotRecord, then directly return
mosaicSpec (typed as Spec | null). Update the function body in
MosaicDashboardVgPlotPanelRenderer.tsx to eliminate the try/catch block and
associated catch logging while preserving the early return of null on invalid
input and the cast to Spec for vgplotRecord.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/mosaic/src/dashboard/MosaicDashboardPanelLayout.tsx (2)

29-39: ⚡ Quick win

Memoize onResize with useCallback to avoid passing a new function reference on every render.

onResize is recreated on every render and passed as a prop to ResizablePanel. Wrap it in useCallback with [isOpen, onIsOpenChange] as dependencies.

♻️ Proposed fix

-const onResize = () => {
+const onResize = React.useCallback(() => {
   const isCollapsed = panelRef.current?.isCollapsed();

   if (isCollapsed && isOpen) {
     onIsOpenChange?.(false);
   }

   if (!isCollapsed && !isOpen) {
     onIsOpenChange?.(true);
   }
-};
+}, [isOpen, onIsOpenChange]);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mosaic/src/dashboard/MosaicDashboardPanelLayout.tsx` around lines 29
- 39, The onResize handler is recreated each render and should be memoized: wrap
the onResize function in React's useCallback with dependencies [isOpen,
onIsOpenChange] so the same function reference is passed to ResizablePanel;
inside the memoized callback keep the existing logic that reads
panelRef.current?.isCollapsed() and calls onIsOpenChange?.(false) or
onIsOpenChange?.(true) based on isCollapsed and isOpen.

---

21-27: ⚡ Quick win

Guard expand()/collapse() with a current-state check to avoid redundant calls.

The effect unconditionally calls expand() or collapse() even when the panel is already in the desired state. This can fire onResize unnecessarily (e.g., when the user manually drags the panel, which already updates isOpen via onResize → onIsOpenChange, and then the effect re-runs and calls collapse() on an already-collapsed panel, triggering onResize a second time). The safe pattern used elsewhere in the codebase guards on current state first:

// Context: packages/layout/src/node-renderers/split-node-renderer/CollapsiblePanelWrapper.tsx
useEffect(() => {
  if (collapsed && !handle.isCollapsed()) {
    handle.collapse();
  } else if (!collapsed && handle.isCollapsed()) {
    handle.expand();
  }
}, [collapsed]);

♻️ Proposed fix

 useEffect(() => {
-  if (isOpen) {
-    panelRef.current?.expand();
-  } else {
-    panelRef.current?.collapse();
+  if (isOpen && panelRef.current?.isCollapsed()) {
+    panelRef.current.expand();
+  } else if (!isOpen && !panelRef.current?.isCollapsed()) {
+    panelRef.current?.collapse();
   }
 }, [isOpen]);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mosaic/src/dashboard/MosaicDashboardPanelLayout.tsx` around lines 21
- 27, The effect in MosaicDashboardPanelLayout.tsx currently calls
panelRef.current?.expand() or ?.collapse() unconditionally; change it to first
read the panel's current state (e.g., panelRef.current?.isCollapsed()) and only
call expand() when isOpen is true and isCollapsed() is true, or call collapse()
when isOpen is false and isCollapsed() is false, to avoid redundant calls and
extra onResize triggers; update the useEffect that references isOpen and
panelRef to perform that guarded check before invoking expand()/collapse().

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

15-29: ⚡ Quick win

Preserve TSettings through exported aliases/helpers

The new generic ChartTypeDefinition<TSettings> is currently collapsed to default any in this adapter layer. Threading generics here keeps settings typing intact for downstream callers.

Suggested patch

-export interface ChartBuilderTemplate extends ChartTypeDefinition {
+export interface ChartBuilderTemplate<TSettings = any>
+  extends ChartTypeDefinition<TSettings> {
   /** Icon component */
   icon: ComponentType<{className?: string}>;
 }
@@
-export type ChartSpec = ChartTypeDefinition;
+export type ChartSpec<TSettings = any> = ChartTypeDefinition<TSettings>;
@@
 export function toChartTypeDefinition(
-  template: ChartBuilderTemplate,
-): ChartTypeDefinition {
+  template: ChartBuilderTemplate<TSettings>,
+): ChartTypeDefinition<TSettings> {
@@
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mosaic/src/chart-builders/types.ts` around lines 15 - 29, The
exported types and helper should preserve the ChartTypeDefinition generic
parameter so TSettings isn't lost: make ChartBuilderTemplate generic
(ChartBuilderTemplate<TSettings> extends ChartTypeDefinition<TSettings>) and
update ChartSpec to be a generic alias (ChartSpec<TSettings> =
ChartTypeDefinition<TSettings>), then change toChartTypeDefinition to accept
template: ChartBuilderTemplate<TSettings> and return
ChartTypeDefinition<TSettings>; update any internal references to use the
generic TSettings so downstream callers keep the settings type.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes