Repository navigation
fix(mcp): clarify chart preview URL metadata - #39731
richardfogaca merged 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39731 +/- ##
==========================================
- Coverage 64.49% 64.49% -0.01%
==========================================
Files 2566 2566
Lines 133960 133958 -2
Branches 31116 31115 -1
==========================================
- Hits 86394 86392 -2
Misses 46071 46071
Partials 1495 1495
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates MCP chart preview metadata/documentation to reflect that URL previews now return interactive Explore links (not screenshot image URLs), and clarifies the available preview formats.
Changes:
- Update
URLPreviewschema descriptions for width/height and defaultsupports_interaction=True - Refresh tool docstring and MCP default instructions to describe supported preview formats (ASCII/URL/table/Vega-Lite)
- Adjust unit tests to expect Explore URLs and interactive defaults
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py | Updates expectations to Explore URL format and interactive default. |
| superset/mcp_service/chart/tool/get_chart_preview.py | Revises docstring to describe supported preview output types beyond screenshots. |
| superset/mcp_service/chart/schemas.py | Clarifies URL preview metadata and defaults supports_interaction to true. |
| superset/mcp_service/app.py | Updates general MCP instructions to reflect new chart preview output options. |
aminghadersohi
left a comment
There was a problem hiding this comment.
Review by Claude (AI assistant) on behalf of @preset-io/amin
Thanks for the cleanup! A few things worth discussing before this merges:
Scope of PR: The description says "schema/documentation metadata only" but the diff includes API-breaking field removals from ChartInfo (owners, changed_by, created_by), a new sanitization_warnings field + model validator on GenerateChartRequest, and a change to the exception handler in a production path. It would help future reviewers and changelog readers to call these out explicitly in the summary.
Exception narrowing (get_chart_preview.py, outer except clause): The original except Exception ensured callers always received a structured ChartError. The new explicit union omits IndexError, RuntimeError, PermissionError, etc. An unexpected exception type will now propagate unhandled. Consider either restoring the catch-all with an inner isinstance guard for re-raise, or adding Exception as a final catch with a distinct log level for unexpected types.
Default format change: GetChartPreviewRequest.format silently changes default from "url" to "ascii". Clients relying on the default will now get different output. Worth a note in SUMMARY and UPDATING.md if this is intentional.
created_by_fk removal: Any client currently passing {"col": "created_by_fk", ...} to list_charts will get a Pydantic validation error instead of results. If removing this is intentional for privacy, a migration note ("use search parameter instead") would help.
Test host: Copilot's comment about localhost:8088 in the unit test is valid and easy to fix — the dimension test a few lines below already uses example.com.
None check after typed required field (get_chart_preview.py): The if request.identifier is None guard is dead code — identifier is typed int | str and marked required, so it can never reach that branch.
The privacy guardrails (app.py instructions, ChartInfo field removals) are well-designed and appropriate. The GenerateChartRequest aliases and sanitization_warnings design are reasonable. The main blocker is the exception handler regression.
Code Review Agent Run #9e46a0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
Thanks @aminghadersohi. I checked #39731 against the current I pushed I agree the exception-handler concern is worth a focused follow-up, but since it predates this metadata cleanup, I’d prefer to handle it separately so this PR stays scoped to the chart preview wording/schema-test updates. |
Code Review Agent Run #1cd4b5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
aminghadersohi
left a comment
There was a problem hiding this comment.
Thanks for the quick fix on the test host — is the right call there. Approving; the exception narrowing concern is worth a follow-up but not a blocker given FastMCP's top-level error handling. LGTM otherwise.
aminghadersohi
left a comment
There was a problem hiding this comment.
Thanks for the quick fix on the test host — using example.com is the right call there. Approving; the exception narrowing concern is worth a follow-up but not a blocker given FastMCP's top-level error handling. LGTM otherwise.
SUMMARY
Clarifies MCP chart preview metadata and guidance now that URL previews return interactive Explore links rather than static screenshot image URLs.
This follow-up to #39719 updates:
URLPreviewwidth/height descriptions to refer to requested Explore viewport dimensionsURLPreview.supports_interactionto default totrueBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; MCP schema/documentation metadata only.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION