Render Mermaid diagrams inline#3679
Conversation
rumpl
commented
Jul 16, 2026
rumpl
commented
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
1 high-severity finding in new Mermaid rendering code.
Lower-confidence findings (not posted inline)
- [low] pkg/mermaid/parser.go:33 — Sequence diagrams with only notes (no messages) incorrectly rejected as ErrInvalidDiagram (confidence: weak 35/100)
- [low] pkg/mermaid/parser.go:69 — Escape check in splitMermaidStatements uses simple look-behind, mishandling double-escaped backslashes (confidence: negligible 25/100 — dropped)
- [low] pkg/mermaid/parser.go:144 — readDelimited byte-level escape check can misfire on multi-byte UTF-8 content (confidence: negligible 25/100 — dropped)
- [low] pkg/tui/components/mermaid/render_sequence.go:115 — Self-message label may overflow canvas width silently (confidence: negligible 20/100 — dropped)
- [low] pkg/tui/components/mermaid/render_graph.go:14 — append(roots, order...) may mutate roots backing array if it has spare capacity (confidence: negligible 20/100 — dropped)
| func writeMermaidCanvas(canvas []rune, start int, value string) { | ||
| for _, r := range value { | ||
| if start >= 0 && start < len(canvas) { | ||
| canvas[start] = r | ||
| } | ||
| start++ | ||
| } | ||
| } |
There was a problem hiding this comment.
[high] writeMermaidCanvas skips a column for every wide Unicode character, corrupting diagram layout
The loop advances start by 1 for every rune, but double-width characters (CJK, emoji) occupy 2 terminal columns. After writing a wide rune to canvas[start], the next character is placed at start+1 — the second visual cell of the wide character — causing characters to visually overlap and misalign.
The rest of the file correctly uses runewidth.StringWidth() and runewidth.RuneWidth() for width accounting; this function is the odd one out. The fix is to advance by the rune's display width and, for the two-column case, also blank out the adjacent cell so it is not filled with stale content:
| func writeMermaidCanvas(canvas []rune, start int, value string) { | |
| for _, r := range value { | |
| if start >= 0 && start < len(canvas) { | |
| canvas[start] = r | |
| } | |
| start++ | |
| } | |
| } | |
| func writeMermaidCanvas(canvas []rune, start int, value string) { | |
| for _, r := range value { | |
| if start >= 0 && start < len(canvas) { | |
| canvas[start] = r | |
| } | |
| rw := runewidth.RuneWidth(r) | |
| if rw == 2 && start+1 >= 0 && start+1 < len(canvas) { | |
| canvas[start+1] = ' ' | |
| } | |
| start += rw | |
| } | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
There was a problem hiding this comment.
Fixed in aba4fe7. I changed the canvas from []rune to display-cell-aware []string storage: wide runes reserve continuation cells that serialize to zero additional width, and zero-width combining runes are appended to the preceding cell. This avoids the extra terminal column that a literal space in the continuation cell would introduce. Added regression coverage for CJK and combining characters in render_primitives_test.go.
There was a problem hiding this comment.
Excellent fix! The cell-aware storage approach is much cleaner than my suggestion. Storing wide-rune continuation cells as empty strings and appending zero-width combining characters to the preceding cell correctly handles both categories of non-trivial Unicode without introducing phantom width. The test coverage for both CJK and combining characters confirms it works as designed.
Thank you for the thorough implementation!
Signed-off-by: Djordje Lukic <[email protected]>
c20c17b to
aba4fe7
Compare
|
Review follow-up in
Validation passes with |
aheritier
added
area/tui
- Add Mermaid diagram section under Thinking and Tool Details documenting which diagram types are rendered inline and the fallback behavior for unsupported types (PR #3679) - Update the Lean TUI section to list supported slash commands and explicitly note that /model is available (PR #3674) - Update Runtime Model Switching to clarify that /model works in both the full TUI and the lean TUI, while Ctrl+M is full TUI only
4 participants
