Skip to content

feat(web-shell): support compact echarts full data blocks - #6232

Merged
wenshao merged 35 commits into
QwenLM:mainfrom
zhangxy-zju:feat/web-shell-code-block-renderer
Jul 4, 2026
Merged

feat(web-shell): support compact echarts full data blocks#6232
wenshao merged 35 commits into
QwenLM:mainfrom
zhangxy-zju:feat/web-shell-code-block-renderer

Conversation

@zhangxy-zju

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

Copy link
Copy Markdown
Collaborator

Summary

  • Add an echarts-fulldata envelope with version, data, and option so compact inline datasets can be injected into ECharts options.
  • Support host-resolved dataset refs through resolveDataRef(ref, meta) for large data artifacts.
  • Reuse the parsed dataset for the table view through the enhanced table path, while preserving compatibility with existing native ECharts options.

Validation

  • vitest run client/components/messages/EchartsFullDataBlock.test.tsx (42 tests)
  • vite build
  • vite build --config vite.lib.config.ts
  • tsc -p tsconfig.lib.json
  • dataworks-chart eval iteration 5 passed 12/60/180/500-row generation cases in the companion skill repo.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

On direction: this is a natural extension of the existing Web Shell customization surface. The codebase already has transformMarkdown, custom components, remarkPlugins, and rehypePlugins — adding a renderCodeBlock callback is the missing piece that lets hosts intercept specific fenced code blocks without replacing the entire Markdown renderer. Aligned with where Web Shell is heading. Claude Code has a /dataviz skill and chart rendering improvements, so the area is clearly relevant across agent CLIs.

On approach: the scope is tight — one extension point, one bundled skill, two tests, no drive-by changes. The regex fix (\w+[^\s]+) is the right call for hyphenated language tags. Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向:这是 Web Shell 现有自定义接口(transformMarkdowncomponentsremarkPluginsrehypePlugins)的自然延伸,补上了让宿主拦截特定 fenced code block 而无需替换整个 Markdown renderer 的缺失环节。与 Web Shell 的发展方向一致。Claude Code 也有 /dataviz skill 和图表渲染改进,说明这个领域在 agent CLI 中确实有价值。

方案:范围紧凑——一个扩展点、一个内置 skill、两个测试、没有夹带无关改动。正则修复(\w+[^\s]+)正确处理了带连字符的 language tag。进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review: clean. The implementation follows established Web Shell patterns (context providers, optional render callbacks, fallback-to-default). No correctness bugs, no security issues, no unnecessary abstraction. The regex fix from \w+ to [^\s]+ is the right call — \w doesn't match hyphens so echarts-fulldata would have been truncated to echarts.

Two things I verified specifically:

  • The source gate (source ? ... : undefined) correctly skips the custom renderer when MarkdownSourceContext hasn't been provided (e.g. if MarkdownCode is ever used outside the Markdown component tree). Safe default.
  • className is properly typed as optional in WebShellCodeBlockRenderInfo, matching the MarkdownCode prop type. No TS strict-mode issue.

Tests: 40 Markdown tests pass (including 2 new custom-renderer tests covering both the intercept and fallback paths), 12 bundled-skills integration tests pass (including parsing the new web-shell-charts/SKILL.md).

Build + typecheck: clean across all packages.

Real-scenario verification:

This PR adds a library-level React callback prop — the renderCodeBlock path only fires when a Web Shell host registers one. A CLI qwen -p session can't exercise it because there's no host providing the callback. Unit tests are the right verification layer here.

I did verify the bundled skill is discoverable at runtime:

$ npm run dev -- -p "list available skills"
| `web-shell-charts` | Produce ECharts code blocks for Web Shell |

Skill appears in the listing and parses with all required fields (name, description, body) per the integration test.

中文说明

代码审查: 无问题。实现遵循了 Web Shell 现有模式(Context Provider、可选渲染回调、回退到默认)。无正确性 bug、无安全问题、无不必要的抽象。正则从 \w+ 改为 [^\s]+ 是正确的——\w 不匹配连字符,echarts-fulldata 会被截断为 echarts

特别验证了两点:

  • source 判空(source ? ... : undefined)在 MarkdownSourceContext 未提供时正确跳过自定义渲染器。默认行为安全。
  • classNameWebShellCodeBlockRenderInfo 中正确标记为可选,与 MarkdownCode 的 prop 类型一致。TS 严格模式无问题。

测试: 40 个 Markdown 测试通过(含 2 个新增的自定义渲染器测试,覆盖拦截和回退两条路径),12 个 bundled-skills 集成测试通过(含解析新增 web-shell-charts/SKILL.md)。

构建 + 类型检查: 全部通过。

实际场景验证: 此 PR 添加的是库级别的 React 回调属性——只有 Web Shell 宿主注册了 renderCodeBlock 时才会触发。CLI qwen -p 无法触达该路径,因为没有宿主提供回调。单元测试是正确的验证层。

已验证内置 skill 在运行时可被发现:

$ npm run dev -- -p "list available skills"
| `web-shell-charts` | Produce ECharts code blocks for Web Shell |

Skill 出现在列表中,且集成测试确认所有必填字段(name、description、body)解析正确。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-run at maintainer request after multiple review iterations. Here's where things stand.

The PR has evolved significantly since the initial triage. What started as a tight extension point + bundled skill has grown into a full-featured chart rendering system (~6700 additions, 14 files). The growth is justified — the EchartsFullDataBlock component handles envelope parsing, two-layer option sanitization (allowlist + denylist), data-ref resolution with cancellation/timeout, theme-aware rendering, ResizeObserver-based responsiveness, chart/data toggle with enhanced table reuse, and comprehensive i18n. Each concern maps to a real requirement, not speculative engineering.

Security posture is strong. Defense-in-depth: prototype pollution blocking, URI scheme filtering, HTML tag scanning, path traversal prevention in data refs, tree-walking sanitization with depth limits, and key allow/deny lists. Tests verify each protection layer individually.

wenshao's latest suggestions (7 items, commit 1dffa3b9):

# Suggestion Current state Verdict
1 extractRawFenceLanguage regex affects all code blocks Not addressed. [^\s]+ captures full tag, then [\w+.#-]+ constrains to safe chars. Correct behavior. Hyphenated tags now pass fully to resolveFenceLanguage — this is the fix, not a regression. Document if desired, but not blocking.
2 Fragile .replace() in normalizeEnvelopeDataset Still present (line 1091). Non-blocking. Error message cosmetic, edge case path.
3 Identical 19-line export blocks in index.ts/index.tsx Not addressed. Non-blocking. Maintenance concern, not correctness.
4 t in render effect deps Already fixed. tRef stores t, effect uses tRef.current (line 1691, 1813). t is not in the dep array. Resolved ✓
5 HTML tag blocklist misses <video>, <audio>, etc. Not addressed. Non-blocking. Defense-in-depth; blocklist covers the dangerous tags.
6 url() CSS wrapper not detected Not addressed. Non-blocking. backgroundImage already in UNSAFE_OPTION_KEYS.
7 sanitizeDatasetCell uses isUnsafeUriString not isUnsafeOptionString Not addressed. Non-blocking. Dataset cells render as text in <td>, not innerHTML.

None of the outstanding suggestions are correctness or security blockers. They're hardening and code quality improvements that can be addressed in follow-up work.

Reflection: This is a well-built feature. The extension point (renderCodeBlock) is genuinely useful, the chart renderer is security-conscious, the tests are thorough (100+ test cases covering sanitization invariants, error boundaries, source gating, i18n, streaming), and the author has been responsive through every review round. The dual entry-point export duplication is the most actionable cleanup item — worth a follow-up, not a merge gate.

Approving. ✅

中文说明

按 maintainer 要求重新执行 triage,经过多轮 review 迭代后的当前状态:

PR 从初始 triage 以来大幅演进。 从紧凑的扩展点 + 内置 skill 成长为完整的图表渲染系统(约 6700 行新增,14 个文件)。增长合理——EchartsFullDataBlock 组件处理 envelope 解析、两层 option 净化(allowlist + denylist)、data-ref 解析(含取消/超时)、主题感知渲染、ResizeObserver 响应式、图表/数据切换(复用 enhanced table)、完整 i18n。每个关注点对应真实需求,不是投机性工程。

安全姿态扎实。 纵深防御:prototype pollution 阻断、URI scheme 过滤、HTML tag 扫描、data ref 路径遍历防护、带深度限制的树遍历净化、key allow/deny 列表。测试逐一验证每层防护。

wenshao 最新建议(7 项,commit 1dffa3b9):

# 建议 当前状态 判定
1 extractRawFenceLanguage 正则影响所有 code block 未处理。[^\s]+ 捕获完整标签,然后 [\w+.#-]+ 约束为安全字符。 行为正确。连字符标签现在完整传给 resolveFenceLanguage——这是修复,不是回归。
2 normalizeEnvelopeDataset 脆弱的 .replace() 仍存在(行 1091)。 不阻塞。错误消息装饰性,边缘路径。
3 index.ts/index.tsx 重复的 19 行 export 块 未处理。 不阻塞。维护性顾虑,非正确性问题。
4 render effect deps 中的 t 已修复。 tRef 存储 t,effect 使用 tRef.current(行 1691, 1813)。t 不在 deps 数组中。 已解决 ✓
5 HTML tag 黑名单遗漏 <video><audio> 未处理。 不阻塞。纵深防御;黑名单覆盖了危险标签。
6 未检测 url() CSS 包装 未处理。 不阻塞。backgroundImage 已在 UNSAFE_OPTION_KEYS 中。
7 sanitizeDatasetCell 使用 isUnsafeUriString 而非 isUnsafeOptionString 未处理。 不阻塞。数据集单元格渲染为 <td> 文本,非 innerHTML。

所有未解决建议都不是正确性或安全阻塞项。它们是可以在后续工作中处理的加固和代码质量改进。

总结: 这是一个构建良好的功能。扩展点(renderCodeBlock)确实有用,图表渲染器注重安全,测试全面(100+ 测试用例覆盖净化不变量、错误边界、source 门控、i18n、streaming),作者在每轮 review 中都积极响应。双入口 export 重复是最可操作的清理项——值得后续处理,不阻塞合并。

批准 ✅

Qwen Code · qwen3.7-max

@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.

LGTM, looks ready to ship. ✅

Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.test.ts
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
@wenshao

wenshao commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

✅ Maintainer verification — built & exercised locally

Verified at head d0ef6d88 (matches head.sha) in a fresh git worktree with a full workspace build. Built the bundle, ran the suites, mutation-tested the new tests, and drove the extension point end-to-end in a real browser. Recommendation: LGTM — safe to merge.

Scope: 6 files, +207 / −11 — a source-gated renderCodeBlock extension point on the Web Shell Markdown renderer, plus a bundled web-shell-charts skill.

What I ran

Check Result
web-shell · Markdown.test.ts 40/40 (incl. the 2 new custom-code-block tests)
core · bundled-skills.integration.test.ts 12/12 — incl. web-shell-charts/SKILL.md parses with required fields ✓
Full web-shell vitest suite 700/700 (55 files) ¹
npm run build --workspace=packages/web-shell (vite + lib + tsc -p tsconfig.lib.json) ✅ exit 0
ESLint (changed TS/TSX) · Prettier (all 6 files) · git show --check ✅ clean

¹ Run after building — a few specs (incl. build-artifact.test.ts) read the built dist/ and can't collect without it, so a build-less run under-counts.

Real-UI E2E — the extension point genuinely renders a chart

I served the real <Markdown> component through the web-shell vite dev server and registered a host renderCodeBlock that turns an echarts-fulldata fence into a live ECharts chart (the PR intentionally ships no chart runtime — a host-provided renderer is exactly the contract). 14/14 DOM assertions passed. The host received precisely:

{ language: "echarts-fulldata", className: "language-echarts-fulldata",
  code: "const option = { … }", isStreaming: false, source: "assistant", theme }

1 · echarts-fulldata fence → live host chart (dark) — the fence is replaced; no leftover code block

chart dark

2 · Same fence, theme threaded to the host (light) — the only change is theme=light, and the chart re-themes

chart light

3 · Handled chart + unhandled json fence — the declined fence stays a normal copyable code block (decline → fallback)

mixed fallback

4 · <Markdown> without a source prop → renderer gated OFF (fired 0×) — the fence falls back, labelled with the full ECHARTS-FULLDATA tag (not truncated to echarts)

nosource gate

Mutation tests — the 2 new tests are load-bearing

Mutation applied to Markdown.tsx Expected Result
Revert to base (remove the renderCodeBlock plumbing) both fail ✅ both fail — spy never called
Hard-code theme: 'dark' theme case fails ✅ test 1 fails on theme, test 2 passes → theme genuinely comes from useTheme()
([^\s]+)(\w+) in extractRawFenceLanguage language cases fail ✅ both fail (echarts-fulldataecharts, custom-chartcustom) → the full-tag capture is load-bearing

Observations (non-blocking)

  • undefined vs null contract is sound: returning undefined = decline → default code block; any other ReactNode (incl. null) = handled. Only undefined + a real element are unit-tested; the null-suppresses-the-block path is not (minor).
  • Source-gated by design: renderCodeBlock fires only when source is set (assistant / thinking), matching the existing transformMarkdown gating — confirmed by screenshot 4.
  • extractRawFenceLanguage now also feeds the default CodeBlock, so hyphenated fence languages (e.g. objective-c) resolve to the full tag instead of truncating at the first -. A latent improvement; full suite stays green.
  • Cross-platform: author marked 🪟 / 🐧 ⚠️. The diff is pure TS/React + one markdown skill with no platform-specific APIs — low risk. I verified on macOS.
  • Repro note: the production vite build needs the sibling workspace dist (@qwen-code/webui, @qwen-code/sdk) built first; npm ci --ignore-scripts alone fails on @qwen-code/webui/daemon-react-sdk resolution and on build-artifact.test.ts — both clear after a full npm run build. Not a PR issue.
🇨🇳 中文报告(完整对应)

✅ 维护者验证 —— 已在本地构建并实测

在 PR head d0ef6d88(与 head.sha 一致)新建 git worktree、完整构建 workspace 后验证。构建了产物、跑了测试套件、对新增测试做了变异测试,并在真实浏览器里端到端驱动了这个扩展点。结论:LGTM,可以合并。

改动范围: 6 个文件,+207 / −11 —— 在 Web Shell 的 Markdown renderer 上新增一个受 source 门控的 renderCodeBlock 扩展点,外加一个内置 web-shell-charts skill。

跑了哪些检查

检查项 结果
web-shell · Markdown.test.ts 40/40(含 2 个新增的自定义代码块测试)
core · bundled-skills.integration.test.ts 12/12 —— 含 web-shell-charts/SKILL.md parses with required fields ✓
web-shell 完整 vitest 套件 700/700(55 个文件) ¹
npm run build --workspace=packages/web-shell(vite + lib + tsc -p tsconfig.lib.json ✅ exit 0
ESLint(改动的 TS/TSX)· Prettier(全部 6 个文件)· git show --check ✅ 干净

¹ 构建后运行 —— 少数用例(含 build-artifact.test.ts)会读取已构建的 dist/,未构建时无法 collect,会导致计数偏少。

真实 UI 端到端 —— 扩展点确实能渲染图表

我用 web-shell 的 vite dev server 挂载了真实的 <Markdown> 组件,并注册了一个宿主 renderCodeBlock,把 echarts-fulldata fence 渲染成真实的 ECharts 图表(本 PR 有意不内置图表 runtime —— 由宿主提供 renderer 正是它的契约)。14/14 条 DOM 断言全部通过。宿主精确收到:

{ language: "echarts-fulldata", className: "language-echarts-fulldata",
  code: "const option = { … }", isStreaming: false, source: "assistant", theme }
  • 图 1 · echarts-fulldata fence → 宿主实时图表(暗色) —— fence 被替换,没有残留代码块。
  • 图 2 · 同一个 fence,theme 透传给宿主(亮色) —— 只改了 theme=light,图表随之换主题,证明 theme 真的传到了宿主。
  • 图 3 · 已处理的图表 + 未处理的 json fence —— 被拒绝的 fence 仍然是普通可复制代码块(decline → 回退)。
  • 图 4 · <Markdown> 不传 source → renderer 被门控关闭(触发 0 次) —— fence 回退为默认代码块,标签是完整的 ECHARTS-FULLDATA(没有被截断成 echarts)。

(截图见上方英文部分。)

变异测试 —— 2 个新增测试是「有效的」

Markdown.tsx 施加的变异 预期 实际
回退到 base(移除 renderCodeBlock 接线) 两个都挂 ✅ 两个都挂 —— spy 从未被调用
theme 写死成 'dark' theme 用例挂 ✅ 测试 1 在 theme 上挂,测试 2 通过 → theme 确实来自 useTheme()
extractRawFenceLanguage 里的 ([^\s]+) 改回 (\w+) language 用例挂 ✅ 两个都挂(echarts-fulldataechartscustom-chartcustom)→ 完整标签捕获是「承重」的

观察项(不阻塞合并)

  • undefinednull 契约合理:返回 undefined = 弃权 → 默认代码块;返回其它任意 ReactNode(含 null)= 已处理。单测只覆盖了 undefined 和真实元素两种;null 抑制整块的路径未测(次要)。
  • 按设计受 source 门控: renderCodeBlock 只在 source 存在(assistant / thinking)时触发,与既有 transformMarkdown 的门控一致 —— 图 4 已证实。
  • extractRawFenceLanguage 现在也供默认 CodeBlock 使用, 因此带连字符的 fence 语言(如 objective-c)会解析成完整标签,而不再在第一个 - 处截断。是一处隐性改进;完整套件仍全绿。
  • 跨平台: 作者把 🪟 / 🐧 标为 ⚠️。改动是纯 TS/React + 一个 markdown skill,无平台相关 API,风险低。我在 macOS 上验证。
  • 复现提示: 生产 vite build 需要先构建好同 workspace 的 dist(@qwen-code/webui@qwen-code/sdk);单跑 npm ci --ignore-scripts 会在 @qwen-code/webui/daemon-react-sdk 解析和 build-artifact.test.ts 上失败 —— 完整 npm run build 后即消失。非本 PR 问题。

Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.test.ts
wenshao
wenshao previously approved these changes Jul 3, 2026
Comment thread packages/web-shell/client/components/messages/Markdown.test.ts Outdated
@wenshao

wenshao commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

✅ Maintainer re-verification at head 6d706dc5 — after the harden + docs commits

Follow-up to my earlier check at d0ef6d88. The two newer commits — 634772ea6 (harden) and 6d706dc59 (docs) — change runtime behavior, so I rebuilt and re-exercised the PR from scratch in a fresh git worktree. The harden commit resolves the exact null-path gap I flagged last round. Recommendation: LGTM — safe to merge.

Scope (net vs main): 7 files, +818 / −13. The runtime surface is a source-gated renderCodeBlock hook on the Web Shell Markdown renderer plus a bundled web-shell-charts skill. The +323-line docs/design/skill-required-capabilities.md is a discussion draft onlygrep finds zero code references to required-capabilities.

What changed since d0ef6d88, and what I checked

  • 634772ea6 (harden):
    • null now declines → default code block (previously only undefined did — I flagged the null path as untested last time). New test returns null ✓, mutation-proven below.
    • Two independent safety nets around the host renderer, both falling back to the built-in CodeBlock:
      • try/catch when renderCodeBlock(...) throws synchronouslyconsole.error('[web-shell] … failed:', error) (2 args). Test ✓.
      • a CustomCodeBlockBoundary error boundary when the returned node throws while renderingconsole.error(…, error, componentStack) (3 args), and it resets on resetKey={code}. Test ✓.
    • fsharp language + c++ / c# / f# aliases added to resolveFenceLanguage.
  • 6d706dc59 (docs): design draft, no runtime.

Test suites — fresh worktree @ 6d706dc5, full npm run build first

Check Result
web-shell · Markdown.test.ts 46/46 (was 40 — +6: null, renderer-throws, rendered-node-throws, c++/c#/f#)
Full web-shell vitest suite 706/706 (55 files)
core · bundled-skills.integration.test.ts 12/12 — incl. web-shell-charts/SKILL.md parses with required fields ✓
npm run build (all packages, incl. web-shell lib + tsc) ✅ exit 0
Prettier (7 files) · ESLint (5 TS/TSX) · git show --check ✅ clean

Real-UI E2E — the hook genuinely renders a chart

I served the real <Markdown> component through the web-shell vite dev server (serve-mode src aliases → no stubs, no mocks) and registered a host renderCodeBlock that turns an echarts-fulldata fence into a live Apache ECharts chart. The PR ships no chart runtime by design — a host-provided renderer is exactly the contract. DOM assertions are embedded in the capture; 0 console errors.

1 · echarts-fulldata → live host chart; the adjacent plain ts fence still uses the built-in highlighter (dark). The hook is scoped, not a takeover — echartsCanvas=1, preCode=1.

chart dark

2 · Only theme changes → the chart re-themes (light). Proves theme is threaded from useTheme() through the render info to the host.

chart light

3 · <Markdown> with no source prop → hook gated OFF. Both fences fall back to default code blocks, and the echarts-fulldata block keeps its full language tag (not truncated to echarts) — echartsCanvas=0, preCode=2.

nosource gate

Mutation tests — the new tests are load-bearing

Each mutation to Markdown.tsx is caught by exactly the test that guards that behavior; restoring the file leaves all 8 custom-code-block tests green.

Mutation applied to Markdown.tsx Proves Result
theme: appThemetheme: 'dark' theme comes from useTheme(), not hard-coded replace test fails
language-([^\s]+)(\w+) in extractRawFenceLanguage the full hyphenated fence tag is captured replace test fails (echarts-fulldataecharts)
custom != nullcustom !== undefined null also declines (harden contract) returns null test fails
bypass CustomCodeBlockBoundary (return node directly) the error boundary is load-bearing (harden) rendered content throws test fails
drop !!source from the gate source-gating actually blocks the host renderer source omitted test fails (spy called 1×)

Notes (non-blocking)

  • The docs/design/skill-required-capabilities.md draft leaves open whether web-shell-charts should be a core bundled skill or host-injected. As shipped it is core-bundled and unconditionally exposed, so a non-Web-Shell session that activates it could have the model emit an echarts-fulldata block that renders as raw code on clients without the renderer — the exact gap the draft names. Worth a follow-up, not a blocker for this hook.
  • Cross-platform: pure TS/React plus one markdown skill, no platform-specific APIs; I verified on macOS.

Re-verified in an isolated git worktree at 6d706dc5 (= PR head.sha). Screenshots are element-captures of the real <Markdown> output, hosted from a branch on my fork.

🇨🇳 中文报告(完整对应)

✅ 维护者在 head 6d706dc5 的复验 —— harden + docs 两个提交之后

这是我上次在 d0ef6d88 验证之后的跟进。新增的两个提交 —— 634772ea6harden 加固)和 6d706dc59docs 设计文档)—— 改动了运行时行为,所以我在一个全新的 git worktree 里从零重新构建并再次实测了本 PR。harden 提交恰好补上了我上次指出的 null 路径缺口。 结论:LGTM,可以合并。

改动范围(相对 main 净值): 7 个文件,+818 / −13。运行时改动是在 Web Shell 的 Markdown renderer 上新增一个受 source 门控的 renderCodeBlock 钩子,外加一个内置 web-shell-charts skill。那份 +323 行的 docs/design/skill-required-capabilities.md 只是讨论草稿 —— grep 在代码里找不到任何对 required-capabilities 的引用。

d0ef6d88 以来的变化,以及我核对了什么

  • 634772ea6(harden 加固):
    • null 现在也会“婉拒” → 回退到默认代码块(之前只有 undefined 才回退 —— 上次我指出 null 路径没被测到)。新增测试 returns null ✓,下方变异测试已证明其承重。
    • host renderer 外面加了两道相互独立的保险,都会回退到内置的 CodeBlock
      • try/catch:当 renderCodeBlock(...) 同步抛错时 → console.error('[web-shell] … failed:', error)(2 个参数)。已有测试 ✓。
      • 一个 CustomCodeBlockBoundary 错误边界:当返回的节点在渲染阶段抛错时 → console.error(…, error, componentStack)(3 个参数),并且以 resetKey={code} 复位。已有测试 ✓。
    • resolveFenceLanguage 新增 fsharp 语言以及 c++ / c# / f# 别名。
  • 6d706dc59(docs): 设计草稿,无运行时代码。

测试套件 —— 全新 worktree @ 6d706dc5,先跑完整 npm run build

检查项 结果
web-shell · Markdown.test.ts 46/46(原为 40 —— 新增 6:null、renderer 抛错、渲染节点抛错、c++/c#/f#
web-shell 完整 vitest 套件 706/706(55 个文件)
core · bundled-skills.integration.test.ts 12/12 —— 含 web-shell-charts/SKILL.md parses with required fields ✓
npm run build(所有包,含 web-shell lib + tsc ✅ exit 0
Prettier(7 文件)· ESLint(5 个 TS/TSX)· git show --check ✅ 干净

真实 UI 端到端 —— 钩子确实渲染出了图表

我通过 web-shell 的 vite 开发服务器(serve 模式的 src 别名 → 不打桩、不 mock)挂载了真实的 <Markdown> 组件,并注册了一个 host renderCodeBlock,把 echarts-fulldata 代码围栏变成一张实时的 Apache ECharts 图表。本 PR 有意不自带图表运行时 —— 由 host 提供 renderer 正是它约定的契约。DOM 断言直接内嵌在截图流程里;0 条 console 报错

1 · echarts-fulldata → host 实时图表;相邻的普通 ts 围栏仍走内置高亮器(暗色)。 钩子是有作用域的,不是全盘接管 —— echartsCanvas=1, preCode=1

chart dark

2 · 只改 theme → 图表随之换主题(亮色)。 证明 theme 是从 useTheme() 经 render info 一路传给 host 的。

chart light

3 · <Markdown> 不带 source prop → 钩子被门控关闭。 两个围栏都回退成默认代码块,且 echarts-fulldata 块保留完整语言标签(没被截断成 echarts)—— echartsCanvas=0, preCode=2

nosource gate

变异测试 —— 新增测试确实承重

Markdown.tsx 的每一处变异,都恰好被守护该行为的那条测试逮住;把文件还原后,8 条自定义代码块测试全绿。

Markdown.tsx 施加的变异 证明了什么 结果
theme: appThemetheme: 'dark' theme 来自 useTheme(),而非写死 replace 测试失败
extractRawFenceLanguagelanguage-([^\s]+)(\w+) 完整的带连字符围栏标签被捕获 replace 测试失败(echarts-fulldataecharts
custom != nullcustom !== undefined null 也会婉拒(harden 契约) returns null 测试失败
绕过 CustomCodeBlockBoundary(直接返回节点) 错误边界是承重的(harden) rendered content throws 测试失败
从门控里去掉 !!source source 门控确实拦住了 host renderer source omitted 测试失败(spy 被调用 1 次)

说明(不阻塞合并)

  • docs/design/skill-required-capabilities.md 草稿留了个未定问题:web-shell-charts 到底该做核心内置 skill,还是由 host 注入。当前实现里它核心内置且无条件暴露的,因此非 Web-Shell 会话若激活它,模型可能产出一个 echarts-fulldata 块、在没有该 renderer 的客户端上就渲染成原始代码 —— 正是草稿点名的那个缺口。值得后续跟进,但不阻塞本钩子。
  • 跨平台:纯 TS/React 加一个 markdown skill,无平台相关 API;我在 macOS 上验证。

在隔离的 git worktree 中于 6d706dc5(= PR head.sha)复验。截图是对真实 <Markdown> 输出的元素级截取,图片托管在我 fork 的一个分支上。

@wenshao wenshao 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.

⚠️ Downgraded from Approve to Comment: CI still running.

Rendering extension point and tests look clean. One suggestion on the bundled skill visibility below.

Comment thread packages/core/src/skills/bundled/web-shell-charts/SKILL.md Outdated
Comment thread packages/core/src/skills/bundled/web-shell-charts/SKILL.md Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx
Comment thread packages/web-shell/client/components/messages/Markdown.test.ts Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/customization.tsx
Comment thread packages/web-shell/client/components/messages/Markdown.test.ts
@wenshao

wenshao commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Decision on docs/design/skill-required-capabilities.md: go with Option B (client-supplied skill)

The design doc asks whether core should add a generic required-capabilities skill frontmatter + client capability negotiation (Option A), or keep web-shell-charts out of core bundled skills and let rendering-capable clients supply it themselves (Option B). After going through both: Option B.

The decisive point: echarts-fulldata is not a "Web Shell capability"

The core feature of this PR is customization.markdown.renderCodeBlock — an extension point supplied by the embedder. Web Shell itself ships no ECharts renderer; a bare qwen serve --web renders the same giant code block. So the capability is really owned by "a host app that registered this specific renderer", not by the client type.

That means Option A has no simplified form. It cannot hardcode "web client ⇒ capable"; it needs full capability propagation from the host React layer → daemon/SDK/ACP → core Config, plus session-scoped semantics — the doc's own Open Questions list (late attach, multi-client shared sessions, capability registry, …) is exactly this class of problem, and session/connection-scoped state propagation has been a recurring bug source in this codebase (e.g. #6066, where late-attaching connections lost session commands). Building a distributed capability-negotiation layer for a single skill is out of proportion.

Supporting reasons for B

  1. Ownership symmetry. The party that registers the renderer is the same party that injects the skill, so the capability and the prompt contract can never drift apart. Analogy: Java's ServiceLoader — the JAR shipping the implementation ships its own META-INF/services entry; you don't put the registration into the JDK and then add a protocol to probe whether some JAR happens to contain the implementation. Option A is the latter: the frontmatter capability string and the actual renderer registration can still go out of sync.
  2. The contract isn't settled yet. The [Critical] on SKILL.md (payload defined as executable JavaScript) is unresolved. Bundling the skill in core canonicalizes a security-contested output contract; client-owned keeps the contract next to the host that executes it, and each host can choose a JSON-only payload or a sandboxed execution model.
  3. Existing mechanisms suffice today. File-system skills (.qwen/skills/, project or user level) go through the same skill scanning in serve/daemon mode. A host deploying Web Shell drops in one SKILL.md at the same time it registers the renderer — zero core change.
  4. B doesn't foreclose A. required-capabilities is an optional, purely additive field (the doc's own Migration section says so). Adopt it when a second or third host-specific skill shows up (mermaid-interactive, vega-lite, …) — by then there will be real demand shapes to design against. Doing A now means building a framework for a single consumer.

Option A's two selling points both have cheap substitutes:

  • "One canonical copy" → ship SKILL.md as a copyable template under web-shell docs/examples; the source of truth stays single, it just isn't core-bundled.
  • "Cross-client consistency" → where the renderer doesn't exist, the skill should be invisible; the "inconsistency" is the correct behavior, not a defect.

Concrete asks for this PR

  • The renderCodeBlock extension point itself is uncontested — keep as is.
  • Move packages/core/src/skills/bundled/web-shell-charts/SKILL.md out of core bundled skills; publish it as a copyable template under web-shell docs/examples, and document that a host registering an echarts-fulldata renderer should install the skill into .qwen/skills/.
  • Keep the design doc; add a short decision note at the top: Option B adopted for now, revisit A when a second capability-gated skill appears.
  • On the disable-model-invocation: true stopgap suggested earlier: it hides the skill by default in all environments including Web Shell — bundled-but-hidden-everywhere is equivalent to not bundled, which itself points to B being the honest shape.
中文版(完整对应)

关于 docs/design/skill-required-capabilities.md 的决定:采用 Option B(客户端自带 skill)

设计文档问的是:core 是否要加通用的 required-capabilities skill frontmatter + 客户端能力协商机制(Option A),还是 core 不打包 web-shell-charts、由支持该渲染的客户端自己提供(Option B)。两个方案过了一遍之后:选 B

决定性理由:echarts-fulldata 根本不是 "Web Shell 的能力"

这个 PR 的主体功能是 customization.markdown.renderCodeBlock —— 一个由 embedder 传入的扩展点。Web Shell 本身不带 ECharts 渲染器,裸 qwen serve --web 同样只会显示一大块代码。所以能力的真实归属是"某个宿主应用注册了这个特定 renderer",而不是"客户端类型是 Web Shell"。

这意味着 Option A 没有简化版可做:它不能写死 "web client ⇒ 有能力",必须把能力集从宿主 React 层 → daemon/SDK/ACP → core Config 全链路传播,还要回答 session 级语义——文档自己列的 Open Questions(晚 attach、多客户端共享 session、能力注册表……)全是这类问题,而 session/connection 级状态传播在这个 codebase 里是反复出事故的地方(例如 #6066,延迟 attach 的连接丢失 session commands)。为一个 skill 引入一套分布式能力协商机制,成本收益完全不成比例。

B 的支撑理由

  1. 所有权对称。注册 renderer 的一方和注入 skill 的一方是同一方,能力与提示词契约天然不会漂移。类比 Java 的 ServiceLoader:提供实现的 JAR 自带 META-INF/services 注册项——你不会把注册项放进 JDK,再给 JDK 加一套协议去探测"某个 JAR 是否恰好有实现"。Option A 就是后者:frontmatter 里的能力字符串和宿主实际注册的 renderer 仍然可能不同步。
  2. 契约还没定型。SKILL.md 上那条 [Critical](payload 被定义为可执行 JavaScript)还未解决。core bundled = 把一个有安全争议的输出契约固化为官方契约;client-owned 让契约跟执行它的宿主待在一起,各宿主可以选 JSON-only 或沙箱执行方案。
  3. 现成机制今天就够用。文件系统 skill(.qwen/skills/,项目级/用户级)在 serve/daemon 模式下走同一套扫描。宿主部署 Web Shell 时,在注册 renderer 的同时写入一个 SKILL.md 即可——core 零改动。
  4. B 不堵死 Arequired-capabilities 是可选字段、纯增量(文档 Migration 节自己也这么写)。等出现第二、第三个 host-specific skill(mermaid-interactive、vega-lite……)再上通用机制,届时有真实的需求形状可参考。现在上 A 是给唯一消费者建框架。

Option A 的两个卖点都有便宜的替代:

  • "canonical 只有一份" → 把 SKILL.md 作为可复制模板放到 web-shell 的 docs/examples;源头仍然唯一,只是不进 core bundled。
  • "跨客户端一致" → renderer 不存在的地方 skill 本来就该不可见,"不一致"恰恰是正确行为,不是缺陷。

对这个 PR 的具体要求

  • renderCodeBlock 扩展点部分没有争议,保持现状。
  • packages/core/src/skills/bundled/web-shell-charts/SKILL.md 从 core bundled 移出,作为可复制模板发布到 web-shell 的 docs/examples,并在文档里写清:注册 echarts-fulldata renderer 的宿主应同时把该 skill 装进 .qwen/skills/
  • 设计文档保留,在开头加一段 decision 记录:本次采用 B;当出现第二个需要能力门控的 skill 时再评估 A。
  • 关于之前提的 disable-model-invocation: true 临时缓解:它的实际效果是让这个 skill 在所有环境(包括 Web Shell)默认不可见——bundled 但处处隐藏等于没 bundled,这本身就侧面说明 B 才是诚实的形态。

@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.

⚠️ Downgraded from Approve to Comment: CI failing: Post Coverage Comment, Test (ubuntu-latest, Node 22.x).

No critical issues found. Two minor suggestions below on defense-in-depth and test coverage gaps.

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/components/messages/Markdown.tsx
Comment thread packages/web-shell/client/components/messages/Markdown.test.ts

@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.

Code Review Summary

Overall this is a well-structured PR. The renderCodeBlock customization hook is clean, the ErrorBoundary integration is solid, and the test coverage for the Markdown pipeline is thorough. A few actionable items in EchartsFullDataBlock:

Issues to fix

  1. getCell produces blank table cells for named dimensions + array-format source (Medium) — When dimensions are named strings like ['day', 'orders'] but source rows are arrays like ['Mon', 120], Number('day') yields NaN and every cell returns undefined. This is a common ECharts dataset format that ECharts itself handles by mapping named dimensions to array indices internally.

  2. option reference instability causes chart re-init on every parent re-render (Medium) — createEchartsFullDataRenderer calls parseOption(info.code) on every render, producing a fresh object. The useEffect dependency on option causes dispose + re-init on every parent re-render even after streaming settles (theme change, upstream toggle, etc.). A useMemo on info.code for the parsed result would fix this.

  3. No loading indicator while loadEcharts() resolves (Low-Medium) — ChartLoadingState exists but is only shown for parseError && isStreaming. During async loadEcharts() resolution, users see a blank 360px container.

Minor items

  • The .catch handler in the chart useEffect sets chartError state but never calls console.error — makes production debugging harder. Other error paths in this codebase (e.g., MarkdownFencedCode's custom renderer catch) do log to console.
  • Test gap: no test exercises the loadEcharts=undefined path ("Chart runtime is unavailable") or the useEffect .catch branch.

What's good

  • Clean separation of MarkdownFencedCode from MarkdownCode for the custom renderer logic
  • ErrorBoundary with resetKeys tied to language and isStreaming/code is a smart strategy
  • Comprehensive test suite for the Markdown pipeline (51 tests covering decline/null/false fallback, resolved aliases, error boundary reset, components.code precedence)
  • Object.hasOwn guard on LANGUAGE_ALIASES prevents prototype pollution

Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/README.md

@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.

Automated Code Review

9 parallel review agents analyzed correctness, security, code quality, performance, test coverage, attacker/oncall/maintainer mindset, and build/tests. Reverse audit pruned 6 false positives.

Build: ✅ passed | Tests: ✅ 102 web-shell + 14 CLI passed

Severity Count
Critical 1
Suggestion 4

See inline comments below for details.

Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated

@wenshao wenshao 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.

/review (medium effort) on 4191476: 8 findings — 3 bugs (array-row {c} labels break in the preferred envelope format, stale chart-error overlay during data-ref re-resolution, fence-language highlighting regression for glued-meta info strings) and 5 suggestions. All posted inline with concrete repro and a suggested fix; the two heavier bug claims were verified against real echarts 5.6.0 / react-markdown 9.1.0 rather than from memory.

中文

4191476 的 /review(中等强度):共 8 条——3 个 bug(推荐的 envelope 数组行格式下 {c} 标签渲染整行、data-ref 重新解析期间旧图表错误覆盖层滞留、粘连 meta 的 fence 语言高亮回归)+ 5 条建议。全部内联附具体复现与修法;两条较重的 bug 用真实 echarts 5.6.0 / react-markdown 9.1.0 实测验证,非凭记忆推断。

— claude-fable-5 via Claude Code /review

Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/customization.tsx Outdated

@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.

Review Summary

Verdict: Approve

The PR adds a well-structured echarts-fulldata code block renderer with two-layered sanitization (top-level allowlist + nested denylist), proper async lifecycle management, and solid test coverage (106 tests passing).

Strengths

  • Two-layered sanitization approach (SAFE_TOP_LEVEL_OPTION_KEYS allowlist + UNSAFE_OPTION_KEYS denylist) is defense-in-depth
  • renderRequestRef cancellation pattern correctly prevents stale async operations
  • isEchartsFullDataEnvelope type guard is thorough with fallback tryParse path
  • Clean separation between data resolution, parsing, sanitization, and rendering
  • Good i18n coverage (EN/ZH)

Minor Observations

  • One inline comment posted below re: array index shifting in sanitizeOptionValue
  • Test coverage is comprehensive but could benefit from a streaming-to-settled transition test for EchartsFullDataBlockFromCode (verifying the isStreaming prop flip)

Deterministic Analysis

  • tsc: 0 errors
  • eslint: 0 errors
  • Tests: 106/106 passing (50 EchartsFullDataBlock + 56 Markdown)

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/Markdown.tsx Outdated

@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.

Re-review of PR #6232

The code has evolved significantly across 3 prior review rounds. Many previously reported issues have been addressed (getCell delegation, parseOption memoization, chartError clearing, ResizeObserver, theme init, accessibility attributes, etc.).

Build & Tests: All passing (50/50 EchartsFullDataBlock, 56/56 Markdown, ESLint clean, TypeCheck clean on changed files).

This review focuses on 8 new issues not covered in prior rounds. The most impactful:

  1. URI sanitization gap -- isUnsafeUriString misses protocol-relative URLs and uncommon schemes
  2. Silent annotation disabling -- data in UNSAFE_OPTION_KEYS strips markLine/markPoint data at all depths
  3. Zero diagnostic logging in the parse/validation/data-ref pipeline

The security design is solid overall -- the two-layer allowlist+denylist with leaf-value filtering is well-structured, and normalizeDataRef has thorough path traversal prevention.

Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/index.ts
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated
Comment thread packages/web-shell/client/components/messages/EchartsFullDataBlock.tsx Outdated

@wenshao wenshao 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 new review findings. Downgraded from Approve to Comment because the presubmit CI-status check could not be completed due a GitHub API TLS timeout.

— GPT-5 via Qwen Code /review

@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Suggestions — commit 1dffa3b9

File Issue Suggested fix
Markdown.tsx:418 extractRawFenceLanguage regex change from \w+ to [^\s]+ affects ALL code blocks globally, not just echarts-fulldata. Hyphenated language tags now pass the full tag to resolveFenceLanguage. Document as a breaking change for renderCodeBlock consumers, or gate the new regex to only apply when a custom renderer is registered.
EchartsFullDataBlock.tsx:1027 normalizeEnvelopeDataset uses .replace('Chart data row', 'Chart envelope data.source row') on error messages — fragile string substitution that breaks if validateDatasetCell rewords. Pass a rowLabel prefix parameter to validateDatasetCell instead of post-hoc string replacement.
index.ts:41-57 + index.tsx:129-145 Identical 19-line echarts export blocks duplicated in both entry points. Have one file re-export from the other, or extract a shared barrel file.
EchartsFullDataBlock.tsx:1648 Render effect includes t (i18n function) in dependency array. Language switch causes every chart to dispose and reinitialize. Store t in a ref and remove from deps, like loadEchartsRef.
EchartsFullDataBlock.tsx:176 UNSAFE_OPTION_HTML_TAG_PATTERN blocklist misses <video>, <audio>, <details>, <math>, etc. Broaden to /\/?[a-z][a-z0-9]*(?=[\s/>])/i to block all HTML-like tags.
EchartsFullDataBlock.tsx:290 isUnsafeUriString doesn't detect CSS url() wrappers (e.g., url(javascript:alert(1))). Add /^url\s*\(/i.test(normalized) check.
EchartsFullDataBlock.tsx:302 sanitizeDatasetCell uses isUnsafeUriString but not isUnsafeOptionString — HTML tags in dataset cells pass through. Use isUnsafeOptionString for consistency with option-value sanitization.

— 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.

Automated Review Summary

Verdict: Comment — No blocking issues found. The security architecture is well-designed with effective layered defenses (top-level allowlist, nested denylist, forced tooltip safety overrides, JSON cloning, depth/size limits). Test coverage is thorough (2820+ lines of tests covering sanitization, prototype pollution, URI injection, and HTML injection).

Deterministic analysis: 0 findings (tsc, eslint)
Tests: 854/859 passed (5 pre-existing build-artifact.test.ts env failures, unrelated to PR)

9 suggestions for improvement below, focused on defense-in-depth, accessibility, and API contract clarity.

Comment thread packages/web-shell/client/components/messages/Markdown.tsx

@wenshao wenshao 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 high-confidence issues found. The sanitization architecture (top-level allowlist + nested denylist + URI/HTML stripping + size limits) is thorough and well-designed. Lib build and both test suites (130 tests) pass cleanly.

Low-confidence items that may deserve a human look:

  • DATA_REF_TIMEOUT_MS (30s) is shared between data-ref resolution and ECharts runtime loading — different operations with different expected durations.
  • UNSAFE_OPTION_HTML_TAG_PATTERN denylist omits <video>, <audio>, <source>, <track>, <math> (mitigated by renderMode: 'richText').
  • resolvedDataRefCacheRef has no error invalidation — transient failures become permanent for that component instance.
  • No console.warn when resolveDataRef is undefined and chart uses data.kind: "ref" — all other failure paths log.
  • ECharts exports (3 values + 10 types) copy-pasted across index.ts and index.tsx.
  • Test gaps: no markPoint.data sanitization test, no #/\ in data-ref validation, no e2e Markdown render for c#/f# fences.

— 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.

Summary

This PR adds a well-structured custom code block renderer hook (renderCodeBlock) to the web-shell Markdown component and a comprehensive EchartsFullDataBlock implementation with a two-layered sanitization strategy, data-ref resolution, and theme-aware chart rendering.

Build: ✅ PASS (vite, both builds clean)
Tests: ✅ PASS (862 tests across 61 files, 0 failures)
Deterministic checks (tsc/eslint): ✅ 0 findings (pre-verified)

The overall design is solid — the two-layer sanitizer (allowlist top-level / denylist nested), the envelope/legacy data model, and the host-supplied ECharts runtime loader are all well-reasoned. Findings below are primarily defense-in-depth gaps and maintenance concerns, no critical correctness bugs were found.

Findings overview

Severity Count Themes
Suggestion 5 Sanitization consistency, dual entry-point files, useEffect cleanup safety, observability, type contract
Nice to have 2 File size, test-only export surface

The most impactful item is the sanitizeDatasetCell HTML-tag gap (#1) — not exploitable today (React auto-escapes, ECharts renders on canvas), but a latent defense-in-depth hole if any future render path trusts dataset cell content. The dual entry-point files (#2) and the unprotected dispose() (#3) are the next highest-leverage cleanups.

Comment thread packages/web-shell/client/customization.tsx
Comment thread packages/web-shell/client/index.ts
@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@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.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 4, 2026
Merged via the queue into QwenLM:main with commit e9a7917 Jul 4, 2026
161 checks passed
@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

✅ Local end-to-end verification (maintainer)

I rebuilt and exercised this PR locally at head 31e05f70 — full test suite, mutation testing, a production lib build, and real ECharts driven through the actual Markdown → renderCodeBlock → EchartsFullDataBlock path in a browser (not mocks). Everything checks out. This is a confirmatory pass to back the existing approval and serve as a merge reference.

1. Build & tests

Check Result
web-shell vitest — full suite 862 / 862 passed · 61 files
  └ EchartsFullDataBlock.test.tsx 72 passed
  └ Markdown.test.ts 58 passed
  └ EnhancedMarkdownTable.test.tsx 50 passed
  └ build-artifact.test.ts (imports the built dist) 5 passed — collected, so no silent under-count
web-shell production build (vite build + lib build + tsc -p tsconfig.lib.json) exit 0 — new public exports & .d.ts compile cleanly
CI Test (ubuntu-latest, Node 22.x) · review-pr 🟢 green (PR is MERGEABLE; mac/win legs skip per CI config)

The full suite was run after building sibling + web-shell dist, so the dist-importing specs actually collect (a pre-build run silently drops them from the total).

2. Mutation testing — are the tests load-bearing?

I reverted one piece of logic at a time and confirmed the PR's own tests catch it, then restored (tree ends clean). All 7 mutations were detected:

# Mutation Targeted test Result
M1 Disable renderCodeBlock hook entirely lets host renderers replace assistant fenced code blocks ✓ 13 tests fail
M2 Drop source-gating (!!source) does not call host renderers when markdown source is omitted ✓ exactly 1 fails
M3 Accept boolean returns (drop typeof !== 'boolean') falls back … when the host renderer returns false ✓ exactly 1 fails
M4 Truncate hyphen/punctuation language capture ([^\s]+\w+) passes punctuation language aliases to host renderers ✓ 13 tests fail
M5 Neuter option-sanitizer denylist (UNSAFE_OPTION_KEYS.hasfalse) sanitizes unsafe chart option fields before calling ECharts ✓ exactly 1 fails
M6 Skip theme re-init on theme change reinitializes the chart when the theme changes ✓ exactly 1 fails
M7 Drop assistant-only source gate does not render echarts blocks from thinking markdown ✓ exactly 1 fails

The surgical single-test failures (M2/M3/M5/M6/M7) show the specs pinpoint each behavior; the broad cascades (M1/M4) show the plumbing and hyphenated-language handling are load-bearing across many cases.

3. Real UI — rendered from a real fence, with a host-supplied ECharts runtime

A lightweight harness mounts the real <Markdown source="assistant"> with createEchartsFullDataRenderer({ loadEcharts: () => window.echarts }) (ECharts 5.6 UMD, exactly like a real host would inject it), and Playwright drives it. Zero console/page errors in every scenario.

Real charts

Toggle, sanitizer, errors, i18n

Highlights worth calling out:

  • Envelope + legacy payloads both render; theme-aware repaint (dark/light) works.
  • Chart ↔ Data toggle reuses the extracted EnhancedTable (sort / filter / quick-copy), localized to zh-CN.
  • Sanitizer, concretely proven: a payload genuinely carrying a <script> tag, a javascript: URL, a __proto__ key and a non-allowlisted toolbox block still renders the chart — the dangerous fields are stripped while the allowed markLine annotation survives (dashed average line lands exactly at 320 = mean of 500/320/140). Driving it in a real browser fired 0 alert() dialogs and injected 0 <script> nodes into the DOM.
  • Malformed JSON degrades to a contained error card — no raw-fence leak, no transcript crash.

4. Open review threads — honest read

138 threads total: 121 resolved, 17 unresolved — all 17 are qwen-code-ci-bot suggestions, none outdated, none a correctness blocker. Having read the full source, I'd classify them as defense-in-depth hardening (e.g. also HTML-filter dataset cells, block file: without authority), doc/comment nits, and maintainability (1888-line file, dual entry-points, a test-only exported tripwire constant). One is a genuine minor UX nit worth a follow-up: echartsChart.defaultTitle ("Chart Loading" / "图表加载中") is used as the permanent card heading and aria-label for a title-less chart, so a chart with no title.text reads "Chart Loading" even after it has loaded (visible in the error-card shot). None of these block merge.

Verdict

Functionally correct, safe, and thoroughly tested. Real end-to-end rendering works across chart types, both themes, the data view, i18n, sanitization and error paths; the tests are load-bearing; the packaged public API type-checks. This supports merging — the 17 bot notes are good optional follow-ups.

中文报告(点击展开)

✅ 本地端到端验证(维护者)

我在 head 31e05f70 上本地重建并实际运行了这个 PR —— 完整测试套件、变异测试、生产 lib 构建,以及在浏览器里跑通真实 ECharts、走完整的 Markdown → renderCodeBlock → EchartsFullDataBlock 路径(不是 mock)。全部通过。这是一次确认性验证,用于支撑已有的 approve,并作为合并参考。

1. 构建与测试

检查项 结果
web-shell vitest —— 完整套件 862 / 862 通过 · 61 个文件
  └ EchartsFullDataBlock.test.tsx 72 通过
  └ Markdown.test.ts 58 通过
  └ EnhancedMarkdownTable.test.tsx 50 通过
  └ build-artifact.test.ts(会 import 构建产物 dist 5 通过 —— 成功收集,因此总数没有被静默少算
web-shell 生产构建vite build + lib 构建 + tsc -p tsconfig.lib.json exit 0 —— 新增的公共导出与 .d.ts 均能干净编译
CI Test (ubuntu-latest, Node 22.x) · review-pr 🟢 绿(PR 状态 MERGEABLE;mac/win 腿按 CI 配置 skip)

完整套件是在构建完 sibling 与 web-shell dist 之后跑的,这样那些 import dist 的用例才能真正被收集(构建前跑会把它们从总数里静默丢掉)。

2. 变异测试 —— 这些测试是否真的承重?

我逐条把关键逻辑改坏,确认 PR 自带的测试能抓到,然后还原(最终工作区干净)。7 个变异全部被检出

# 变异 命中的测试 结果
M1 完全禁用 renderCodeBlock 钩子 lets host renderers replace assistant fenced code blocks ✓ 13 个测试挂
M2 去掉 source 门控(!!source does not call host renderers when markdown source is omitted ✓ 恰好挂 1 个
M3 接受布尔返回值(去掉 typeof !== 'boolean' falls back … when the host renderer returns false ✓ 恰好挂 1 个
M4 截断连字符/标点语言捕获([^\s]+\w+ passes punctuation language aliases to host renderers ✓ 13 个测试挂
M5 阉割 option 净化黑名单(UNSAFE_OPTION_KEYS.hasfalse sanitizes unsafe chart option fields before calling ECharts ✓ 恰好挂 1 个
M6 主题切换时跳过重建 reinitializes the chart when the theme changes ✓ 恰好挂 1 个
M7 去掉「仅 assistant」source 门控 does not render echarts blocks from thinking markdown ✓ 恰好挂 1 个

M2/M3/M5/M6/M7 精确地只挂 1 个测试,说明用例精准锚定了各自的行为;M1/M4 的大面积连锁失败说明整条插件链路和连字符语言处理是全局承重的。

3. 真实 UI —— 从真实 fence 渲染,ECharts 运行时由宿主注入

一个轻量 harness 挂载真实<Markdown source="assistant">,并注入 createEchartsFullDataRenderer({ loadEcharts: () => window.echarts })(ECharts 5.6 UMD,跟真实宿主注入方式完全一致),用 Playwright 驱动。所有场景 console/page 报错。

(截图见上方英文部分的两张拼图。)

值得强调的点:

  • envelope 与 legacy 两种载荷都能渲染;主题感知(深/浅色)重绘正常。
  • 图表 ↔ 数据 切换复用了抽取出来的 EnhancedTable(排序 / 过滤 / 快捷复制),并已本地化为 zh-CN。
  • 净化器,实证: 一个真正带有 <script> 标签、javascript: URL、__proto__ 键和不在白名单里的 toolbox 块的载荷仍然能渲染出图表 —— 危险字段被剥离,而被允许的 markLine 标注被保留(虚线均值线正好落在 320 = 500/320/140 的平均值)。在真实浏览器里驱动它触发了 0alert() 弹窗、往 DOM 里注入了 0<script> 节点。
  • 非法 JSON 优雅降级为受控的错误卡片 —— 不泄漏原始 fence,也不会让 transcript 崩溃。

4. 未解决的 review 线程 —— 如实说明

共 138 条线程:121 条已解决,17 条未解决 —— 这 17 条全部是 qwen-code-ci-bot 的建议,均未过期,没有一条是正确性阻断项。我读完了完整源码后的判断:它们属于纵深防御类加固(例如对 dataset cell 也做 HTML 标签过滤、拦截不带 authority 的 file:)、文档/注释小问题,以及可维护性(1888 行的大文件、双入口文件、把一个仅供测试的常量导出到公共表面)。其中有一条是真正值得后续跟进的小 UX 问题:echartsChart.defaultTitle("Chart Loading" / "图表加载中")被用作无标题图表的永久卡片标题与 aria-label,所以一个没有 title.text 的图表即使已经加载完,标题仍显示「图表加载中」(在错误卡片截图里可见)。这些都不阻塞合并。

结论

功能正确、安全、测试充分。 真实端到端渲染在各种图表类型、深浅两主题、数据视图、i18n、净化与错误路径上都跑通;测试确实承重;打包后的公共 API 类型检查通过。支持合并 —— 那 17 条 bot 建议是很好的可选后续项。

Verified locally on a clean worktree at 31e05f70; screenshots are real Playwright captures of the actual components (ECharts 5.6 UMD supplied via loadEcharts). Harness/artifacts were not committed to the PR.

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.

3 participants