Repository navigation
feat: support per-cluster direction (rankdir) for clusters/subgraphs - #511
Conversation
…n\nThis change allows clusters (subgraphs) to specify their own direction (rankdir), enabling correct layout for Mermaid diagrams and other use cases that require mixed directions within a single graph. The layout pipeline now recursively applies layout for clusters with a custom rankdir, and the data model is extended to support this property. All tests pass.
Updated implementation — all tests passing ✅I've pushed a corrected implementation to Bugs fixed in the previous implementationThree bugs were preventing the feature from working:
What the new approach does
Test coverage6 new Jest tests added in
All 312 tests pass (272 pre-existing + 40 new assertions across 6 test cases). |
…ates - Patch dagre-d3-es@7.0.14 via pnpm patch to include applyClusterDirections (runs after main dagre layout) and preserve rankdir on cluster nodes in buildLayoutGraph. This is a forward-port of the same fix in the dagre fork (sjackson0109/dagre@feature/per-cluster-direction, dagrejs/dagre#511). - Fix mermaid-graphlib.js extractor predicate: * Restore the original !externalConnections branch (was accidentally dropped in commit 415af35, causing GLB test regressions where all normal clusters without external connections stopped being extracted) * Keep the new clusterData?.dir branch as the FIRST check, so subgraphs with an explicit direction keyword are ALWAYS given their own sub-graph, even when they have external connections (this is the actual mermaid-js#4648 fix) * Normalize TD→TB in both branches when setting rankdir on clusterGraph (dagre's makeSpaceForEdgeLabels uses 'TB', not 'TD') - Remove premature TD→TB normalization from flowDb.ts addSubGraph — it was changing the parsed dir value before the parser tests could check it, breaking subgraph.spec.js line 325. Direction normalization now happens inside mermaid-graphlib.js when rankdir is set on the cluster sub-graph.
|
Note on pre-existing test failures in the Mermaid test suite When the Three specific failures stand out as worth noting for reviewers:
The remaining 11 failures are in None of the 14 failures are regressions introduced by |
|
Any chance one of you guys can review and/or approve this PR? @rustedgrail |
|
@wandri - Thanks for you testing this; and for the improved demo.html - very useful. I don't see the same issues you are showing:
|
This comment was marked as outdated.
This comment was marked as outdated.
- Fix recursiveClusterLayout: only isolate top-level clusters (skip nested cluster nodes that are children of another cluster), collect all descendants via BFS so grandchildren are also removed from the parent graph - Pre-save parent info before any g.removeNode() calls to avoid stale refs - Add collapse/expand interactivity to demo.html: click any cluster header to collapse (removes children, reflows layout) or expand; direction dropdown hidden when collapsed; state tracked per example+cluster - Rebuild dist with fixed layout code - All 313 tests pass
Straight-line edges between internal nodes of different clusters cut visually through unrelated clusters (e.g. start->lint going through the entire Build Pipeline box, or bundle->unit lines crossing). Fix: for each edge endpoint that lives inside a cluster, use the cluster's own bounding-box as the effective endpoint when the two sides belong to different clusters. Same-cluster edges (e.g. lint->compile inside a BT cluster) still draw between the actual node positions.
…er controls - Remove SVG foreignObject+select from cluster nodes (unreliable across browsers) - Add a 'Clusters:' control row below the main controls bar that lists each cluster in the current example with TB/BT/LR/RL direction buttons - Cluster labels now show direction tag inline: e.g. 'Build Pipeline [BT]' - Default perClusterSimple Build Pipeline cluster to BT so the BT fix is immediately visible without any user interaction required
|
I'm now seeing @wandri |
|
All TB - shows a nice vertical stack: And an overall LR, with cluster specific TBs, looks good: And BT with cluster specific LRs, again looks good: Commit '0e15ce7' Replaced the broken foreignObject dropdowns with HTML cluster controls, defaulted Build Pipeline to BT (which is why you saw BT in your examples @wandri) |
The default was temporarily changed to BT to make the BT bug fix immediately visible during review. The correct default for this example is LR (horizontal pipeline inside a vertical TB graph), which is the intended use-case. BT correctness is verified by the test suite (313/313 pass) and can be tested interactively via the cluster direction buttons.
|
Bravo 👏! Seems good. Just improve the code with this changes from #511 (review) and it will be good for me 👍 @sjackson0109 |
|
@wandri thanks for review #511 (review) Please can I ask you to review my comments and get back to me? |
|
@sjackson0109 It seems that you have done changes on your branch |
9391f74 to
0523ca3
Compare
|
cowboy error :) fpush completed |
0f6dd1c to
0523ca3
Compare
…types - lib/types.ts: add ranksep, nodesep, align to NodeLabel with strict types (per wandri r3218977487 suggestion) - test/per-cluster-direction-test.ts: implement three TODO tests: nested clusters with different rankdirs, cross-cluster edge routing, all rankdir combinations (TB/BT/LR/RL); move imports to top of file - lib/layout.ts: fix nested cluster position restoration - remove incorrect LR/RL coordinate swap in second-pass; dagre coordinate-system already transforms coords correctly, the swap was inverting them for nested clusters
… casts Per wandri review comments r3218926668, r3218932102, r3218936771, r3218942391, r3218947215, r3218950415, r3218952270, r3218958667, r3218966578: - Add ClusterNodeLabel interface extending NodeLabel with optional _dagreClusterSubgraph property to avoid all (node as any) casts - Replace all 6 occurrences of (node as any)._dagreClusterSubgraph with (node as ClusterNodeLabel)._dagreClusterSubgraph - Add null guard (if (!node) return) in clusterChildrenMap.forEach so node.width/height can be assigned without ! assertion - Change valid() param type from any to unknown in positionSelfEdges - Replace (node as any).selfEdges with (node as ExtendedNodeLabel).selfEdges
|
@wandri - I think i've now addressed all your suggestions. Any further thoughts? |
|
@sjackson0109 I see many comments resolved but the code didn't change on this parts. |
…m layout.ts - Remove both Per-Cluster Direction Architecture block comments (r3220026340) - Remove duplicate LayoutContext interface and its surrounding plan artefacts - Remove // Add more settings as needed from LayoutContext (r3220028630) - Remove stale // Entry point for recursive per-cluster layout (to be implemented) header 313/313 tests pass.
|
hopefully this is the last round @wandri |
wandri
left a comment
There was a problem hiding this comment.
Great work, I still have 3 omments
…, clean up parent-dummy-chains
|
@wandri - need a beer after this one :) |
wandri
left a comment
There was a problem hiding this comment.
Good Job @sjackson0109, thanks for your effort.
@rustedgrail, it looks good to me.
|
@rustedgrail - any chance of another review, and if possible a workflow approval? |
|
@wandri or @rustedgrail - Please can I ask for a progress update? |
|
Good for me @sjackson0109. @rustedgrail ? |
|
This has been pushed in the 3.1 version. Thank you to both of you for the help! |
|
This pleases me :) thank you @rustedgrail |








This change allows clusters (subgraphs) to specify their own direction (rankdir), enabling correct layout for Mermaid diagrams and other use cases that require mixed directions within a single graph. The layout pipeline now recursively applies layout for clusters with a custom rankdir, and the data model is extended to support this property. All tests pass.