Skip to content

Mermaid flowchart subgraphs#3716

Merged
rumpl merged 1 commit into
docker:mainfrom
rumpl:feat/mermaid-subgraphs
Jul 17, 2026
Merged

Mermaid flowchart subgraphs#3716
rumpl merged 1 commit into
docker:mainfrom
rumpl:feat/mermaid-subgraphs

Conversation

@rumpl

@rumpl rumpl commented Jul 17, 2026

Copy link
Copy Markdown
Member
Screenshot 2026-07-17 at 12 30 45

@rumpl
rumpl requested a review from a team as a code owner July 17, 2026 10:30
@rumpl
rumpl enabled auto-merge July 17, 2026 10:30
@aheritier aheritier added area/tui For features/issues/fixes related to the TUI kind/feat PR adds a new feature (maps to feat:). Use on PRs only. labels Jul 17, 2026
@rumpl
rumpl merged commit 236f1ea into docker:main Jul 17, 2026
10 of 11 checks passed

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assessment: 🟡 NEEDS ATTENTION

Two bugs introduced by this PR's new subgraph parsing and rendering code:

  1. 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.
  2. 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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:

Suggested change
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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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 docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +81 to +87
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
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":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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:

  • subgraph with no text after the keyword (len(fields) < 2)
  • subgraph "" (all quotes, stripped by cleanMermaidLabel)
  • 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.

Suggested change
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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

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

Labels

area/tui For features/issues/fixes related to the TUI kind/feat PR adds a new feature (maps to feat:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants