feat: add Vega-Lite chart library with Plotly/Vega-Lite toggle

[cyyeh]

Summary

• Add dual chart library support with a Plotly/Vega-Lite toggle in the agent panel header, persisted to localStorage
• Backend dynamically generates system prompts and the render_chart MCP tool accepts a library parameter for native Vega-Lite spec output
• Frontend routes chart specs to either ChartWidget (Plotly) or VegaLiteChartWidget (vega-embed) based on the library field
• Fix conversation export to properly handle Vega-Lite charts (extract specs, replace with placeholders, re-render via Vega CDN)
• Add utils/run_vega_lite.html for standalone Vega-Lite spec testing
• Update README with dual chart support, response time feature, and project structure

Test plan

• Toggle between Plotly and Vega-Lite in the agent panel header
• Ask for a chart with each library selected and verify inline rendering
• Export a conversation containing a Vega-Lite chart and verify the exported HTML renders correctly
• Verify dark/light theme support for Vega-Lite charts
• Verify chart library preference persists across page reloads

🤖 Generated with Claude Code

[claude]

Claude finished @cyyeh's task in 2m 14s —— View job

---

Code Review: feat: add Vega-Lite chart library with Plotly/Vega-Lite toggle

Tasks

• Gather context and understand the PR
• Review backend changes (agent.py, mcp_sse.py, chat.py)
• Review frontend changes (components, contexts, hooks)
• Review export conversation logic
• Compile and post final review

---

Overall Assessment

This is a well-structured feature addition. The dual-library approach is clean, the architecture follows existing patterns, and the code is generally readable. Below are specific findings by area.

---

Code Quality

Positive:

• The build_system_prompt split in agent.py:22 is clean — library-specific prompt sections are well-separated and easy to maintain.
• ChartLibraryContext + useChartLibrary hook follows the established context pattern in the codebase correctly.
• VegaLiteChartWidget properly cleans up the Vega view on unmount and on spec/theme changes (viewRef.current?.finalize()).
• The data-vegalite-spec attribute approach for export is smart — it decouples spec extraction from React internals.

Issues:

1. render_chart tool schema has no required fields (backend/app/mcp_sse.py:117): "required": [] means neither data/layout (for Plotly) nor spec (for Vega-Lite) are formally required. The tool handler validates these at runtime, but the schema should ideally reflect conditional requirements to aid the LLM in choosing correctly. Consider at minimum requiring library. Fix this →

2. InlineQueryResult.tsx:94 — unsafe nested optional chaining: The title extraction ((result.chart_spec.spec as Record<string, unknown>)?.title as string) casts the entire spec to Record<string,unknown> and then access .title directly. If the Vega-Lite spec uses a title object instead of a string (which is valid in the Vega-Lite spec), this will silently render [object Object] as the chart label. Should guard with typeof 'string'. Fix this →

3. Significant code duplication in AgentContext.tsx: The sendMessage and editMessage callbacks are nearly identical in their stream event handling (onTextChunk, onThinkingDone, onToolCall, onToolResult, onSubagentStart, onSubagentEnd, onDone, onError, onUserQuestion). This was pre-existing but the PR adds chartLibrary to both without extracting a shared helper. This is not blocking, but worth noting for future refactoring.

4. AgentPanel.tsx:71 — inline confirm() call: confirm(t('clearConfirm')) is a blocking, non-dismissable browser dialog. This was pre-existing but is worth noting — for a better UX the existing isConfirmingDelete pattern from MessageBubble could be adopted here.

---

Security

1. XSS risk in exported HTML — clone.outerHTML is not sanitized (exportConversation.ts:196): The cleanClone function removes javascript: hrefs on <a> tags (line 81), but the cloned DOM still contains all user-provided content (message text, SQL results rendered as table cells, etc.) that was previously processed through React's XSS-safe rendering. Once serialized via outerHTML, any content injected through the DOM (e.g. from Markdown that React safely rendered) is re-embedded as raw HTML in the exported file — which is a self-contained standalone document opened by the browser. The risk is limited because: (a) the content was already React-rendered safely, and (b) the file is opened by the same user who generated it. However, if a malicious dataset contains values that look like HTML tags, they may have been rendered as text by React but serialize differently via innerHTML. Recommend verifying this path with a table containing <script> values.

2. Vega-Lite spec from the LLM is passed directly to vegaEmbed (VegaLiteChartWidget.tsx:60): The spec originates from the LLM (via the render_chart MCP tool). Vega-Lite supports signal expressions which can execute JavaScript expressions. vega-embed runs in a sandboxed renderer (renderer: 'svg'), but Vega expressions can still access window in some configurations. This is an accepted risk in Vega applications, but worth documenting. The SVG renderer is the safer choice (already used), and the actions panel doesn't expose raw execution. No immediate action needed, but be aware if moving to canvas renderer.

3. data-vegalite-spec attribute on the DOM (VegaLiteChartWidget.tsx:97): The full Vega-Lite JSON spec (potentially large, with up to 200 rows of data) is serialized into a DOM attribute. This is visible in DevTools and in the exported HTML's outerHTML. The export deliberately reads this attribute, so it's intentional. But large specs will significantly bloat the DOM and the exported file. Consider a ref-based or in-memory approach for export to avoid storing large data in the DOM. Fix this →

---

Performance

1. gatherCSS() copies all document stylesheets on every export (exportConversation.ts:55-67): This is called once per export action (user-triggered), so it's not a hot path. No concern.

2. fullSpec in VegaLiteChartWidget overrides the spec's own config (VegaLiteChartWidget.tsx:52-57): The spread { ...spec, config: isDark ? darkConfig : lightConfig } completely replaces any config the LLM may have provided in the spec. This is intentional for theme consistency, but if a user explicitly asks for a spec with custom colors, those will be silently overridden. Consider merging configs: config: { ...(spec.config as object), ...(isDark ? darkConfig : lightConfig) }.

3. No memoization of darkConfig/lightConfig objects in VegaLiteChartWidget.tsx: These are re-created on every render. Since they're constants, they should be defined outside the component or memoized. The current useEffect dependency is [spec, isDark] so it only re-runs when those change — but the config objects being recreated each render is wasteful. Fix this →

---

Testing

No automated tests were added or modified. This is consistent with the existing pattern in the repository, but the following scenarios would benefit from test coverage:

• build_system_prompt in agent.py — verify the correct chart tool description and charting workflow is injected for each chart_library value.
• render_chart handler in mcp_sse.py — the Vega-Lite path validation (missing spec, empty data.values) is good logic that could be unit tested.
• exportConversation — the placeholder replacement logic and chart index mapping could be unit tested without a DOM.
• extractVegaLiteCharts — ensure malformed JSON in the attribute is skipped (it has a try/catch, which is good).

---

Documentation

1. docs/plans/ files are committed to the repository. These are detailed implementation plan documents (785 lines and 121 lines). This is a stylistic choice — these can be useful for historical context, but also add clutter. No action required, just a note.

2. utils/run_vega_lite.html — a standalone dev utility for testing Vega-Lite specs. Good addition for local debugging. No documentation about how to use it is included, but the file is self-explanatory.

3. README.md is updated with dual-chart support details. The update is accurate and helpful.

4. Missing i18n key documentation: The chartRenderError i18n key is used in VegaLiteChartWidget.tsx:87 but the translations (en.json, zh-TW.json) only show 3 new keys added. Verify chartRenderError is included in both locale files — if it's missing, the error UI will show the raw key string.

---

Minor Nits

• agentService.ts:9 — the onSubagentEnd callback type includes a very long inline type annotation for chart_spec. This was pre-existing but the PR extends it with the spec field. Consider extracting it as a named type (ChartSpec) defined in types.ts (where ContentSegment.chart_spec and ToolCallResult.chart_spec already exist).
• AgentPanel.tsx:66 — onClick={() => exportConversation()} can be simplified to onClick={exportConversation}.
• mcp_sse.py:28 — there's a leading space before sse_transport = SseServerTransport(...) that's inconsistent with the surrounding code style (pre-existing).

---

Summary: The PR is well-implemented and follows established patterns. The most actionable issues are the render_chart missing required field in the JSON schema, the DOM attribute approach for large Vega-Lite specs, and the config override silently replacing user-specified chart styles. The XSS note in the exported HTML is worth verifying but is likely not a real vulnerability given React's rendering pipeline.