Skip to content

fix(flowchart): render nested subgraph with numeric id without dagre rank crash#7771

Open
devareddy05 wants to merge 3 commits into
mermaid-js:developfrom
devareddy05:fix/7609-numeric-subgraph-id
Open

fix(flowchart): render nested subgraph with numeric id without dagre rank crash#7771
devareddy05 wants to merge 3 commits into
mermaid-js:developfrom
devareddy05:fix/7609-numeric-subgraph-id

Conversation

@devareddy05
Copy link
Copy Markdown

📑 Summary

Fixes TypeError: Cannot set properties of undefined (setting 'rank') when rendering a flowchart whose nested subgraph uses a numeric id, e.g. subgraph 1 ["inner"].

Resolves #7609

Root Cause

dagre-d3-es's graphlib stores nodes in a plain JS object, so graph.nodes() enumerates integer-like keys ("1") before others ("outer"). mermaid-graphlib.js's extractor iterated that list directly, so the inner cluster 1 was extracted before its parent outer — flattening cluster sub into the cluster-1 subgraph as a regular leaf with internal still parented under it. The nested recursion then bailed out early because 1 looked like a leaf in cg_outer, leaving a compound node referenced by an edge. dagre's longest-path DFS hit a node with no label and crashed on label.rank = …. Non-numeric ids preserve insertion order, so the issue only surfaces with numeric subgraph ids.

Changes

  • Sort extractor's iteration by depth so outermost clusters are processed before inner ones, making cluster extraction independent of graph.nodes() ordering.
  • Applied in both renderer paths: rendering-util/layout-algorithms/dagre/mermaid-graphlib.js (flowchart-v2) and dagre-wrapper/mermaid-graphlib.js (legacy, still used by class and block diagrams).

Testing

  • New regression test in mermaid-graphlib.spec.js exercising the issue diagram structure; verifies that the inner cluster becomes a proper clusterNode with its own subgraph after adjustClustersAndEdges.
  • Full packages/mermaid test suite passes (1202 tests).

📋 Tasks

  • 📖 have read the contribution guidelines
  • 💻 have added necessary unit/e2e tests.
  • 📓 documentation — N/A (internal bugfix).
  • 🦋 changeset added (fix-7609-numeric-subgraph-id.md, patch).

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 21, 2026

🦋 Changeset detected

Latest commit: 73a1b1f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mermaid Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link
Copy Markdown

netlify Bot commented May 21, 2026

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 73a1b1f
🔍 Latest deploy log https://app.netlify.com/projects/mermaid-js/deploys/6a102835463ad200079d14dc
😎 Deploy Preview https://deploy-preview-7771--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added the Type: Bug / Error Something isn't working or is incorrect label May 21, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 21, 2026

Open in StackBlitz

@mermaid-js/examples

npm i https://pkg.pr.new/@mermaid-js/examples@7771

mermaid

npm i https://pkg.pr.new/mermaid@7771

@mermaid-js/layout-elk

npm i https://pkg.pr.new/@mermaid-js/layout-elk@7771

@mermaid-js/layout-tidy-tree

npm i https://pkg.pr.new/@mermaid-js/layout-tidy-tree@7771

@mermaid-js/mermaid-zenuml

npm i https://pkg.pr.new/@mermaid-js/mermaid-zenuml@7771

@mermaid-js/parser

npm i https://pkg.pr.new/@mermaid-js/parser@7771

@mermaid-js/tiny

npm i https://pkg.pr.new/@mermaid-js/tiny@7771

commit: 73a1b1f

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 3.26%. Comparing base (46e8044) to head (73a1b1f).

Files with missing lines Patch % Lines
...ages/mermaid/src/dagre-wrapper/mermaid-graphlib.js 0.00% 13 Missing ⚠️
...g-util/layout-algorithms/dagre/mermaid-graphlib.js 0.00% 13 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #7771      +/-   ##
==========================================
- Coverage     3.26%   3.26%   -0.01%     
==========================================
  Files          599     600       +1     
  Lines        60839   60876      +37     
  Branches       917     917              
==========================================
  Hits          1986    1986              
- Misses       58853   58890      +37     
Flag Coverage Δ
unit 3.26% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ages/mermaid/src/dagre-wrapper/mermaid-graphlib.js 0.00% <0.00%> (ø)
...g-util/layout-algorithms/dagre/mermaid-graphlib.js 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@argos-ci
Copy link
Copy Markdown

argos-ci Bot commented May 21, 2026

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 6 changed, 1 added May 22, 2026, 10:07 AM

@devareddy05
Copy link
Copy Markdown
Author

Codecov Report

❌ Patch coverage is 0% with 26 lines in your changes missing coverage. Please review. ✅ Project coverage is 3.26%. Comparing base (46e8044) to head (db60d58).

Files with missing lines Patch % Lines
...ages/mermaid/src/dagre-wrapper/mermaid-graphlib.js 0.00% 13 Missing ⚠️
...g-util/layout-algorithms/dagre/mermaid-graphlib.js 0.00% 13 Missing ⚠️
Additional details and impacted files
Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #7771      +/-   ##
==========================================
- Coverage     3.26%   3.26%   -0.01%     
==========================================
  Files          599     599              
  Lines        60839   60865      +26     
  Branches       917     917              
==========================================
  Hits          1986    1986              
- Misses       58853   58879      +26     

Flag Coverage Δ
unit 3.26% <0.00%> (-0.01%) ⬇️
Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ages/mermaid/src/dagre-wrapper/mermaid-graphlib.js 0.00% <0.00%> (ø)
...g-util/layout-algorithms/dagre/mermaid-graphlib.js 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:

The new code path is exercised by the regression test added in both spec files for the same diagram structure described in #7609. Running pnpm exec vitest run --coverage packages/mermaid/src/dagre-wrapper/mermaid-graphlib.spec.js packages/mermaid/src/rendering-util/layout-algorithms/dagre/mermaid-graphlib.spec.js locally shows:

  • dagre-wrapper/mermaid-graphlib.js: 86.46% statements
  • rendering-util/layout-algorithms/dagre/mermaid-graphlib.js: 86.89% statements

Every line of the added nodeDepth helper and the sort call is hit at least 36 times during the test. The 0% in the bot's report seems to come from the carry-forward flags (overall project coverage is 3.26%), so the uploaded run is not including these specs.

@pbrolin47
Copy link
Copy Markdown
Collaborator

Hi @devareddy05,
Thanks for this PR. Some initial comments to adress:

[sisyphos-bot]


What's working well

🎉 Root cause and fix are correct — graphlib backs its node store with a plain JS object, so integer-like keys ("1") are enumerated before other string keys in V8's property
iteration order. Sorting by depth (ascending) before iteration guarantees outer clusters are processed first, making extractor order-independent regardless of node ID type. This
is exactly the right fix.

🎉 The fix is applied to both copies — the deprecated dagre-wrapper path and the active rendering-util path both get the sort. Good consistency.

🎉 Test coverage for the regression — both spec files get a direct regression test that reconstructs the exact topology from issue #7609 (outer > 1 > sub > internal with a
sub→external edge) and asserts on the extracted graph structure. The GLB-DIR1–4 tests in the rendering-util spec additionally cover the new explicitDir and
cross-boundary-rebinding branches with clear, commented diagrams.


Things to address

🟡 [important] No Cypress E2E test for the crash scenario.

The changes touch rendering-util/layout-algorithms/dagre/mermaid-graphlib.js, which is shared code that affects every diagram using dagre layout. Per project conventions, shared
rendering-util changes require E2E visual coverage — unit tests confirm the graph manipulation logic, but a Cypress test would confirm that the actual rendered SVG output for
the fixed scenario is correct (nodes present, edges routed, no visual artifacts).

It would be great to add a test case to cypress/integration/rendering/flowchart/flowchart.spec.js with the exact diagram from issue #7609:

graph LR
subgraph outer
subgraph 1 ["inner"]
external
subgraph sub
internal
end
sub-->external
end
end

🟡 [important] The rendering-util version goes well beyond the stated PR scope without any description.

The PR title and body describe only the numeric ID ordering fix (#7609). But rendering-util/layout-algorithms/dagre/mermaid-graphlib.js also adds:

  • Cross-boundary edge rebinding in copy() — prevents orphan nodes when an edge has one endpoint outside the cluster
  • An explicitDir branch in extractor — always creates a subgraph for clusters with clusterData.explicitDir = true (related to issue Direction in subgraphs inconsistent #4648)
  • Two new helper functions isNodeInExtractableCluster and findSafeAnchorNode — re-anchors cluster edge endpoints when the anchor node is inside an extractable sibling subgraph

These are meaningful correctness improvements but they're invisible in the PR description. A couple of sentences covering each would help reviewers and make future bisects much
easier if a regression appears. Worth updating the PR body to describe these additions.


Minor observations

🟢 [nit] nodeDepth is recomputed on every sort comparison, so it's called O(n log n) times with each call walking O(d) up the parent chain. For typical diagrams with shallow
nesting this is negligible, but a one-line Map-based memo would be cleaner:

const depthCache = new Map();
const nodeDepth = (v) => {
if (depthCache.has(v)) return depthCache.get(v);
let d = 0, cur = graph.parent(v);
while (cur != null) { d++; cur = graph.parent(cur); }
depthCache.set(v, d);
return d;
};

Not a blocker — just a readability and micro-performance nicety.

🟢 [nit] isNodeInExtractableCluster and findSafeAnchorNode are new private helpers without isolated unit tests. The GLB-DIR tests exercise them indirectly, but explicit tests
for their edge cases (e.g., findSafeAnchorNode returning null when all children are in the excluded cluster) would make the coverage explicit.


Security

No XSS or injection issues. The new code operates exclusively on abstract graphlib graph structures — no DOM sinks, no string interpolation into SVG attributes, no
user-controlled text reaching any output surface. The label clearing in the dagre-wrapper version (startLabelLeft = '' etc.) strictly reduces the data flowing to the rendering
pipeline for split cyclic edges, which is a defensive improvement.


@devareddy05
Copy link
Copy Markdown
Author

Thanks for the review @pbrolin47.

On the two points raised by sisyphos-bot:

Scope is narrower than the bot suggests. git diff origin/develop..HEAD --stat on this branch shows 5 files / 99 insertions:

.changeset/fix-7609-numeric-subgraph-id.md         |  5 ++++
packages/mermaid/src/dagre-wrapper/mermaid-graphlib.js                            | 13 +++++++++
packages/mermaid/src/dagre-wrapper/mermaid-graphlib.spec.js                       | 34 ++++++++++++
packages/mermaid/src/rendering-util/layout-algorithms/dagre/mermaid-graphlib.js   | 13 +++++++++
packages/mermaid/src/rendering-util/layout-algorithms/dagre/mermaid-graphlib.spec.js | 34 ++++++++++++

The 13-line change in each mermaid-graphlib.js is purely the depth sort inside extractor. The cross-boundary rebinding in copy(), the explicitDir branch, and the isNodeInExtractableCluster / findSafeAnchorNode helpers all already exist on develop (introduced in earlier commits like a3130c785, a819c8bf7, fc6ce97dd), so they are not in scope here and there is nothing for me to describe about them.

No Cypress E2E test for the crash scenario. Fair point, added in 73a1b1fdc. New test #7609: should render nested subgraph with numeric id without crashing in cypress/integration/rendering/flowchart/flowchart.spec.js snapshots the exact diagram from the issue. Before the fix this scenario threw TypeError: Cannot set properties of undefined (setting 'rank') and produced no SVG at all, so the snapshot serves as a render-vs-crash regression guard.

On the nits: happy to add a Map-based memo to nodeDepth if you would like, but for the depth-of-nesting we see in practice the cost is negligible and a single line of code feels easier to audit than a closure-scoped cache. Let me know if you prefer the memoized version.

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

Labels

Type: Bug / Error Something isn't working or is incorrect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Internal error on subgraph with numeric identifier

2 participants