Mermaid flowchart subgraphs#3716
Conversation
rumpl
commented
Jul 17, 2026
Signed-off-by: Djordje Lukic <[email protected]>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two bugs introduced by this PR's new subgraph parsing and rendering code:
- addMermaidSubgraphNode (parser_flowchart.go:77): nodes are deduplicated across all subgraphs globally, causing nodes in nested subgraphs to be silently omitted from the child's node list.
- findMermaidTextBounds (render_subgraph.go:92): plain substring matching can place subgraph bounding boxes at incorrect positions when one node label is a substring of another.
| return id, label, id != "" | ||
| } | ||
|
|
||
| func addMermaidSubgraphNode(doc *Document, stack []int, id string) { |
There was a problem hiding this comment.
[medium] addMermaidSubgraphNode: global deduplication prevents nodes from being assigned to nested subgraphs
The early-return loop checks all subgraphs for the node ID before deciding whether to add it to the active subgraph. When a node is first encountered in a parent subgraph scope, it is recorded there. Later, if the same node appears inside a nested subgraph (e.g. as part of an edge A --> B within a child subgraph), slices.Contains finds it in the parent and returns early — so the node is never added to the child subgraph's Nodes list.
Impact: The child subgraph's rendered bounding box is computed from its (incomplete) node list, potentially omitting nodes that are visually inside it. The box will be drawn at the wrong position or sized incorrectly.
Fix: Only deduplicate within the target subgraph (the one at the top of the stack), not across all subgraphs:
| func addMermaidSubgraphNode(doc *Document, stack []int, id string) { | |
| func addMermaidSubgraphNode(doc *Document, stack []int, id string) { | |
| if len(stack) == 0 { | |
| return | |
| } | |
| index := stack[len(stack)-1] | |
| doc.Subgraphs[index].Nodes = appendUniqueMermaid(doc.Subgraphs[index].Nodes, id) | |
| } |
| Confidence | Score |
|---|---|
| 🟢 strong | 97/100 |
| return result | ||
| } | ||
|
|
||
| func findMermaidTextBounds(lines []string, text string) mermaidBounds { |
There was a problem hiding this comment.
[medium] findMermaidTextBounds: plain substring match can locate the wrong node when one label is a prefix/substring of another
strings.Cut(line, text) finds the first occurrence of text anywhere in the rendered diagram line. If node label "A" is searched and the line first contains a node rendered as [ AB ] or ╭─ AB ─╮, the search matches "A" inside "AB" before reaching the actual [ A ] node.
Trigger: Any flowchart with two node labels where one is a substring of the other — for example "Deploy" and "DeployProd", or "A" and "AB". The computed x offset will be wrong, placing the subgraph bounding box at the substring's position rather than the intended node's position.
Impact: The subgraph box is drawn at an incorrect horizontal position, potentially enclosing the wrong nodes or missing the correct node entirely.
Fix: Match against the full node rendering (e.g., the label surrounded by box characters like [ label ] or the full box fragment), or scan for a word/cell-boundary match rather than a raw substring.
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two correctness bugs were introduced in the subgraph parser and one in the renderer, with a high-confidence fix available for the directive fallback issue.
| for _, subgraph := range doc.Subgraphs { | ||
| if slices.Contains(subgraph.Nodes, id) { | ||
| return | ||
| } | ||
| } | ||
| index := stack[len(stack)-1] | ||
| doc.Subgraphs[index].Nodes = appendUniqueMermaid(doc.Subgraphs[index].Nodes, id) |
There was a problem hiding this comment.
[medium] addMermaidSubgraphNode prevents a node from appearing in more than one subgraph
The guard scans all doc.Subgraphs before adding the node to the current subgraph. Once a node ID appears in any subgraph (even an inner one), every subsequent call for the same ID returns early — the node is never recorded in the outer (parent) subgraph's Nodes list.
mermaidSubgraphBounds merges the Nodes lists of each subgraph with the already-computed bounds of child subgraphs. A node that belongs both to an inner subgraph's Nodes list and to the outer scope will be covered for the outer subgraph only if the inner subgraph's bounds are already recorded in known. For cases where the outer subgraph has no recorded child bounds (e.g. the inner subgraph itself had no valid bounds), the outer subgraph's own Nodes list is the only source — and the node will be missing from it, making the outer box too small or absent.
Fix: check only the current subgraph (indexed by stack[len(stack)-1]) instead of all subgraphs. The existing appendUniqueMermaid call already prevents double-entry within the same subgraph.
| for _, subgraph := range doc.Subgraphs { | |
| if slices.Contains(subgraph.Nodes, id) { | |
| return | |
| } | |
| } | |
| index := stack[len(stack)-1] | |
| doc.Subgraphs[index].Nodes = appendUniqueMermaid(doc.Subgraphs[index].Nodes, id) | |
| index := stack[len(stack)-1] | |
| doc.Subgraphs[index].Nodes = appendUniqueMermaid(doc.Subgraphs[index].Nodes, id) |
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
| } | ||
| switch strings.ToLower(fields[0]) { | ||
| case "direction", "subgraph", "end", "classdef", "class", "style", "click", "linkstyle": | ||
| case "direction", "classdef", "class", "style", "click", "linkstyle": |
There was a problem hiding this comment.
[medium] Subgraph line falls through to node parser when parseMermaidSubgraph returns false
subgraph was removed from the mermaidFlowchartDirective switch (the old code treated it as a directive to skip). The new code relies entirely on parseMermaidSubgraph to catch every line that starts with subgraph. However, parseMermaidSubgraph returns false when cleanMermaidLabel produces an empty string for the definition — for example:
subgraphwith no text after the keyword (len(fields) < 2)subgraph ""(all quotes, stripped bycleanMermaidLabel)subgraph :::(all punctuation, stripped to empty)
When it returns false, the end check is skipped (the line starts with subgraph), mermaidFlowchartDirective returns false (no entry for subgraph), and cursor.readNode() is called with the full statement. readNode happily parses subgraph as a node identifier, injecting a spurious subgraph node into the diagram.
Fix: add "subgraph" back to the mermaidFlowchartDirective switch as a fallback so that any subgraph line that parseMermaidSubgraph cannot handle is silently skipped rather than misinterpreted as a node.
| case "direction", "classdef", "class", "style", "click", "linkstyle": | |
| case "direction", "classdef", "class", "style", "click", "linkstyle", "subgraph": |
| Confidence | Score |
|---|---|
| 🟢 strong | 92/100 |
| return mermaidBounds{} | ||
| } | ||
| for y, line := range lines { | ||
| before, _, found := strings.Cut(line, text) |
There was a problem hiding this comment.
[medium] findMermaidTextBounds uses plain substring match — a short label can match inside a longer label
strings.Cut(line, text) is a prefix-aware substring search with no word-boundary guarantee. If two nodes have labels where one is a substring of the other (e.g. "API" and "API Gateway"), searching for "API" will match at the start of the "API Gateway" cell if that node's row appears first in the rendered canvas — returning coordinates for the wrong node.
The rendered diagram encodes each node's label inside box-drawing characters ([ ], ( ), [[ ]], etc.), so a typical rendered line looks like │ [ API ] ─── [ API Gateway ] │. Searching for the raw label API will match the first API occurrence, which could be inside API Gateway's box rather than the node named API.
This causes the computed subgraph bounding box to be anchored at the wrong cell, rendering the subgraph border around the wrong area.
Consider searching for the rendered label text bounded by box-drawing delimiters (e.g., match [ API ] rather than API) so that exact-node matching is guaranteed. Alternatively, search for the widest matching occurrence rather than the first.
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |