Repository navigation
fix(mcp): default chart previews to ascii - #39719
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 #39719 +/- ##
=======================================
Coverage 64.47% 64.47%
=======================================
Files 2566 2566
Lines 134027 134027
Branches 31121 31121
=======================================
Hits 86410 86410
Misses 46122 46122
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
Updates the MCP chart preview tool defaults so a standard get_chart_preview call returns an in-response text preview (instead of defaulting to a URL path that can fail), and ensures chart preview responses are subject to the response-size guard.
Changes:
- Defaulted
GetChartPreviewRequest.formatfromurltoasciiand updated schema/instructions text accordingly. - Removed
get_chart_previewfrom the response-size guard exclusion list. - Updated/added unit tests to reflect the new default and configuration behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
superset/mcp_service/chart/schemas.py |
Changes preview request default format and updates schema description text. |
superset/mcp_service/app.py |
Updates MCP “default instructions” text to reflect non-URL preview output. |
superset/mcp_service/mcp_config.py |
Removes get_chart_preview from response-size guard excluded tools. |
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py |
Updates unit test expectation for the new default format. |
tests/unit_tests/mcp_service/test_middleware.py |
Adds a test asserting chart preview is not excluded from size-guard config. |
Comments suppressed due to low confidence (1)
superset/mcp_service/chart/schemas.py:1760
- The
widthfield description still references "url/base64 formats", butGetChartPreviewRequest.formatno longer includes anybase64option. Please update the width/height descriptions (or the supported format list) so the schema docs don’t advertise a non-existent format.
),
)
width: int | None = Field(
default=800,
description="Preview image width in pixels (for url/base64 formats)",
|
The suggestion improves the test by verifying runtime behavior of the middleware factory with default config, ensuring get_chart_preview is size-checked and health_check remains excluded. tests/unit_tests/mcp_service/test_middleware.py |
aminghadersohi
left a comment
There was a problem hiding this comment.
The fix is correct and the test hardening is solid — good call on the follow-up commit to address the Copilot/Bito feedback.
A few pre-existing stale descriptions in URLPreview that were partially touched in this PR worth cleaning up as a follow-up:
URLPreview.width/height(schemas.py:1780-1781) still say "Image width/height in pixels" — but theurlformat now returns an Explore link, not an image.URLPreview.supports_interaction=False, description="Static image, no interaction"(schemas.py:1782-1784) contradicts the fact that the Explore URL is fully interactive.- The tool docstring in
get_chart_preview.py:978-989still describes the tool as URL/image-only ("URLs for LLM embedding", "screenshot endpoints", etc.) — this could mislead LLMs into expecting image output.
None of these are blockers — they're pre-existing and outside this PR's direct scope. Happy to see them cleaned up in a follow-up.
…rt-preview-supported-default # Conflicts: # tests/unit_tests/mcp_service/test_middleware.py
Code Review Agent Run #a7801eActionable 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 Richard's agent here. Thanks for calling out the stale chart preview descriptions. I opened a follow-up draft PR to address those notes: #39731 It updates the |
SUMMARY
get_chart_previewwas defaulting toformat=url, but URL-based screenshot previews are not supported by the tool. As a result, a normal/default call could fail withUnsupportedFormatbefore returning any useful preview.This PR changes the default preview format to
ascii, which is already supported, so callers get a text-based visual preview by default instead of an error.When a user asks the chatbot to show a visual preview of a chart, the LLM may call
get_chart_previewwithout explicitly setting a format. Before this change, that default path usedurland failed with:The chatbot could recover, but the first tool call still produced an error and the user did not get the preview they asked for.
get_chart_previewsupports multiple preview formats through format-specific strategies:urlreturns an Explore link.asciiqueries the chart data and renders a lightweight text preview.tablequeries the chart data and renders a text table.vega_litebuilds a Vega-Lite specification.The default format matters because callers often omit optional parameters. With the previous default, a normal call selected the URL path, which does not provide screenshot rendering and could fail with
UnsupportedFormat.With this change, the default path selects the existing ASCII strategy instead. That strategy uses the saved chart configuration, runs the chart data query, and returns preview content directly in the tool response. The response still includes the Explore URL for opening the full chart interactively.
Changes included:
GetChartPreviewRequest.formatdefault fromurltoascii.get_chart_previewis not described as image-URL-only.get_chart_previewfrom the response-size guard exclusion list.asciiandtablepreviews can contain chart data, so they should be protected by the normal response-size limits.This does not implement URL screenshot generation. The
urlformat still returns an explore link where supported, but it is no longer the default path.This also does not guarantee every chart type can produce an ASCII preview. Unsupported chart configurations can still return chart-specific errors. The fix is specifically for the unsupported default
urlformat.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. This is an MCP tool behavior change with text-based output.
TESTING INSTRUCTIONS
Run focused MCP tests:
Expected result:
Manual verification performed through the local chatbot flow:
Show me a visual preview of chart 11get_chart_previewformat=asciiUnsupportedFormatADDITIONAL INFORMATION