Repository navigation
feat: charts refactoring - #673
Merged
Merged
Conversation
Co-authored-by: Copilot <[email protected]>
Contributor
|
Warning Review limit reached
More reviews will be available in 15 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRepo-wide refactor: chart-types moved under charts/, new Mosaic chart components/hooks and worksheet block renderers added, dashboard text panels removed, AI tools consolidated to ai/ai.ts, DataTableExplorer/table selection APIs updated, and many consumers/tests adapted. ChangesMosaic charts refactor, dashboard cleanup, and AI consolidation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
packages/mosaic/src/editor/MosaicChartContainer.tsx (1)
49-52: 💤 Low valueJSDoc example is inconsistent/incorrect.
Two issues in the example: the closing tag on line 52 still reads
</MosaicChart.Container>(should beMosaicSpecChart.Container), and line 49 usesMosaicSpecChart.Chart, which isn't a member of the compound — the display subcomponent isMosaicSpecChart.Display.📝 Proposed doc fix
- * <MosaicSpecChart.Chart /> + * <MosaicSpecChart.Display /> * <MosaicSpecChart.SpecEditor /> * <MosaicSpecChart.Actions /> - * </MosaicChart.Container> + * </MosaicSpecChart.Container>🤖 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/editor/MosaicChartContainer.tsx` around lines 49 - 52, The JSDoc usage example incorrectly references MosaicSpecChart.Chart and closes with MosaicChart.Container; update the example to use the correct subcomponent names and container: replace MosaicSpecChart.Chart with MosaicSpecChart.Display, ensure the other members (MosaicSpecChart.SpecEditor and MosaicSpecChart.Actions) remain, and change the closing tag to </MosaicSpecChart.Container> so the example uses MosaicSpecChart.Container, MosaicSpecChart.Display, MosaicSpecChart.SpecEditor, and MosaicSpecChart.Actions consistently.apps/sqlrooms-cli-ui/src/workspace/WorksheetChartRenderer.tsx (1)
46-46: 💤 Low valueRemove the commented-out
fallbackFieldline.Leftover debug artifact;
fallbackFieldis unused now.🧹 Proposed cleanup
- // const fallbackField = fallbackTable?.columns?.[0]?.name; const effectiveTableName = tableName || fallbackTable?.table.table || '';🤖 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 `@apps/sqlrooms-cli-ui/src/workspace/WorksheetChartRenderer.tsx` at line 46, Remove the leftover commented-out debug line referencing fallbackField in WorksheetChartRenderer.tsx; locate the commented line "const fallbackField = fallbackTable?.columns?.[0]?.name;" in the WorksheetChartRenderer component and delete it so no unused/commented artifact remains in the file.packages/mosaic/__tests__/dashboard-ai.test.ts (1)
1-9: ⚡ Quick winKeep this suite on the public AI entrypoint.
Importing from
../src/ai/aibypasses the newpackages/mosaic/src/ai.tsbarrel, so a broken re-export in this PR would no longer be caught here. Prefer../src/ai, or add a tiny barrel smoke test, to keep the package contract covered.🤖 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/__tests__/dashboard-ai.test.ts` around lines 1 - 9, The test imports directly from the internal module '../src/ai/ai' which bypasses the package barrel; update the import source to the public entry '../src/ai' (or add a tiny barrel smoke test) so the package contract is validated — change the import for createDashboardAiTools, createDashboardToolDeps, DASHBOARD_AGENT_INSTRUCTIONS, DASHBOARD_AI_INSTRUCTIONS, MAP_TOOL_KEY, and the types DashboardAiAdapter and DashboardAiTable to come from '../src/ai' instead of '../src/ai/ai'.packages/mosaic/src/ai/ai.ts (1)
319-324: ⚡ Quick winPrefer a tagged
Errorhelper over a custom class here.This module is otherwise functional, and the dedicated class only exists to branch in the catch path. A small helper keeps the same behavior without introducing class-based control flow. As per coding guidelines, "Use object-oriented functional programming style over classes in JavaScript/TypeScript".
🤖 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/ai/ai.ts` around lines 319 - 324, Replace the custom DashboardAgentException class with a small tagged-error helper: implement a factory like createDashboardAgentError(message: string): Error that returns a new Error with error.name = 'DashboardAgentException' (and optionally an isDashboardAgentError(err): boolean that checks err?.name === 'DashboardAgentException'), then update any throw sites that currently do new DashboardAgentException(...) to call createDashboardAgentError(...) and update catch branches that rely on instanceof DashboardAgentException to instead check isDashboardAgentError(err) or err.name === 'DashboardAgentException'; keep symbol names DashboardAgentException (as the tag string), createDashboardAgentError and isDashboardAgentError to make the changes easy to locate.packages/mosaic/src/charts/MosaicChartView.tsx (1)
50-54: ⚡ Quick winReword selector concern:
getPanelIssueByKeyis a direct stored lookup
getPanelIssueByKey(runtimeKey)inpackages/mosaic/src/dashboard/MosaicDashboardSlice.tsxreturnsget().mosaicDashboard.runtime.panelIssuesByPanelId[runtimeKey](no per-call object construction), so the “fresh value each render” concern is unlikely.- The selector will change only when the underlying
panelIssuesByPanelId[runtimeKey]entry reference changes; the suggesteduseMemorefactor is optional for style/consistency with “select raw state first”.🤖 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/charts/MosaicChartView.tsx` around lines 50 - 54, Update the review comment and/or code comment to clarify that getPanelIssueByKey(runtimeKey) performs a direct lookup (it returns get().mosaicDashboard.runtime.panelIssuesByPanelId[runtimeKey]) so it does not construct a new object per call, and therefore the selector used with useStoreWithMosaicDashboard will only change when that panelIssuesByPanelId[runtimeKey] reference changes; state that wrapping the result in useMemo is optional and purely stylistic for consistency with “select raw state first” if desired.packages/mosaic/src/charts/chart-settings/MosaicChartSpecViewerPanel.tsx (1)
15-20: 💤 Low value
useMemois effectively a no-op here.
toRenderableMosaicSpec(spec)returns a new reference on every render, so the[renderableSpec]dependency always changes andJSON.stringifyre-runs each render anyway. Memoize on the stablespecinput instead.♻️ Proposed change
- const renderableSpec = toRenderableMosaicSpec(spec); - - const serializedValue = useMemo( - () => (renderableSpec ? JSON.stringify(renderableSpec, null, 2) : ''), - [renderableSpec], - ); + const serializedValue = useMemo(() => { + const renderableSpec = toRenderableMosaicSpec(spec); + return renderableSpec ? JSON.stringify(renderableSpec, null, 2) : ''; + }, [spec]);🤖 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/charts/chart-settings/MosaicChartSpecViewerPanel.tsx` around lines 15 - 20, The useMemo for serializedValue is ineffective because toRenderableMosaicSpec(spec) returns a new reference each render; change it to memoize based on the stable spec instead by moving the toRenderableMosaicSpec(spec) call inside the useMemo and using [spec] as the dependency. Concretely, update the serializedValue useMemo to compute const renderable = toRenderableMosaicSpec(spec) and then JSON.stringify(renderable, null, 2) (or '' if falsy) with [spec] as dependency so JSON.stringify only re-runs when spec changes; keep the existing renderableSpec variable only if needed elsewhere.packages/mosaic/src/chart-builders/createChartBuilderStore.ts (1)
3-3: ⚡ Quick winUse a type-only import for
ChartTypeIn
packages/mosaic/src/chart-builders/createChartBuilderStore.ts,ChartTypeis only used in type positions, so switching to a type-only import avoids an unnecessary runtime dependency on the../charts/chart-typesbarrel.♻️ Proposed fix
-import {ChartType} from '../charts/chart-types'; +import type {ChartType} from '../charts/chart-types';🤖 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/createChartBuilderStore.ts` at line 3, ChartType is only used as a type in createChartBuilderStore, so replace the regular import with a type-only import to avoid a runtime dependency; update the import of ChartType to use the TypeScript type-only form (so the compiler knows it's erased at runtime) and keep all usages of ChartType in createChartBuilderStore unchanged.packages/mosaic/src/charts/chart-types/index.ts (1)
42-54: ⚡ Quick winKeep the
chart-typesbarrel focused on chart APIs.Lines 42-54 pull AI/dashboard tooling into a chart-types entrypoint. That makes this public surface harder to reason about and couples chart consumers to a higher-level module they did not ask for. Prefer exporting these from the dedicated AI barrel and leaving
charts/chart-typesscoped to chart schemas, definitions, and chart-specific tools.As per coding guidelines, "Public exports in
packages/should be kept generic, composable, documented, and free of app/example-specific assumptions."🤖 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/charts/chart-types/index.ts` around lines 42 - 54, The chart-types barrel currently re-exports AI/dashboard tools (specifically '../../ai/data-table-explorer-tool', '../../ai/list-panels-tool', and '../../ai/remove-panel-tool'), which couples chart consumers to app-level tooling; remove those three exports from packages/mosaic/src/charts/chart-types/index.ts and keep only chart-specific exports (e.g., './histogram/tool', './line-chart/tool', './count-plot/tool', './heatmap/tool', './bubble-chart/tool', './box-plot/tool' and chart schema/definition exports). Then add the three dashboard tool exports to the dedicated AI barrel (the AI package entrypoint) so they remain public but no longer surface from chart-types.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@examples/deckgl-mosaic/src/components/filters/EarthquakeProfiler.tsx`:
- Around line 8-10: The component is currently declared as export const
EarthquakeProfiler: FC but its parameter is annotated as
EarthquakeProfilerProps; update the declaration to export const
EarthquakeProfiler: FC<EarthquakeProfilerProps> and remove the redundant
parameter type annotation so the signature becomes FC-typed like
WorksheetChartRenderer and DataTableBlockRenderer (keep the destructured {
className } param).
In `@packages/mosaic/src/ai/ai.ts`:
- Around line 606-620: calculateAgentMetadata currently returns
dashboard.panels?.length which is the total panels after run; change it to
report only newly added panels by accepting an additional input for the
previous/initial panel count (e.g., previousPanelsCount or initialPanelsLength)
and compute panelsCreated as Math.max(0, currentPanelsLength -
previousPanelsCount). Update the calculateAgentMetadata<TState> signature to
include that parameter, use adapter.getDashboard(state, dashboardId) to get
currentPanelsLength, and replace the panelsCreated calculation accordingly;
apply the same signature and change to the other similar helper at the 681-688
block so both functions compute delta rather than total.
- Around line 582-603: getOrCreateDashboard currently only checks
adapter.getCurrentDashboardArtifactId and then creates a new dashboard, which
bypasses run-scoped resolution used by createDashboardToolDeps.resolveArtifact;
update getOrCreateDashboard to first try resolving a run-scoped dashboard via
adapter.resolveArtifact (passing the equivalents of
resolveContextDashboardArtifactId and makeDashboardPrimaryForRun) and use that
id if returned, otherwise fall back to the existing logic that calls
adapter.createDashboardArtifact, adapter.setCurrentArtifact and
adapter.ensureDashboard; reference getOrCreateDashboard,
createDashboardAgentTool, adapter.resolveArtifact,
resolveContextDashboardArtifactId, makeDashboardPrimaryForRun,
getCurrentDashboardArtifactId, createDashboardArtifact and setCurrentArtifact
when making the change.
- Around line 657-667: Before constructing the ToolLoopAgent tool map (the
dashboardAgent), detect and reject any name collisions where a host-provided
extraTools entry uses the reserved name "query" so it cannot silently override
the SQL tool from createQueryTools(). Specifically, in the code path that builds
the tools object for new ToolLoopAgent (where model is options.getModel({state})
and tools merges queryTools.query with createDashboardAiTools(..., extraTools:
options.extraTools)), check options.extraTools (or the extraTools passed to
createDashboardAiTools) for the key "query" and throw a clear error (or return a
rejected promise) indicating the reserved name collision; do this before calling
createDashboardAiTools or merging the maps so the built-in
createQueryTools().query is preserved.
In `@packages/mosaic/src/chart-builders/TemporalGranularitySelector.tsx`:
- Line 10: Change the import of TemporalInterval to a type-only import to avoid
pulling in the runtime zod enum; update the import statement that currently
reads importing TemporalInterval from '../charts/chart-types/line-chart/schema'
to use a type-only import (i.e., import type { TemporalInterval } ...), so
TemporalGranularitySelector (and its references to TemporalInterval) only brings
in the TypeScript type and not the runtime value, matching other chart-builders
usage.
In
`@packages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.ts`:
- Around line 49-51: The guard that checks for incomplete config currently only
clears clientRef.current leaving the component state stale; update the branch
that runs when (!config || !config.x || !config.y) to also reset the local
box-plot state (e.g., call the state setter used in useBoxPlotClient like
setState or setBoxPlotState to its initial/empty value and ensure loading is
false) before returning, so both clientRef and state are cleared when config
becomes incomplete.
In `@packages/mosaic/src/charts/chart-types/box-plot/schema.ts`:
- Around line 5-9: The exported persisted BoxPlotChartSettings schema currently
makes both axes optional, which allows invalid configs that useBoxPlotClient and
buildBoxPlotQuery can't execute; update the box-plot schema (the exported
schema/type called BoxPlotChartSettings) so that the required renderable config
enforces a required numeric y and a required categorical x (i.e., make x and y
required), and if you need support for in-progress drafts create a separate
DraftBoxPlotChartSettings schema with optional fields; ensure useBoxPlotClient
and buildBoxPlotQuery continue to accept the renderable BoxPlotChartSettings
type so invalid charts are caught at the config boundary.
In `@packages/mosaic/src/charts/chart-types/chart-config.ts`:
- Around line 32-34: KNOWN_CHART_TYPES is built using
config.shape.chartType.value, but Zod v4's ZodLiteral exposes .values (a Set)
not .value, causing the array to be empty; update the mapping over
KNOWN_CHART_CONFIGS to extract the single literal string from
config.shape.chartType.values (e.g.,
Array.from(config.shape.chartType.values)[0] or
[...config.shape.chartType.values][0]) so KNOWN_CHART_TYPES contains the actual
discriminator strings; ensure this change fixes the reserved-name refine in
CustomChartConfig which relies on KNOWN_CHART_TYPES.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/workspace/WorksheetChartRenderer.tsx`:
- Line 46: Remove the leftover commented-out debug line referencing
fallbackField in WorksheetChartRenderer.tsx; locate the commented line "const
fallbackField = fallbackTable?.columns?.[0]?.name;" in the
WorksheetChartRenderer component and delete it so no unused/commented artifact
remains in the file.
In `@packages/mosaic/__tests__/dashboard-ai.test.ts`:
- Around line 1-9: The test imports directly from the internal module
'../src/ai/ai' which bypasses the package barrel; update the import source to
the public entry '../src/ai' (or add a tiny barrel smoke test) so the package
contract is validated — change the import for createDashboardAiTools,
createDashboardToolDeps, DASHBOARD_AGENT_INSTRUCTIONS,
DASHBOARD_AI_INSTRUCTIONS, MAP_TOOL_KEY, and the types DashboardAiAdapter and
DashboardAiTable to come from '../src/ai' instead of '../src/ai/ai'.
In `@packages/mosaic/src/ai/ai.ts`:
- Around line 319-324: Replace the custom DashboardAgentException class with a
small tagged-error helper: implement a factory like
createDashboardAgentError(message: string): Error that returns a new Error with
error.name = 'DashboardAgentException' (and optionally an
isDashboardAgentError(err): boolean that checks err?.name ===
'DashboardAgentException'), then update any throw sites that currently do new
DashboardAgentException(...) to call createDashboardAgentError(...) and update
catch branches that rely on instanceof DashboardAgentException to instead check
isDashboardAgentError(err) or err.name === 'DashboardAgentException'; keep
symbol names DashboardAgentException (as the tag string),
createDashboardAgentError and isDashboardAgentError to make the changes easy to
locate.
In `@packages/mosaic/src/chart-builders/createChartBuilderStore.ts`:
- Line 3: ChartType is only used as a type in createChartBuilderStore, so
replace the regular import with a type-only import to avoid a runtime
dependency; update the import of ChartType to use the TypeScript type-only form
(so the compiler knows it's erased at runtime) and keep all usages of ChartType
in createChartBuilderStore unchanged.
In `@packages/mosaic/src/charts/chart-settings/MosaicChartSpecViewerPanel.tsx`:
- Around line 15-20: The useMemo for serializedValue is ineffective because
toRenderableMosaicSpec(spec) returns a new reference each render; change it to
memoize based on the stable spec instead by moving the
toRenderableMosaicSpec(spec) call inside the useMemo and using [spec] as the
dependency. Concretely, update the serializedValue useMemo to compute const
renderable = toRenderableMosaicSpec(spec) and then JSON.stringify(renderable,
null, 2) (or '' if falsy) with [spec] as dependency so JSON.stringify only
re-runs when spec changes; keep the existing renderableSpec variable only if
needed elsewhere.
In `@packages/mosaic/src/charts/chart-types/index.ts`:
- Around line 42-54: The chart-types barrel currently re-exports AI/dashboard
tools (specifically '../../ai/data-table-explorer-tool',
'../../ai/list-panels-tool', and '../../ai/remove-panel-tool'), which couples
chart consumers to app-level tooling; remove those three exports from
packages/mosaic/src/charts/chart-types/index.ts and keep only chart-specific
exports (e.g., './histogram/tool', './line-chart/tool', './count-plot/tool',
'./heatmap/tool', './bubble-chart/tool', './box-plot/tool' and chart
schema/definition exports). Then add the three dashboard tool exports to the
dedicated AI barrel (the AI package entrypoint) so they remain public but no
longer surface from chart-types.
In `@packages/mosaic/src/charts/MosaicChartView.tsx`:
- Around line 50-54: Update the review comment and/or code comment to clarify
that getPanelIssueByKey(runtimeKey) performs a direct lookup (it returns
get().mosaicDashboard.runtime.panelIssuesByPanelId[runtimeKey]) so it does not
construct a new object per call, and therefore the selector used with
useStoreWithMosaicDashboard will only change when that
panelIssuesByPanelId[runtimeKey] reference changes; state that wrapping the
result in useMemo is optional and purely stylistic for consistency with “select
raw state first” if desired.
In `@packages/mosaic/src/editor/MosaicChartContainer.tsx`:
- Around line 49-52: The JSDoc usage example incorrectly references
MosaicSpecChart.Chart and closes with MosaicChart.Container; update the example
to use the correct subcomponent names and container: replace
MosaicSpecChart.Chart with MosaicSpecChart.Display, ensure the other members
(MosaicSpecChart.SpecEditor and MosaicSpecChart.Actions) remain, and change the
closing tag to </MosaicSpecChart.Container> so the example uses
MosaicSpecChart.Container, MosaicSpecChart.Display, MosaicSpecChart.SpecEditor,
and MosaicSpecChart.Actions consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 747f0018-5792-4ea1-a936-fb871e3d20c7
📒 Files selected for processing (134)
apps/sqlrooms-cli-ui/src/workspace/WorksheetChartRenderer.tsxapps/sqlrooms-cli-ui/src/workspace/worksheetChartConfig.tsexamples/deckgl-mosaic/src/components/filters/EarthquakeProfiler.tsxexamples/deckgl-mosaic/src/components/filters/FiltersPanel.tsxpackages/mosaic/__tests__/BoxPlotClient.test.tspackages/mosaic/__tests__/ChartSettings.test.tsxpackages/mosaic/__tests__/dashboard-ai.test.tspackages/mosaic/src/MosaicChart.tspackages/mosaic/src/MosaicColorLegend.tsxpackages/mosaic/src/ai.tspackages/mosaic/src/ai/ai.tspackages/mosaic/src/ai/data-table-explorer-tool.tspackages/mosaic/src/ai/list-panels-tool.tspackages/mosaic/src/ai/remove-panel-tool.tspackages/mosaic/src/ai/tool-helpers.tspackages/mosaic/src/ai/tool-schemas.tspackages/mosaic/src/ai/tool-types.tspackages/mosaic/src/ai/tool-validation.tspackages/mosaic/src/chart-builders/AggregationSelector.tsxpackages/mosaic/src/chart-builders/ChartBuilderActions.tsxpackages/mosaic/src/chart-builders/ChartBuilderContent.tsxpackages/mosaic/src/chart-builders/ChartBuilderContext.tsxpackages/mosaic/src/chart-builders/ChartBuilderDialog.tsxpackages/mosaic/src/chart-builders/ChartBuilderFields.tsxpackages/mosaic/src/chart-builders/ChartBuilderRoot.tsxpackages/mosaic/src/chart-builders/FieldSelectorInput.tsxpackages/mosaic/src/chart-builders/MultiFieldSelector.tsxpackages/mosaic/src/chart-builders/TemporalGranularitySelector.tsxpackages/mosaic/src/chart-builders/chartSpecTitle.tspackages/mosaic/src/chart-builders/chartTypeUtils.tspackages/mosaic/src/chart-builders/createChartBuilderStore.tspackages/mosaic/src/chart-types/errors.tspackages/mosaic/src/chart-types/index.tspackages/mosaic/src/chart-types/text-panel-tool.tspackages/mosaic/src/chart/MosaicChartSettingsPanel.tsxpackages/mosaic/src/chart/MosaicChartView.tsxpackages/mosaic/src/chart/MosaicDashboardChart.tsxpackages/mosaic/src/chart/MosaicDashboardChartContent.tsxpackages/mosaic/src/chart/MosaicDashboardChartRenderer.tsxpackages/mosaic/src/chart/MosaicDashboardComponentChart.tsxpackages/mosaic/src/chart/MosaicDashboardVgPlotChart.tsxpackages/mosaic/src/chart/MosaicDashboardVgPlotError.tsxpackages/mosaic/src/chart/useGenerateSpec.tspackages/mosaic/src/charts/MosaicChart.tsxpackages/mosaic/src/charts/MosaicChartError.tsxpackages/mosaic/src/charts/MosaicChartRuntimeIssuePanel.tsxpackages/mosaic/src/charts/MosaicChartSettingsPanel.tsxpackages/mosaic/src/charts/MosaicChartView.tsxpackages/mosaic/src/charts/addChartPanelAction.tspackages/mosaic/src/charts/chart-settings/MosaicChartSettings.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettingsContent.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettingsContext.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettingsPanel.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSpecViewerPanel.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartTypeSelector.tsxpackages/mosaic/src/charts/chart-settings/useTableColumns.tspackages/mosaic/src/charts/chart-types/base-types.tspackages/mosaic/src/charts/chart-types/box-plot/BoxPlotSettings.tsxpackages/mosaic/src/charts/chart-types/box-plot/definition.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotClient.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotErrorBoundary.tsxpackages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotPanelRenderer.tsxpackages/mosaic/src/charts/chart-types/box-plot/renderer/constants.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/plot.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/utils.tspackages/mosaic/src/charts/chart-types/box-plot/schema.tspackages/mosaic/src/charts/chart-types/box-plot/tool.tspackages/mosaic/src/charts/chart-types/bubble-chart/BubbleChartSettings.tsxpackages/mosaic/src/charts/chart-types/bubble-chart/definition.tspackages/mosaic/src/charts/chart-types/bubble-chart/schema.tspackages/mosaic/src/charts/chart-types/bubble-chart/spec.tspackages/mosaic/src/charts/chart-types/bubble-chart/tool.tspackages/mosaic/src/charts/chart-types/chart-config.tspackages/mosaic/src/charts/chart-types/count-plot/CountPlotSettings.tsxpackages/mosaic/src/charts/chart-types/count-plot/definition.tspackages/mosaic/src/charts/chart-types/count-plot/schema.tspackages/mosaic/src/charts/chart-types/count-plot/spec.tspackages/mosaic/src/charts/chart-types/count-plot/tool.tspackages/mosaic/src/charts/chart-types/createChartTools.tspackages/mosaic/src/charts/chart-types/createDefaultChartTypes.tspackages/mosaic/src/charts/chart-types/custom-spec/CustomSpecSettings.tsxpackages/mosaic/src/charts/chart-types/custom-spec/definition.tspackages/mosaic/src/charts/chart-types/custom-spec/schema.tspackages/mosaic/src/charts/chart-types/custom-spec/spec.tspackages/mosaic/src/charts/chart-types/data-policy-schema.tspackages/mosaic/src/charts/chart-types/errors.tspackages/mosaic/src/charts/chart-types/heatmap/HeatmapSettings.tsxpackages/mosaic/src/charts/chart-types/heatmap/definition.tspackages/mosaic/src/charts/chart-types/heatmap/schema.tspackages/mosaic/src/charts/chart-types/heatmap/spec.tspackages/mosaic/src/charts/chart-types/heatmap/tool.tspackages/mosaic/src/charts/chart-types/histogram/HistogramSettings.tsxpackages/mosaic/src/charts/chart-types/histogram/definition.tspackages/mosaic/src/charts/chart-types/histogram/schema.tspackages/mosaic/src/charts/chart-types/histogram/spec.tspackages/mosaic/src/charts/chart-types/histogram/tool.tspackages/mosaic/src/charts/chart-types/index.tspackages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsxpackages/mosaic/src/charts/chart-types/line-chart/definition.tspackages/mosaic/src/charts/chart-types/line-chart/schema.tspackages/mosaic/src/charts/chart-types/line-chart/spec.tspackages/mosaic/src/charts/chart-types/line-chart/tool.tspackages/mosaic/src/charts/chart-types/mosaicChartTypes.tspackages/mosaic/src/charts/dashboard/MosaicDashboardChartHeaderActions.tsxpackages/mosaic/src/charts/dashboard/MosaicDashboardChartRenderer.tsxpackages/mosaic/src/charts/useBrushSelectionParams.tsxpackages/mosaic/src/charts/useChartDataPolicy.tsxpackages/mosaic/src/charts/useChartRetainer.tsxpackages/mosaic/src/charts/useChartTypeDefinition.tspackages/mosaic/src/charts/useMosaicChartRenderContext.tspackages/mosaic/src/charts/useParseChartConfig.tspackages/mosaic/src/charts/useRuntimeIssueReporter.tsxpackages/mosaic/src/dashboard/DefaultMosaicDashboardBlock.tsxpackages/mosaic/src/dashboard/MosaicDashboard.tsxpackages/mosaic/src/dashboard/MosaicDashboardEmptyState.tsxpackages/mosaic/src/dashboard/MosaicDashboardSlice.tspackages/mosaic/src/dashboard/action-types.tspackages/mosaic/src/dashboard/core-types.tspackages/mosaic/src/dashboard/createDefaultMosaicDashboardPanelRenderers.tspackages/mosaic/src/dashboard/createMosaicDashboardBlockDefinition.tsxpackages/mosaic/src/dashboard/dashboard-types.tspackages/mosaic/src/dashboard/defaultPanelActions.tspackages/mosaic/src/dashboard/panel/MosaicDashboardPanel.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanelDragOverlay.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanelErrorBoundary.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanelHeader.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanelLayout.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanels.tsxpackages/mosaic/src/data-table-explorer/DataTableBlockRenderer.tsxpackages/mosaic/src/editor/MosaicChartContainer.tsxpackages/mosaic/src/index.tspackages/mosaic/src/text/MosaicDashboardTextRenderer.tsxpackages/mosaic/src/text/addTextPanelAction.ts
💤 Files with no reviewable changes (18)
- packages/mosaic/src/chart/MosaicChartSettingsPanel.tsx
- packages/mosaic/src/chart/MosaicDashboardComponentChart.tsx
- packages/mosaic/src/chart/MosaicDashboardChartRenderer.tsx
- packages/mosaic/src/chart-types/errors.ts
- packages/mosaic/src/chart/MosaicDashboardVgPlotError.tsx
- packages/mosaic/src/chart/MosaicChartView.tsx
- packages/mosaic/src/chart/MosaicDashboardChartContent.tsx
- packages/mosaic/src/chart-types/text-panel-tool.ts
- packages/mosaic/src/chart/useGenerateSpec.ts
- packages/mosaic/src/chart/MosaicDashboardVgPlotChart.tsx
- packages/mosaic/src/MosaicColorLegend.tsx
- packages/mosaic/src/chart/MosaicDashboardChart.tsx
- packages/mosaic/src/chart-types/index.ts
- packages/mosaic/src/dashboard/core-types.ts
- apps/sqlrooms-cli-ui/src/workspace/worksheetChartConfig.ts
- packages/mosaic/src/text/addTextPanelAction.ts
- packages/mosaic/src/dashboard/MosaicDashboardEmptyState.tsx
- packages/mosaic/src/text/MosaicDashboardTextRenderer.tsx
Comment on lines
+582
to
+603
| function getOrCreateDashboard<TState>( | ||
| state: TState, | ||
| adapter: DashboardAiAdapter<TState>, | ||
| tableName: string, | ||
| dashboardTitle?: string, | ||
| ): string { | ||
| const dashboardId = adapter.getCurrentDashboardArtifactId(state); | ||
| if (dashboardId) { | ||
| adapter.ensureDashboard(state, dashboardId); | ||
| return dashboardId; | ||
| } | ||
|
|
||
| const suggestedTitle = | ||
| dashboardTitle || | ||
| `${tableName.charAt(0).toUpperCase() + tableName.slice(1)} Insights`; | ||
| const newDashboardId = adapter.createDashboardArtifact( | ||
| state, | ||
| suggestedTitle, | ||
| 'grid', | ||
| ); | ||
| adapter.setCurrentArtifact(state, newDashboardId); | ||
| return newDashboardId; |
Contributor
There was a problem hiding this comment.
Propagate run-scoped dashboard context into the agent bootstrap.
createDashboardToolDeps.resolveArtifact() already honors resolveContextDashboardArtifactId and makeDashboardPrimaryForRun, but createDashboardAgentTool() bypasses that path and only looks at getCurrentDashboardArtifactId(). In hosts that use run-scoped dashboard context, the sub-agent can attach work to the wrong dashboard or hit No primary dashboard artifact is available in the current run context. after this helper creates one without marking it primary.
Suggested fix
function getOrCreateDashboard<TState>(
state: TState,
adapter: DashboardAiAdapter<TState>,
tableName: string,
dashboardTitle?: string,
+ context?: ChartToolExecutionContext,
): string {
- const dashboardId = adapter.getCurrentDashboardArtifactId(state);
+ const dashboardId =
+ adapter.resolveContextDashboardArtifactId?.(state, context) ??
+ adapter.getCurrentDashboardArtifactId(state);
if (dashboardId) {
adapter.ensureDashboard(state, dashboardId);
+ adapter.makeDashboardPrimaryForRun?.(state, dashboardId, context);
return dashboardId;
}
const suggestedTitle =
dashboardTitle ||
`${tableName.charAt(0).toUpperCase() + tableName.slice(1)} Insights`;
const newDashboardId = adapter.createDashboardArtifact(
state,
suggestedTitle,
'grid',
);
adapter.setCurrentArtifact(state, newDashboardId);
+ adapter.makeDashboardPrimaryForRun?.(state, newDashboardId, context);
return newDashboardId;
} dashboardId = getOrCreateDashboard(
state,
adapter,
tableName,
dashboardTitle,
+ toolOptions as ChartToolExecutionContext,
);Also applies to: 648-654
🤖 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/ai/ai.ts` around lines 582 - 603, getOrCreateDashboard
currently only checks adapter.getCurrentDashboardArtifactId and then creates a
new dashboard, which bypasses run-scoped resolution used by
createDashboardToolDeps.resolveArtifact; update getOrCreateDashboard to first
try resolving a run-scoped dashboard via adapter.resolveArtifact (passing the
equivalents of resolveContextDashboardArtifactId and makeDashboardPrimaryForRun)
and use that id if returned, otherwise fall back to the existing logic that
calls adapter.createDashboardArtifact, adapter.setCurrentArtifact and
adapter.ensureDashboard; reference getOrCreateDashboard,
createDashboardAgentTool, adapter.resolveArtifact,
resolveContextDashboardArtifactId, makeDashboardPrimaryForRun,
getCurrentDashboardArtifactId, createDashboardArtifact and setCurrentArtifact
when making the change.
Comment on lines
+657
to
+667
| const dashboardAgent = new ToolLoopAgent({ | ||
| model: options.getModel({state}), | ||
| tools: { | ||
| ...(queryTools ? {query: queryTools.query} : {}), | ||
| ...createDashboardAiTools({ | ||
| store, | ||
| adapter, | ||
| chartTypes: options.chartTypes, | ||
| extraTools: options.extraTools, | ||
| }), | ||
| }, |
Contributor
There was a problem hiding this comment.
Reject query collisions before building the agent tool map.
createDashboardAiTools() only guards the built-ins it owns. At this merge point, a host extraTools entry named query will silently overwrite createQueryTools().query, so exploratory runs lose the SQL tool that the prompt depends on.
Suggested fix
+ const dashboardTools = createDashboardAiTools({
+ store,
+ adapter,
+ chartTypes: options.chartTypes,
+ extraTools: options.extraTools,
+ });
+ if (queryTools && 'query' in dashboardTools) {
+ throw new Error(
+ 'Dashboard extraTools cannot override the agent query tool.',
+ );
+ }
+
const dashboardAgent = new ToolLoopAgent({
model: options.getModel({state}),
tools: {
...(queryTools ? {query: queryTools.query} : {}),
- ...createDashboardAiTools({
- store,
- adapter,
- chartTypes: options.chartTypes,
- extraTools: options.extraTools,
- }),
+ ...dashboardTools,
},🤖 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/ai/ai.ts` around lines 657 - 667, Before constructing the
ToolLoopAgent tool map (the dashboardAgent), detect and reject any name
collisions where a host-provided extraTools entry uses the reserved name "query"
so it cannot silently override the SQL tool from createQueryTools().
Specifically, in the code path that builds the tools object for new
ToolLoopAgent (where model is options.getModel({state}) and tools merges
queryTools.query with createDashboardAiTools(..., extraTools:
options.extraTools)), check options.extraTools (or the extraTools passed to
createDashboardAiTools) for the key "query" and throw a clear error (or return a
rejected promise) indicating the reserved name collision; do this before calling
createDashboardAiTools or merging the maps so the built-in
createQueryTools().query is preserved.
ilyabo
approved these changes
Jun 3, 2026
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/mosaic/src/charts/chart-settings/MosaicChartSettings.tsx (1)
4-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale
tableNameprop in the doc example.
MosaicChartSettingsRootno longer acceptstableName(props are nowconfig,columns,onChange), but the example still passes it. This will mislead consumers and produce a type error if copied.📝 Proposed doc fix
* <MosaicChartSettings.Root -* tableName={tableName} * config={config} * columns={columns} * onChange={handleChange} * >🤖 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/charts/chart-settings/MosaicChartSettings.tsx` around lines 4 - 15, The example docs still pass a stale tableName prop to MosaicChartSettings.Root; remove tableName from the example and update the props list to only show the current props (config, columns, onChange) so the example matches the MosaicChartSettings.Root API and won't cause type errors when copied.
🧹 Nitpick comments (1)
packages/mosaic/src/charts/worksheet/ChartBlockRenderer.tsx (1)
3-7: ⚡ Quick winSame self-barrel import concern as
ChartBlockHeader.
MosaicChart,useParseChartConfig, andChartConfigare imported from@sqlrooms/mosaicwhile this file lives inpackages/mosaic/src. Use relative imports to avoid potential circular dependencies through the package entry point. (Root cause shared withChartBlockHeader.tsxline 2.)🤖 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/charts/worksheet/ChartBlockRenderer.tsx` around lines 3 - 7, ChartBlockRenderer.tsx currently imports MosaicChart, useParseChartConfig and ChartConfig from the package entry ('`@sqlrooms/mosaic`') which can create circular dependencies; update the import to use the package's internal relative module that directly exports these symbols (the same relative import pattern used in ChartBlockHeader.tsx) so MosaicChart, useParseChartConfig and ChartConfig are imported from their local source modules rather than the root barrel.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/mosaic/src/charts/worksheet/ChartBlockHeader.tsx`:
- Line 2: ChartBlockHeader/ChartBlockRenderer are importing types and symbols
from the package barrel (`@sqlrooms/mosaic`) which creates a self-referential
cycle; change the type-only import of ChartConfig in ChartBlockHeader.tsx to a
relative import from ../chart-types/chart-config and update
ChartBlockRenderer.tsx to import MosaicChart and useParseChartConfig from their
local module files (the modules that actually export them inside this package)
instead of from `@sqlrooms/mosaic` so you remove the circular dependency and
improve build ordering/tree-shaking.
---
Outside diff comments:
In `@packages/mosaic/src/charts/chart-settings/MosaicChartSettings.tsx`:
- Around line 4-15: The example docs still pass a stale tableName prop to
MosaicChartSettings.Root; remove tableName from the example and update the props
list to only show the current props (config, columns, onChange) so the example
matches the MosaicChartSettings.Root API and won't cause type errors when
copied.
---
Nitpick comments:
In `@packages/mosaic/src/charts/worksheet/ChartBlockRenderer.tsx`:
- Around line 3-7: ChartBlockRenderer.tsx currently imports MosaicChart,
useParseChartConfig and ChartConfig from the package entry ('`@sqlrooms/mosaic`')
which can create circular dependencies; update the import to use the package's
internal relative module that directly exports these symbols (the same relative
import pattern used in ChartBlockHeader.tsx) so MosaicChart, useParseChartConfig
and ChartConfig are imported from their local source modules rather than the
root barrel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32b25423-4356-4815-bd31-ea2a04f7f763
📒 Files selected for processing (62)
apps/sqlrooms-cli-ui/src/workspace/WorksheetArtifact.tsxexamples/deckgl-mosaic/src/components/filters/EarthquakeProfiler.tsxpackages/deck/src/MapSettings.tsxpackages/mosaic/__tests__/DataTableExplorerCompound.test.tsxpackages/mosaic/__tests__/DataTableExplorerStatusBar.test.tsxpackages/mosaic/src/MosaicChartBuilder.tspackages/mosaic/src/chart-builders/chartTypeUtils.tspackages/mosaic/src/charts/MosaicChart.tsxpackages/mosaic/src/charts/MosaicChartSettingsPanel.tsxpackages/mosaic/src/charts/MosaicChartView.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettings.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettingsContext.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettingsPanel.tsxpackages/mosaic/src/charts/chart-types/box-plot/BoxPlotSettings.tsxpackages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.tspackages/mosaic/src/charts/chart-types/box-plot/tool.tspackages/mosaic/src/charts/chart-types/bubble-chart/BubbleChartSettings.tsxpackages/mosaic/src/charts/chart-types/bubble-chart/tool.tspackages/mosaic/src/charts/chart-types/count-plot/CountPlotSettings.tsxpackages/mosaic/src/charts/chart-types/count-plot/tool.tspackages/mosaic/src/charts/chart-types/custom-spec/CustomSpecSettings.tsxpackages/mosaic/src/charts/chart-types/heatmap/HeatmapSettings.tsxpackages/mosaic/src/charts/chart-types/heatmap/tool.tspackages/mosaic/src/charts/chart-types/histogram/HistogramSettings.tsxpackages/mosaic/src/charts/chart-types/histogram/tool.tspackages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsxpackages/mosaic/src/charts/chart-types/line-chart/schema.tspackages/mosaic/src/charts/chart-types/line-chart/tool.tspackages/mosaic/src/charts/dashboard/MosaicDashboardChartRenderer.tsxpackages/mosaic/src/charts/useChartDataPolicy.tsxpackages/mosaic/src/charts/useMosaicChartRenderContext.tspackages/mosaic/src/charts/worksheet/ChartBlockHeader.tsxpackages/mosaic/src/charts/worksheet/ChartBlockRenderer.tsxpackages/mosaic/src/charts/worksheet/ChartSelectorEmptyState.tsxpackages/mosaic/src/column-types-utils.tspackages/mosaic/src/components/AggregationSelector.tsxpackages/mosaic/src/components/ColumnSelector.tsxpackages/mosaic/src/components/ColumnsContext.tsxpackages/mosaic/src/components/DataTableSelector.tsxpackages/mosaic/src/components/Field.tsxpackages/mosaic/src/components/FieldSelectorInput.tsxpackages/mosaic/src/components/MultiFieldSelector.tsxpackages/mosaic/src/components/TableSelector.tsxpackages/mosaic/src/components/TemporalGranularitySelector.tsxpackages/mosaic/src/dashboard/createDefaultMosaicDashboardPanelRenderers.tspackages/mosaic/src/dashboard/initial-state/BuildDashboardManuallyPanel.tsxpackages/mosaic/src/dashboard/initial-state/BuildDashboardWithAIPanel.tsxpackages/mosaic/src/dashboard/useMosaicDashboardAddPanelActionContext.tspackages/mosaic/src/dashboard/useSelectedOrFirstTable.tspackages/mosaic/src/data-table-explorer/DataTableExplorer.tsxpackages/mosaic/src/data-table-explorer/DataTableExplorerStatusBar.tsxpackages/mosaic/src/data-table-explorer/dashboard/MosaicDashboardDataTableExplorerPanelRenderer.tsxpackages/mosaic/src/data-table-explorer/types.tspackages/mosaic/src/data-table-explorer/useDataTableExplorer.tspackages/mosaic/src/data-table-explorer/worksheet/DataTableBlockHeader.tsxpackages/mosaic/src/data-table-explorer/worksheet/DataTableBlockRenderer.tsxpackages/mosaic/src/hooks/useDataTable.tspackages/mosaic/src/hooks/useTablesWithColumns.tspackages/mosaic/src/index.tspackages/mosaic/src/schemas/common.tspackages/mosaic/src/schemas/index.tspackages/mosaic/tsconfig.json
💤 Files with no reviewable changes (3)
- packages/mosaic/src/chart-builders/chartTypeUtils.ts
- packages/mosaic/src/components/FieldSelectorInput.tsx
- packages/mosaic/tests/DataTableExplorerCompound.test.tsx
✅ Files skipped from review due to trivial changes (11)
- packages/mosaic/src/dashboard/initial-state/BuildDashboardWithAIPanel.tsx
- packages/mosaic/src/dashboard/useMosaicDashboardAddPanelActionContext.ts
- packages/mosaic/src/hooks/useTablesWithColumns.ts
- packages/mosaic/src/dashboard/useSelectedOrFirstTable.ts
- packages/mosaic/src/schemas/index.ts
- packages/mosaic/src/components/AggregationSelector.tsx
- packages/mosaic/src/components/ColumnSelector.tsx
- packages/mosaic/tsconfig.json
- packages/mosaic/src/charts/chart-types/line-chart/tool.ts
- packages/mosaic/src/charts/chart-types/bubble-chart/tool.ts
- packages/mosaic/src/charts/chart-types/heatmap/tool.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/mosaic/src/charts/chart-types/custom-spec/CustomSpecSettings.tsx
- packages/mosaic/src/charts/chart-types/box-plot/BoxPlotSettings.tsx
- packages/mosaic/src/charts/chart-types/histogram/HistogramSettings.tsx
- packages/mosaic/src/charts/chart-types/bubble-chart/BubbleChartSettings.tsx
- packages/mosaic/src/charts/chart-types/count-plot/CountPlotSettings.tsx
- packages/mosaic/src/charts/MosaicChartSettingsPanel.tsx
- packages/mosaic/src/charts/chart-settings/MosaicChartSettingsPanel.tsx
- packages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.ts
- packages/mosaic/src/charts/chart-types/count-plot/tool.ts
- packages/mosaic/src/dashboard/createDefaultMosaicDashboardPanelRenderers.ts
- packages/mosaic/src/charts/MosaicChartView.tsx
- packages/mosaic/src/charts/dashboard/MosaicDashboardChartRenderer.tsx
- packages/mosaic/src/charts/chart-types/histogram/tool.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/mosaic/src/dashboard/panel/MosaicDashboardPanels.tsx (1)
46-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the persisted table selection before treating the dashboard as initialized.
Lines 97-104 only check whether a table name is stored, not whether that table still exists in
tables. If the saved table was deleted or renamed, this view skips the selector empty state, andMosaicDashboardToolbaralso drops the selector because it cannot resolveselectedTableNameback to aDataTable. That leaves the dashboard stuck with no in-UI way to recover.Suggested fix
const tables = useTablesWithColumns(); const selectedTable = useStoreWithMosaicDashboard( (state) => state.mosaicDashboard.config.dashboardsById[dashboardId]?.selectedTable, ); + const hasSelectedTable = useMemo( + () => + selectedTable != null && + tables.some((table) => table.table.toString() === selectedTable), + [selectedTable, tables], + ); const setSelectedTable = useStoreWithMosaicDashboard( (state) => state.mosaicDashboard.setSelectedTable, ); @@ - if (!selectedTable) { + if (!hasSelectedTable) { return ( <DataTableSelectorEmptyState onChange={handleTableChange} tables={tables} />🤖 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/panel/MosaicDashboardPanels.tsx` around lines 46 - 104, The code currently treats any persisted selectedTable string as valid; change the check so selectedTable is validated against the current tables list and treated as missing if it can't be resolved. Concretely, in the component replace the simple if (!selectedTable) branch with a lookup (e.g. const selectedTableObj = tables.find(t => t.table.toString() === selectedTable)) and use selectedTableObj to decide whether to render DataTableSelectorEmptyState (call onChange={handleTableChange} with tables={tables}) and to pass into MosaicDashboardToolbar; if lookup fails, call setSelectedTable(dashboardId, undefined or null) or simply render the empty selector so the UI can recover. Ensure you update all references that assumed selectedTable was a valid DataTable (e.g. any use of selectedTableName/selectedTableObj) to use the resolved object.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@packages/mosaic/src/dashboard/panel/MosaicDashboardPanels.tsx`:
- Around line 46-104: The code currently treats any persisted selectedTable
string as valid; change the check so selectedTable is validated against the
current tables list and treated as missing if it can't be resolved. Concretely,
in the component replace the simple if (!selectedTable) branch with a lookup
(e.g. const selectedTableObj = tables.find(t => t.table.toString() ===
selectedTable)) and use selectedTableObj to decide whether to render
DataTableSelectorEmptyState (call onChange={handleTableChange} with
tables={tables}) and to pass into MosaicDashboardToolbar; if lookup fails, call
setSelectedTable(dashboardId, undefined or null) or simply render the empty
selector so the UI can recover. Ensure you update all references that assumed
selectedTable was a valid DataTable (e.g. any use of
selectedTableName/selectedTableObj) to use the resolved object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e57e2ca5-e65a-4876-a88f-0bffa600b7a9
📒 Files selected for processing (11)
apps/sqlrooms-cli-ui/src/components/DataSourcesPanel.tsxpackages/mosaic/src/charts/worksheet/ChartBlockHeader.tsxpackages/mosaic/src/charts/worksheet/ChartBlockRenderer.tsxpackages/mosaic/src/dashboard/MosaicDashboard.tsxpackages/mosaic/src/dashboard/MosaicDashboardContext.tsxpackages/mosaic/src/dashboard/initial-state/BuildDashboardManuallyPanel.tsxpackages/mosaic/src/dashboard/initial-state/BuildDashboardWithAIPanel.tsxpackages/mosaic/src/dashboard/initial-state/MosaicDashboardInitialState.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanels.tsxpackages/mosaic/src/dashboard/toolbar/MosaicDashboardToolbar.tsxpackages/mosaic/src/index.ts
💤 Files with no reviewable changes (6)
- packages/mosaic/src/dashboard/initial-state/BuildDashboardWithAIPanel.tsx
- packages/mosaic/src/dashboard/initial-state/MosaicDashboardInitialState.tsx
- packages/mosaic/src/dashboard/MosaicDashboardContext.tsx
- packages/mosaic/src/dashboard/initial-state/BuildDashboardManuallyPanel.tsx
- apps/sqlrooms-cli-ui/src/components/DataSourcesPanel.tsx
- packages/mosaic/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/mosaic/src/charts/worksheet/ChartBlockRenderer.tsx
- packages/mosaic/src/charts/worksheet/ChartBlockHeader.tsx
This was referenced Jun 9, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor