feat(cli): expand TUI markdown rendering#3680
Conversation
wenshao
left a comment
There was a problem hiding this comment.
[Critical] resolveCliGenerationConfig() no longer considers OPENAI_MODEL / QWEN_MODEL when looking up the matching modelProvider. Users selecting a custom OpenAI-compatible model through env vars can silently lose provider-specific baseUrl, envKey, or generationConfig, breaking auth/startup/API behavior.
Suggested fix:
const requestedModel =
authType === AuthType.USE_OPENAI
? argv.model ||
env['OPENAI_MODEL'] ||
env['QWEN_MODEL'] ||
settings.model?.name
: argv.model || settings.model?.name;[Suggestion] resetDispatcherCache() clears cached undici Agent / ProxyAgent instances without closing them. Reset/reconfiguration paths can drop references to live dispatchers, leaving sockets/timers open.
Suggested fix:
export function resetDispatcherCache(): void {
for (const dispatcher of dispatcherCache.values()) {
void dispatcher.close();
}
dispatcherCache.clear();
}— gpt-5.5 via Qwen Code /review
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
已更新本轮反馈:
验证已完成: cd packages/cli && npx vitest run src/ui/utils/MarkdownDisplay.test.tsx src/ui/utils/InlineMarkdownRenderer.test.tsx src/ui/utils/mermaidImageRenderer.test.ts
npm run typecheck --workspace=packages/cli
npm run lint --workspace=packages/cli
npm run build --workspace=packages/cli
npm run build && npm run typecheck
git diff --check结果:targeted tests 88/88 passed;CLI build/typecheck/lint passed;root build/typecheck passed。root build 仍有既有 VS Code companion lint warnings,无新增 error。 |
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Table cells still use TableRenderer.renderMarkdownToAnsi(), a separate inline parser that does not include the new $...$ inline math path. This means inline math now renders differently in normal Markdown text vs table cells, and the duplicated inline parser can keep drifting from RenderInline. Consider sharing inline tokenization/rendering with RenderInline, or adding the same inline-math support to table cells with a regression test.
— gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Re-verification at 715bb44
CI is green; re-checked the prior blockers and the latest qwen3.6-plus suggestions against current head. Detailed findings inline.
Blockers still open: §1 (µ key regression) and §6 (unrelated Prettier churn) from the earlier comprehensive review.
Inline math in tables — still unaddressed (out-of-diff so flagged here): MarkdownDisplay.tsx:799-811 RenderTable does not thread enableInlineMath into TableRenderer. As a result $\alpha$ renders raw inside table cells but as α everywhere else, and the duplicated inline parser in TableRenderer.renderMarkdownToAnsi keeps drifting from RenderInline. Either share inline tokenization with RenderInline or thread the prop through RenderTable → TableRenderer and add a regression test.
Already fixed (please retract from the open list):
mermaidImageRenderer.tsenv spread — current code already routes throughcreateRendererChildEnv+RENDERER_ENV_ALLOWLIST(lines 72, 925, 1043, 1090, 1137); the prior reviewer's line numbers were stale.InlineMarkdownRenderer.tsxbacktick alternation order — verified empirically: at the backtick position only`+.+?`+can match (leftmost-match rule), so`**not bold**`correctly resolves to a code span.
Diagram-family fallback coverage and the Mermaid image pipeline have improved meaningfully since the prior pass.
wenshao
left a comment
There was a problem hiding this comment.
Re-review at 715bb44
CI 绿;针对此前几轮 inline review 复核,加上一轮新角度审计。本轮 inline 评论只列新发现或之前未单独锚到具体行的项目;上一轮已 inline 的代码细节状态见末尾清单,行号仍指向 head。
新增(本轮 inline):
mermaidImageRenderer.ts:240—forced协议绕过!process.stdout.isTTY守卫,shell rc 里固定该变量并把 stdout 重定向到管道时会把裸 Kitty 转义吐进非 TTY 流。MermaidDiagram.tsx:92— async cleanup 仅置cancelled = true,不杀mmdc/headless Chrome 子进程;流式渲染 / 频繁 resize 期间会让多个 mmdc 实例并存到自然超时。AppContainer.test.tsx:1066— 用mockedUseKeypress.mock.calls拿到 wiring 后并未调用,紧接着对handleRenderModeToggleKey直接传入新的vi.fn();helper 在 isolation 下能通过,但 AppContainer 自己的 keypress handler 被改坏了不会被这条用例发现。
仍未落地的 blocker(详见此前各轮 inline,行号未变):
- §1 Typed
µ回归 —KeypressContext.tsx:656把裸µ重写成meta+m,BaseTextInput.tsx:148再吞掉 → 输入框无法输入µ;BaseTextInput.test.tsx:80只覆盖paste:true路径。 - §6 Prettier-only churn —
packages/cli/src/commands/review/{cleanup,deterministic,fetch-pr,load-rules,pr-context,presubmit}.ts、packages/core/src/skills/{skill-manager,skill-manager.test,symlinkScope.test}.ts、packages/core/src/services/monitorRegistry.ts与 TUI Markdown 渲染无关,仍建议拆出去单独走。
此前已 inline 的代码细节(仍未修复,复述以便跟踪):
mermaidVisualRenderer.ts:231—(.+)$贪婪匹配吞掉A --> B --> C的第二条边mermaidVisualRenderer.ts:453—mergeCanvasChar('', '─')因'▼▲◀▶→←↩'.includes('')在 JS 中为true走错分支,CJK / 全角标签后画布单元被腐蚀MarkdownDisplay.tsx:253/:259/:288—split(/(?<!\\)\|/)不跳过反引号 code span 里的|,| col1 | `a|b` |会切成 5 列latexRenderer.ts:122—[^{}]+不匹配嵌套{},\frac{\sqrt{x}}{2}静默原样输出latexRenderer.ts:150—replace(/\\left|\\right/g, '')只去关键字不去界定符,\left.\frac{a}{b}\right.→. a/b .mermaidImageRenderer.ts:1193— 子进程 timeout 触发child.kill()默认 SIGTERM,无 SIGKILL 升级;mmdc+ Chromium 在重负载下可忽略 SIGTERMmermaidImageRenderer.ts:370—renderMermaidImageSync仅测试用;async 路径已禁掉 iTerm2,sync 仍在生成 iTerm2 序列,两份实现已开始漂移
— claude opus 4.7 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Re-verification at 4bc06e0
Local npm test (326 tests across 11 files) ✓ · npm run build ✓ · npm run typecheck ✓ · upstream Lint and CodeQL green; OS test matrix in flight.
Both prior critical blockers are resolved, and 7 of 9 substantive findings landed with regression tests. Detail per-file inline.
§1 fix confirmed (out of net diff). KeypressContext.tsx's normalizeKey and the µ → meta+m rewrite were added then fully removed within this PR, so the file has zero net change against main and GitHub won't accept an inline anchor there. Verified manually that broadcast(key) now passes the original key through, and keyboard-shortcuts.md documents the macOS Option-as-Meta requirement for Alt+M. The new typed-µ test in BaseTextInput.test.tsx:86-96 locks the behaviour in place — see inline.
§6 fix confirmed. dc4def7d cleanly reverts all the unrelated Prettier churn (13 files, 0 semantic content).
Still open:
- Inline math in tables —
MarkdownDisplay.tsx:836-848RenderTablestill does not threadenableInlineMathintoTableRenderer, so$\alpha$renders raw inside table cells but asαeverywhere else. This region of the file is also out of net diff. Fine to triage as a follow-up issue rather than re-block this PR.
Recommendation: PR is in mergeable shape modulo the table-cell inline-math gap above.
|
已处理最新 review body 里提到的 inline math in tables gap:新增 |
wenshao
left a comment
There was a problem hiding this comment.
Test coverage gaps (Suggestion): mermaidVisualRenderer.test.ts (49 lines) covers only flowchart of 11 diagram types — sequenceDiagram, classDiagram, stateDiagram, erDiagram, gantt, pie, journey, mindmap, gitGraph, requirementDiagram are all untested (600+ lines of rendering logic). MermaidDiagram.test.tsx does not cover the image.kind === 'ansi' (chafa) render path. mermaidImageRenderer.test.ts doesn't test terminal protocol auto-detection (TERM/TERM_PROGRAM/KITTY_WINDOW_ID branches).
Code quality (Suggestion): mermaidImageRenderer.ts has ~90% code duplication between sync/async render paths (renderPngWithMmdc / renderPngWithMmdcAsync, etc.). InlineMarkdownRenderer.tsx and copyCommand.ts duplicate the INLINE_MATH_MAX_CHARS constant and inline math regex — consider extracting to a shared module. mermaidVisualRenderer.ts is 1560 lines with 12 diagram types in one file — consider splitting into separate modules.
— deepseek-v4-pro via Qwen Code /review
|
已处理最新 review:\n\n- 修复 table cell inline math 中的 pipe 被拆列: |
wenshao
left a comment
There was a problem hiding this comment.
Approving at ee3b9dc
Local npm test (291 targeted tests across 8 files) ✓ · npm run build ✓ · npm run typecheck ✓.
The two new commits since my last pass close every functional finding I raised:
a450d6ac— inline math in tables.RenderTablenow threadsenableInlineMathintoTableRenderer;renderMarkdownToAnsihandles$...$viarenderInlineLatexwith the same guard rails as the prose path. Regression test inMarkdownDisplay.test.tsx.ee3b9dc9— proactive hardening I didn't ask for:- Latex recursion bound (
MAX_RENDER_DEPTH = 10) prevents stack blow-ups on adversarial nested\frac{\frac{...}}input. - Mermaid input/label/source paths now run through
sanitizeTerminalText(strip ANSI + C0/C1 controls). This closes a real terminal-escape-injection vector from model-controlled Mermaid blocks — nice catch. - Table-row tokenizer now consumes
$...$spans atomically so pipes inside inline math no longer split cells.
- Latex recursion bound (
Two low-priority advisory items remain open by design (and explicitly accepted in the prior pass):
renderMermaidImageSyncstill test-only; kept with@internalannotation.String.fromCharCode(45,45,62)obfuscation kept to silence the CodeQL alert.
Neither blocks merge. Approving.
wenshao
left a comment
There was a problem hiding this comment.
Re-approving at 5b0029c
Auto-dismissed by branch protection when 5b0029c66 landed. The new commit tightens sanitizeTerminalText to also drop C1 controls (0x80-0x9F), which catches the 8-bit CSI introducer 0x9B — the alternative encoding for terminal escape sequences that the previous C0-only filter would have let through. Test coverage extended accordingly.
This actually closes the small gap in my prior approval body (which described the filter as "C0/C1" when the implementation only handled C0+DEL). With this commit the description and the code match.
All 142 targeted tests still pass locally. Re-approving.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* feat(cli): expand markdown rendering in tui * fix(cli): render finalized mermaid images synchronously * fix(cli): harden markdown rendering paths * fix(cli): preserve mermaid source fallbacks * test(cli): make mermaid image renderer mock cross-platform * fix(cli): run windows mmdc shims through shell * fix(cli): address markdown rendering review comments * fix(cli): validate mermaid render timeout * feat(cli): expose rendered markdown source blocks * fix(cli): align mermaid source copy controls * fix(cli): make markdown render toggle visible * fix(cli): keep markdown render toggle quiet * fix(cli): generalize markdown render mode * fix(cli): broaden render mode and copy latex blocks * fix(cli): align rendered copy source indices * fix(cli): support copying inline latex expressions * feat(cli): document markdown render controls * test(cli): cover markdown render controls * fix(cli): tighten markdown render fallbacks * fix(cli): bound mermaid renderer cache * fix(cli): address markdown render review feedback * fix(cli): address markdown render review comments * test(cli): strengthen render mode shortcut coverage * fix(cli): address mermaid image review comments * fix(cli): stabilize renderer output truncation * test(cli): flush fake renderer stderr before exit * fix(cli): address markdown renderer review feedback * docs(cli): clarify mermaid image limits * fix(cli): refresh mermaid images on height resize * fix(cli): address markdown render review * chore: revert unrelated review formatting churn * fix(cli): avoid mermaid regex codeql alert * fix(cli): silence mermaid operator codeql alert * fix(cli): render inline math in markdown tables * fix(cli): harden markdown visual renderers * fix(cli): strip c1 controls from mermaid previews --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
Co-authored-by: Jacob Richman <jacob314@gmail.com> Co-authored-by: Scott Densmore <scottdensmore@mac.com>
* feat(cli): expand markdown rendering in tui * fix(cli): render finalized mermaid images synchronously * fix(cli): harden markdown rendering paths * fix(cli): preserve mermaid source fallbacks * test(cli): make mermaid image renderer mock cross-platform * fix(cli): run windows mmdc shims through shell * fix(cli): address markdown rendering review comments * fix(cli): validate mermaid render timeout * feat(cli): expose rendered markdown source blocks * fix(cli): align mermaid source copy controls * fix(cli): make markdown render toggle visible * fix(cli): keep markdown render toggle quiet * fix(cli): generalize markdown render mode * fix(cli): broaden render mode and copy latex blocks * fix(cli): align rendered copy source indices * fix(cli): support copying inline latex expressions * feat(cli): document markdown render controls * test(cli): cover markdown render controls * fix(cli): tighten markdown render fallbacks * fix(cli): bound mermaid renderer cache * fix(cli): address markdown render review feedback * fix(cli): address markdown render review comments * test(cli): strengthen render mode shortcut coverage * fix(cli): address mermaid image review comments * fix(cli): stabilize renderer output truncation * test(cli): flush fake renderer stderr before exit * fix(cli): address markdown renderer review feedback * docs(cli): clarify mermaid image limits * fix(cli): refresh mermaid images on height resize * fix(cli): address markdown render review * chore: revert unrelated review formatting churn * fix(cli): avoid mermaid regex codeql alert * fix(cli): silence mermaid operator codeql alert * fix(cli): render inline math in markdown tables * fix(cli): harden markdown visual renderers * fix(cli): strip c1 controls from mermaid previews --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
Summary
Supported Scope
This is a first integrated Markdown syntax extension, not full CommonMark / Mermaid / TeX parity.
User-facing controls and source access:
ui.renderModecontrols the default Markdown view."render"shows rich previews and"raw"starts in source-oriented mode.Alt/Option+Mtoggles the active session between rendered previews and raw/source mode without rewriting settings.Rendered Mermaid headings show Mermaid-specific source hints such as
/copy mermaid 1.Rendered LaTeX block headings show source hints such as
/copy latex 1; inline LaTeX can be copied with/copy latex inline Nor/copy inline-latex N./copy codestill counts all fenced code blocks, while/copy mermaid Ncounts only Mermaid blocks.User-facing docs were added under
docs/users/features/markdown-rendering.md, with keyboard shortcut and settings references.Mermaid fenced code blocks:
mermaid.flowchart/graph,sequenceDiagram, and bounded summaries for common Mermaid families includingclassDiagram,stateDiagram,erDiagram,gantt,pie,journey,mindmap,gitGraph, andrequirementDiagram.A --> B, labeled edges, basic Mermaid node shapes, TD/LR-style layout, and cycle summaries. It is not a complete Mermaid layout engine.participant/actorlines and common message arrows such as->>,-->>,->,-->,-x, and--x.LaTeX / math:
$...$within a line.$$fences on their own lines.\frac{a}{b}, simple\sqrt{x}, and limited superscript/subscript characters.Markdown lists and quotes:
[x],[X], and[ ]render as terminal checked/unchecked symbols.Tables and code:
Implementation Details
ui.renderModeis an enum setting (render/raw) that seeds the default render mode and can be changed without restart. The keyboard shortcut changes only the current session view.$...$math handling alongside bold, italic, strikethrough, links, inline code, underline tags, and URLs.mmdc/@mermaid-js/mermaid-clirenders Mermaid source to PNG. The PNG is then emitted through Kitty-compatible virtual placement for Ghostty/Kitty, iTerm2 inline image sequences for iTerm2/WezTerm-style support, orchafaANSI output when no terminal image protocol is detected.QWEN_CODE_MERMAID_IMAGE_RENDERING=1.QWEN_CODE_MERMAID_IMAGE_PROTOCOL=kitty|iterm2|off.QWEN_CODE_MERMAID_MMD_CLIwhen set, otherwise searchesmmdconPATH; repo-localnode_modules/.binrenderers requireQWEN_CODE_MERMAID_ALLOW_LOCAL_RENDERERS=1.QWEN_CODE_MERMAID_ALLOW_NPX=1allows ad-hocnpx -y @mermaid-js/mermaid-cli@11.12.0, but the first run can be slow.docs/design/markdown-syntax-extension.md, based on the earlierdocs/design/tui-optimizationresearch.Validation
/copy mermaid 1plus/copy latex 1.mmdcplus a supported terminal image path are available.@mermaid-js/mermaid-clisommdcis onPATH, or setQWEN_CODE_MERMAID_MMD_CLI.Scope / Risk
npxMermaid CLI path can be slow on first use because it may install/run Mermaid CLI dependencies.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs