fix(flowchart): apply ELK mergeEdges in subgraphs#7703
Conversation
Propagate the ELK mergeEdges option to subgraph layout options so nested flowchart edges are merged consistently with root-level edges. Add a rendering regression test for the approved issue case.
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: a4e803a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes ELK flowchart rendering so the configured mergeEdges option is also applied to subgraphs, matching root-level behavior and addressing the reported subgraph rendering inconsistency from #7659.
Changes:
- Propagates
elk.layered.mergeEdgesinto subgraph layout options in the ELK renderer. - Preserves the existing explicit-direction subgraph branch and hierarchy handling behavior.
- Adds a Cypress image regression test covering merged edges inside subgraphs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/mermaid-layout-elk/src/render.ts |
Adds mergeEdges to default subgraph ELK layout options so nested layouts honor the global config. |
cypress/integration/rendering/flowchart/flowchart-elk.spec.js |
Adds a regression snapshot test for merged edges within subgraphs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7703 +/- ##
==========================================
- Coverage 3.33% 3.29% -0.05%
==========================================
Files 542 560 +18
Lines 56881 58566 +1685
Branches 839 873 +34
==========================================
+ Hits 1899 1928 +29
- Misses 54982 56638 +1656
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for picking this up — a clean, surgical fix with a reproducing test that mirrors the exact case from #7659. Really appreciate the contribution.
What's working well
- 🎉 [praise] The fix is well-scoped: one line in
packages/mermaid-layout-elk/src/render.ts:815propagatingelk.layered.mergeEdgesto the default subgraph branch, exactly mirroring the option already set on the root graph (render.ts:722) and on the explicit-direction subgraph branch (render.ts:822). Leaving theSEPARATE_CHILDREN/ hierarchy behavior alone is the right call — it keeps the blast radius small. - 🎉 [praise] The new Cypress test in
cypress/integration/rendering/flowchart/flowchart-elk.spec.jsreproduces the issue's diagram verbatim, which makes the regression coverage immediately recognizable from the bug report. The4a-elknumbering slots in nicely next to the existing4-elk. - 🎉 [praise] Validation in the PR description (prettier / eslint / tsc / cypress) is thorough — that's exactly the level of pre-flight detail that makes review fast.
Things to address
- 🟡 [important] No changeset for this user-facing fix. Most fixes in
.changeset/ship with a one-line entry (see e.g..changeset/fix-style-unknown-node-warning.mdfor the format). Could you runpnpm changeset, choosemermaidwith apatchbump, and use afix:prefix? Something along the lines offix: apply ELK mergeEdges option to subgraph layoutwould do it.
Suggestions
- 💡 [suggestion] Just an observation, not a request: when
config.elk?.mergeEdgesis unset, this assigns'elk.layered.mergeEdges': undefined. That matches the existing pattern on the root graph at line 722, so consistency wins — but if you ever want to clean up both sites in a follow-up, conditionally spreading the option only when defined would read a touch more cleanly. Definitely out of scope here.
Security
No XSS or injection concerns — the change adds a single boolean-ish layout option consumed by ELK; no DOM sinks, no DOMPurify-relevant code paths.
Once the changeset lands this looks good to merge from my end. Thanks again for the careful fix and the reproduction-faithful test! 🙌
|
Thanks for this the update! It was resolved by this PR #7712 at the same time |
Summary
Fixes #7659.
This propagates the configured ELK
mergeEdgesoption to subgraph layout options. Previously, Mermaid setelk.layered.mergeEdgeson the root ELK graph, but subgraphs only received that option when they declared their own direction. As a result,mergeEdges: trueworked for root-level flowchart edges but not for equivalent edges inside subgraphs.Why this approach
elk.layered.mergeEdgesoption to all subgraphs.SEPARATE_CHILDRENbehavior intact to avoid changing unrelated hierarchy handling.Validation
pnpm exec prettier --check packages/mermaid-layout-elk/src/render.ts cypress/integration/rendering/flowchart/flowchart-elk.spec.jspnpm exec eslint --quiet packages/mermaid-layout-elk/src/render.ts cypress/integration/rendering/flowchart/flowchart-elk.spec.jspnpm --filter @mermaid-js/layout-elk exec tsc --noEmitpnpm run load:env -- start-server-and-test dev http://localhost:9000/ "cypress run --spec cypress/integration/rendering/flowchart/flowchart-elk.spec.js --browser electron"