Skip to content

feat(web-shell): render streaming charts with markdown-chart - #7916

Merged
zhangxy-zju merged 8 commits into
QwenLM:mainfrom
zhangxy-zju:feat/web-shell-markdown-chart
Jul 29, 2026
Merged

feat(web-shell): render streaming charts with markdown-chart#7916
zhangxy-zju merged 8 commits into
QwenLM:mainfrom
zhangxy-zju:feat/web-shell-markdown-chart

Conversation

@zhangxy-zju

@zhangxy-zju zhangxy-zju commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Replaces WebShell's duplicated echarts-fulldata implementation with the shared @datafe-open/markdown-chart* 0.1.12 renderer stack. WebShell now includes the default ECharts runtime and registry, so canonical markdown-chart blocks render without host-side ECharts wiring. Hosts only need to install the canonical markdown-chart skill, and only hosts using data.kind="ref" need to provide resolveDataRef. The host resolver receives a validated, normalized ref and is bounded by the existing 30-second deadline. Shared chart controls and error states follow WebShell's language, while hosts can override individual labels.

The existing createEchartsFullDataRenderer, EchartsFullDataBlock, and markdown.renderCodeBlock integration surfaces remain compatible and delegate to the shared renderer. The React Markdown integration now distinguishes a closed chart fence from the active incomplete tail fence, so completed charts render immediately and stay mounted while later text continues streaming.

The obsolete skill-required-capabilities proposal and the WebShell-owned qwencode-viz skill template are removed. New integrations use the canonical skill maintained by the markdown-chart project instead of a second WebShell-specific model contract.

Why it's needed

Maintaining a separate parser, sanitizer, data resolver, Chart/Data view, and ECharts lifecycle inside WebShell duplicates the public Markdown Chart implementation and allows the two implementations to drift. Moving WebShell to the shared renderer centralizes protocol validation, safety behavior, streaming boundaries, and lifecycle fixes while preserving existing integrations.

Reviewer Test Plan

How to verify

  1. Install dependencies with npm ci.
  2. Run npm run test --workspace=packages/web-shell; expect 147 test files and 2,383 tests to pass, including the 25 MarkdownChartRenderer tests and 65 Markdown tests.
  3. Run npm run typecheck --workspace=packages/web-shell and npm run lint --workspace=packages/web-shell.
  4. Run npm run build --workspace=packages/web-shell; confirm the production app, library bundle, and declarations build.
  5. Install the canonical markdown-chart skill in a fresh Qwen workspace, start qwen serve, and ask the model for canonical inline charts. Confirm closed fences render immediately, the active unclosed tail shows the loading state, Chart/Data switching works, and persisted charts restore after a reload.
  6. Optionally configure createMarkdownChartRegistry({ resolveDataRef }) and confirm a controlled artifact:// or session-file:// ref is resolved only through the host callback.

Evidence (Before & After)

Before: WebShell used its own EchartsFullDataBlock implementation and new hosts had to supply ECharts explicitly.

After: a real qwen serve v0.21.0 session with the project-level markdown-chart skill restored three canonical charts after reload; all three chart canvases were present with no loading or alert state and no chart-related console warnings or errors. A new model turn produced a fourth chart through the built-in runtime. The session also verified the incomplete-tail loading state, localized controls, and Chart/Data switching. Closed charts inside streaming blockquotes are covered by the renderer integration suite because the current canonical skill explicitly asks models not to nest chart fences in Markdown containers.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

macOS, Node.js 22.23.1, Qwen Code/WebShell 0.21.0, local qwen serve, fresh workspace sandbox with the canonical markdown-chart skill.

Risk & Scope

  • Main risk or tradeoff: WebShell adds the shared Markdown Chart packages and ECharts as runtime dependencies; ECharts remains lazily loaded by the default renderer.
  • Not validated / out of scope: Windows/Linux browser verification, additional chart runtimes, arbitrary URL or filesystem ref loading, and daemon/ACP protocol changes.
  • Breaking changes / migration notes: the built-in registry intentionally renders its registered aliases (echarts-fulldata and the two echarts-chatbi_* families) even without host opt-in, where an unconfigured WebShell previously showed those fences as code. Existing host-provided createEchartsFullDataRenderer integrations keep their precedence, missing-runtime behavior, normalized ref callback input, 30-second callback deadline, resolved-ref reuse across streaming-state changes, and direct-component loader identity stability. The deprecated direct component now uses shared chart DOM, so private legacy data-testid hooks are not preserved. New integrations should install only the canonical markdown-chart skill and provide resolveDataRef only when referenced data is required.

Linked Issues

中文说明

本 PR 做了什么

使用共享的 @datafe-open/markdown-chart* 0.1.12 renderer 体系替换 WebShell 内重复维护的 echarts-fulldata 实现。WebShell 现在内置默认 ECharts 运行时和 registry,因此无需宿主接入 ECharts 即可渲染 canonical markdown-chart block。宿主只需要安装标准 markdown-chart skill;只有使用 data.kind="ref" 的宿主需要提供 resolveDataRef。宿主 resolver 收到经过校验和规范化的 ref,并继续受既有的 30 秒超时保护。共享图表控件和错误提示会跟随 WebShell 语言,宿主也可以覆盖个别文案。

现有 createEchartsFullDataRendererEchartsFullDataBlockmarkdown.renderCodeBlock 接入接口继续保持兼容,并委托给共享 renderer。React Markdown 接入现在可以区分已经闭合的图表 fence 和尾部仍未闭合的活动 fence,因此已完成的图表会立即渲染,并在后续文本继续流式输出时保持挂载。

同时删除已过时的 skill-required-capabilities 提案和 WebShell 自己维护的 qwencode-viz skill 模板。新接入统一使用 markdown-chart 项目维护的标准 skill,不再维护第二套 WebShell 专用模型输出契约。

为什么需要

WebShell 内单独维护 parser、sanitizer、数据解析、Chart/Data 视图和 ECharts 生命周期,既重复了公共 Markdown Chart 实现,也可能导致两个实现逐渐产生差异。迁移到共享 renderer 后,协议校验、安全行为、流式边界和生命周期修复都集中在一处,同时保留已有接入方式。

Reviewer 测试计划

如何验证

  1. 使用 npm ci 安装依赖。
  2. 运行 npm run test --workspace=packages/web-shell;应有 147 个测试文件、2,383 个测试通过,其中包括 25 个 MarkdownChartRenderer 测试和 65 个 Markdown 测试。
  3. 运行 npm run typecheck --workspace=packages/web-shellnpm run lint --workspace=packages/web-shell
  4. 运行 npm run build --workspace=packages/web-shell;确认生产应用、library bundle 和声明文件均构建成功。
  5. 在全新的 Qwen workspace 中安装标准 markdown-chart skill,启动 qwen serve,并让模型生成 canonical inline 图表。确认已闭合 fence 会立即渲染、尾部活动未闭合 fence 会显示 loading、Chart/Data 切换正常,并且刷新后可以恢复持久化图表。
  6. 可选配置 createMarkdownChartRegistry({ resolveDataRef }),确认受控的 artifact://session-file:// ref 只会通过宿主回调解析。

前后证据

改动前:WebShell 使用自己维护的 EchartsFullDataBlock 实现,新宿主需要显式提供 ECharts。

改动后:真实 qwen serve v0.21.0 会话搭配项目级 markdown-chart skill,刷新后成功恢复三个 canonical 图表;三个图表 canvas 均存在,不再有 loading 或 alert 状态,也没有图表相关的控制台 warning/error。新的模型轮次继续通过内置 runtime 生成了第四个图表。同一个会话还验证了未闭合尾部 loading 状态、本地化控件和 Chart/Data 切换。由于当前标准 skill 明确要求模型不要把图表 fence 嵌入 Markdown 容器,流式 blockquote 内已闭合图表由 renderer 集成测试覆盖。

测试平台

OS 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

macOS、Node.js 22.23.1、Qwen Code/WebShell 0.21.0、本地 qwen serve,以及安装了标准 markdown-chart skill 的全新 workspace sandbox。

风险与范围

  • 主要风险或取舍:WebShell 新增共享 Markdown Chart packages 和 ECharts 运行时依赖;默认 renderer 仍然延迟加载 ECharts。
  • 未验证 / 不在范围内:Windows/Linux 浏览器验证、其他图表运行时、任意 URL 或文件系统 ref 读取,以及 daemon/ACP 协议变更。
  • Breaking change / 迁移说明:内置 registry 会有意默认渲染已经注册的 alias(echarts-fulldata 和两类 echarts-chatbi_*),而未配置 renderer 的旧 WebShell 会将这些 fence 显示为代码。已有宿主提供的 createEchartsFullDataRenderer 接入继续保持原有优先级、缺少 runtime 时的错误行为、规范化后的 ref 回调输入、30 秒回调超时、流式状态切换时对已解析 ref 的复用,以及直接组件在 loader 回调身份变化时保持图表挂载。已废弃的直接组件现在使用共享图表 DOM,因此不保留私有的旧 data-testid。新接入只需安装标准 markdown-chart skill,仅在需要引用数据时提供 resolveDataRef

关联 Issue

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 8b7e262. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 7 render-shaping files:

  • packages/web-shell/client/components/messages/EchartsFullDataBlock.module.css
  • packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx
  • packages/web-shell/client/components/messages/Markdown.tsx
  • packages/web-shell/client/components/messages/MarkdownChartRenderer.tsx
  • packages/web-shell/client/customization.tsx
  • packages/web-shell/client/i18n.tsx
  • packages/web-shell/client/index.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@wenshao

wenshao commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification report — local build + live end-to-end

Verdict: I'd merge this. Every number in the Reviewer Test Plan reproduces exactly on a clean checkout of head 9c66b46, and a live run against a real qwen serve daemon in a real browser with the real echarts@6.0.0 runtime confirms the streaming behaviour this PR is built around. Three non-blocking follow-ups are at the bottom; none of them justifies holding the merge.

How this was verified

An isolated git worktree at head 9c66b46 with its own npm ci, so no shared checkout could swap branches under the build.

The live run deliberately avoids the mock harness. It is a real qwen serve v0.21.0 serving this PR's own built packages/web-shell/dist, talking to a scripted OpenAI-compatible endpoint that streams a two-chart answer in 40-character chunks at 90 ms intervals, driven by a real headless Chromium. Nothing in the render path is faked — the ECharts runtime is the real echarts@6.0.0 that the PR adds.


1. The four gates from the Reviewer Test Plan

Step Result
npm ci ✅ exit 0
npm run test --workspace=packages/web-shell 145 files / 2,335 tests passed — exactly the claimed numbers, including 13 MarkdownChartRenderer and 65 Markdown tests
npm run typecheck --workspace=packages/web-shell ✅ exit 0
npm run lint --workspace=packages/web-shell ✅ exit 0
npm run build --workspace=packages/web-shell ✅ production app, library bundle and declarations all built

Two repo-level gates that the new dependencies touch also pass: npm run check:lockfile and npm run audit:runtime:critical (0 critical vulnerabilities across the new @datafe-open/*, echarts, zrender and papaparse entries; all resolve from the public npm registry with integrity hashes, under MIT / Apache-2.0 / BSD-3-Clause).


2. The streaming boundary — the behaviour this PR exists for

Sampling the DOM every 120 ms from the moment the prompt was submitted:

Time after submit Chart cards Loading placeholders Live canvases
5 ms 0 0 0
632 ms 0 1 0
2,373 ms 1 0 1
2,691 ms 1 1 1
4,073 ms 2 0 2

The 2,691 ms row is the claim in the PR description, observed in the running product: the first chart is already a painted canvas while the second fence is still arriving and correctly shows the loading state. The canvas count never goes 1 → 0 → 1, so a completed chart is not being torn down and rebuilt as later text streams in.

closed chart rendered while the tail fence is still streaming


3. Before / after, with one variable changed

Identical daemon binary, identical scripted model, identical prompt. The only thing that differs is which packages/web-shell/dist the daemon serves — the base build at fceb755 or this PR's build.

Bundle Chart cards Canvases Plain <pre><code> blocks
Base (fceb755) 0 0 2
This PR (9c66b46) 2 2 0
Before — the fence renders as raw JSONAfter — rendered, and restored after a page reload

The "after" image is taken after a full page reload, so it doubles as the persistence check: both charts come back from the stored transcript with live canvases and zero raw chart fences.


4. Chart / Data view, and the browser console

The Chart/Data toggle works per card and renders the inline dataset as a table (2 tables, 13 rows total across the two charts). Note in the screenshot that chart 1 is in Data mode while chart 2 is still in Chart mode — the view state is per chart, not global.

Chart/Data toggle

Across the whole session — page load, streaming, toggling, reload — the browser reported 0 console errors, 0 uncaught page errors, and 0 failed HTTP requests.

One thing worth flagging for anyone reproducing this: my first attempt showed a 500 on POST /session/:id/model. That was entirely my harness — the daemon rejected my fake model id with Model 'mock-model' not found for authType 'openai'. Registering the model properly in settings.json made it disappear. It is not related to this PR, and I mention it only so a future reviewer does not chase it.


5. Are the new tests real? (mutation testing)

The 13 new tests all drive a fake ECharts runtime, so I checked whether they actually constrain the production code. Three source mutations, three precise kills:

Mutation to Markdown.tsx Killed by How it failed
isIncompleteTailFence always returns false Markdown.test.tsmarks only the unterminated tail fence as incomplete [false, true] became [false, false]
Drop the useMemo around chartPre, so the pre component identity changes every render MarkdownChartRenderer.test.tsxrenders a closed chart immediately while later Markdown is streaming runtime.init called 2 times instead of once
streaming={!!isStreaming}streaming={false} on the provider loads only after the active tail fence closes (+1 more) loading placeholder never appeared

The second one matters most: it is the test that pins the anti-remount guarantee, and it genuinely catches a regression in it. These tests are not vacuous.


6. Bundle cost — the "ECharts stays lazily loaded" claim holds

Artifact Base This PR Delta
App entry chunk (raw / gzip) 2,919.79 kB / 876.26 kB 2,990.68 kB / 900.22 kB +70.89 kB / +23.96 kB
New ECharts chunk (raw / gzip) 1,128.18 kB / 375.20 kB lazily loaded
Library bundle dist/index.js (raw / gzip) 3,199.86 kB / 564.87 kB 3,164.24 kB / 557.17 kB −35.62 kB / −7.70 kB
Published npm tarball 646.6 kB, 228 files 637.4 kB, 227 files −9.2 kB

ECharts 6 lands in its own 1.1 MB chunk that index.html does not reference, and the browser confirms it is genuinely deferred: the entry chunk is requested 35 ms after navigation, and the ECharts chunk only at 3,271 ms — at the moment the first chart fence closed. So the cost to initial page load is ~24 kB gzip, not 375 kB.

The library bundle got smaller, because the 1,889-line in-tree implementation is gone and the three @datafe-open/* packages plus echarts are correctly externalised in vite.lib.config.ts (verified: zero zrender bytes inlined, all four appear as bare imports).


Findings — all non-blocking

F1 — In a Chinese UI, the chart chrome is now English. Should fix, but as a follow-up. The PR removes 10 echartsChart.* keys from both locales and keeps only echartsChart.rendering, which is still wired through loadingLabel. Everything else now comes from the shared package as hardcoded English, and @datafe-open/markdown-chart-react exposes loadingLabel as its only string override — so neither Web Shell nor a host can translate the rest. In a live ?language=zh session the whole app chrome is Chinese while the chart error reads Chart unavailable:

zh-CN session showing an English chart error

The deleted component used all 11 echartsChart.* keys; the replacement uses one. The strings that regress: Chart unavailable (visible in the transcript), Showing N of M rows and A of B columns (the data-view truncation notice), View mode, Chart, Data, No data, and the Show chart / Show data accessible names — previously 图表渲染失败。, 显示 N/M 行,A/B 列, 视图模式, 图表, 数据, 暂无数据, 显示图表 / 显示数据. This needs an upstream messages/labels option on the shared adapter; it is not fixable inside this PR.

F2 — A closed chart fence inside a blockquote sits in "Rendering chart" until the turn ends. Nit. Only blockquotes are affected — the > prefix on continuation lines defeats the closing-fence scan. It self-heals the moment streaming stops, and I confirmed list items are fine, so real-world impact is small:

Shape at the streaming tail ECharts init Cards Loading
Plain closed fence 1 1 0
Closed fence in a blockquote 0 0 1
Closed fence in a blockquote, streaming finished 1 1 0
Closed fence in a list item (2- or 3-space indent) 1 1 0

This reproduces on both the default registry path and the legacy renderCodeBlock path, which suggests it lives in the shared package's fence scan rather than in isIncompleteTailFence.

F3 — Two deletions are not mentioned in the PR description. Nit. docs/design/skill-required-capabilities.md (466 lines) and packages/web-shell/docs/examples/qwencode-viz/SKILL.md (122 lines). Both are coherent with the change — the design note was added by #6232 alongside the very echarts-fulldata approach being replaced, and the two new design docs supersede it — but a line in the description would save the next reader the archaeology.


Checked and found fine

  • No dangling references to any deleted surface: qwencode-viz, the removed echartsChart.* keys, and the EchartsFullDataBlock module path are all unreferenced outside the new re-export shim and its tests.
  • isIncompleteTailFence boundary matrix. A closed fence that is the last node of a still-streaming message correctly reports false (this is the case that would have been most damaging to get wrong). Two closed fences report [false, false]; trailing whitespace after a closed fence still reports false; nothing reports true once streaming ends.
  • Host-precedence gating. Custom code/pre components and renderCodeBlock all correctly suppress the default registry, and DEFAULT_WEB_SHELL_MARKDOWN_CHART is a stable module-level constant so the shipped Web Shell never thrashes it.
  • The chart object-identity footgun is pre-existing, not new. A host passing a fresh customization object literal on every render remounts its charts (4 inits / 3 disposes over 3 re-renders). I ran the same experiment against the legacy renderCodeBlock path on this same build and got identical numbers, so this is a property of WebShellCustomizationProvider, not something this PR introduces. Worth knowing that the new doc comment ("the registry must remain referentially stable") understates it — the whole customization object must be stable — but that is a documentation nit on long-standing behaviour.
  • Safety. An unsafe spec.graphic.image pointing at an external URL is rejected into the error state, and no <img> for that host ever reaches the DOM. The artifact:// / session-file:// ref allowlist still rejects .., percent-encoded traversal, Windows drive letters and query strings before the host resolver is called.
  • Packaging. dist/types correctly re-exports the @datafe-open/* types, all three packages plus echarts are pinned exactly with lockfile integrity, and the tarball no longer ships the removed SKILL template — matching the files change.

One note rather than a finding: @datafe-open/markdown-chart-react depends on react-markdown@^10 while Web Shell is on ^9, so both copies end up in the app bundle. That is a meaningful slice of the +70 kB entry-chunk growth. Aligning the two would be a nice cleanup once the shared package supports React Markdown 9, but it is not a reason to hold this.

中文版本

维护者验证报告 —— 本地构建 + 真实端到端

结论:我认为可以合并。 Reviewer Test Plan 中的每一个数字都在 head 9c66b46 的干净检出上完整复现;在真实 qwen serve 守护进程、真实浏览器和真实 echarts@6.0.0 运行时下的实测,也确认了本 PR 核心的流式渲染行为。文末列出三条不阻塞合并的后续项。

验证方式

使用独立的 git worktree 检出 head 9c66b46 并单独执行 npm ci,避免共享检出在构建过程中被切换分支。

实测部分刻意没有使用 mock 测试环境:真实的 qwen serve v0.21.0 直接服务本 PR 自己构建出的 packages/web-shell/dist,后端连接一个脚本化的 OpenAI 兼容接口,以 40 字符为一块、每 90 ms 一块的节奏流式返回包含两个图表的回答,并由真实的 headless Chromium 驱动。渲染链路上没有任何 mock —— ECharts 就是本 PR 引入的真实 echarts@6.0.0

1. Reviewer Test Plan 的四道门禁

步骤 结果
npm ci ✅ 退出码 0
npm run test --workspace=packages/web-shell 145 个测试文件 / 2,335 个测试全部通过 —— 与 PR 声称的数字完全一致,其中包含 13 个 MarkdownChartRenderer 测试和 65 个 Markdown 测试
npm run typecheck --workspace=packages/web-shell ✅ 退出码 0
npm run lint --workspace=packages/web-shell ✅ 退出码 0
npm run build --workspace=packages/web-shell ✅ 生产应用、library bundle、声明文件均构建成功

另外两道与新依赖相关的仓库级门禁也通过:npm run check:lockfilenpm run audit:runtime:critical(新增的 @datafe-open/*echartszrenderpapaparse 条目 0 个 critical 漏洞;全部来自公共 npm registry 并带 integrity 校验,许可证为 MIT / Apache-2.0 / BSD-3-Clause)。

2. 流式边界 —— 本 PR 的核心行为

从提交 prompt 起,每 120 ms 采样一次 DOM:

提交后耗时 图表卡片 loading 占位 已绘制 canvas
5 ms 0 0 0
632 ms 0 1 0
2,373 ms 1 0 1
2,691 ms 1 1 1
4,073 ms 2 0 2

2,691 ms 这一行正是 PR 描述中的论断,在真实产品中被观测到:第一个图表已经是绘制完成的 canvas,而第二个 fence 仍在传输并正确显示 loading 状态。canvas 数量全程没有出现 1 → 0 → 1 的变化,说明已完成的图表不会因为后续文本继续流式输出而被卸载重建。

3. 单变量前后对比

守护进程二进制、脚本化模型、prompt 完全相同,唯一变量是守护进程服务的 packages/web-shell/dist —— base(fceb755)构建产物还是本 PR 的构建产物。

构建产物 图表卡片 canvas 普通 <pre><code> 代码块
Base(fceb755 0 0 2
本 PR(9c66b46 2 2 0

"After" 截图是在整页刷新之后拍摄的,因此同时也验证了持久化:两个图表都能从存储的会话记录中恢复出真实 canvas,且没有残留任何未升级的图表 fence。

4. Chart / Data 视图与浏览器控制台

Chart/Data 切换按卡片独立生效,并把内联数据渲染成表格(两个图表共 2 张表、13 行)。截图中图表 1 处于 Data 模式而图表 2 仍在 Chart 模式 —— 视图状态是每个图表独立的,不是全局的。

整个会话过程中(页面加载、流式输出、视图切换、刷新),浏览器报告 0 个 console error、0 个未捕获页面异常、0 个失败的 HTTP 请求

有一点供后续复现者参考:我第一次尝试时出现过 POST /session/:id/model 返回 500。那完全是我的测试环境问题 —— 守护进程拒绝了我伪造的 model id(Model 'mock-model' not found for authType 'openai')。在 settings.json 中正确注册该模型后即消失。这与本 PR 无关,特此说明以免后来者误查。

5. 新增测试是否有效?(变异测试)

13 个新测试都使用假的 ECharts 运行时,因此我验证了它们是否真正约束了生产代码。三次源码变异,三次精准命中:

Markdown.tsx 的变异 被哪个测试杀死 失败表现
isIncompleteTailFence 恒返回 false Markdown.test.tsmarks only the unterminated tail fence as incomplete [false, true] 变成 [false, false]
去掉 chartPre 外层的 useMemo,使 pre 组件标识每次渲染都变化 MarkdownChartRenderer.test.tsxrenders a closed chart immediately while later Markdown is streaming runtime.init 被调用 2 次而非 1 次
provider 上的 streaming={!!isStreaming}streaming={false} loads only after the active tail fence closes(外加另一个) loading 占位始终不出现

第二条最关键:它正是钉住"不重新挂载"这一保证的测试,并且确实能捕获该保证的回归。这些测试不是空测试。

6. 包体积成本 —— "ECharts 仍然延迟加载"的说法成立

产物 Base 本 PR 变化
应用入口 chunk(原始 / gzip) 2,919.79 kB / 876.26 kB 2,990.68 kB / 900.22 kB +70.89 kB / +23.96 kB
新增 ECharts chunk(原始 / gzip) 1,128.18 kB / 375.20 kB 延迟加载
Library bundle dist/index.js(原始 / gzip) 3,199.86 kB / 564.87 kB 3,164.24 kB / 557.17 kB −35.62 kB / −7.70 kB
发布的 npm tarball 646.6 kB,228 个文件 637.4 kB,227 个文件 −9.2 kB

ECharts 6 被拆分到独立的 1.1 MB chunk,index.html 并不引用它;浏览器侧也确认它确实被延迟加载:入口 chunk 在导航后 35 ms 被请求,而 ECharts chunk 直到 3,271 ms(第一个图表 fence 闭合的时刻)才被请求。因此对首屏加载的成本约为 24 kB(gzip),而不是 375 kB。

Library bundle 反而变小了,因为 1,889 行的仓库内实现被移除,并且三个 @datafe-open/* 包和 echartsvite.lib.config.ts 中被正确外部化(已验证:bundle 中没有内联任何 zrender 字节,四者均以裸 import 出现)。

发现的问题 —— 均不阻塞合并

F1 —— 中文界面下图表外壳变成英文。建议修复,但可作为后续项。 本 PR 从两个语言包中删除了 10 个 echartsChart.* 键,只保留 echartsChart.rendering(仍通过 loadingLabel 生效)。其余文案现在全部来自共享包的硬编码英文,而 @datafe-open/markdown-chart-react 只暴露了 loadingLabel 一个字符串覆盖入口 —— 因此无论 Web Shell 还是宿主都无法翻译其余文案。在真实的 ?language=zh 会话中,整个应用外壳是中文,而图表错误显示为 Chart unavailable(见上文截图)。

被删除的组件用到了全部 11 个 echartsChart.* 键,替代实现只用到 1 个。发生回退的文案包括:Chart unavailable(在会话中可见)、Showing N of M rows and A of B columns(数据视图的截断提示)、View modeChartDataNo data,以及 Show chart / Show data 无障碍名称 —— 此前分别为 图表渲染失败。显示 N/M 行,A/B 列视图模式图表数据暂无数据显示图表 / 显示数据。这需要共享 adapter 上游增加 messages/labels 选项,无法在本 PR 内解决。

F2 —— 位于引用块(blockquote)内的已闭合图表 fence 会一直停留在 "Rendering chart",直到本轮结束。小问题。 只有 blockquote 受影响 —— 续行的 > 前缀破坏了闭合 fence 的扫描。流式结束后会自动恢复,而且我确认列表项不受影响,因此实际影响很小:

流式尾部的结构 ECharts init 卡片 loading
普通已闭合 fence 1 1 0
blockquote 内的已闭合 fence 0 0 1
blockquote 内的已闭合 fence,流式已结束 1 1 0
列表项内的已闭合 fence(2 或 3 空格缩进) 1 1 0

该现象在默认 registry 路径和旧版 renderCodeBlock 路径上都能复现,说明问题位于共享包的 fence 扫描逻辑,而不是 isIncompleteTailFence

F3 —— 两处删除未在 PR 描述中说明。小问题。 docs/design/skill-required-capabilities.md(466 行)和 packages/web-shell/docs/examples/qwencode-viz/SKILL.md(122 行)。两处删除本身是合理的 —— 该设计说明由 #6232 与本次被替换的 echarts-fulldata 方案一并引入,且已被新增的两份设计文档取代 —— 但在描述中补一句可以省去后来者的考古工作。

已检查且确认无问题的部分

  • 没有悬空引用qwencode-viz、被删除的 echartsChart.* 键、EchartsFullDataBlock 模块路径,在新的 re-export 兼容层及其测试之外均无任何引用。
  • isIncompleteTailFence 边界矩阵:仍在流式输出的消息中,作为最后一个节点的已闭合 fence 正确返回 false(这是出错代价最大的一种情况)。两个已闭合 fence 返回 [false, false];已闭合 fence 后仅有空白字符时仍返回 false;流式结束后不会有任何 fence 返回 true
  • 宿主优先级门控:自定义 code/pre 组件以及 renderCodeBlock 都能正确抑制默认 registry;DEFAULT_WEB_SHELL_MARKDOWN_CHART 是模块级常量,因此发布版 Web Shell 不会出现标识抖动。
  • chart 对象标识的坑是既有行为,并非本 PR 引入:宿主若在每次渲染时传入新的 customization 对象字面量,图表会被重新挂载(3 次重渲染产生 4 次 init / 3 次 dispose)。我在同一构建上对旧版 renderCodeBlock 路径做了同样的实验,得到完全相同的数字,因此这是 WebShellCustomizationProvider 的固有特性。值得注意的是新增的文档注释("registry 必须保持引用稳定")表述偏弱 —— 实际上整个 customization 对象都必须稳定 —— 但这只是对既有行为的文档措辞问题。
  • 安全性:指向外部 URL 的不安全 spec.graphic.image 会被拒绝并进入错误状态,该域名的 <img> 从未进入 DOM。artifact:// / session-file:// 引用白名单仍会在调用宿主解析器之前拒绝 ..、百分号编码的路径穿越、Windows 盘符和查询串。
  • 打包dist/types 正确重导出 @datafe-open/* 类型;三个包与 echarts 均为精确版本锁定并带 integrity 校验;tarball 中已不再包含被移除的 SKILL 模板,与 files 字段的改动一致。

最后一点属于提示而非问题:@datafe-open/markdown-chart-react 依赖 react-markdown@^10,而 Web Shell 使用 ^9,因此两份副本都会进入应用 bundle,这也是入口 chunk 增长 70 kB 中不小的一部分。等共享包支持 React Markdown 9 之后统一版本会是不错的清理,但不构成阻塞本 PR 的理由。

@wenshao

wenshao commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Follow-up: the deprecated-API compatibility surface

After posting the report above I went back over the surface the PR describes as preserved. Everything in that report still stands — the zero-config default path works end-to-end exactly as claimed, and all the measurements reproduce. But this part of the description is not accurate for hosts on the deprecated API:

Existing host-provided createEchartsFullDataRenderer integrations keep their precedence and callback behavior. […] Breaking changes / migration notes: none intended.

Five concrete divergences below. All are verified first-hand against the PR source and the published @datafe-open/markdown-chart-echarts@0.1.10 dist (installed standalone from the registry), not inferred.

C1. The default registry claims echarts-fulldata — and two vendor languages — with no host opt-in · should document

Markdown.tsx installs DEFAULT_WEB_SHELL_MARKDOWN_CHART for every assistant message when the host has not overridden code/pre/renderCodeBlock. The registry's ECharts renderer matches four language families, not one:

markdown-chart
echarts-fulldata
/^echarts-chatbi_query_(\d+)-(\d+)$/
/^echarts-chatbi_sandbox_filepath_(\S+)$/i

Before this PR, echarts-fulldata rendered as a plain code block unless a host explicitly wired renderCodeBlock: createEchartsFullDataRenderer({ loadEcharts }). It now renders as a chart out of the box. The two echarts-chatbi_* patterns are third-party legacy formats that Web Shell has never claimed before; an assistant that emits one now gets a chart card — or a Chart unavailable alert — instead of readable code.

This may well be intended, but it is a behaviour change in the shipped product and is not in the description.

C2. A legacy host that deliberately omitted loadEcharts now silently gets the bundled ECharts · should fix or document

adaptLegacyRuntimeLoader returns undefined when options.loadEcharts is falsy (MarkdownChartRenderer.tsx:232-234), and the library then falls through to its own default:

// @datafe-open/markdown-chart-echarts/dist/index.js:1820
const loadECharts = options.loadECharts ?? (async () => await import("echarts"));

The old component rendered echartsChart.runtimeUnavailable ("Chart runtime is unavailable.") in that case. A host that omitted the loader as a deliberate kill-switch now renders charts.

C3. resolveDataRef now receives the raw reference instead of the normalized one · should fix

The old normalizeDataRef validated and canonicalized, returning `${prefix}${segments.join('/')}` — lowercased scheme, percent-decoded path — and handed that to the host resolver. The replacement isSupportedLegacyDataRef (MarkdownChartRenderer.tsx:182-215) is a pure boolean predicate, and the library forwards the envelope value verbatim:

// @datafe-open/markdown-chart-echarts/dist/index.js:1822-1830
if (options.validateDataRef && !options.validateDataRef(data.ref)) {  }
resolvedDataset = await options.resolveDataRef(data.ref, { format, dimensions, signal });

So for an input of ARTIFACT://charts/%6Frders.csv — which passes the predicate, since it lowercases the scheme and decodes the path for checking — a host resolver that used to receive artifact://charts/orders.csv now receives the raw string. This is not a safety hole (traversal, .., drive letters, query strings and double-encoding are all still rejected before the resolver runs, which I confirmed still works), but it is a silent change to a documented callback's input contract. The PR's own test only round-trips an already-canonical ref, so it cannot see this.

C4. EchartsFullDataBlock's parseError rendering changed shape · nit

Old: the message rendered inside the card, with toolbar and title, in a plain <div> with no ARIA role. New (MarkdownChartRenderer.tsx:395-397): a bare <div role="alert"> with no card chrome. The role="alert" is an accessibility improvement, but the DOM shape and any host CSS keyed to it change. While isStreaming is true the two are effectively equivalent (both show a loading state), so only the settled case differs.

Related: the data-testid="echarts-fulldata-{rendered,chart,table}" hooks are gone, which will break any host e2e keyed on them.

C5. The 30-second timeouts exist only on the deprecated path · worth noting

withTimeout (MarkdownChartRenderer.tsx:217-227) is applied exclusively through adaptLegacyRuntimeLoader / adaptLegacyDataResolver. DEFAULT_WEB_SHELL_MARKDOWN_CHART passes neither, and the library has no deadline of its own — it threads an AbortSignal (so unmount cancels) but never a timer. A hung resolveDataRef on the primary path therefore spins indefinitely rather than failing at 30 s. Ironically the deprecated path is the better-protected one.


On test coverage — a correction to my own report

In the report above I listed the isIncompleteTailFence-always-false mutation as killed. That is true, but I under-read the result: it was killed only by Markdown.test.ts:534, and all 13 MarkdownChartRenderer tests stayed green. The reason is structural — the shared pre component renders MarkdownChartBlock directly and never renders children, so for a chart fence MarkdownCode is never invoked and isIncompleteTailFence never runs. It serves the legacy renderCodeBlock path only; the default path uses the library's own equivalent tail detection.

So renders a closed chart immediately while later Markdown is streaming and loads only after the active tail fence closes are good integration smoke tests of the vendored library, but they should not be read as coverage of web-shell's new streaming logic. That single Markdown.test.ts case is the only thing pinning it — and per finding F2 above, it does not cover container-nested fences.

Three documented surfaces have no test at all, which is where I would spend the effort:

Surface Suggested one-line mutation that should fail a test
parseError — a public prop of EchartsFullDataBlock delete the if (parseError && !isStreaming) block at :395-397
withTimeout — the 30 s bound return promise; as the first line of :217
Theme plumbing hardcode theme={'light'} at :162

Not a regression — flagging so nobody over-corrects

The object-identity footgun from the report above (a host passing a fresh markdown.chart literal per render remounts its charts) looks alarming and the PR's README does show that exact shape at README.md:306. But the base README showed the same inline shape for renderCodeBlock, and I measured identical remount counts on both paths on this build. It is long-standing WebShellCustomizationProvider behaviour, not something to hold this PR for. Tightening the useMemo to key on chart.registry / chartClassName / chartStyle would be a cheap improvement, and the docstring at customization.tsx:70 should say the whole chart object must be stable, not just the registry.


Revised bottom line

I would still merge this — the feature is well built, the default path is solid under real end-to-end load, and the new tests are not vacuous. But I would ask for two small things first, because both are cheap and both are currently mis-stated in the description:

  1. Correct the "no breaking changes" claim, and say plainly that echarts-fulldata (C1) now renders by default and that resolveDataRef receives an unnormalized ref (C3) — or restore normalization, which is a three-line change.
  2. Note the zh-CN string regression (F1 in the report above) as a known follow-up with an upstream issue, so it does not get lost.

Everything else here is a nit or a coverage suggestion and can land afterwards.

中文版本

补充:已废弃 API 的兼容性面

在发布上面的报告之后,我又重新审视了本 PR 声称"保持兼容"的那部分接口。上面报告中的结论依然成立 —— 零配置默认路径的端到端行为完全符合声明,所有测量数据也都可复现。但对于仍在使用已废弃 API 的宿主来说,描述中的这段话并不准确:

已有宿主提供的 createEchartsFullDataRenderer 接入继续保持原有优先级和回调行为。[…] 预期没有 breaking change。

以下五处具体差异,全部是我基于 PR 源码和从 registry 单独安装的 @datafe-open/markdown-chart-echarts@0.1.10 产物第一手验证得出,并非推断。

C1. 默认 registry 会接管 echarts-fulldata(以及两种厂商专用语言),宿主无需也无法选择退出 · 建议补充说明

只要宿主没有覆盖 code/pre/renderCodeBlockMarkdown.tsx 就会为每一条助手消息安装 DEFAULT_WEB_SHELL_MARKDOWN_CHART。而该 registry 的 ECharts renderer 匹配的不止一种语言:

markdown-chart
echarts-fulldata
/^echarts-chatbi_query_(\d+)-(\d+)$/
/^echarts-chatbi_sandbox_filepath_(\S+)$/i

在本 PR 之前,echarts-fulldata 只有在宿主显式接入 renderCodeBlock: createEchartsFullDataRenderer({ loadEcharts }) 时才会渲染成图表,否则显示为普通代码块。现在它开箱即用地渲染为图表。两个 echarts-chatbi_* 模式是 Web Shell 从未接管过的第三方遗留格式;助手若输出这类 fence,现在会得到图表卡片或 Chart unavailable 提示,而不是可读的代码。

这可能是有意为之,但它确实是发布产品中的行为变更,且未写入 PR 描述。

C2. 有意不提供 loadEcharts 的遗留宿主现在会静默使用内置 ECharts · 建议修复或说明

options.loadEcharts 为假值时,adaptLegacyRuntimeLoader 返回 undefinedMarkdownChartRenderer.tsx:232-234),随后库会回落到自己的默认实现:

// @datafe-open/markdown-chart-echarts/dist/index.js:1820
const loadECharts = options.loadECharts ?? (async () => await import("echarts"));

旧组件在这种情况下会渲染 echartsChart.runtimeUnavailable("图表运行时不可用。")。把不提供 loader 当作开关来禁用图表的宿主,现在会照常渲染出图表。

C3. resolveDataRef 现在收到的是原始 ref,而不是归一化后的 ref · 建议修复

旧的 normalizeDataRef 既做校验也做规范化,返回 `${prefix}${segments.join('/')}`(scheme 小写、路径百分号解码),并把该值交给宿主 resolver。替代它的 isSupportedLegacyDataRefMarkdownChartRenderer.tsx:182-215)是纯布尔谓词,库随后原样转发 envelope 中的值:

// @datafe-open/markdown-chart-echarts/dist/index.js:1822-1830
if (options.validateDataRef && !options.validateDataRef(data.ref)) {  }
resolvedDataset = await options.resolveDataRef(data.ref, { format, dimensions, signal });

因此对于输入 ARTIFACT://charts/%6Frders.csv(它能通过谓词,因为谓词在校验时会小写 scheme 并解码路径),宿主 resolver 此前收到的是 artifact://charts/orders.csv,现在收到的是原始字符串。这不是安全漏洞(路径穿越、..、盘符、查询串、双重编码仍然会在调用 resolver 之前被拒绝,我已确认这部分仍然有效),但它是对一个有文档的回调输入契约的静默变更。PR 自带的测试只回环了一个本就规范的 ref,因此无法发现这一点。

C4. EchartsFullDataBlockparseError 渲染结构发生变化 · 小问题

旧实现:错误信息渲染在卡片内部,带 toolbar 和标题,使用没有 ARIA role 的普通 <div>。新实现(MarkdownChartRenderer.tsx:395-397):一个没有卡片外壳的裸 <div role="alert">。新增 role="alert" 是无障碍方面的改进,但 DOM 结构以及依赖它的宿主 CSS 都会改变。在 isStreaming 为 true 时两者实际等价(都显示加载状态),因此只有结束态存在差异。

相关:data-testid="echarts-fulldata-{rendered,chart,table}" 这些钩子已被移除,依赖它们的宿主 e2e 会失效。

C5. 30 秒超时只存在于已废弃路径上 · 值得留意

withTimeoutMarkdownChartRenderer.tsx:217-227)只通过 adaptLegacyRuntimeLoader / adaptLegacyDataResolver 生效。DEFAULT_WEB_SHELL_MARKDOWN_CHART 两者都不传,而库自身也没有超时 —— 它只传递了 AbortSignal(因此卸载会取消),但没有任何定时器。所以主路径上如果 resolveDataRef 卡死,会一直挂着而不是在 30 秒时失败。讽刺的是,受保护更好的反而是已废弃路径。


关于测试覆盖 —— 对我自己报告的一处更正

在上面的报告中我把"isIncompleteTailFence 恒返回 false"这条变异列为被杀死。这没错,但我当时读得不够细:它Markdown.test.ts:534 杀死,而 13 个 MarkdownChartRenderer 测试全部保持绿色。原因是结构性的 —— 共享的 pre 组件直接渲染 MarkdownChartBlock 而从不渲染 children,因此对图表 fence 而言 MarkdownCode 根本不会被调用,isIncompleteTailFence 也就永远不会执行。它只服务于遗留的 renderCodeBlock 路径;默认路径使用的是库自己的等价尾部检测逻辑。

所以 renders a closed chart immediately while later Markdown is streamingloads only after the active tail fence closes 是对内置库的良好集成冒烟测试,但不应被理解为对 web-shell 新增流式逻辑的覆盖。真正钉住该逻辑的只有 Markdown.test.ts 中那一个用例 —— 而根据上面的 F2,它并未覆盖嵌套在容器中的 fence。

有三处有文档的接口完全没有测试,我认为这是最值得补投入的地方:

接口 建议的单行变异(应当导致测试失败)
parseError —— EchartsFullDataBlock 的公开 prop 删除 :395-397 处的 if (parseError && !isStreaming)
withTimeout —— 30 秒上限 :217 的第一行加 return promise;
主题传递 :162 硬编码 theme={'light'}

这一条不是回归 —— 特此说明以免过度修正

上面报告中提到的对象标识陷阱(宿主每次渲染传入新的 markdown.chart 字面量会导致图表重新挂载)看起来很吓人,而且 PR 的 README 在 README.md:306 正好展示了这种写法。但 base 版 README 对 renderCodeBlock 展示的是同样的内联写法,而且我在同一构建上测得两条路径的重新挂载次数完全一致。这是 WebShellCustomizationProvider 的既有行为,不足以阻塞本 PR。把 useMemo 的依赖收紧为 chart.registry / chartClassName / chartStyle 是一处成本很低的改进;同时 customization.tsx:70 的文档注释应改为"整个 chart 对象必须保持稳定",而不只是 registry。


修订后的结论

我依然认为可以合并 —— 功能实现质量不错,默认路径在真实端到端压力下表现稳定,新增测试也不是空测试。但我建议在合并前补两件成本很低、且目前在描述中被表述错误的事:

  1. 修正"没有 breaking change"的说法,明确写出 echarts-fulldata 现在会默认渲染(C1),以及 resolveDataRef 收到的是未归一化的 ref(C3)—— 或者直接恢复归一化,这只是三行改动。
  2. 把 zh-CN 文案回退(上面报告的 F1)记为已知后续项并附上游 issue,以免遗漏。

其余各条都属于小问题或测试补充建议,可以后续再处理。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/i18n.tsx
@zhangxy-zju

Copy link
Copy Markdown
Collaborator Author

Implemented the review follow-ups in fd54f3dfa5 and 444958bbe1:

Validation on the current head:

  • WebShell tests: 145 files / 2,342 tests (20 renderer + 65 Markdown tests)
  • typecheck, lint, production/library/declaration build: passed
  • real qwen serve v0.21.0 with a project-level markdown-chart skill: persisted charts restored after reload, a new model turn rendered through the bundled ECharts runtime, Chart/Data switching and zh-CN controls worked, with no chart-related console warnings/errors

The PR description has been updated with the compatibility notes, upstream links, and current evidence.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

中文说明

已审查。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/package.json Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx
@zhangxy-zju

Copy link
Copy Markdown
Collaborator Author

Addressed the latest review round in 44029db4aa:

Current validation: lockfile check passed; focused 87/87; full WebShell 145 files / 2,344 tests; typecheck, lint, production/library/declaration build all passed. All five new review threads have been replied to and resolved.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. Run review failed. See workflow logs for details. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

Comment thread docs/design/web-shell-markdown-chart.md Outdated
Comment thread packages/web-shell/package.json Outdated
@wenshao

wenshao commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Local verification against a real stack (Linux)

I ran this PR locally on the head commit 44029db — repo gates plus an end-to-end run through a real qwen serve daemon and a real browser, with a before/after arm against the merge base fceb755. Everything in the reviewer test plan reproduces, including the streaming boundary, the referenced-data boundary and the documented back-compat behavior. I found no blocking issue. Details, numbers and screenshots below so the next reviewer does not have to redo it.

Stack: scripted OpenAI-compatible SSE model → real qwen serve v0.21.0 (--workspace, isolated QWEN_HOME) → web-shell vite dev server built from this branch → Chromium (Playwright). WebShell was mounted with no chart configuration of any kind — no loadEcharts, no registry, no renderCodeBlock.
Environment: Debian 13, Linux 6.12.63, Node v22.22.2. Linux was the untested row in the PR's OS table.

1. Repo gates, on a clean PR-head tree

Check Result
vitest run (packages/web-shell) 145 files / 2344 tests passed — exactly the counts the PR claims
MarkdownChartRenderer.test.tsx + Markdown.test.ts 87 passed (22 + 65, as claimed)
tsc -p tsconfig.json --noEmit clean
eslint packages/web-shell --max-warnings 0 clean
npm run build (app + library bundle + tsc -p tsconfig.lib.json) exit 0, all three artifacts produced
prettier --check packages/web-shell 2 warnings — client/index.html and components/GitModePopover.module.css, both untouched by this PR and identical on main. Pre-existing, not yours.

2. One real streaming turn

The model streamed: text → closed chart fence → text → closed chart fence → text → a chart fence held unterminated across several chunks → close → trailing text.

Observation Measured
Live ECharts canvases mounted while the tail fence was still loading 2
Cards / canvases after the turn settled 3 / 3, all with non-zero canvas pixel dimensions
[data-markdown-chart-loading] after settle 0
[role="alert"] after settle 0
Chart→Data→Chart switch on chart #1 works both directions
After a full page reload (persisted transcript) 3 cards, 3 canvases, 0 loading, 0 alerts
Chart-related console errors / warnings / page errors 0 / 0 / 0

The streaming-boundary claim is the interesting one, and it holds — chart #2 is fully mounted and interactive while the tail fence below it still shows Rendering chart and the turn is still streaming:

streaming boundary

Settled turn, Data view, and after reload

settled
data view
after reload

3. Before / after on the identical turn

Same daemon, same scripted model output, same workspace — only packages/web-shell/client swapped between the merge base and this branch.

Arm chart cards canvases raw envelope code blocks
before (fceb755) 0 0 3
after (this PR) 3 3 0

before/after

4. Host resolveDataRef boundary — and a positive control

I built a harness that wires a real createMarkdownChartRegistry({ resolveDataRef }) and records every ref the renderer hands to the host, then fed it 11 data.kind="ref" envelopes.

Ref Reached host resolver Rendered
artifact://sales/2026-q1.json ✅ yes (normalized, with format + dimensions) chart
session-file://reports/rows.json ✅ yes chart
https://evil.example.com/data.json ❌ no error state
file:///etc/passwd ❌ no error state
artifact://../../etc/passwd ❌ no error state
artifact://a%2F..%2F..%2Fetc%2Fpasswd ❌ no error state
artifact://a\..\..\etc\passwd ❌ no error state
artifact://C:/Windows/win.ini ❌ no error state
artifact://sales/ 2026.json ❌ no error state
artifact://sales/rows.json?token=abc ❌ no error state
/etc/passwd ❌ no error state

To make sure that pass was not vacuous, I ran a positive control: neutering normalizeSupportedDataRef to return every ref unchanged. 7 of the 9 hostile refs immediately leaked to the resolver and the test failed — so the assertion is live. (Interesting side note: https:// and file:// stayed rejected even with the control patch, i.e. the shared package's own scheme check is a second layer behind WebShell's normalizer.)

Data-ref harness screenshot

data-ref boundary

5. Integration contract probes

Probe Expected Measured
canonical fence, zero host chart config renders 1 canvas
echarts-fulldata alias, no host opt-in renders (the PR's documented breaking change) 1 canvas
same fence under source="thinking" must not become a chart surface 0 canvases, stays a code block
host renderCodeBlock present host keeps precedence, built-in registry suppressed host node rendered, 0 canvases

contract probes

The breaking change described in the PR body is real and behaves exactly as documented: an unconfigured WebShell that previously showed echarts-fulldata fences as code will now render them as charts.

6. Bundle impact (production app build, uncompressed)

Metric before after delta
eager entry chunk 2,952,682 B 3,026,962 B +72.5 KiB
total JS across all chunks 15,000,096 B 16,203,228 B +1.15 MiB
chunk count 326 327 +1

The +1 chunk is ECharts/zrender and it is reached through import("./index-C2GE3DAF.js") with no modulepreload in index.html — so the "ECharts remains lazily loaded" claim holds; only ~72 KiB of markdown-chart core lands in the eager path. The library build keeps echarts and all three @datafe-open/* packages external, so embedders still own the runtime.

One thing I specifically checked because the lockfile looks alarming: @datafe-open/markdown-chart-react pins its own react-markdown@10.1.0 next to WebShell's 9.1.0, and its dist/index.js does import ReactMarkdown from "react-markdown". It is tree-shaken out — react-markdown pipeline markers are byte-for-byte identical in count between the two builds (3 and 4), and the chart chunk contains zero react-markdown references. No duplicate markdown pipeline ships.

7. Streaming re-render cost

Markdown.tsx now does per-code-node offset slicing plus a fence-closed check on every render, so I measured it: a 156 KB assistant document with 40 code fences plus an unterminated tail, re-rendered 30 times, 3 runs per arm.

Arm median ms / re-render
before 32.74
after 33.46

Run ranges overlap (before 32.2–35.5, after 33.3–36.3). The added work is in the noise.

Caveats on my run

  • The lockfile itself is not independently validated by me. The box had under 1 GB free, so instead of npm ci I hardlink-copied a main-based node_modules and installed the four new packages from the registry at exactly the locked versions and paths. Resolution and versions match the lockfile entries, but CI's npm ci remains the authority there.
  • Harness noise, unrelated to this PR and identical in both arms: my scripted model id is not in the CLI's openai model registry, so POST /session/:id/model returns 500 and the shell shows a Set model failed toast. I hid the toast host for the screenshots only; chart errors were still asserted through [role="alert"] and the console listeners.
  • Still unvalidated by anyone: Windows, and macOS beyond your own run.
  • Worth a maintainer decision independent of correctness: this makes @qwen-code/web-shell depend at runtime on a third-party npm scope (@datafe-open/*) plus echarts. Versions are pinned exact, which is the right call.

LGTM from my side.

中文说明

本地真实环境验证(Linux)

我在本地对本 PR 的 head commit 44029db 做了完整验证:既跑了仓库自身的各项检查,也用真实 qwen serve daemon + 真实浏览器做了端到端验证,并以 merge base fceb755 作为对照组做了 before/after 对比。Reviewer 测试计划中的每一项都能复现,包括流式边界、引用数据边界和向后兼容行为。没有发现阻塞性问题。 下面是数据和截图,后续 reviewer 不用再重做一遍。

验证链路: 脚本化的 OpenAI 兼容 SSE 模型 → 真实 qwen serve v0.21.0(--workspace,独立 QWEN_HOME)→ 用本分支源码启动的 web-shell vite dev server → Chromium(Playwright)。WebShell 完全没有任何图表相关配置 —— 没有 loadEcharts,没有 registry,也没有 renderCodeBlock
环境: Debian 13、Linux 6.12.63、Node v22.22.2。Linux 正是 PR 的 OS 表格中未测试的那一行。

1. 仓库检查(干净的 PR head 工作树)

检查项 结果
vitest run(packages/web-shell) 145 个文件 / 2344 个测试通过 —— 与 PR 声称的数字完全一致
MarkdownChartRenderer.test.tsx + Markdown.test.ts 87 个通过(22 + 65,与声称一致)
tsc -p tsconfig.json --noEmit 通过
eslint packages/web-shell --max-warnings 0 通过
npm run build(应用 + library bundle + tsc -p tsconfig.lib.json exit 0,三个产物均构建成功
prettier --check packages/web-shell 2 个 warning —— client/index.htmlcomponents/GitModePopover.module.css,这两个文件本 PR 未改动,且与 main 完全一致。属于既有问题,与本 PR 无关。

2. 一次真实的流式回答

模型依次输出:文本 → 已闭合图表 fence → 文本 → 已闭合图表 fence → 文本 → 跨多个 chunk 保持未闭合的图表 fence → 闭合 → 收尾文本。

观测项 实测
尾部 fence 仍处于 loading 时已挂载的 ECharts canvas 数 2
回答结束后的 card / canvas 数 3 / 3,canvas 像素尺寸均非零
结束后 [data-markdown-chart-loading] 0
结束后 [role="alert"] 0
第 1 个图表的 Chart→Data→Chart 切换 双向均正常
整页刷新后(持久化 transcript) 3 个 card、3 个 canvas、0 loading、0 alert
图表相关 console error / warning / page error 0 / 0 / 0

流式边界这一条是最关键的,结论成立 —— 下方尾部 fence 还显示 Rendering chart、整个回答仍在流式输出时,图表 #2 已经完全挂载并可交互:

streaming boundary

结束状态、Data 视图、刷新后

settled
data view
after reload

3. 同一次回答的 before / after 对比

相同 daemon、相同脚本化模型输出、相同 workspace —— 只把 packages/web-shell/client 在 merge base 和本分支之间切换。

分支 图表 card canvas 原始 envelope 代码块
before(fceb755 0 0 3
after(本 PR) 3 3 0

before/after

4. 宿主 resolveDataRef 边界 —— 以及一次正向对照

我写了一个 harness,接入真实的 createMarkdownChartRegistry({ resolveDataRef }),记录 renderer 交给宿主的每一个 ref,然后喂了 11 个 data.kind="ref" envelope。

Ref 是否到达宿主 resolver 渲染结果
artifact://sales/2026-q1.json ✅ 是(已规范化,带 format + dimensions 图表
session-file://reports/rows.json ✅ 是 图表
https://evil.example.com/data.json ❌ 否 错误态
file:///etc/passwd ❌ 否 错误态
artifact://../../etc/passwd ❌ 否 错误态
artifact://a%2F..%2F..%2Fetc%2Fpasswd ❌ 否 错误态
artifact://a\..\..\etc\passwd ❌ 否 错误态
artifact://C:/Windows/win.ini ❌ 否 错误态
artifact://sales/ 2026.json ❌ 否 错误态
artifact://sales/rows.json?token=abc ❌ 否 错误态
/etc/passwd ❌ 否 错误态

为了确认这个"通过"不是空转,我做了一次正向对照:把 normalizeSupportedDataRef 改成原样返回所有 ref。9 个恶意 ref 中立刻有 7 个泄漏到 resolver,测试随即失败 —— 说明这个断言是真实生效的。(一个有意思的细节:即使打了对照补丁,https://file:// 依然被拒绝,也就是说共享包自身的 scheme 校验是 WebShell 规范化之后的第二道防线。)

数据引用 harness 截图

data-ref boundary

5. 接入契约验证

验证项 预期 实测
canonical fence,宿主图表配置 渲染 1 个 canvas
echarts-fulldata alias,宿主未 opt-in 渲染(PR 中记录的 breaking change) 1 个 canvas
同样的 fence 在 source="thinking" 不得成为图表渲染面 0 个 canvas,仍是代码块
宿主提供了 renderCodeBlock 宿主优先,内置 registry 被抑制 渲染宿主节点,0 个 canvas

contract probes

PR 描述中的 breaking change 真实存在,且行为与文档完全一致:此前把 echarts-fulldata fence 当作代码显示的未配置 WebShell,现在会直接渲染成图表。

6. 打包体积影响(生产应用构建,未压缩)

指标 before after 增量
首屏 entry chunk 2,952,682 B 3,026,962 B +72.5 KiB
全部 chunk 的 JS 总量 15,000,096 B 16,203,228 B +1.15 MiB
chunk 数量 326 327 +1

新增的那个 chunk 就是 ECharts/zrender,它是通过 import("./index-C2GE3DAF.js") 加载的,index.html没有 modulepreload —— 所以"ECharts 仍然延迟加载"的说法成立,首屏路径只多了约 72 KiB 的 markdown-chart 核心。Library 构建把 echarts 和三个 @datafe-open/* 都保持为 external,嵌入方仍然掌握运行时。

还有一点我特意查了,因为 lockfile 看上去有点吓人:@datafe-open/markdown-chart-react 自带了 react-markdown@10.1.0,与 WebShell 的 9.1.0 并存,而且它的 dist/index.js 确实有 import ReactMarkdown from "react-markdown"这段被 tree-shaking 掉了 —— 两次构建中 react-markdown pipeline 的特征标记数量完全相同(3 和 4),图表 chunk 里没有任何 react-markdown 引用。不会打进第二套 markdown 流水线。

7. 流式重渲染开销

Markdown.tsx 现在每次渲染都会对每个 code node 做 offset 切片和 fence 闭合判断,所以我实测了一下:156 KB 的 assistant 文档、40 个代码 fence 加一个未闭合尾部,重渲染 30 次,每个分支跑 3 轮。

分支 每次重渲染中位数(ms)
before 32.74
after 33.46

两组区间是重叠的(before 32.2–35.5,after 33.3–36.3)。新增开销在噪声范围内。

关于本次验证的说明

  • lockfile 本身没有被我独立验证。 机器可用空间不足 1 GB,所以我没有跑 npm ci,而是用硬链接复制了一份基于 mainnode_modules,再按 lockfile 中完全一致的版本和路径从 registry 安装了这 4 个新包。解析结果和版本与 lockfile 条目一致,但这一项仍应以 CI 的 npm ci 为准。
  • 与本 PR 无关、且两个分支表现一致的 harness 噪声:我脚本化的模型 id 不在 CLI 的 openai 模型注册表中,因此 POST /session/:id/model 返回 500,界面会弹出 Set model failed toast。我只在截图时隐藏了 toast 容器;图表错误仍然通过 [role="alert"] 和 console 监听独立断言。
  • 目前仍未被任何人验证的:Windows;macOS 除你自己那次之外也没有第二次验证。
  • 与正确性无关、但值得维护者单独判断的一点:这会让 @qwen-code/web-shell 在运行时依赖第三方 npm scope(@datafe-open/*)以及 echarts。版本都做了精确 pin,这个处理是对的。

我这边 LGTM。

Verified locally with Claude Code (Claude Opus 5, 1M context).

@zhangxy-zju

Copy link
Copy Markdown
Collaborator Author

Addressed the latest review round in eccc70560d:

  • added an explicit no-raw-fence assertion for invalid chart specs;
  • added Web Shell contract coverage for canonical fail-closed handling of unsafe HTML, URL/CSS URL, image-symbol, forbidden-key, and prototype-key options, while retaining setOption assertions for forced tooltip safety invariants;
  • updated the design decision to @datafe-open/markdown-chart* 0.1.12;
  • aligned the new runtime dependencies with the package caret-range convention while keeping the validated artifacts locked.

Validation completed locally:

  • focused renderer tests: 23/23;
  • renderer + Markdown tests: 88/88;
  • full Web Shell suite: 145 files / 2,345 tests;
  • Prettier, typecheck, ESLint, lockfile dry-run, production build, library bundle, and declaration build: passed.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

中文说明

已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。

— qwen3.7-max via Qwen Code /review

@zhangxy-zju

Copy link
Copy Markdown
Collaborator Author

Addressed the latest review suggestions in 8b7e262298:

  • added a Markdown-level regression test proving an unterminated legacy echarts-fulldata fence stays in the loading state and never initializes ECharts;
  • restored mixed-case ECharts-FullData fence coverage through the deprecated adapter;
  • updated the PR test-plan counts after merging current main.

Local validation on the current head:

  • focused renderer tests: 25/25;
  • renderer + Markdown tests: 90/90;
  • full Web Shell suite: 147 files / 2,383 tests;
  • Prettier, ESLint, and pre-commit checks: passed.

Both inline threads have been replied to and resolved.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

中文说明

未发现问题。LGTM!✅

— qwen3.7-max via Qwen Code /review

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean migration from duplicated EchartsFullDataBlock to shared @datafe-open/markdown-chart renderer. Net -4278 lines, deletion is complete with no orphaned imports/CSS/i18n keys. Security is solid (data ref validation defense-in-depth, no dangerouslySetInnerHTML, 30s timeout guards). Streaming fence detection correct.

Non-blocking suggestions for follow-up:

  1. Memoize chart variable or document referential stability requirement to avoid streaming re-renders
  2. Consider ^6.0.0 for echarts (peer range is >=5.5 <7) or comment the exact-pin policy
  3. Add resize and multi-chart-in-one-message tests
  4. Document chart vs renderCodeBlock precedence in CodeBlockRenderer type

@zhangxy-zju
zhangxy-zju added this pull request to the merge queue Jul 29, 2026
Merged via the queue into QwenLM:main with commit 1d55d29 Jul 29, 2026
99 of 100 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants