Conversation
nicosuave
force-pushed
the
mcp-apps-vega
branch
from
ac97484 to
3c0d507
Compare
![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: ac97484b58
ℹ️ 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".
| }; | ||
|
|
||
| app.ontoolinput = () => { | ||
| container.innerHTML = '<div class="loading">Running query...</div>'; |
There was a problem hiding this comment.
Replacing #chart with innerHTML in ontoolinput removes the existing #fullscreen-btn node, but later rendering only removes non-button children and never recreates that button. In hosts that emit ontoolinput before each result (the normal flow), fullscreen support disappears after the first query, so users can no longer toggle display mode even when the host advertises it.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
mcp-apps-vega
branch
2 times, most recently
from
e008848 to
4ad2da7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ad2da725b
ℹ️ 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".
| const ro = new ResizeObserver(() => result.view.resize().run()); | ||
| ro.observe(container); |
There was a problem hiding this comment.
Each tool result creates a new ResizeObserver bound to a new Vega view, but neither the observer nor the prior view is ever cleaned up when a subsequent query rerenders the chart. In sessions where users run multiple queries, observers accumulate and continue firing on every resize, causing redundant view.resize().run() calls and growing memory/CPU usage over time.
Useful? React with 👍 / 👎.
| const prefersDark = window.matchMedia?.("(prefers-color-scheme: dark)").matches; | ||
|
|
||
| embed(container, spec as any, { | ||
| actions: false, | ||
| theme: prefersDark ? "dark" : undefined, |
There was a problem hiding this comment.
The chart theme is chosen from window.matchMedia('(prefers-color-scheme: dark)') instead of MCP host context, so embedded charts can render with the wrong Vega theme whenever host theme and OS theme differ (or when the host theme changes at runtime). This leads to inconsistent colors/contrast in clients like Claude Desktop that provide their own theme via host context.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
mcp-apps-vega
branch
from
4ad2da7 to
bd00f50
Compare
- Vite-built widget bundles ext-apps SDK + Vega-Lite + vega-interpreter into a single HTML file (no CDN deps, no eval, CSP-safe) - Widget receives tool result via MCP Apps protocol (ontoolresult) - Supports fullscreen toggle when host advertises it - create_chart returns vega_spec only (no PNG, widget renders interactively) - Register ui://sidemantic/chart resource with proper MCP Apps metadata - Remove _apps_enabled flag (MCP Apps works via protocol, not a runtime flag) - Remove vendored mcp-ui-server (widget is self-contained) - Remove --apps CLI flag dependency on mcp-ui-server import - structured_output=False on all tools (fixes Claude Desktop tool visibility) - Replace | None params with falsy defaults (removes anyOf from schemas)
nicosuave
force-pushed
the
mcp-apps-vega
branch
from
bd00f50 to
dc3ba0e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc3ba0ec61
ℹ️ 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".
| spec.width = "container"; | ||
| spec.height = 500; |
There was a problem hiding this comment.
renderChart always forces spec.width = "container" and spec.height = 500, so the widget ignores the width/height requested via create_chart and remains 500px tall even in fullscreen mode. In practice this can clip dense charts (many categories/labels) and makes fullscreen mostly unused vertical space; the renderer should preserve the incoming size or compute height from the current container/display mode instead of hard-coding 500.
Useful? React with 👍 / 👎.
1 participant
Summary
create_chartreturns Vega spec only (no 374KB PNG bloat)structured_output=Falseon all tools + falsy defaults to fix Claude Desktop tool visibility_apps_enabledflag, and CDN dependencies