Repository navigation
feat: Box plot for mosaic dashboards - #588
Merged
dmitriy-kostianetskiy merged 29 commits intoMay 14, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors chart types from a vgplot-only model to a unified ChartConfig/ChartType system, adds a component-based Box Plot (client, renderer, utilities, tests), relocates chart-settings to shared chart modules, and updates dashboard panel types/factories to use chart-panel configs. ChangesUnified chart model, Box Plot feature, and dashboard integration
sequenceDiagram
participant User
participant Dashboard
participant Store
participant ChartRenderer
participant TypeDef
participant ChartContent
participant SpecChart
participant CompChart
participant BoxPlotRenderer
participant BoxPlotClient
User->>Dashboard: create panel (createMosaicDashboardChartPanelConfig)
Dashboard->>Store: add panel
Store->>ChartRenderer: render panel
ChartRenderer->>TypeDef: useChartTypeDefinition(chartType)
TypeDef-->>ChartRenderer: ChartTypeDefinition
ChartRenderer->>ChartContent: dispatch by chart type
alt spec-based
ChartContent->>SpecChart: render VgPlot chart (useGenerateSpec)
SpecChart->>Store: request data (coordinator)
SpecChart-->>User: render VgPlot
else component-based (box-plot)
ChartContent->>CompChart: createElement(renderer, props)
CompChart->>BoxPlotRenderer: mount
BoxPlotRenderer->>BoxPlotClient: useBoxPlotClient (query)
BoxPlotClient->>Store: execute SQL
Store-->>BoxPlotClient: rows
BoxPlotClient-->>BoxPlotRenderer: summaries/outliers
BoxPlotRenderer-->>User: render Observable Plot
User->>BoxPlotRenderer: brush Y
BoxPlotRenderer->>BoxPlotClient: updateYBrush -> update selection
end
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
ilyabo
force-pushed
the
feat--Box-plot-for-mosaic-dashboards
branch
from
May 5, 2026 13:22
8ce43bb to
38ecc15
Compare
- Add renderer component pattern to ChartTypeDefinition - Remove createSpec from interface (logic moves to renderers) - Create HistogramRenderer as example - Define ChartRendererProps interface Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Completes Task 4 of the box-plot migration plan by creating dedicated renderer components for all remaining vgPlot chart types. Chart types migrated: - line-chart: LineChartRenderer - bubble-chart: BubbleChartRenderer - count-plot: CountPlotRenderer - ecdf: EcdfRenderer - heatmap: HeatmapRenderer - custom-spec: CustomSpecRenderer Each renderer follows the established pattern: - Generates vgPlot spec internally using useMemo - Renders via VgPlotChart component - Depends on tableName and settings for proper memoization The createSpec logic from each definition has been moved directly into the corresponding renderer component, preserving all existing behavior including styling constants, mark types, and crossfilter configuration. Backward compatibility: - Added deprecated createSpec/outputKind/createOutput fields to ChartTypeDefinition to maintain compatibility during transition - Added BoxPlotStubRenderer as temporary placeholder for Task 5 - Updated toChartTypeDefinition helper to include renderer field This establishes a uniform pattern where ALL chart types specify a renderer component, setting the stage for Task 6 to update the dashboard to use renderers directly. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Create BoxPlotPanelRenderer in chart-types/box-plot/ - Adapt existing BoxPlotClient integration - Update definition to use BoxPlotPanelRenderer - Preserve all custom rendering and interaction logic Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Update ChartRendererProps to include params and retention - Update all chart renderer components to accept and forward params/retention to VgPlotChart - Update MosaicDashboardVgPlotPanelRenderer to use chartType.renderer instead of pre-generated spec - Remove toRenderableMosaicSpec helper (no longer needed) - Update chart builders to work with renderer pattern: - Use createOutput for custom dashboard panels (box-plot) - Create vgplot panels with chartType and settings (no pre-generated spec) - Keep createSpec, createOutput, outputKind as deprecated fields for backward compatibility - Simplify rendering logic: dashboard gets renderer from chart type and passes it the required props Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Remove BoxPlotStubRenderer (temporary placeholder, no longer needed) - Remove backward compatibility fields from box-plot definition (outputKind, createOutput) - Remove box-plot to boxplot panel type conversion logic from VgPlotPanelRenderer - Update ChartSettings to not pre-generate vgplot specs (renderers generate specs internally) - Deprecate generateMosaicChartSpec function - Remove generateMosaicChartSpec tests (function is deprecated) - Update chartTypes tests to reflect new renderer pattern All chart types now use the unified renderer pattern: - VgPlot charts generate specs internally in their renderers - Box-plot uses custom BoxPlotClient rendering via BoxPlotPanelRenderer - Dashboard calls renderer components which handle their own spec generation Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
…saic-dashboards Co-authored-by: Copilot <[email protected]>
…ced chart rendering Co-authored-by: Copilot <[email protected]>
…ontent for improved structure
…ashboardVgPlotError components for improved user experience Co-authored-by: Copilot <[email protected]>
…d add createSpec methods for enhanced functionality Co-authored-by: Copilot <[email protected]>
… plot chart rendering
…d y configuration Co-authored-by: Copilot <[email protected]>
![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: 7
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/index.ts (1)
189-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInternal VgPlot naming migration is incomplete and lacks deprecation documentation.
The backward-compatibility alias at line 39 provides compatibility for the old
MOSAIC_DASHBOARD_VGPLOT_PANEL_TYPEname, but internal usages remain heavily tied toVgPlot*naming:
VgPlotChartcomponent (packages/mosaic/src/VgPlotChart.tsx) and its types (RetainedVgPlotChart,ChartBuilderVgPlotOutput) are still the canonical internal implementation- Multiple dashboard and chart files import and use
VgPlotChartandRetainedVgPlotChartdirectly (MosaicDashboardVgPlotChart.tsx, MosaicDashboardVgPlotError.tsx, base-types.ts, etc.)- The panel type constant value remains
'vgplot'in the sliceIf the intent is to migrate fully to
Chart*naming, internal types and the component should be renamed. IfVgPlot*is meant as an internal implementation detail, that should be documented (currently no deprecation notice exists for the old alias).🤖 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/index.ts` around lines 189 - 215, The codebase still mixes old VgPlot* names with new Chart* names and lacks deprecation documentation for the alias MOSAIC_DASHBOARD_VGPLOT_PANEL_TYPE; to fix, pick one approach and make changes consistently: either fully migrate internals to Chart* (rename VgPlotChart -> ChartChart or ChartComponent, RetainedVgPlotChart -> RetainedChart, ChartBuilderVgPlotOutput -> ChartBuilderOutput, update imports in MosaicDashboardVgPlotChart.tsx, MosaicDashboardVgPlotError.tsx, base-types.ts, slice where panel type value 'vgplot' is used) or explicitly mark VgPlot* as private/legacy by adding clear deprecation comments and export a compatibility alias that logs the deprecation and points to the new Chart* names; update the MOSAIC_DASHBOARD_VGPLOT_PANEL_TYPE handling to document the legacy name and ensure all internal references are aligned with the chosen approach.
🧹 Nitpick comments (4)
packages/mosaic/src/boxplot/BoxPlotClient.ts (1)
68-79: 💤 Low valueMinor:
Array.fromis redundant.The
toArray()method already returns an array, so wrapping it inArray.fromis unnecessary. However, this doesn't cause any issues.♻️ Simplification
- return Array.from((data as {toArray(): T[]}).toArray()); + return (data as {toArray(): T[]}).toArray();🤖 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/boxplot/BoxPlotClient.ts` around lines 68 - 79, The rowsFromQueryResult function redundantly wraps the toArray() result in Array.from; change the return to directly return the array from toArray() (e.g., return (data as {toArray(): T[]}).toArray();), keeping the existing guards intact so rowsFromQueryResult<T> still validates data and then returns the toArray() result without the extra Array.from call.packages/mosaic/__tests__/ChartSettings.test.tsx (1)
45-74: ⚡ Quick winConsider using parameterized tests instead of
forEachfor better failure diagnostics.When a test fails inside
forEach, the error message doesn't clearly indicate which chart type caused the failure. Jest providesit.eachfor this purpose.♻️ Suggested refactor using it.each
- it('renders with different chart types', () => { - const chartTypes: ChartConfig['chartType'][] = [ - 'histogram', - 'count-plot', - 'line-chart', - 'heatmap', - 'box-plot', - 'bubble-chart', - ]; - - chartTypes.forEach((chartType) => { - const config: ChartConfig = { - chartType, - settings: {}, - }; - - const markup = renderToStaticMarkup( - <ChartSettings.Root - tableName="test_table" - config={config} - columns={mockColumns} - onChange={mockOnChange} - > - <div>Test content</div> - </ChartSettings.Root>, - ); - - expect(markup).toContain('Test content'); - }); - }); + it.each([ + 'histogram', + 'count-plot', + 'line-chart', + 'heatmap', + 'box-plot', + 'bubble-chart', + ] as const)('renders with chart type: %s', (chartType) => { + const config: ChartConfig = { + chartType, + settings: {}, + }; + + const markup = renderToStaticMarkup( + <ChartSettings.Root + tableName="test_table" + config={config} + columns={mockColumns} + onChange={mockOnChange} + > + <div>Test content</div> + </ChartSettings.Root>, + ); + + expect(markup).toContain('Test content'); + });🤖 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__/ChartSettings.test.tsx` around lines 45 - 74, The test currently iterates chartTypes with chartTypes.forEach inside a single it block which hides which chartType fails; refactor to a parameterized test using Jest's it.each (or test.each) so each chartType runs as its own test case and failures show the failing value — replace the it('renders with different chart types', ...) body with an it.each([...]) that supplies each ChartConfig chartType and constructs the same ChartConfig and renderToStaticMarkup call against ChartSettings.Root (keeping tableName, columns, mockOnChange and the 'Test content' assertion) so failures indicate the specific chartType.packages/mosaic/src/chart/chart-settings/ChartTypeSelector.tsx (1)
21-59: 💤 Low valueConsider explicit handling when chart types are unavailable.
The component uses optional chaining (
chartTypes?.find,chartTypes?.map) to handle potentially undefinedchartTypes, which prevents crashes but results in an empty dropdown. If chart types should always be available, consider adding a loading state or error message for better UX.🤖 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/chart-settings/ChartTypeSelector.tsx` around lines 21 - 59, ChartTypeSelector currently uses optional chaining on chartTypes which yields an empty/selectable UI when chartTypes is undefined; update the component to explicitly handle missing chartTypes by showing a loading or error state and disabling the Select until data is present. Inside ChartTypeSelector, check chartTypes before rendering Select (or inside SelectTrigger/SelectContent): if chartTypes is undefined render a small loading indicator or “Loading chart types…”/“No chart types available” message and set the Select to disabled (or return early), and only map chartTypes to SelectItem when it’s a defined non-empty array; ensure selectedChartType logic with selectedChartType?.icon remains intact.packages/mosaic/src/chart/chart-settings/ChartSettingsContext.tsx (1)
74-85: ⚡ Quick winType cast on line 82 may hide errors; consider preserving generic type parameter.
The
as ChartConfigcast widens from the specific config typeTto the entireChartConfigunion. This loses type safety—if settings are modified incorrectly, TypeScript won't catch it.The root issue is that the spread operation doesn't preserve the discriminated union type. A safer approach would be to keep the generic type parameter throughout:
♻️ Suggested refactor to preserve type safety
const onChangeConfig = useCallback( - function <K extends keyof ChartSetting>(key: K, value: ChartSetting) { + function <K extends keyof ChartSetting<T>>(key: K, value: ChartSetting<T>[K]) { onChange({ ...config, settings: { ...config.settings, [key]: value, }, - } as ChartConfig); + } as T); }, [config, onChange], );This preserves the specific chart config type rather than widening to the union, improving type safety for callers.
🤖 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/chart-settings/ChartSettingsContext.tsx` around lines 74 - 85, The current onChangeConfig implementation widens the specific config type to the ChartConfig union via "as ChartConfig", losing type safety; change onChangeConfig to preserve the generic config type (T) instead of casting: make the callback generic over the concrete config type and the specific setting key/value (referencing onChangeConfig, onChange, ChartConfig, ChartSetting), accept a value typed as the specific ChartSetting property rather than ChartSetting, and construct the new config while keeping its original generic type so you can pass the correctly typed config to onChange without using "as ChartConfig".
🤖 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/FiltersPanel.tsx`:
- Line 8: There is a name collision: ChartConfig is imported from both
`@sqlrooms/mosaic` and ./filterPlots; update one import to use a distinct local
alias (for example import { ChartConfig as PlotChartConfig } from
'./filterPlots' or import { ChartConfig as MosaicChartConfig } from
'@sqlrooms/mosaic') and then update all usages in FiltersPanel (references to
ChartConfig within this file) to the new alias (e.g., PlotChartConfig or
MosaicChartConfig) so the two bindings no longer collide; ensure you update any
type annotations, props, or function parameters that reference ChartConfig.
In `@packages/mosaic/src/chart-builders/ChartBuilderRoot.tsx`:
- Around line 24-25: The JSDoc for the ChartBuilderRoot prop chartTypes is
inaccurate: it says the default is "all registered types" but the implementation
(see chartTypes?: ChartTypeDefinition[] and the defaulting at the component
where chartTypes falls back to an empty array) uses an empty list. Update the
prop comment for chartTypes in ChartBuilderRoot (and any duplicate doc at the
other location around the defaulting code) to state that the default is an empty
array (no chart types) rather than all registered types so the docs match the
actual fallback behavior.
In `@packages/mosaic/src/chart-types/box-plot/renderer/BoxPlotPanelRenderer.tsx`:
- Around line 132-140: In BoxPlotPanelRenderer.tsx inside the state.yBrush.map
block, guard the division by (yDomain[1] - yDomain[0]) to avoid NaN/Infinity
when the Y domain collapses: compute a safe denominator (e.g., const span =
yDomain[1] - yDomain[0] || 1) before doing ((yDomain[1] - value) / span) *
plotHeight, and then clamp the final pixel value to the plot bounds (between
MARGINS.top and MARGINS.top + plotHeight) so the brush style always receives a
finite, valid number; update the mapping logic where yDomain and plotHeight are
used in BoxPlotPanelRenderer to use these safe values.
In `@packages/mosaic/src/chart/chart-settings/ChartSettings.tsx`:
- Line 55: The ChartSettingsViewSpecButton currently makes onClick optional
which permits rendering the button without an action; update the component prop
type in ChartSettingsViewSpecButton to require onClick (change FC<{onClick?: ()
=> void}> to FC<{onClick: () => void}>) so the click handler is enforced at
compile time, and adjust any call sites to pass a valid handler (or explicitly
pass a no-op if intentional) to preserve type-safety for action wiring.
In `@packages/mosaic/src/chart/chart-settings/ChartSettingsPanel.tsx`:
- Around line 55-56: The ViewSpecButton is currently rendered whenever spec is
truthy even if onViewSpec is undefined, making the clickable UI inert; change
the render condition to require both spec and onViewSpec (or alternatively
render a disabled/non-clickable variant) so ChartSettings.ViewSpecButton is only
interactive when the onViewSpec handler exists — update the conditional around
ChartSettings.ViewSpecButton to check both spec and onViewSpec (or render a
disabled state) to prevent a non-functional button.
In `@packages/mosaic/src/dashboard/MosaicDashboardSlice.ts`:
- Around line 711-715: isChartPanelConfig currently narrows by only comparing
panel.type to MOSAIC_DASHBOARD_CHART_PANEL_TYPE which misidentifies
LegacyPanelConfig entries; update isChartPanelConfig to also validate the
payload shape (e.g., that the panel has the ChartPanelConfig-specific fields
such as a non-null config/spec/chart object and expected keys like encoding or
mark) before returning true. In other words, inside isChartPanelConfig (and
still checking MOSAIC_DASHBOARD_CHART_PANEL_TYPE), add runtime checks that panel
has the ChartPanelConfig properties (for example existence and typeof checks for
panel.config or panel.spec and nested keys like encoding/mark) so the predicate
only narrows to ChartPanelConfig when the shape matches.
---
Outside diff comments:
In `@packages/mosaic/src/index.ts`:
- Around line 189-215: The codebase still mixes old VgPlot* names with new
Chart* names and lacks deprecation documentation for the alias
MOSAIC_DASHBOARD_VGPLOT_PANEL_TYPE; to fix, pick one approach and make changes
consistently: either fully migrate internals to Chart* (rename VgPlotChart ->
ChartChart or ChartComponent, RetainedVgPlotChart -> RetainedChart,
ChartBuilderVgPlotOutput -> ChartBuilderOutput, update imports in
MosaicDashboardVgPlotChart.tsx, MosaicDashboardVgPlotError.tsx, base-types.ts,
slice where panel type value 'vgplot' is used) or explicitly mark VgPlot* as
private/legacy by adding clear deprecation comments and export a compatibility
alias that logs the deprecation and points to the new Chart* names; update the
MOSAIC_DASHBOARD_VGPLOT_PANEL_TYPE handling to document the legacy name and
ensure all internal references are aligned with the chosen approach.
---
Nitpick comments:
In `@packages/mosaic/__tests__/ChartSettings.test.tsx`:
- Around line 45-74: The test currently iterates chartTypes with
chartTypes.forEach inside a single it block which hides which chartType fails;
refactor to a parameterized test using Jest's it.each (or test.each) so each
chartType runs as its own test case and failures show the failing value —
replace the it('renders with different chart types', ...) body with an
it.each([...]) that supplies each ChartConfig chartType and constructs the same
ChartConfig and renderToStaticMarkup call against ChartSettings.Root (keeping
tableName, columns, mockOnChange and the 'Test content' assertion) so failures
indicate the specific chartType.
In `@packages/mosaic/src/boxplot/BoxPlotClient.ts`:
- Around line 68-79: The rowsFromQueryResult function redundantly wraps the
toArray() result in Array.from; change the return to directly return the array
from toArray() (e.g., return (data as {toArray(): T[]}).toArray();), keeping the
existing guards intact so rowsFromQueryResult<T> still validates data and then
returns the toArray() result without the extra Array.from call.
In `@packages/mosaic/src/chart/chart-settings/ChartSettingsContext.tsx`:
- Around line 74-85: The current onChangeConfig implementation widens the
specific config type to the ChartConfig union via "as ChartConfig", losing type
safety; change onChangeConfig to preserve the generic config type (T) instead of
casting: make the callback generic over the concrete config type and the
specific setting key/value (referencing onChangeConfig, onChange, ChartConfig,
ChartSetting), accept a value typed as the specific ChartSetting property rather
than ChartSetting, and construct the new config while keeping its original
generic type so you can pass the correctly typed config to onChange without
using "as ChartConfig".
In `@packages/mosaic/src/chart/chart-settings/ChartTypeSelector.tsx`:
- Around line 21-59: ChartTypeSelector currently uses optional chaining on
chartTypes which yields an empty/selectable UI when chartTypes is undefined;
update the component to explicitly handle missing chartTypes by showing a
loading or error state and disabling the Select until data is present. Inside
ChartTypeSelector, check chartTypes before rendering Select (or inside
SelectTrigger/SelectContent): if chartTypes is undefined render a small loading
indicator or “Loading chart types…”/“No chart types available” message and set
the Select to disabled (or return early), and only map chartTypes to SelectItem
when it’s a defined non-empty array; ensure selectedChartType logic with
selectedChartType?.icon remains intact.
🪄 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: a3d2004b-4934-4df3-92dd-81cc67efcb34
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (79)
apps/sqlrooms-cli-ui/src/createChartToolDeps.tsexamples/deckgl-mosaic/src/components/filters/FiltersPanel.tsxpackages/mosaic/README.mdpackages/mosaic/__tests__/BoxPlotClient.test.tspackages/mosaic/__tests__/ChartSettings.test.tsxpackages/mosaic/__tests__/MosaicDashboardSlice.test.tspackages/mosaic/__tests__/chartTypes.test.tspackages/mosaic/__tests__/generateMosaicChartSpec.test.tspackages/mosaic/package.jsonpackages/mosaic/src/MosaicSlice.tspackages/mosaic/src/boxplot/BoxPlotClient.tspackages/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/ColumnSelector.tsxpackages/mosaic/src/chart-builders/MultiFieldSelector.tsxpackages/mosaic/src/chart-builders/createChartBuilderStore.tspackages/mosaic/src/chart-types/base-types.tspackages/mosaic/src/chart-types/box-plot/BoxPlotSettings.tsxpackages/mosaic/src/chart-types/box-plot/definition.tspackages/mosaic/src/chart-types/box-plot/renderer/BoxPlotPanelRenderer.tsxpackages/mosaic/src/chart-types/box-plot/renderer/constants.tspackages/mosaic/src/chart-types/box-plot/renderer/plot.tspackages/mosaic/src/chart-types/box-plot/renderer/useBoxPlotClient.tspackages/mosaic/src/chart-types/box-plot/renderer/utils.tspackages/mosaic/src/chart-types/box-plot/schema.tspackages/mosaic/src/chart-types/box-plot/spec.tspackages/mosaic/src/chart-types/bubble-chart/BubbleChartSettings.tsxpackages/mosaic/src/chart-types/bubble-chart/definition.tspackages/mosaic/src/chart-types/chart-config.tspackages/mosaic/src/chart-types/count-plot/CountPlotSettings.tsxpackages/mosaic/src/chart-types/count-plot/definition.tspackages/mosaic/src/chart-types/custom-spec/CustomSpecSettings.tsxpackages/mosaic/src/chart-types/custom-spec/definition.tspackages/mosaic/src/chart-types/custom-spec/spec.tspackages/mosaic/src/chart-types/errors.tspackages/mosaic/src/chart-types/heatmap/HeatmapSettings.tsxpackages/mosaic/src/chart-types/heatmap/definition.tspackages/mosaic/src/chart-types/histogram/HistogramSettings.tsxpackages/mosaic/src/chart-types/histogram/definition.tspackages/mosaic/src/chart-types/index.tspackages/mosaic/src/chart-types/line-chart/LineChartSettings.tsxpackages/mosaic/src/chart-types/line-chart/definition.tspackages/mosaic/src/chart-types/registry.tspackages/mosaic/src/chart-types/tool-types.tspackages/mosaic/src/chart-types/useChartTypeDefinition.tspackages/mosaic/src/chart/MosaicDashboardChart.tsxpackages/mosaic/src/chart/MosaicDashboardChartContent.tsxpackages/mosaic/src/chart/MosaicDashboardChartHeaderActions.tsxpackages/mosaic/src/chart/MosaicDashboardChartRenderer.tsxpackages/mosaic/src/chart/MosaicDashboardComponentChart.tsxpackages/mosaic/src/chart/MosaicDashboardVgPlotChart.tsxpackages/mosaic/src/chart/MosaicDashboardVgPlotError.tsxpackages/mosaic/src/chart/chart-settings/ChartSettings.tsxpackages/mosaic/src/chart/chart-settings/ChartSettingsContent.tsxpackages/mosaic/src/chart/chart-settings/ChartSettingsContext.tsxpackages/mosaic/src/chart/chart-settings/ChartSettingsPanel.tsxpackages/mosaic/src/chart/chart-settings/ChartSpecViewerPanel.tsxpackages/mosaic/src/chart/chart-settings/ChartTypeSelector.tsxpackages/mosaic/src/chart/chart-settings/useTableColumns.tspackages/mosaic/src/chart/useBrushSelectionParams.tsxpackages/mosaic/src/chart/useChartRetainer.tsxpackages/mosaic/src/chart/useGenerateSpec.tspackages/mosaic/src/dashboard/ChartSpecErrorDisplay.tsxpackages/mosaic/src/dashboard/MosaicDashboard.tsxpackages/mosaic/src/dashboard/MosaicDashboardSlice.tspackages/mosaic/src/dashboard/MosaicDashboardVgPlotPanelRenderer.tsxpackages/mosaic/src/dashboard/chart-settings/ChartSettingsPanel.tsxpackages/mosaic/src/dashboard/chart-settings/index.tspackages/mosaic/src/dashboard/defaultPanelRenderers.tspackages/mosaic/src/dashboard/generateMosaicChartSpec.tspackages/mosaic/src/dashboard/useGenerateSpec.tspackages/mosaic/src/dashboard/utils.tspackages/mosaic/src/index.tspackages/mosaic/src/profiler/MosaicDashboardProfilerPanelRenderer.tsxpython/sqlrooms-cli/sqlrooms/web/launcher.py
💤 Files with no reviewable changes (10)
- packages/mosaic/tests/generateMosaicChartSpec.test.ts
- packages/mosaic/src/dashboard/ChartSpecErrorDisplay.tsx
- packages/mosaic/src/dashboard/useGenerateSpec.ts
- packages/mosaic/src/dashboard/generateMosaicChartSpec.ts
- packages/mosaic/src/dashboard/chart-settings/index.ts
- packages/mosaic/src/dashboard/chart-settings/ChartSettingsPanel.tsx
- packages/mosaic/src/chart-types/box-plot/spec.ts
- packages/mosaic/src/chart-types/registry.ts
- packages/mosaic/src/dashboard/MosaicDashboardVgPlotPanelRenderer.tsx
- packages/mosaic/src/chart-types/errors.ts
…ypes and improve validation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/chart-types/box-plot/renderer/BoxPlotErrorBoundary.tsx`:
- Around line 37-39: The component BoxPlotErrorBoundary currently renders raw
error text via this.state.error.message in its render output; replace that UI
display with a generic user-facing message (e.g., "An unexpected error occurred
while rendering the chart") and remove any direct exposure of error details from
the DOM, while keeping the original error object sent to logging/telemetry (or
console.error) inside the componentDidCatch or error handling path for
diagnostics; update references in the render method and componentDidCatch to
ensure only generic text is shown to users and detailed error info is recorded
in telemetry/logging.
🪄 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: e99b2f0b-8ed6-4a13-80cb-d530409fbcd3
📒 Files selected for processing (12)
examples/deckgl-mosaic/src/components/filters/FiltersPanel.tsxexamples/deckgl-mosaic/src/components/filters/filterPlots.tspackages/mosaic/README.mdpackages/mosaic/src/boxplot/BoxPlotClient.tspackages/mosaic/src/chart-builders/ChartBuilderRoot.tsxpackages/mosaic/src/chart-types/box-plot/renderer/BoxPlotErrorBoundary.tsxpackages/mosaic/src/chart-types/box-plot/renderer/BoxPlotPanelRenderer.tsxpackages/mosaic/src/chart-types/box-plot/renderer/constants.tspackages/mosaic/src/chart-types/box-plot/renderer/useBoxPlotClient.tspackages/mosaic/src/chart-types/chart-config.tspackages/mosaic/src/chart-types/line-chart/LineChartSettings.tsxpackages/mosaic/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- examples/deckgl-mosaic/src/components/filters/filterPlots.ts
- packages/mosaic/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/mosaic/src/chart-types/box-plot/renderer/constants.ts
- packages/mosaic/src/chart-types/chart-config.ts
- packages/mosaic/src/index.ts
- packages/mosaic/src/chart-types/box-plot/renderer/BoxPlotPanelRenderer.tsx
- packages/mosaic/src/boxplot/BoxPlotClient.ts
- packages/mosaic/src/chart-types/box-plot/renderer/useBoxPlotClient.ts
Comment on lines
+37
to
+39
| <div className="mt-2 text-xs opacity-75"> | ||
| {this.state.error.message} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Avoid exposing raw error messages in end-user UI.
Rendering this.state.error.message can leak internal details (query text/system info). Show a generic message here and keep detailed diagnostics in logging/telemetry only.
Suggested change
- <div className="mt-2 text-xs opacity-75">
- {this.state.error.message}
- </div>
+ <div className="mt-2 text-xs opacity-75">
+ Please try refreshing the panel or adjusting chart settings.
+ </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="mt-2 text-xs opacity-75"> | |
| {this.state.error.message} | |
| </div> | |
| <div className="mt-2 text-xs opacity-75"> | |
| Please try refreshing the panel or adjusting chart settings. | |
| </div> |
🤖 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-types/box-plot/renderer/BoxPlotErrorBoundary.tsx`
around lines 37 - 39, The component BoxPlotErrorBoundary currently renders raw
error text via this.state.error.message in its render output; replace that UI
display with a generic user-facing message (e.g., "An unexpected error occurred
while rendering the chart") and remove any direct exposure of error details from
the DOM, while keeping the original error object sent to logging/telemetry (or
console.error) inside the componentDidCatch or error handling path for
diagnostics; update references in the render method and componentDidCatch to
ensure only generic text is shown to users and detailed error info is recorded
in telemetry/logging.
dmitriy-kostianetskiy
self-requested a review
May 14, 2026 12:32
dmitriy-kostianetskiy
approved these changes
May 14, 2026
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.
Solving #606
Summary by CodeRabbit
New Features
Improvements
Documentation
Dependencies