feat(mcp): render charts as native interactive widgets via MCP Apps - #43483
Draft
aminghadersohi wants to merge 57 commits into
Draft
feat(mcp): render charts as native interactive widgets via MCP Apps#43483aminghadersohi wants to merge 57 commits into
aminghadersohi wants to merge 57 commits into
Conversation
Add a `render_chart` MCP tool that returns chart data plus a `_meta.ui.resourceUri` descriptor so MCP Apps hosts (Claude, ChatGPT, VS Code Copilot, Cursor, Goose, ...) render the result as a real, interactive ECharts visualization inline in the conversation instead of a prose summary — the SEP-1865 MCP Apps standard. Highlights: - `render_chart` / `render_chart_requery` tools wrap the shared `get_chart_data_core`, preserving the existing Chart/read RBAC, dataset access, guest scoping and RLS on every call (incl. widget drill-down). - `ui://superset/chart-viewer/v1` resource serves a self-contained React + ECharts widget (line/bar/area/big-number/table + styled-table fallback) with animated view morphing, click-to-drill, brush-to-zoom, and an "ask about this" model-context hook. - `@tool(meta=...)` plumbing threads `_meta` onto tool descriptors. - Scope the structured-content stripper via a keep-list so widget tools retain `structuredContent`; pin the widget tools past tool-search. - The widget never renders sample data when embedded (shows a connection error), surfaces ChartError states, and treats unknown host capabilities as unsupported. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ECharts renders a tooltip formatter's return value as HTML. formatFull() falls back to String(value) for non-numeric input, so an unescaped data value could inject markup into the tooltip. Escape it, and harden escapeHtml() to cover quotes. Adds a regression test asserting series names, axis values, dimension labels and values are all escaped while ECharts' own marker markup is preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MANIFEST.in did not cover superset/mcp_service, so the built widget bundle was excluded from sdist/wheel builds — a pip-installed Superset would serve the 'not built' fallback instead of the chart widget. Include only the built artifact, not the widget source or node_modules. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The widget hardcoded its palette (#20A7C9 — Superset's *previous* primary color), so charts rendered in a chat client drifted from the customer's configured branding. Consistency is the reason customers asked for Superset theming, and it is the main complaint about AI-generated charts: every regeneration looks different. render_chart now forwards an allow-listed subset of the instance's antd design tokens (colorPrimary, colorLink, status colors, fontFamily) on the tool result, and the widget uses them for its accent, font and the lead categorical series color. Falls back to the built-in palette when a deployment has no theme configured. Only presentational tokens are forwarded — no logo URLs, no secrets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Committing dist/index.html put an 836 KB minified build artifact in the source tree. That is out of step with how this repo already handles built frontend assets — superset/static is gitignored (zero tracked files) and shipped via MANIFEST.in at packaging time — and it inlines third-party code (React, ECharts, d3), which a source tree should not carry. Gitignore the build output and rely on the same packaging path. The ui:// resource already degrades to a placeholder page that tells the operator to run the build, so a source checkout stays functional. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The widget's tests and build do not run in CI, and no release step produces the bundle that MANIFEST.in expects. Document both, plus the fact that the widget's location inside the Python package differs from every other npm project in the repo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both widget tools are typed `-> ChartData | ChartError`, and FastMCP wraps
union returns in a synthetic envelope, so the payload on the wire is
`{"result": {...}}` (the outputSchema carries `x-fastmcp-wrap-result`).
`coerceToolResultData` returned that verbatim and `isChartData()` tests for
top-level `columns`/`data`, so the predicate was always false: the widget
rendered an error while its data sat one level down. The same wrapping hid
`ChartError` payloads, so a not-found produced neither data nor an error and
the widget spun forever.
Unwrap when `result` is the sole key, leaving any other payload untouched.
The existing contract test asserted the *unwrapped* shape — an assumption
about FastMCP's behavior rather than the observed wire format — which is why
it stayed green through the defect. Corrected to the real shape, with cases
for the wrapped form, the unwrapped form, and a payload where `result` is not
the only key. Verified end-to-end by capturing structuredContent from a live
in-process FastMCP call and parsing it with the widget's own bridge.
Reported by a sibling session testing against a live local MCP server.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keep-list tools keep their `outputSchema` advertised, but the stripper's
generic exception handler returned a text-only ToolResult with no
structured content. For every other tool that is harmless — their schema
was already stripped — but for `render_chart` / `render_chart_requery` a
strict client rejects the whole call:
MCP error -32600: Tool render_chart has an output schema but did not
return structured content
So every server-side error surfaced as an opaque protocol error and the
real cause was invisible to the user (observed in Claude Desktop).
Emit an `MCPBaseError`-shaped payload wrapped in the `result` envelope for
keep-list tools, matching the declared `ChartData | ChartError` union, so
failures arrive as readable messages the widget renders as its error state.
Reported by a sibling session testing against a live local MCP server.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rols The chart-viewer widget rendered the service's <UNTRUSTED-CONTENT> data-boundary markers literally in the title, insight bar, big-number label, and drill message; strip them at every display site while leaving escaping to React. Add user-driven sizing: a drag-to-resize handle, a maximize toggle that first requests the host's fullscreen display mode and falls back to growing in place, and a size-changed report on data load so hosts that honor it can size the frame. Bump the widget resource URI to ui://superset/chart-viewer/v2: MCP Apps hosts cache the bundle per conversation keyed by URI, so an unchanged URI keeps serving stale copies after the widget changes.
…s' into research-mcp-native-viz-clients # Conflicts: # superset/mcp_service/chart/resources/chart_viewer.py # superset/mcp_service/chart/resources/chart_viewer/src/App.tsx # superset/mcp_service/chart/resources/chart_viewer/src/adapter.ts # superset/mcp_service/chart/resources/chart_viewer/src/bridge.ts # superset/mcp_service/chart/resources/chart_viewer/src/format.test.ts
Everything about the widget is verified programmatically except whether it renders. Only a person with a real MCP Apps host can close that, so this is the minimal path from a clean machine to a screenshot, plus the 12 checks worth walking and what to capture. Leads with the per-conversation bundle caching rule, which is the single biggest time sink when testing bundle changes, and states plainly that nothing here may be marked verified from wire-level evidence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The adapter suite asserts the shape of the option object, which keeps passing even if ECharts rejects it at runtime — the blind spot that let the ECharts 5 -> 6 major bump land visually unverified. These tests call setOption on a live instance, so an option key ECharts no longer accepts, an unregistered chart/component, or a breaking series contract change fails here rather than in a host. Covers single- and multi-metric line/bar/area, the activeMetrics subset, categorical bars, the big-number sparkline, empty data, and bar->line->area switching on one instance (the universalTransition morph path). jsdom has no canvas, so tests register the SVG renderer. That exercises the same option parsing, series construction and coordinate system; pixel output remains out of scope and is covered by host verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The MCP Apps chart widget under superset/mcp_service/chart/resources/chart_viewer/ was not referenced by any workflow, so its vitest suite and Vite build never ran in CI and would have rotted silently. Adds a path-filtered workflow modelled on superset-websocket.yml: npm ci, npm test, npm run build (which also runs `tsc --noEmit`), plus an explicit assertion that the single-file dist/index.html bundle was emitted, since that file is what the ui://superset/chart-viewer resource serves. Pins Node via a new .nvmrc matching the repo-wide v24.16.0. Co-Authored-By: Claude <noreply@anthropic.com>
MANIFEST.in ships the widget's dist/index.html, but nothing produced it at package time, so wheels/sdists shipped without the widget and the ui://superset/chart-viewer resource fell back to a placeholder. Investigated how superset/static/assets is produced: it is NOT built by setup.py or pyproject.toml. The frontend build is a separate step owned by the release process (RELEASING/README.md's "Create the distribution" block, run before `python -m build`) and by the Dockerfile's superset-node stage. The widget build is wired into the former, next to the existing frontend build. Verified locally with setuptools 80.9.0 that the MANIFEST.in entry works: the wheel contains dist/index.html when it exists, and the sdist contains only dist/index.html — not package.json or src/ — so the widget cannot be rebuilt from a released tarball and must be built beforehand. The Docker image gap is left open and documented rather than papered over: the Dockerfile has no widget build step, so official images still serve the placeholder. The README spells out exactly what closing it requires. Co-Authored-By: Claude <noreply@anthropic.com>
Investigated whether the server needs to declare the MCP Apps extension during
initialize. It already does, and not from Superset code: FastMCP's low-level
server unconditionally attaches
"extensions": {"io.modelcontextprotocol/ui": {}}
to ServerCapabilities as a pydantic extra field (the type is extra="allow"),
the same mechanism it uses for `tasks`. Verified by round-tripping initialize
against the real superset.mcp_service.app.mcp instance on fastmcp 3.4.2 — the
extension is advertised on the wire with no code change, and is present even
for a bare FastMCP() with no experimental_capabilities.
FastMCP does accept experimental_capabilities={"io.modelcontextprotocol/ui":
...} without error, but it lands in the legacy `experimental` map next to the
already-correct `extensions` entry, giving hosts two sources of truth for one
capability. So app.py is left alone.
Since the declaration is an implicit SDK dependency, it is pinned by a test:
an SDK bump that drops it now fails CI instead of silently degrading every MCP
Apps host to a plain-text tool result. A companion test asserts the extension
is not mirrored under `experimental`.
Co-Authored-By: Claude <noreply@anthropic.com>
buildSparklineOption takes the {x, y} series that resolveBigNumber
produces, not a ChartData. vitest does not typecheck, so `npm test` was
green while `npm run build` (tsc --noEmit) failed — which would have
red-lit the new chart-viewer CI workflow on its first run.
Rewritten to go through the real resolveBigNumber -> buildSparklineOption
path that BigNumber.tsx uses, which is the more faithful test anyway.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two defects made the chart widget misrender and its drill-down unusable.
Trust delimiters leaked into the rendering. `stripUntrustedMarkers` was
only applied to the chart title, drill label, and insight, so string cell
values and category axis labels displayed the raw
`<UNTRUSTED-CONTENT>` tags. Strip at the two display choke points
instead: the string branch of `formatByColumn` (table cells) and
`normalizeDim` (axis labels, tooltips, drill payloads).
The re-query call could never succeed. The widget sent a flat
`{chart_id, ...}` payload, but `render_chart_requery` takes a single
`request` model keyed by `identifier`; the server rejects the flat form
with a validation error. It also sent the raw, marker-wrapped dimension
value as the filter, which would match no rows even once the envelope
was accepted. Wrap the arguments correctly and strip the filter value.
Adds coverage for both display paths.
The widget shipped sending a flat {chart_id, filter, ...} payload while
render_chart_requery takes a single `request` model keyed by `identifier`,
so every drill-down and brush-to-zoom failed validation. No test caught it:
the bridge tests cover the bridge's own plumbing, and nothing compared the
payload the widget builds against the schema the server publishes.
Adds a two-sided contract around one artifact — the tool's inputSchema,
checked in under the widget's __fixtures__. TypeScript validates the
payloads App.tsx builds against it (including negative cases for the flat
form, a chart_id-keyed request, and the removed group_by); Python asserts
the live schema still equals it. Drift on either side now fails a test.
Verified falsifiable: simulating an identifier -> chart_id rename fails 5
of the TypeScript cases, and restoring the fixture makes them pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Repo lint bans the stdlib json import (TID251). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hosts cache the widget bundle keyed by its resource URI, so a corrected bundle served under an unchanged URI is never fetched. The marker-strip and re-query fixes in 2d4fb4c are invisible for exactly that reason. Bump v2 -> v3 in both constants that declare it (the resource module and the tool descriptor) and in the docs that quote it. The registration test pinned the literal "/v2", which would have failed on every future bump. Assert the URI carries *a* version suffix instead; the existing equality check between the two constants still guards the drift that duplication invites.
The versioned ui:// URI was declared independently in the resource that serves the bundle and in the tool descriptors that point at it. Bumping the version — the only reliable way to defeat per-conversation host caching — meant editing two constants, and only an equality assertion in the tests kept them from drifting apart. Moved both the URI and the mime type into an import-free chart/constants.py. The resource imports the FastMCP app, so the tools cannot import the resource; a neutral module is what lets both share one declaration without a cycle. Verified: tool descriptor and registered resource both resolve to the same constant, no import cycle, 1258 chart + middleware tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Superset `pie`, `donut`, `scatter` and `bubble` charts fell back to the styled table because the widget only knew line/bar/area/table/big_number. Registers PieChart + ScatterChart in the tree-shaken ECharts build (+26 KB, 864 KB against the 1.5 MB budget) and adds two builders: - pie: donut, largest wedge first, long tails collapsed into one "Other" wedge, non-positive values dropped (they cannot be a share of a whole), and the measure's series id shared with the cartesian series so the bar <-> pie switch morphs instead of cutting. - scatter: first measure on x, second on y, value axes on both sides, degrading to the cartesian renderer when a second measure is missing. Pie and scatter reorder or collapse rows, so both carry the source row index on each data item; the click handler resolves that back to the row before drilling or sharing a point with the assistant. Adds 18 unit tests plus three live-ECharts smoke tests (pie, scatter, and a bar -> pie -> bar morph) so a missing chart-type registration fails in the suite rather than in a host. Co-Authored-By: Claude <noreply@anthropic.com>
The table rendered every row it was given. A Superset result can carry ~1000 rows, so switching to the table view built a 1000-row DOM inside a 420px iframe — slow to mount and unreadable without endless scrolling. Adds a paged window (25/50/100/250 rows) with a footer showing the visible range, the page number and prev/next controls. Sorting still runs across the whole result and then pages, so "sort descending" surfaces the global maximum rather than reordering the visible page; a re-query, a re-sort or a page-size change returns to the first page. The footer is omitted entirely when the result fits on one page. The paging arithmetic lives in a pure `paginate()` helper (clamps out-of-range pages instead of rendering an empty table) and the sticky header, zebra striping, right-aligned numerics and untrusted-marker stripping are unchanged. Adds 9 tests. Co-Authored-By: Claude <noreply@anthropic.com>
Two widget controls were broken in a real host. "Open in Superset" did nothing. `openLink` only fell back to `window.open` when *not* embedded; inside a host it fired `ui/open-link`, waited out the full 8s default timeout when the host did not implement it, then swallowed the rejection. The click had no effect and no error. Use a short timeout, fall through to a direct open, and return whether either route worked so the caller can react. When both fail — a sandboxed iframe with no popup permission — the widget now copies the URL to the clipboard and says so, rather than silently doing nothing. Maximize was one-way. It only ever requested 'fullscreen'; nothing requested 'inline' and the button was not a toggle, so an expanded widget could not be restored from inside the widget at all. Track the display mode (following host-initiated changes too), toggle both ways, restore the previous height on the no-display-mode-support path, and bind Escape. The button now reflects and announces its state. Also collapses the duplicated toast set/clear into one helper that cancels a pending dismissal, so a second toast no longer inherits the first one's timer.
Adds CSV and PNG export, gated on what the frame can actually do rather than on what would look good in a toolbar. Measured first, in headless Chrome 146: loading the built bundle in a `sandbox="allow-scripts"` iframe under the declared empty CSP gives the document an opaque origin, and a blob-URL download there throws nothing and downloads nothing — no downloadWillBegin, no file. A "Download CSV" button would have been a dead button. So `isDownloadRestricted()` probes for that origin and the download actions are omitted, with the menu saying why. Loaded top-level, the same bundle writes a real 375-byte CSV and a valid 135 KB PNG. `navigator.clipboard.writeText` also rejects with NotAllowedError in the sandboxed frame (a cross-origin iframe has no clipboard-write permission), so "Copy CSV" falls through to execCommand and then to a selectable panel. Because execCommand can report success without the clipboard changing, a "Show CSV" action — which cannot silently fail — is always offered when downloads are blocked. Hosts that support ui/message also get "Send data to the assistant" (first 100 rows). The CSV carries the same formula-injection hardening as superset/utils/csv.py: a leading =, +, -, @ or control character in a *text* cell is prefixed with a quote (numbers are left alone so they survive re-import). Adds 26 tests. README documents the two cases this cannot detect (allow-same-origin without allow-downloads, and a lying execCommand) and states plainly that none of it has been run against Claude or ChatGPT. Co-Authored-By: Claude <noreply@anthropic.com>
The bundle changed substantially since v3 was published — pie/scatter views, table pagination, the Open-in-Superset fallback, and the maximize toggle. Hosts cache the widget by resource URI, so a client that already fetched v3 keeps serving the older bundle. Also refreshes the docs and module docstrings that quote the URI.
The widget had never been audited. Four gaps mattered: 1. The chart was a bare canvas — nothing for a screen reader to read. `describeChart()` now produces a sentence naming the view, the measures, the dimension and the span, fed to ECharts' `aria` option (which sets role="img" + aria-label on the container, verified) and repeated in a role="status" region so switching views is announced. 2. Every chip was its own tab stop, so reaching the export menu meant tabbing past every view and metric chip. Chip groups are now a single tab stop with arrow/Home/End movement inside them (WAI-ARIA toolbar pattern); the tab stop follows focus so Tab leaves from where the user is, and arrows move without selecting. 3. Table sorting was a click handler on a <th> — unreachable by keyboard. It is a real button inside the header cell now, with aria-sort on the cell and a screen-reader caption on the table. 4. There was no focus indicator. One :focus-visible ring covers every control, and prefers-reduced-motion collapses the animations. Escape now dismisses only the top-most layer (menu/panel) instead of also collapsing a maximized widget. Verified in headless Chrome inside the sandboxed frame: role="img" with the description on the chart, exactly one tab stop for the six view chips, tab order Maximize -> chips -> Export, and arrow keys plus Enter switching views with a visible 2px focus ring. Adds 12 tests. Not audited with a real screen reader — the README says so. Co-Authored-By: Claude <noreply@anthropic.com>
The 'Other' bucket is not a category. Wearing a palette color let it read as one, and on a high-cardinality dimension it is usually the largest wedge — so the chart implied a top category that does not exist. Completes a half-applied edit that computed the greyed wedges but never fed them to the series, which failed tsc (TS6133) and broke the build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
It read "The values are correct but their order and shape may differ". That is false, and it is the one direction that matters, because it reassures a reader out of checking. Chart 113 is named "Monthly Revenue Trend". Its dates come back as 2003-11-12, 2003-10-17, 2005-02-08 — individual days, not month boundaries under any grain. The time grain was never applied, so each row is one day's revenue presented as a month's total: the magnitudes are wrong, not the sequence. A pivot table collapsing to a single total is the absence of the breakdown that is the chart's entire content, and a mixed chart missing its second series is absent data, not reordered data. The scope of what Superset's buildQuery contributes is therefore wider than sorting: it carries the time grain, the groupby/series columns, the pivot structure and the second query for mixed charts. The notice now says both the values and their arrangement may differ, and names the failure concretely — a monthly chart returning days, a pivot returning one total — so it reads as a reason to check rather than a formality. Same defect class as every other bug on this branch: asserting an outcome that was never verified. This was the most consequential instance, because the assertion was the thing a reader would trust instead of comparing.
Merged rather than rebased. The branch carries 53 commits and was 384 behind master; rebasing replayed our own commits against intermediate states that no longer exist, producing conflicts between successive versions of files master has never seen (chart_viewer/ does not exist on master at all). Merging resolves once, against the final tree that is actually tested. Conflicts, all resolved as unions of genuinely additive changes: - middleware.py: master added error sanitisation and the MCP_ERROR_HOOK last-resort capture; we added the schema-shaped error for keep-list tools. The same `return ToolResult(...)` consumes both — master's `error_text` and our `tool_name` — so both are kept. - test_middleware.py: master added error-handler test classes, we added the structured-content keep-list ones. Import line takes master's (its ToolError import subsumes ours) plus our two extras. Also drops files master deleted (superset/viz.py, pooled_screenshot.py, test_viz_query_obj.py, preset-chart-deckgl/utils/explore.ts). The branch "modified" all four, but every change was a file-mode bit and no content, so there was nothing to preserve. Which exposed the larger problem: 904 files carried an accidental 100644 -> 100755 mode change with zero content difference, swept in by `git add -A` over a working tree something had chmod'd. Restored to master's modes. The diff against master goes from 973 files to 69 — the 69 being the actual change.
aminghadersohi
requested review from
betodealmeida,
dpgaspar,
eschutho,
geido,
hainenber,
kgabryje,
mistercrunch,
nytai,
rusackas,
sadpandajoe,
sha174n and
villebro
as code owners
August 24, 2026 18:16
dosubot
Bot
added
change:backend
Contributor
|
Bito Automatic Review Skipped - Large PR |
github-actions
Bot
added
the
github_actions
aminghadersohi
marked this pull request as draft
August 24, 2026 18:19
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| () => paginate(sorted, page, pageSize), | ||
| [sorted, page, pageSize], | ||
| ); | ||
| const multiPage = slice.total > PAGE_SIZE_OPTIONS[0]; |
Contributor
There was a problem hiding this comment.
Suggestion: multiPage is determined against the smallest available page size rather than the selected pageSize. When a user selects 50, 100, or 250 rows and the result contains between 26 and that selected size, the table displays pagination controls even though slice.pageCount is one and there is no next page. Compare the total against the active page size instead. [incorrect condition logic]
Severity Level: Minor 🧹
- ⚠️ Table shows unnecessary pagination controls.
- ⚠️ Single-page results display misleading page navigation.
- ⚠️ Affects results with 26–250 rows after larger-page selection.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/resources/chart_viewer/src/components/DataTable.tsx
**Line:** 111:111
**Comment:**
*Incorrect Condition Logic: `multiPage` is determined against the smallest available page size rather than the selected `pageSize`. When a user selects 50, 100, or 250 rows and the result contains between 26 and that selected size, the table displays pagination controls even though `slice.pageCount` is one and there is no next page. Compare the total against the active page size instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+299
to
+307
| with event_logger.log_context(action="mcp.render_dashboard.layout"): | ||
| layout = ModelGetInfoCore( | ||
| dao_class=DashboardDAO, | ||
| output_schema=DashboardLayout, | ||
| error_schema=DashboardError, | ||
| serializer=dashboard_layout_serializer, | ||
| supports_slug=True, | ||
| logger=logger, | ||
| ).run_tool(request.identifier) |
Contributor
There was a problem hiding this comment.
Suggestion: The dashboard layout lookup does not handle exceptions from ModelGetInfoCore.run_tool. Unlike the existing get_dashboard_layout tool, which converts lookup and serialization failures into a structured error response, any database or serializer exception here escapes the render tool and causes the entire dashboard render to fail instead of returning a ChartError or per-dashboard error. [error handling]
Severity Level: Major ⚠️
- ❌ Dashboard widget rendering fails on layout lookup errors.
- ⚠️ Clients receive no structured `ChartError` response.
- ⚠️ A single layout/serialization failure prevents dashboard composition.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/render_chart.py
**Line:** 299:307
**Comment:**
*Error Handling: The dashboard layout lookup does not handle exceptions from `ModelGetInfoCore.run_tool`. Unlike the existing `get_dashboard_layout` tool, which converts lookup and serialization failures into a structured error response, any database or serializer exception here escapes the render tool and causes the entire dashboard render to fail instead of returning a `ChartError` or per-dashboard error.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+140
to
+145
| const [activeTab, setActiveTab] = useState<string | null>(() => { | ||
| if (!tabs.length) return null; | ||
| const requested = render.active_tab_id; | ||
| if (requested && tabs.some((t) => t.id === requested)) return requested; | ||
| return (tabs.find((t) => countFor(t.id) > 0) ?? tabs[0]).id; | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: The selected tab is initialized only once, so when the same widget receives a subsequent dashboard result with a different active_tab_id, changed tab contents, or a different tab list, activeTab remains stale and can display the wrong tab or the “not included” placeholder. Synchronize the selection when render changes, while preserving user tab selections where appropriate. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Requery results can display the wrong dashboard tab.
- ⚠️ Removed tabs can leave the widget showing an empty placeholder.
- ⚠️ User-visible dashboard state diverges from the latest tool result.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/resources/chart_viewer/src/components/DashboardGrid.tsx
**Line:** 140:145
**Comment:**
*Stale Reference: The selected tab is initialized only once, so when the same widget receives a subsequent dashboard result with a different `active_tab_id`, changed tab contents, or a different tab list, `activeTab` remains stale and can display the wrong tab or the “not included” placeholder. Synchronize the selection when `render` changes, while preserving user tab selections where appropriate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+147
to
+149
| const cells = activeTab | ||
| ? render.cells.filter((c) => c.tab_id === activeTab) | ||
| : render.cells; |
Contributor
There was a problem hiding this comment.
Suggestion: When a dashboard has tabs, every cell is filtered by c.tab_id === activeTab. However, the dashboard contract explicitly allows tab_id to be null for charts that are not inside a tab. Such cells are silently omitted from every tab despite the component's guarantee that no leaf is dropped. Include unassigned cells in the appropriate dashboard view or render them separately instead of filtering them out. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Unassigned dashboard charts are silently omitted.
- ⚠️ Composite renders do not satisfy their no-dropped-cells contract.
- ⚠️ Users may mistake an incomplete dashboard for a complete one.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/resources/chart_viewer/src/components/DashboardGrid.tsx
**Line:** 147:149
**Comment:**
*Incomplete Implementation: When a dashboard has tabs, every cell is filtered by `c.tab_id === activeTab`. However, the dashboard contract explicitly allows `tab_id` to be null for charts that are not inside a tab. Such cells are silently omitted from every tab despite the component's guarantee that no leaf is dropped. Include unassigned cells in the appropriate dashboard view or render them separately instead of filtering them out.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+337
to
+350
| const next = (await bridge.callTool(REQUERY_TOOL_NAME, { | ||
| request: { | ||
| identifier: data.chart_id, | ||
| ...args, | ||
| }, | ||
| })) as ChartData; | ||
| if (next && Array.isArray(next.columns)) { | ||
| setData(next); | ||
| setActiveMetrics(classifyColumns(next).numeric.map((c) => c.name)); | ||
| setDrill({ | ||
| active: true, | ||
| label: stripUntrustedMarkers(drillLabel), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: Multiple clicks or brush actions can start overlapping re-queries, and each response unconditionally calls setData. If an earlier request finishes after a newer interaction, it overwrites the newer result and drill label with stale data. Track a request generation or cancel/ignore superseded requests before applying the response. [race condition]
Severity Level: Major ⚠️
- ⚠️ Rapid point clicks can display stale drill-down data.
- ⚠️ Brush zoom results can be overwritten by older clicks.
- ⚠️ Drill labels may not match displayed data.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/resources/chart_viewer/src/App.tsx
**Line:** 337:350
**Comment:**
*Race Condition: Multiple clicks or brush actions can start overlapping re-queries, and each response unconditionally calls `setData`. If an earlier request finishes after a newer interaction, it overwrites the newer result and drill label with stale data. Track a request generation or cancel/ignore superseded requests before applying the response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+362
to
+369
| if (this.isEmbedded && this.capabilities.canOpenLinks) { | ||
| if (await this.requestOk('ui/open-link', { url }, timeoutMs)) return true; | ||
| } | ||
| // Synchronously, inside the click's user gesture: awaiting anything first | ||
| // spends transient activation and gets the popup blocked. | ||
| try { | ||
| if (typeof window !== 'undefined' && window.open(url, '_blank', 'noopener')) | ||
| return true; |
Contributor
There was a problem hiding this comment.
Suggestion: When the host advertises openLinks but the host request fails or times out, this await completes before window.open is attempted. That consumes the click's transient user activation, so the direct popup fallback is commonly blocked even though it would have worked when invoked synchronously from the click handler. Open a synchronously-created popup before awaiting the host response, or use a separate synchronous fallback path. [logic error]
Severity Level: Minor 🧹
- ⚠️ Explore links fail to open when host handling times out.
- ⚠️ Embedded users fall back to copying URLs manually.
- ⚠️ The failure affects hosts advertising unavailable link support.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/resources/chart_viewer/src/bridge.ts
**Line:** 362:369
**Comment:**
*Logic Error: When the host advertises `openLinks` but the host request fails or times out, this `await` completes before `window.open` is attempted. That consumes the click's transient user activation, so the direct popup fallback is commonly blocked even though it would have worked when invoked synchronously from the click handler. Open a synchronously-created popup before awaiting the host response, or use a separate synchronous fallback path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+596
to
+598
| private onMessage = (event: MessageEvent): void => { | ||
| const msg = event.data as JsonRpcMessage | undefined; | ||
| if (!msg || msg.jsonrpc !== '2.0') return; |
Contributor
There was a problem hiding this comment.
Suggestion: The message handler accepts responses from any postMessage sender without checking event.source or the expected host origin. Because request IDs are predictable, another frame or window can spoof a response, inject tool-result data, or alter capability and link-operation results. Restrict responses to the initialized parent window and validate the allowed origin where available. [security]
Severity Level: Major ⚠️
- ⚠️ Untrusted frames can inject chart-result notifications.
- ⚠️ Spoofed responses can alter displayed chart integrity.
- ⚠️ Capability responses can influence link or tool operations.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/resources/chart_viewer/src/bridge.ts
**Line:** 596:598
**Comment:**
*Security: The message handler accepts responses from any `postMessage` sender without checking `event.source` or the expected host origin. Because request IDs are predictable, another frame or window can spoof a response, inject tool-result data, or alter capability and link-operation results. Restrict responses to the initialized parent window and validate the allowed origin where available.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+643
to
+658
| tags=( | ||
| [ | ||
| TagInfo.model_validate(tag, from_attributes=True) | ||
| for tag in getattr(chart, "tags", []) | ||
| ] | ||
| if getattr(chart, "tags", None) | ||
| else [] | ||
| ), | ||
| editors=( | ||
| [ | ||
| info | ||
| for editor in getattr(chart, "editors", []) | ||
| if (info := serialize_subject_object(editor)) is not None | ||
| ] | ||
| if getattr(chart, "editors", None) | ||
| else [] |
Contributor
There was a problem hiding this comment.
Suggestion: list_charts serializes each chart through this function without eager-loading tags or editors. Accessing both relationships for every item lazily issues additional queries per chart, turning a multi-chart listing into an N+1 query workload and potentially triggering detached-instance failures after the DAO session is closed. Either eager-load these relationships in the list query or avoid loading them when they were not requested. [performance]
Severity Level: Major ⚠️
- ⚠️ list_charts performs extra queries per returned chart.
- ⚠️ Large chart pages increase database latency.
- ❌ Detached objects can fail chart-list serialization.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 643:658
**Comment:**
*Performance: `list_charts` serializes each chart through this function without eager-loading `tags` or `editors`. Accessing both relationships for every item lazily issues additional queries per chart, turning a multi-chart listing into an N+1 query workload and potentially triggering detached-instance failures after the DAO session is closed. Either eager-load these relationships in the list query or avoid loading them when they were not requested.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+589
to
+591
| # Set only when we fall back to reconstructing the query from | ||
| # form_data, which drops Superset's post_processing stage. | ||
| fidelity_warning: str | None = None |
Contributor
There was a problem hiding this comment.
Suggestion: The warning is initialized but only populated for the saved-chart path that falls back because query_context is absent. The using_unsaved_state path immediately builds a query from cached form_data, which has the same loss of Superset's saved query/post-processing pipeline, but leaves fidelity_warning as None; renderers therefore present potentially rearranged or numerically different results without the warning this change adds. Set the warning whenever the query is reconstructed from form data, including the unsaved-state branch. [logic error]
Severity Level: Major ⚠️
- ⚠️ Unsaved chart renders omit query-fidelity warnings.
- ⚠️ MCP widgets may present rearranged chart data without disclosure.
- ⚠️ Dashboard cells reuse the same unwarned core path.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_data.py
**Line:** 589:591
**Comment:**
*Logic Error: The warning is initialized but only populated for the saved-chart path that falls back because `query_context` is absent. The `using_unsaved_state` path immediately builds a query from cached `form_data`, which has the same loss of Superset's saved query/post-processing pipeline, but leaves `fidelity_warning` as `None`; renderers therefore present potentially rearranged or numerically different results without the warning this change adds. Set the warning whenever the query is reconstructed from form data, including the unsaved-state branch.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+705
to
+707
| DEFAULT_STRUCTURED_CONTENT_KEEP_TOOLS: frozenset[str] = frozenset( | ||
| {"render_chart", "render_chart_requery"} | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggestion: The fallback keep-list omits render_dashboard, even though the dashboard tool is registered as a structured-content widget and the configured keep-list includes it. Whenever this middleware runs without a Flask application context, _keep_tools() returns this default and strips render_dashboard's output_schema and structured_content, preventing dashboard widgets from rendering. Include all structured-content widget tools in the default set. [api mismatch]
Severity Level: Major ⚠️
- ❌ Dashboard MCP Apps lose structured widget rendering.
- ❌ `render_dashboard` output schema is removed on fallback.
- ⚠️ Dashboard results degrade to non-interactive text responses.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 705:707
**Comment:**
*Api Mismatch: The fallback keep-list omits `render_dashboard`, even though the dashboard tool is registered as a structured-content widget and the configured keep-list includes it. Whenever this middleware runs without a Flask application context, `_keep_tools()` returns this default and strips `render_dashboard`'s `output_schema` and `structured_content`, preventing dashboard widgets from rendering. Include all structured-content widget tools in the default set.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+720
to
+723
| payload = MCPBaseError( | ||
| error_type=type(exc).__name__, | ||
| message=str(exc), | ||
| ).model_dump(mode="json") |
Contributor
There was a problem hiding this comment.
Suggestion: The structured error path bypasses _sanitize_error_for_logging and places str(exc) directly into structured content. Exceptions from database or authentication code can contain SQL fragments, connection details, or other sensitive internals, so keep-list tools expose data that the preceding text error path intentionally sanitizes. Sanitize the message before constructing MCPBaseError. [security]
Severity Level: Major ⚠️
- ⚠️ Keep-list error responses can expose internal exception text.
- ⚠️ MCP clients may receive SQL or connection details.
- ⚠️ Dashboard and chart widget failures share this path.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 720:723
**Comment:**
*Security: The structured error path bypasses `_sanitize_error_for_logging` and places `str(exc)` directly into structured content. Exceptions from database or authentication code can contain SQL fragments, connection details, or other sensitive internals, so keep-list tools expose data that the preceding text error path intentionally sanitizes. Sanitize the message before constructing `MCPBaseError`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Every tool call failed with `name 'has_app_context' is not defined`. The merge took master's `from flask import g` — master had removed its own has_app_context guards, replacing them with try/except around get_user_id — while StructuredContentStripperMiddleware._keep_tools(), which is ours and still needs the guard, kept using it. The file compiles, so nothing caught it: the NameError only fires on the path every tool call takes. Audited the rest of the merge rather than assuming this was the only loss. For every file the branch touched, checked that every line the branch ADDED relative to the merge base is still present: zero missing, across all files. The line-count drops elsewhere are master's own refactors, not dropped work. Also fixes a divergence this surfaced. DEFAULT_STRUCTURED_CONTENT_KEEP_TOOLS listed render_chart and render_chart_requery; MCP_STRUCTURED_CONTENT_KEEP_TOOLS also listed render_dashboard. The config wins wherever a Flask app context exists, so a running server was fine and only the contextless fallback differed — a divergence reachable only in the environment nobody runs. A test now pins the two together.
f82efec committed middleware.py and test_middleware.py as 100755. The worktree's filesystem carries ACLs that do not round-trip mode bits, so the executable flag was picked up rather than intended. core.fileMode is now false here, which stops git seeing those flips at all. No mode differences remain between this branch and master.
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
2 participants
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
Superset MCP tools that return a chart as a real, interactive visualization inside the AI chat instead of prose the model has to describe.
Built against the MCP Apps extension (SEP-1865, stable 2026-01-26) — the
ui://resource +_meta.ui.resourceUrimechanism co-authored by Anthropic, OpenAI and the mcp-ui authors. Verified working in Claude Desktop.What it adds
render_chart,render_chart_requery,render_dashboardchart/tool/render_chart.pyui://resource serving the widgetchart/resources/chart_viewer.pychart/resources/chart_viewer/@tool(meta=...)plumbingsuperset-core/.../mcp/decorators.py,core/mcp/core_mcp_injection.pyAll three tools are thin wrappers over
get_chart_data_core— the existing, already-authorized data path — so no new data or authorization path is introduced. Chart/dataset RBAC, guest-token scoping and RLS are re-applied on every interaction, including drill-down. The widget is static and tenant-neutral; per-user data reaches it only through tool results, never baked into the resource.Two existing MCP defaults blocked MCP Apps and are handled without weakening them globally:
StructuredContentStripperMiddlewarestripsstructuredContentfrom every tool (a Claude-bridge workaround). A small keep-list (MCP_STRUCTURED_CONTENT_KEEP_TOOLS) exempts the render tools; everything else is still stripped.search_tools/call_tool, which prevents widget association. The render tools are pinned inalways_visible.Known limitation, stated up front. Superset builds each chart's query in the frontend viz plugin (
buildQuery), which supplies the time grain, sort, pivot structure and any second series. This service rebuilds the query in Python when a chart has no savedquery_context, and that rebuild is lossy — a monthly chart can return individual days. Affected charts carry a visible notice in the widget rather than presenting the data as the chart's. Reusing Superset's ownbuildQuery/transformPropsis the intended fix and is measured as viable (~355 KB for five chart types, ~16 KB marginal each); it is deliberately not in this PR.Also fixed here (pre-existing, not MCP Apps specific): a single NULL in a temporal column made
get_chart_dataunserializable.pandas.NaTsubclassesdatetime.datetime, so pydantic_core reads.year—nan— and raises during serialization after the query has already succeeded. Null-ish scalars are now normalized at the two points rows leave the query result.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
render_chartdid not exist; chart data reached the model as JSON and was described in prose.After: the chart renders inline and interactively in the conversation — view switching, click-to-drill (re-queries live), dashboard grids with tabs.
Screenshots to follow — the widget requires an MCP Apps host to render.
TESTING INSTRUCTIONS
Unit tests
CI also runs the widget suite via
.github/workflows/mcp-chart-viewer.yml(path-filtered).In a real MCP Apps host (full checklist in
chart/CHART_VIEWER_HOST_VERIFICATION.md)Connect Claude Desktop to the local MCP endpoint, then in a brand-new conversation:
ADDITIONAL INFORMATION
Notes for reviewers
chart_viewer/dist/index.html) is not committed, matching thesuperset/static/assetsconvention: gitignored, produced at build time, shipped viaMANIFEST.in. Until built, theui://resource serves a placeholder page that says so.RELEASING/README.md); the Docker image is still a gap and official images would serve the placeholder.🤖 Generated with Claude Code
Environment note for anyone pulling this branch
This branch is merged with a
masterthat bumps SQLAlchemy 1.4 → 2.0.51, so apip install -r requirements/development.txtis required after pulling.That install currently leaves
fastmcpbroken —fastmcpandfastmcp-slim3.4.5 both land butfastmcp/exceptions.pygoes missing. Repair with:Both are environment-setup issues, not defects in this change.