Repository navigation
feat(elk): add keepEntryNodeOnTop option to keep a recursive flow's entry node on top - #7828
Conversation
…ntry node on top When a flow loops back on itself (a back-edge to an earlier node), elk.layered's degree-based cycle-breaking has no notion of an entry point and can rank the first-declared node mid-layout, scrambling the reading order. The new opt-in elk.keepEntryNodeOnTop option (default false) detects the entry node of each recursive flow - grouped per parentId, ignoring self-loops, by finding weakly-connected components with no natural source - and pins it to the first layer via elk.layered.layering.layerConstraint = FIRST. Detection is scoped per container, so cycles inside subgraphs are handled independently. Acyclic flows have a natural source, so nothing is nominated and their layout is unchanged. Resolves #7827
🦋 Changeset detectedLatest commit: 04e120a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7828 +/- ##
==========================================
- Coverage 2.82% 2.82% -0.01%
==========================================
Files 669 668 -1
Lines 72527 72602 +75
Branches 990 990
==========================================
+ Hits 2048 2049 +1
- Misses 70479 70553 +74
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 ↗︎
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this one — it's a model PR for an ELK behavior change. The description alone (problem, approach, and especially the "Why not forceNodeModelOrder?" section with empirical verification of the alternatives) makes this a pleasure to review. Let's get it across the finish line.
What's working well
- 🎉 [praise]
findCyclicEntryNodesis a pure, exported, thoroughly documented helper (packages/mermaid-layout-elk/src/render.ts:168) — the "no in-degree-0 node ⇒ cycle" invariant is explained right in the doc comment, and the per-parentIdscoping keeps subgraph cycles local. This is exactly the DOM-decoupled style the repo encourages for layout logic. - 🎉 [praise] Test coverage matches the acceptance criteria in #7827 point for point: acyclic no-op, pure cycle, the reported recursion case with orphan exclusion, per-container scoping, self-loop handling, plus gating on/off through
buildElkGraphFromLayoutData. E2E covers off, on, and subgraph nesting. All 27 unit tests pass locally. - 🎉 [praise] Opt-in with
default: falseand applied as the last step of graph building via a spread that preserves existinglayoutOptions(render.ts:271) — zero behavior change for existing ELK diagrams. - 🎉 [praise] Generated-file hygiene is correct: I ran
types:verify-configandconfig.type.tsis in sync with the schema; thedocs/config/setup/changes are pure regeneration churn.
Things to address
- 🟡 [important] Changeset bump level (
.changeset/elk-keep-entry-node-on-top.md): both packages are bumped aspatch, but this is a new feature (the description is even prefixedfeat:). Repo convention isminorfor features,patchfor fixes — would be great to bump both@mermaid-js/layout-elkandmermaidtominor. - 💡 [suggestion] The heuristic only fires when a cyclic component has no natural source. If a user feeds the loop from outside it (e.g. adds
start --> briefat the same nesting level), the component gains an in-degree-0 node, nothing is nominated, and the option silently does nothing — even though ELK may still rankbriefmid-graph. That's a reasonable design choice, but worth a sentence in theconfig.schema.yamldescription (e.g. "only applies when the cyclic flow has no node without incoming edges") so users aren't surprised. Same for cycles that span container boundaries, which the per-parentIdscoping intentionally ignores. - 🟢 [nit] In
findCyclicEntryNodes, the edge loop runs once per container group (O(groups × edges)). Completely fine at realistic diagram sizes and gated behind the flag — just noting it in case container counts ever grow; a single pre-pass bucketing edges by container would make it linear.
Security
A dedicated XSS/injection review found no issues: the option value is only used as a boolean gate, and node ids flow only into Map/Set structures and a constant-valued layoutOptions entry consumed by elkjs — nothing reaches a DOM sink.
One out-of-scope observation for a future follow-up (pre-existing, not this PR): nodeDb is a plain object keyed by user-controlled node ids (render.ts:496), so an id like __proto__ hits the prototype setter. Harmless on the path added here, but Object.create(null) or a Map would be sturdier if that file is touched again.
Verdict: comment — one important item (changeset bump) and a couple of optional polish ideas; nothing blocking. 👍
- Bump @mermaid-js/layout-elk and mermaid as minor (new feature, not a fix) - Document in the keepEntryNodeOnTop description that pinning only applies when the cyclic component has no natural source, and that detection is scoped per container (cycles crossing a subgraph boundary are not detected) - Regenerate config.type.ts and docs
263f8cb to
04e120a
Compare
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Re-reviewed at 04e120a5b — both items from the previous round are resolved. Nice turnaround; this is ready from my side.
What's working well
- 🎉 [praise] The changeset now bumps both
@mermaid-js/layout-elkandmermaidasminor, matching the repo convention for features. - 🎉 [praise] The
keepEntryNodeOnTopdescription inconfig.schema.yaml:176now spells out both heuristic boundaries — loops fed from outside gain a natural source and aren't pinned, and detection is scoped per container so cross-subgraph cycles aren't detected. Users won't be surprised when the option is a no-op in those cases.config.type.tsand the docs were correctly regenerated (verified withtypes:verify-config— in sync). - 🎉 [praise] Core implementation unchanged and still solid: pure, well-tested
findCyclicEntryNodes, opt-indefault: false, all 27 ELK unit tests passing, e2e coverage for off/on/subgraph-nested cases.
Remaining (non-blocking)
- 🟢 [nit] Carried over from last round, take it or leave it: the edge loop in
findCyclicEntryNodesruns once per container group (O(groups × edges)). Negligible at realistic sizes and gated behind the flag — a pre-pass bucketing edges by container would make it linear if container counts ever grow.
Security
A fresh XSS/injection pass on the delta confirms it is documentation-only (changeset bump + description text in schema/types/docs) — no new code paths, DOM sinks, or sanitization changes. The prior full review's conclusion still holds at this head: the option value is only a boolean gate, and node ids never reach a DOM sink.
The out-of-scope follow-up from last round still stands for a future PR: pre-existing nodeDb plain-object keying (render.ts:496) is __proto__-susceptible; Object.create(null) or a Map would be sturdier.
Verdict: approve ✅
Upstream release: https://github.com/mermaid-js/mermaid/releases/tag/mermaid%4011.17.0 Release notes: ### Minor Changes - [#7842](mermaid-js/mermaid#7842) [`3670b4e`](mermaid-js/mermaid@3670b4e) Thanks [@filipsajdak](https://github.com/filipsajdak)! - feat(c4): render C4 elements through the unified shape system, using the new person shape - [#7812](mermaid-js/mermaid#7812) [`cdfc0ea`](mermaid-js/mermaid@cdfc0ea) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(class): route `classDiagram` to the unified (v2) renderer by default Set `class: { defaultRenderer: 'dagre-d3' }` in the config to restore the legacy renderer. - [#7785](mermaid-js/mermaid#7785) [`c45cde9`](mermaid-js/mermaid@c45cde9) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(flowchart): add collapsible flowchart subgraphs via `subgraphId@{ view: collapsed }` - [#7828](mermaid-js/mermaid#7828) [`8eb3afc`](mermaid-js/mermaid@8eb3afc) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(elk): add `elk.keepEntryNodeOnTop` config option to keep a recursive flow's entry node on top - [#7803](mermaid-js/mermaid#7803) [`74e44eb`](mermaid-js/mermaid@74e44eb) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(elk): add `elk.nodePlacementAlignment` config option - [#7792](mermaid-js/mermaid#7792) [`ea55b31`](mermaid-js/mermaid@ea55b31) Thanks [@RodrigojndSantos](https://github.com/RodrigojndSantos)! - feat(er): add subgraph support to ER diagrams. - [#7970](mermaid-js/mermaid#7970) [`a2c0fb6`](mermaid-js/mermaid@a2c0fb6) Thanks [@filipsajdak](https://github.com/filipsajdak)! - feat(flowchart): add `folder`, `bucket`, `console` (terminal window) and `browser` shapes - [#7842](mermaid-js/mermaid#7842) [`ae3e115`](mermaid-js/mermaid@ae3e115) Thanks [@filipsajdak](https://github.com/filipsajdak)! - feat(flowchart): add `person` shape (circular head above a rounded body), usable in flowcharts via `A@{ shape: person }` - [#7724](mermaid-js/mermaid#7724) [`0fd7a9f`](mermaid-js/mermaid@0fd7a9f) Thanks [@xdumaine](https://github.com/xdumaine)! - feat(xyChart): add legends for named line and bar series ### Patch Changes - [#7847](mermaid-js/mermaid#7847) [`215fe89`](mermaid-js/mermaid@215fe89) Thanks [@filipsajdak](https://github.com/filipsajdak)! - fix(c4): named attributes such as `$tags`, `$link` and `$sprite` are no longer clobbered to undefined when they arrive in an earlier positional slot of Person/System/Container/Component/Boundary/Rel statements. - [#7871](mermaid-js/mermaid#7871) [`8d874c4`](mermaid-js/mermaid@8d874c4) Thanks [@knsv-bot](https://github.com/knsv-bot)! - fix(flowchart): stop dagre layout from spamming `warn`-level logs on every node/edge/cluster - [#8071](mermaid-js/mermaid#8071) [`b3d1f63`](mermaid-js/mermaid@b3d1f63) Thanks [@pbrolin47](https://github.com/pbrolin47)! - fix(block): sibling blocks overlapping in block diagrams when one has a label wider than 200px - [#7870](mermaid-js/mermaid#7870) [`71b8843`](mermaid-js/mermaid@71b8843) Thanks [@knsv-bot](https://github.com/knsv-bot)! - fix: a `RangeError: Invalid array length` crash when rendering certain edges. - [#7924](mermaid-js/mermaid#7924) [`9cbef5d`](mermaid-js/mermaid@9cbef5d) Thanks [@nightt5879](https://github.com/nightt5879)! - fix(treeView): icons disappearing after strict security sanitization. - [#7850](mermaid-js/mermaid#7850) [`a34cbf0`](mermaid-js/mermaid@a34cbf0) Thanks [@aloisklink](https://github.com/aloisklink)! - fix(block): allow classdefs to update text color - [#7937](mermaid-js/mermaid#7937) [`f9cbe1e`](mermaid-js/mermaid@f9cbe1e) Thanks [@filipsajdak](https://github.com/filipsajdak)! - fix(dagre): let a diagram's own nodeSpacing/rankSpacing take effect in the unified dagre layout - [#8005](mermaid-js/mermaid#8005) [`90eeece`](mermaid-js/mermaid@90eeece) Thanks [@pbrolin47](https://github.com/pbrolin47)! - fix(flowchart): reverts the behavior change from #7672 (fix/4648-directions), since arrows between subgraphs are broken - [#7951](mermaid-js/mermaid#7951) [`afa2f80`](mermaid-js/mermaid@afa2f80) Thanks [@aloisklink](https://github.com/aloisklink)! - perf: use `fastdom` to batch DOM measurements (up to 25% speedup) - Updated dependencies \[[`e848423`](mermaid-js/mermaid@e848423)]: - @mermaid-js/parser@1.2.1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: give me a 98K <240642031+inhuman-0@users.noreply.github.com>
Summary
Adds an opt-in ELK config option,
elk.keepEntryNodeOnTop(defaultfalse), that keeps the entry node of a recursive flow at the top of the layered layout. This generalizes a fix prototyped internally so that any ELK-laid-out diagram can use it via configuration, rather than being tied to one diagram type.Resolves #7827.
Problem
elk.layeredmust break cycles before it can assign layers, and its degree-based cycle-breaking has no notion of an "entry point". When a flow loops back on itself (a back-edge to an earlier node), ELK reverses an arbitrary loop edge and can rank the first-declared node in the middle of the layout — scrambling the reading order and hiding where the flow starts.Approach
findCyclicEntryNodes(pure, unit-tested) groups nodes byparentId(so a cycle inside a subgraph is scoped to that subgraph), looks only at internal edges, ignores self-loops, and finds the weakly-connected components. A component with no in-degree-0 node necessarily contains a cycle, so its first node in declaration order is nominated as the entry.elk.layered.layering.layerConstraint = FIRST, pinning it to the first layer of its container's layered layout.config.schema.yaml, regeneratedconfig.type.ts, and added to the hand-maintainedelkblock indefaultConfig.ts(nested object defaults aren't auto-extracted from the schema, so the key must be listed there for a frontmatter value to survive config merge).Default off → acyclic flows and all existing ELK diagrams (e.g.
flowchart-elk) are unchanged.Why not
forceNodeModelOrder?forceNodeModelOrder(and the model-order cycle-breaking strategies) only reorder nodes within a layer during crossing minimization — they never reassign which layer a node lands in. Verified empirically on the recursion case in bothINCLUDE_CHILDREN(flattened) andSEPARATE_CHILDREN(independent sub-layout) subgraphs: the layout came out identical to baseline, with the entry still stranded mid-graph. OnlylayerConstraint = FIRSTlifts the entry to the top.Tests
render.spec.ts):findCyclicEntryNodes— acyclic no-op, pure cycle, the reported recursion case (incl. orphan exclusion), per-parentIdscoping, self-loop handling; plus application gating on/off throughbuildElkGraphFromLayoutData.flowchart-elk.spec.js): option off, option on, and a recursion nested inside a subgraph.Verification
Confirmed live via agent-browser: with the option on, the entry node
briefmoves to the top of its (sub)graph and the flow reads top-to-bottom; with it off, the layout is unchanged. Subgraph scoping verified — the entry is pinned to the top of its own subgraph, not the whole diagram (start → research → donestays intact withbriefat the top ofresearch).