Repository navigation
feat: Rename bubble chart to scatter chart - #705
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR replaces bubble-chart with scatter-plot across the Mosaic charting system, adding a configurable point-size field, updating AI tool guidance, and refreshing test fixtures to use the new chart type throughout. ChangesScatter-chart Implementation and Adoption
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c4eaad22
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| **Individual dashboard chart tools:** | ||
| - create_dashboard_histogram, create_dashboard_line_chart, create_dashboard_box_plot, create_dashboard_bubble_chart, create_dashboard_count_plot, create_dashboard_heatmap | ||
| - create_dashboard_histogram, create_dashboard_line_chart, create_dashboard_box_plot, create_dashboard_scatter_chart, create_dashboard_count_plot, create_dashboard_heatmap |
There was a problem hiding this comment.
After this tool rename, the generated tools no longer include create_dashboard_bubble_chart, but repo-wide search still finds chart tool descriptions that tell the model to use bubble-chart for two-variable relationships (line-chart/tool.ts and count-plot/tool.ts, plus one heatmap “Do NOT use” hint). When the user asks for a correlation/relationship from those contexts, the model can be steered toward a non-existent chart type/tool instead of scatter-chart; update the remaining prompts as part of this rename.
Useful? React with 👍 / 👎.
![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: 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-types/heatmap/tool.ts (1)
31-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale "bubble-chart" reference should be "scatter-chart".
The description says "use bubble-chart" for individual point plots, but bubble-chart has been replaced by scatter-chart in this PR. This inconsistency could confuse the LLM when selecting appropriate tools.
📝 Proposed fix
-Do NOT use for: individual point plots (use bubble-chart), single variable distribution (use histogram), time trends (use line-chart).`, +Do NOT use for: individual point plots (use scatter-chart), single variable distribution (use histogram), time trends (use line-chart).`,🤖 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/heatmap/tool.ts` at line 31, Update the heatmap tool's description text that currently suggests "use bubble-chart" for individual point plots: change the reference from "bubble-chart" to "scatter-chart" in the heatmap tool's description string so the guidance matches the renamed chart type (replace the literal "bubble-chart" with "scatter-chart" inside the heatmap tool description).
🧹 Nitpick comments (1)
packages/mosaic/src/charts/chart-types/scatter-chart/definition.ts (1)
6-6: Use a scatter-plot icon forscatter-chart
The currentscatterChartChartTypeusesWorkflow(packages/mosaic/src/charts/chart-types/scatter-chart/definition.tsline 17), which reads as a process-flow icon rather than a scatter plot. lucide-react includes a dedicated scatter icon:ChartScatter.
- Change
import {Workflow} from 'lucide-react'toimport {ChartScatter} from 'lucide-react'- Set
icon: ChartScatter🤖 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/scatter-chart/definition.ts` at line 6, The scatter-chart is using the incorrect icon: replace the Workflow import and usage with the ChartScatter icon by changing the import from Workflow to ChartScatter and updating the scatterChartChartType's icon property from Workflow to ChartScatter; specifically update the import line that currently reads "import {Workflow} from 'lucide-react'" to "import {ChartScatter} from 'lucide-react'" and set icon: ChartScatter in the scatterChartChartType definition.
🤖 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/chart-types/scatter-chart/schema.ts`:
- Around line 7-10: The schema advertises an optional numeric "size" field but
the UI and runtime ignore it; update the code to wire size end-to-end or remove
it. Specifically, add a numeric-column selector to ScatterChartSettings.tsx that
binds to settings.size, update scatter-chart/spec.ts to validate/destructure
size (ensure it references a numeric column) and map it into the chart options
instead of the hardcoded r: 3 (use size to compute point radius), and update
scatter-chart/tool.ts documentation to match the implemented behaviour;
alternatively, if you choose not to support size, remove the size entry from the
schema.ts and tool docs and any references in spec.ts and settings UI.
---
Outside diff comments:
In `@packages/mosaic/src/charts/chart-types/heatmap/tool.ts`:
- Line 31: Update the heatmap tool's description text that currently suggests
"use bubble-chart" for individual point plots: change the reference from
"bubble-chart" to "scatter-chart" in the heatmap tool's description string so
the guidance matches the renamed chart type (replace the literal "bubble-chart"
with "scatter-chart" inside the heatmap tool description).
---
Nitpick comments:
In `@packages/mosaic/src/charts/chart-types/scatter-chart/definition.ts`:
- Line 6: The scatter-chart is using the incorrect icon: replace the Workflow
import and usage with the ChartScatter icon by changing the import from Workflow
to ChartScatter and updating the scatterChartChartType's icon property from
Workflow to ChartScatter; specifically update the import line that currently
reads "import {Workflow} from 'lucide-react'" to "import {ChartScatter} from
'lucide-react'" and set icon: ChartScatter in the scatterChartChartType
definition.
🪄 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: 1daec1dd-93d4-419e-96eb-4338ce07858d
📒 Files selected for processing (18)
packages/mosaic/__tests__/ChartSettings.test.tsxpackages/mosaic/__tests__/MosaicDashboardRuntimeIssue.test.tspackages/mosaic/__tests__/chart-runtime.test.tspackages/mosaic/__tests__/dashboard-ai.test.tspackages/mosaic/src/ai/ai.tspackages/mosaic/src/charts/chart-types/bubble-chart/definition.tspackages/mosaic/src/charts/chart-types/chart-config.tspackages/mosaic/src/charts/chart-types/createDefaultChartTypes.tspackages/mosaic/src/charts/chart-types/heatmap/tool.tspackages/mosaic/src/charts/chart-types/histogram/tool.tspackages/mosaic/src/charts/chart-types/index.tspackages/mosaic/src/charts/chart-types/mosaicChartTypes.tspackages/mosaic/src/charts/chart-types/scatter-chart/ScatterChartSettings.tsxpackages/mosaic/src/charts/chart-types/scatter-chart/definition.tspackages/mosaic/src/charts/chart-types/scatter-chart/schema.tspackages/mosaic/src/charts/chart-types/scatter-chart/spec.tspackages/mosaic/src/charts/chart-types/scatter-chart/tool.tspackages/mosaic/src/index.ts
💤 Files with no reviewable changes (1)
- packages/mosaic/src/charts/chart-types/bubble-chart/definition.ts
ilyabo
reviewed
Jun 10, 2026
ilyabo
approved these changes
Jun 10, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47dc228d82
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - create_dashboard_line_chart - trends over time or ordered variable (use with aggregations for >10k rows) | ||
| - create_dashboard_box_plot - compare distributions across categories | ||
| - create_dashboard_bubble_chart - relationship between two numeric columns (avoid for >10k rows, use heatmap instead) | ||
| - create_dashboard_scatter_chart - relationship between two numeric columns (avoid for >10k rows, use heatmap instead) |
There was a problem hiding this comment.
In dashboard-agent contexts that follow this tool list, the registered tool name is derived in createChartTools from chartType.id.replace(/-/g, '_'); with the new id scatter-plot, the actual tool is create_dashboard_scatter_plot, not create_dashboard_scatter_chart. When a user asks for a relationship/correlation chart, these instructions steer the model to call a tool that is not present, so it cannot create the scatter panel via the chart tool.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/mosaic/src/charts/chart-types/scatter-plot/definition.ts (2)
17-17: ⚡ Quick winConsider a more conventional scatter plot icon.
The
Workflowicon is typically associated with processes or DAGs rather than scatter plots. Consider usingScatter(if available in lucide-react) or another icon that better represents point-based visualization, such asCircleorDotSquare.🤖 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/scatter-plot/definition.ts` at line 17, The scatter-plot definition currently uses the Workflow icon; replace it with a point-based icon (e.g., Scatter, Circle, or DotSquare) from lucide-react to better represent a scatter plot: update the icon property in the scatter plot definition (the "icon" field set to Workflow) to one of the suggested icon symbols and adjust the import to pull that icon (e.g., import { Scatter } from 'lucide-react') so the chosen icon is used consistently.
24-25: ⚡ Quick winInconsistent capitalization: "Scatter charts" vs. "Scatter Plot".
The error message uses "Scatter charts" while the chart label at line 15 uses "Scatter Plot". Consider using "Scatter plot charts" or "Scatter plots" for consistency.
🤖 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/scatter-plot/definition.ts` around lines 24 - 25, Update the inconsistent capitalization in the scatter plot chart definition by making the 'reason' string match the chart label "Scatter Plot" formatting; locate the scatter plot definition file and the object with the reason field (in packages/mosaic/src/charts/chart-types/scatter-plot/definition.ts) and change 'Scatter charts render one point per row...' to use "Scatter Plot" or "Scatter plots" (e.g., "Scatter plots render one point per row...") so the terminology and capitalization are consistent with the label "Scatter Plot".packages/mosaic/src/charts/chart-types/scatter-plot/spec.ts (2)
21-35: ⚖️ Poor tradeoffConsider a more specific type for
dotMark.
Record<string, unknown>is quite loose. While this pattern may match other chart specs in the codebase, consider defining a more specific interface for the dot mark configuration to improve type safety and catch potential errors at compile time.🤖 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/scatter-plot/spec.ts` around lines 21 - 35, dotMark is currently typed as Record<string, unknown>, which is too permissive; replace it with a specific interface (e.g., DotMarkSpec) that lists the expected fields (mark: string, data: { from: string; filterBy: string }, x: string, y: string, fill: string, fillOpacity: number, r: string|number) and use that type for dotMark so TypeScript enforces correct properties and types; update any assignments that set dotMark.r (the size logic using sizeColumn and DEFAULT_POINT_SIZE) to match the r type and adjust xColumn.name, yColumn.name, FG_COLOR references to fit the new interface.
90-100: ⚖️ Poor tradeoffOptional: Simplify
sizeColumninitialization.The current pattern uses
letwith conditional assignment. Consider a more functional approach:♻️ Suggested refactor
- // Validate size field if provided - let sizeColumn = undefined; - if (size) { - sizeColumn = dataTable.columns.find((col) => col.name === size); - if (!sizeColumn) { - throw new MissingColumnsError([size]); - } - if (!isNumericType(sizeColumn.type)) { - throw new InvalidColumnTypeError([sizeColumn.name], 'numeric'); - } - } + // Validate size field if provided + const sizeColumn = size + ? (() => { + const col = dataTable.columns.find((col) => col.name === size); + if (!col) { + throw new MissingColumnsError([size]); + } + if (!isNumericType(col.type)) { + throw new InvalidColumnTypeError([col.name], 'numeric'); + } + return col; + })() + : undefined;🤖 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/scatter-plot/spec.ts` around lines 90 - 100, Replace the mutable let pattern for sizeColumn with a single const initialized functionally: assign const sizeColumn = size ? dataTable.columns.find(col => col.name === size) : undefined; then keep the existing validations (if size and !sizeColumn throw MissingColumnsError([size]); if sizeColumn and !isNumericType(sizeColumn.type) throw InvalidColumnTypeError([sizeColumn.name], 'numeric')); update references accordingly to remove the mutable reassignment of sizeColumn.
🤖 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.
Nitpick comments:
In `@packages/mosaic/src/charts/chart-types/scatter-plot/definition.ts`:
- Line 17: The scatter-plot definition currently uses the Workflow icon; replace
it with a point-based icon (e.g., Scatter, Circle, or DotSquare) from
lucide-react to better represent a scatter plot: update the icon property in the
scatter plot definition (the "icon" field set to Workflow) to one of the
suggested icon symbols and adjust the import to pull that icon (e.g., import {
Scatter } from 'lucide-react') so the chosen icon is used consistently.
- Around line 24-25: Update the inconsistent capitalization in the scatter plot
chart definition by making the 'reason' string match the chart label "Scatter
Plot" formatting; locate the scatter plot definition file and the object with
the reason field (in
packages/mosaic/src/charts/chart-types/scatter-plot/definition.ts) and change
'Scatter charts render one point per row...' to use "Scatter Plot" or "Scatter
plots" (e.g., "Scatter plots render one point per row...") so the terminology
and capitalization are consistent with the label "Scatter Plot".
In `@packages/mosaic/src/charts/chart-types/scatter-plot/spec.ts`:
- Around line 21-35: dotMark is currently typed as Record<string, unknown>,
which is too permissive; replace it with a specific interface (e.g.,
DotMarkSpec) that lists the expected fields (mark: string, data: { from: string;
filterBy: string }, x: string, y: string, fill: string, fillOpacity: number, r:
string|number) and use that type for dotMark so TypeScript enforces correct
properties and types; update any assignments that set dotMark.r (the size logic
using sizeColumn and DEFAULT_POINT_SIZE) to match the r type and adjust
xColumn.name, yColumn.name, FG_COLOR references to fit the new interface.
- Around line 90-100: Replace the mutable let pattern for sizeColumn with a
single const initialized functionally: assign const sizeColumn = size ?
dataTable.columns.find(col => col.name === size) : undefined; then keep the
existing validations (if size and !sizeColumn throw MissingColumnsError([size]);
if sizeColumn and !isNumericType(sizeColumn.type) throw
InvalidColumnTypeError([sizeColumn.name], 'numeric')); update references
accordingly to remove the mutable reassignment of sizeColumn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44678765-71b4-4fff-a5da-bdc4bb929993
📒 Files selected for processing (19)
packages/mosaic/__tests__/ChartSettings.test.tsxpackages/mosaic/__tests__/MosaicDashboardRuntimeIssue.test.tspackages/mosaic/__tests__/chart-runtime.test.tspackages/mosaic/__tests__/dashboard-ai.test.tspackages/mosaic/src/ai/ai.tspackages/mosaic/src/charts/chart-types/chart-config.tspackages/mosaic/src/charts/chart-types/count-plot/tool.tspackages/mosaic/src/charts/chart-types/createDefaultChartTypes.tspackages/mosaic/src/charts/chart-types/heatmap/tool.tspackages/mosaic/src/charts/chart-types/histogram/tool.tspackages/mosaic/src/charts/chart-types/index.tspackages/mosaic/src/charts/chart-types/line-chart/tool.tspackages/mosaic/src/charts/chart-types/mosaicChartTypes.tspackages/mosaic/src/charts/chart-types/scatter-plot/ScatterPlotSettings.tsxpackages/mosaic/src/charts/chart-types/scatter-plot/definition.tspackages/mosaic/src/charts/chart-types/scatter-plot/schema.tspackages/mosaic/src/charts/chart-types/scatter-plot/spec.tspackages/mosaic/src/charts/chart-types/scatter-plot/tool.tspackages/mosaic/src/index.ts
✅ Files skipped from review due to trivial changes (6)
- packages/mosaic/src/charts/chart-types/scatter-plot/schema.ts
- packages/mosaic/src/charts/chart-types/line-chart/tool.ts
- packages/mosaic/tests/ChartSettings.test.tsx
- packages/mosaic/tests/chart-runtime.test.ts
- packages/mosaic/src/charts/chart-types/histogram/tool.ts
- packages/mosaic/src/ai/ai.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/mosaic/src/charts/chart-types/createDefaultChartTypes.ts
- packages/mosaic/src/charts/chart-types/chart-config.ts
- packages/mosaic/tests/dashboard-ai.test.ts
- packages/mosaic/src/charts/chart-types/index.ts
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
Removed Features
Improvements