Repository navigation
fix: dashboard chart panel updates - #736
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 (2)
📝 WalkthroughWalkthroughAdds an optional ChangesDashboard chart panel update via panelId
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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 |
ilyabo
changed the title
![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: 40f7e0c3b6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| panelId: z | ||
| .string() | ||
| .optional() | ||
| .describe( | ||
| 'Optional existing dashboard panel ID to update instead of creating a new panel.', | ||
| ), |
There was a problem hiding this comment.
This adds panelId to BaseChartToolInput, which is exported from @sqlrooms/mosaic, so it changes the package's public API. The repo instructions in /workspace/sqlrooms/AGENTS.md say public API changes in @sqlrooms/* packages must update the package README, but packages/mosaic/README.md's Dashboard AI Tools section still does not describe that chart tools can update an existing panel by passing panelId; downstream users wiring these tools directly would not discover the new update path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
packages/mosaic/src/charts/chart-types/tool-types.ts (1)
10-17: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd tsdoc to exported type.
AddChartFunctionArgsis used across multiple chart tools but lacks tsdoc. As per coding guidelines, exported types and functions inpackages/**/*.{ts,tsx}should include tsdoc.📝 Suggested tsdoc
+/** + * Arguments for adding or updating a chart panel. + * + * `@property` tableName - Optional table name (required for worksheet charts, inherited for dashboard charts) + * `@property` panelId - Optional panel ID to update an existing panel instead of creating new + * `@property` title - Chart title + * `@property` config - Chart configuration object + */ type AddChartFunctionArgs = {🤖 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/tool-types.ts` around lines 10 - 17, Add tsdoc comments to the exported types AddChartFunctionArgs and AddChartFunction. For AddChartFunctionArgs, document what the type represents and provide JSDoc-style descriptions for each property including tableName, panelId, title, and config. For AddChartFunction, document its purpose as a function type. Follow the project's tsdoc conventions using /** */ comment blocks placed directly above each type definition.Source: Coding guidelines
packages/mosaic/src/ai/tool-schemas.ts (1)
3-19: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd tsdoc to exported schema.
BaseChartToolInputis exported and imported by multiple chart tools, but lacks tsdoc. As per coding guidelines, exported types and functions inpackages/**/*.{ts,tsx}should include tsdoc.📝 Suggested tsdoc
+/** + * Base input schema for chart AI tools. + * + * `@property` tableName - Required table name to create the chart from + * `@property` title - Optional title for the chart artifact (defaults to "Chart") + * `@property` panelId - Optional existing dashboard panel ID to update instead of creating a new panel + * `@property` reasoning - Brief rationale for the chart choice + */ export const BaseChartToolInput = z.object({🤖 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/tool-schemas.ts` around lines 3 - 19, The exported BaseChartToolInput schema is missing tsdoc documentation, which is required by coding guidelines for all exported types in packages. Add a tsdoc comment block above the BaseChartToolInput constant that describes its purpose as a base input schema for chart tools, and briefly documents the main fields including tableName, title, panelId, and reasoning. Ensure the documentation follows standard tsdoc conventions with appropriate JSDoc tags.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/mosaic/__tests__/dashboard-ai-tools.test.ts (1)
76-128: ⚡ Quick winConsider adding edge case test coverage.
The test covers the happy path where the panel exists and is the correct type. Consider adding tests for edge cases:
panelIdprovided but panel doesn't exist (getPanelreturnsundefined)panelIdprovided but panel has wrong type (notMOSAIC_DASHBOARD_CHART_PANEL_TYPE)These edge cases would verify the validation logic in the update path.
📋 Suggested edge case tests
it('throws error when panelId references non-existent panel', async () => { const dashboardAdapter: DashboardAiAdapter = { getPanel: () => undefined, // Panel not found setSelectedTable: () => {}, addPanel: jest.fn(), updatePanel: jest.fn(), removePanel: () => {}, }; const databaseAdapter: DatabaseAiAdapter = { getTables: () => [], findTable: () => ({ tableName: 'earthquakes', columns: [{name: 'depth', type: 'DOUBLE'}], }) as any, }; const tools = createDashboardAiTools({ databaseAdapter, dashboardAdapter, }); const result = await (tools.create_dashboard_panel_histogram as any).execute({ tableName: 'earthquakes', panelId: 'non-existent', title: 'Depth histogram', settings: {field: 'depth'}, reasoning: 'update non-existent panel', }); expect(result.success).toBe(false); expect(result.errorMessage).toContain('Panel not found'); }); it('throws error when panelId references wrong panel type', async () => { const dashboardAdapter: DashboardAiAdapter = { getPanel: () => ({ id: 'panel-1', type: 'data-table-explorer', // Wrong type title: 'Data Explorer', config: {}, }), setSelectedTable: () => {}, addPanel: jest.fn(), updatePanel: jest.fn(), removePanel: () => {}, }; const databaseAdapter: DatabaseAiAdapter = { getTables: () => [], findTable: () => ({ tableName: 'earthquakes', columns: [{name: 'depth', type: 'DOUBLE'}], }) as any, }; const tools = createDashboardAiTools({ databaseAdapter, dashboardAdapter, }); const result = await (tools.create_dashboard_panel_histogram as any).execute({ tableName: 'earthquakes', panelId: 'panel-1', title: 'Depth histogram', settings: {field: 'depth'}, reasoning: 'update wrong type panel', }); expect(result.success).toBe(false); expect(result.errorMessage).toContain('not a chart panel'); });🤖 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-tools.test.ts` around lines 76 - 128, The test suite for the dashboard AI tools in dashboard-ai-tools.test.ts only covers the happy path scenario where the panel exists and is the correct type. Add two new test cases to cover edge cases: one where getPanel returns undefined when a non-existent panelId is provided to create_dashboard_panel_histogram, and verify it returns an error with a message indicating the panel was not found; another where getPanel returns a panel with the wrong type (not MOSAIC_DASHBOARD_CHART_PANEL_TYPE) and verify it returns an error indicating the panel is not a chart panel. Both tests should verify that the result has success set to false and contains appropriate error messages in the errorMessage property.
🤖 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/ai/tool-schemas.ts`:
- Around line 3-19: The exported BaseChartToolInput schema is missing tsdoc
documentation, which is required by coding guidelines for all exported types in
packages. Add a tsdoc comment block above the BaseChartToolInput constant that
describes its purpose as a base input schema for chart tools, and briefly
documents the main fields including tableName, title, panelId, and reasoning.
Ensure the documentation follows standard tsdoc conventions with appropriate
JSDoc tags.
In `@packages/mosaic/src/charts/chart-types/tool-types.ts`:
- Around line 10-17: Add tsdoc comments to the exported types
AddChartFunctionArgs and AddChartFunction. For AddChartFunctionArgs, document
what the type represents and provide JSDoc-style descriptions for each property
including tableName, panelId, title, and config. For AddChartFunction, document
its purpose as a function type. Follow the project's tsdoc conventions using /**
*/ comment blocks placed directly above each type definition.
---
Nitpick comments:
In `@packages/mosaic/__tests__/dashboard-ai-tools.test.ts`:
- Around line 76-128: The test suite for the dashboard AI tools in
dashboard-ai-tools.test.ts only covers the happy path scenario where the panel
exists and is the correct type. Add two new test cases to cover edge cases: one
where getPanel returns undefined when a non-existent panelId is provided to
create_dashboard_panel_histogram, and verify it returns an error with a message
indicating the panel was not found; another where getPanel returns a panel with
the wrong type (not MOSAIC_DASHBOARD_CHART_PANEL_TYPE) and verify it returns an
error indicating the panel is not a chart panel. Both tests should verify that
the result has success set to false and contains appropriate error messages in
the errorMessage property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a94de26b-8a01-4c5a-873b-6ef6d501376c
📒 Files selected for processing (11)
packages/mosaic/__tests__/dashboard-ai-tools.test.tspackages/mosaic/src/ai/dashboard/createDashboardAgentTool.tspackages/mosaic/src/ai/dashboard/createDashboardChartTools.tspackages/mosaic/src/ai/tool-schemas.tspackages/mosaic/src/charts/chart-types/box-plot/tool.tspackages/mosaic/src/charts/chart-types/count-plot/tool.tspackages/mosaic/src/charts/chart-types/heatmap/tool.tspackages/mosaic/src/charts/chart-types/histogram/tool.tspackages/mosaic/src/charts/chart-types/line-chart/tool.tspackages/mosaic/src/charts/chart-types/scatter-plot/tool.tspackages/mosaic/src/charts/chart-types/tool-types.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8da827c3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
|
|
||
| dashboardAdapter.updatePanel(panelId, {title, config}); |
There was a problem hiding this comment.
When a chart tool is called with panelId for an update but the user did not ask to rename the panel, title is still optional/defaulted by the shared schema, so this always patches the existing panel title to the default "Chart" (or undefined for direct execute callers). In update/edit/fix flows like “change the existing histogram to use depth”, this clobbers the user's current panel title even though only the chart config should change; use the existing panel.title unless a title was explicitly provided.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2de7c8444b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dashboardAdapter.updatePanel(panelId, { | ||
| title: title ?? panel.title, | ||
| config, | ||
| }); |
There was a problem hiding this comment.
In direct createDashboardAiTools usage, the new panelId update path validates settings against the tool's tableName, but this patch only changes the panel title/config. I checked MosaicDashboardChartRenderer, and chart panels render from dashboard.selectedTable, so updating a panel for a different table leaves it rendering against the previous selected table (often as missing-column errors) even though the tool returns success; set the dashboard selected table from the tool input before applying the update.
Useful? React with 👍 / 👎.
Summary
Fix dashboard chart tools so agent-driven dashboard edits can update an existing chart panel instead of always adding a duplicate panel.
What changed
panelIdto shared chart tool input.panelIdthrough all built-in Mosaic chart tools.updatePanelwhenpanelIdis supplied, while preserving add-panel behavior when omitted.panelIdfor edit/update/fix requests.panelId.Why
After dashboard requests were routed through
dashboard_agent, update-style prompts such as "change the existing histogram to use depth" could list panel IDs but had no chart update path. The agent would create a duplicate panel instead of mutating the intended panel.Validation
turbo lint --filter=@sqlrooms/* --knipturbo typecheck(51 tasks successful)turbo test(24 tasks successful)Summary by CodeRabbit
New Features
panelId(instead of always creating new panels).titlepreserves the existing panel title; chart tool wiring was updated across chart types.Documentation