fix(flowchart): respect per-subgraph direction keyword in Dagre layout#7672
fix(flowchart): respect per-subgraph direction keyword in Dagre layout#7672sjackson0109 wants to merge 22 commits into
Conversation
…ions by creating virtual subgraph (fixes mermaid-js#4648)
… connections (issue mermaid-js#4648)
🦋 Changeset detectedLatest commit: 8eccac1 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 |
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
Type: Bug / Error
|
#4648 should be fixed |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
|
The unit tests are failing with errors such as: and and These failures are expected, for now. This PR depends on a new release of Dagre JS, specifically the changes in this PR I submitted here: dagrejs/dagre#511 That update provides the layout engine with the additional attributes required for these tests to pass. |
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for taking on #4648 @sjackson0109 — this is one of the longest-standing layout bugs in the project, and the angle you've gone for (gate on explicit direction rather than on !externalConnections) is genuinely more principled than the existing logic. That said, before this can land I think it needs more guardrails and a couple of fixes, given how broadly mermaid-graphlib.js reaches.
What's promising
- 🎉 The core conceptual change in
mermaid-graphlib.js:327-330— switching the cluster-extraction predicate from!externalConnectionstoclusterData?.dir— is the right shape of fix. The old check was the actual root cause of the "external edge into a subgraph node leaks direction" behavior reported in #4648 and #6785. Treating "user explicitly saiddirection X" as the trigger for a separate sub-layout is much more intentional. - 🎉 The
TD→TBnormalisation inflowDb.ts:703-706(insideaddSubGraph) plugs a real gap —setDirection()already normalises this for the top-level direction (flowDb.ts:451-453), but subgraph directions previously didn't, which is exactly the "direction TDdoesn't work, you have to usedirection TB" complaint in the issue thread. Mirroring the existing normalisation here is the right call.
Things to address
🔴 [blocking] No automated tests for a shared-layout change
packages/mermaid/src/rendering-util/layout-algorithms/dagre/mermaid-graphlib.js is used by every Dagre-based diagram type (flowchart, class, state, etc.). CLAUDE.md is explicit on this:
rendering-util/is shared by all diagram types. Changes here can regress any diagram. Always run full e2e tests (pnpm e2e) after modifying shared rendering code — unit tests alone are insufficient.
The existing Cypress suite already has extensive subgraph-direction coverage in cypress/integration/rendering/flowchart/flowchart-v2.spec.js (around lines 696, 814, 1014+ — multiple test cases mixing top-level and subgraph directions, nested subgraphs, etc.). Two things needed:
- Run those existing tests against the change and post the Argos diff — that's the only way to know whether the new condition regresses existing flowchart layouts. Subgraphs without an explicit
directionkeyword no longer get a separate cluster graph under the new logic (old code: separate graph if!externalConnections; new code: separate graph only ifclusterData?.dir), so default-direction subgraphs may shift visually. - Add a new test in that same file specifically targeting #4648's two patterns: (a) external edge into a subgraph internal node where the subgraph has its own
direction, (b) sibling subgraphs with different directions connected via subgraph-to-subgraph edges. A side-by-side that locks in the fixed behavior would also serve as the long-term regression guard the issue thread has been asking for.
🔴 [blocking] The manual test in test/4648.html won't parse
The mermaid source in the <div class="mermaid"> block has no newlines between top-level statements:
flowchart TD subgraph Group1 direction TB A1 --> A2 A2 --> A3 end subgraph Group2 direction LR
B1 --> B2 B2 --> B3 end subgraph Group3 direction LR C1 --> C2 C2 --> C3 end %% External
connections between subgraphs A3 --- B1 B3 --- C1 %% Wrapper workaround subgraph Wrapper
direction LR subgraph Inner D1 --> D2 D2 --> D3 end end
Mermaid's flowchart parser uses newlines/semicolons as statement separators — flowchart TD subgraph Group1 direction TB A1 --> A2 on a single line is a parse error, not a rendered diagram. Looks like the file was reflowed by an editor at some point. Each statement (flowchart TD, subgraph X, direction Y, each edge, each end) needs its own line. There's also a B1 B3 token sequence in A3 --- B1 B3 --- C1 that won't parse even with newlines — looks like that should be two separate edges (A3 --- B1 and B3 --- C1).
Worth opening this file in the dev server (pnpm dev) and confirming it actually renders before relying on it as the visual proof.
🟡 [important] Test file location
test/ isn't an established directory in this repo — manual test pages live in cypress/platform/ (e.g., cypress/platform/subgraph.html already exists for similar purposes), and automated visual tests live in cypress/integration/rendering/. The new test/4648.html won't be picked up by any tooling and will likely confuse future contributors. Suggest:
- Move the manual page to
cypress/platform/4648.html(or merge it into the existingsubgraph.html), and - Add the automated coverage as a Cypress test in
flowchart-v2.spec.js.
🟡 [important] Missing changeset
This is a user-visible layout-behavior change, so it needs a .changeset/*.md entry. pnpm changeset → patch for mermaid with a fix: description should do it.
🟡 [important] PR description is contradictory
The title says "fix(flowchart): improve subgraph direction handling" and the body says "This PR does not attempt to fix Dagre, but provides a clear test and documentation of the issue." The diff clearly does attempt a fix — both mermaid-graphlib.js and flowDb.ts change runtime layout behavior. Worth rewriting the description to reflect what's actually being changed (which condition, which code path, and what the expected behavior shift is) — that helps future maintainers reading git blame.
Behavioral question worth flagging in the description
Worth being explicit about how the new logic handles subgraphs without an explicit direction keyword:
- Old behavior:
!externalConnections && hasChildren→ create a separate cluster graph, defaulting to the opposite of the parent'srankdir. So a subgraph in aTBflowchart (with nodirection) would lay out internally asLRby default. - New behavior: only
clusterData?.dir && hasChildrentriggers a separate cluster graph. Default-direction subgraphs now inherit the parent'srankdir.
That's a defensible direction (no pun intended), but it's a visible default-behavior change that could shift many existing flowcharts. The full e2e Argos run is the only way to know how broad the impact is — and if the change is intentional, the changeset should call it out so users know what's coming.
Security
No XSS or injection concerns — the layout-algorithm changes don't touch DOM sinks or user-facing string interpolation, and the new manual HTML page is a standard mermaid-init page (not loaded by the published library).
Summary
| Severity | Count |
|---|---|
| 🔴 blocking | 2 |
| 🟡 important | 3 |
| 🟢 nit | 0 |
| 💡 suggestion | 0 |
| 🎉 praise | 2 |
The conceptual fix is the right shape — please don't take the volume of feedback as discouragement. The combination of "shared layout code with broad blast radius" + "no automated tests" + "broken manual repro" is what's pushing this to changes-requested rather than just comments. With the Cypress coverage in place and an Argos diff to look at, this could land cleanly. Happy to help if any of the above is unclear.
sjackson0109
changed the title
sjackson0109
changed the title
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
…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.
…tor branches Both Branch 1 (explicit direction) and Branch 2 (no external connections) were using copy(child, graph, clusterGraph, child) per-child loops, which is a no-op for leaf nodes and incorrectly handles nested sub-clusters. The original code used copy(node, graph, clusterGraph, node) — a single call with the cluster node as the root — which correctly: - Extracts all direct leaf children into the clusterGraph - Recursively handles nested sub-clusters with proper parent relationships - Removes extracted nodes from the outer graph This fixes all 11 mermaid-graphlib unit tests (GLB4-GLB77) that were regressed by the earlier change, and enables test 2050 (issue mermaid-js#4648) to render correctly in e2e tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7672 +/- ##
==========================================
- Coverage 3.29% 3.28% -0.01%
==========================================
Files 561 561
Lines 58576 58642 +66
Branches 873 873
==========================================
Hits 1928 1928
- Misses 56648 56714 +66
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…actor (Branch 1 coverage) Adds 3 new unit tests (GLB-DIR1, GLB-DIR2, GLB-DIR3) covering the new clusterData?.dir branch introduced by fix mermaid-js#4648: - GLB-DIR1: cluster with explicit dir + external connections becomes clusterNode - GLB-DIR2: cluster with explicit dir + no external connections becomes clusterNode - GLB-DIR3: sibling clusters with different explicit dirs and inter-cluster edge These tests exercise the ~47 lines of Branch 1 that were flagged by Codecov as having 0% patch coverage.
…an nodes
When copy() extracts a cluster whose leaf nodes have edges that cross the
cluster boundary, edgeInCluster's OR-logic was copying those edges into the
subgraph and auto-creating the external endpoint as a zero-width orphan node.
recursiveRender then called insertNode(nodes, undefined) and crashed, leaving
window.rendered=false and failing Cypress tests for:
issue-4648: sibling subgraphs with different directions and external connections
issue-4648: nested subgraph with external connection
2050: handling of different rendering direction in subgraphs
Fix: check whether BOTH endpoints are strictly inside rootId's cluster.
- Strictly internal edge: copy to newGraph as before.
- Cross-boundary edge: rebind in the outer graph as rootId to externalNode,
so the cluster connection is preserved after removeNode() drops the leaf.
Every compound state node was receiving dir:'TB' (the DEFAULT_NESTED_DOC_DIR default) even when the user wrote no 'direction' keyword inside that state. That truthy dir value caused mermaid-graphlib Branch 1 (clusterData?.dir && hasChildren) to trigger for ALL compound states, including those with external connections, changing their layout relative to the original !externalConnections-based extraction rule. Fix: only set newNode.dir when parsedItem.doc actually contains a 'dir' statement (i.e. the user explicitly wrote 'direction X' inside the state body), leaving dir as undefined otherwise. This restores correct rendering for: State-diagram-v2-A-compound-state-should-be-able-to-link-to-itself State-diagram-v2-handle-transition-from-one-state-in-a-composite-state-to-a-composite-state Compound states WITH explicit direction still go through Branch 1.
…nch 1 The Branch 1 predicate clusterData?.dir was accidentally catching ALL compound state nodes, because state diagram dataFetcher always sets dir:'TB' (DEFAULT_NESTED_DOC_DIR) even when the user wrote no 'direction' keyword. This caused 36 state diagram layouts to regress. A previous attempted fix set dir:undefined for non-explicit state nodes, which broke 36 OTHER diagrams because Branch 2 also relies on clusterData.dir for its sub-layout direction arithmetic. Proper fix: add an explicitDir:boolean field, true only when the user explicitly wrote a 'direction X' keyword in the subgraph/state body. Branch 1 now checks explicitDir instead of dir, so: flowchart subgraph with direction LR : explicitDir:true -> Branch 1 flowchart subgraph without direction: explicitDir:false -> Branch 2/else state compound state with direction LR: explicitDir:true -> Branch 1 state compound state without direction: explicitDir:false -> Branch 2/else Branch 2's direction override (clusterData?.dir) is untouched and keeps working exactly as before for both flowchart and state diagram.
|
The ci items of concern now are the argos-ci tests.
|
|
Any chance one of you can review and consider approving? |
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Round 2 — and first, thank you for the substantial follow-through on round 1, plus apologies for how long this has been sitting in the queue. You asked for a review on May 2 and the queue has been slow; that's on us, not on you. The work in this revision is genuinely good. 🎉 [praise]
A few things still need attention before this can land — most importantly two test/cleanup issues, plus one architectural call that probably needs a maintainer rather than me. Round 1 → round 2 already cleared the previous blockers; one more pass should put this over the line.
What's working well
- 🎉 The
explicitDirrefinement is better than what I asked for in round 1. Preserving the original!externalConnectionspath for default-direction subgraphs (so they keep inheriting the parent rankdir) and only diverging when the user wrote adirectionkeyword is the right call — it cleanly addresses the round-1 concern about "default-direction subgraphs may shift visually" without giving up the fix. - 🎉 Real test coverage added. Two Cypress visual cases in
flowchart-v2.spec.js:351-92covering #4648's two patterns (sibling subgraphs with different directions, and nested with external edges), plus the manual page at the conventional location (cypress/platform/regression/issue-4648.html). That's exactly what round 1 asked for. - 🎉 State-diagram parity. Threading
explicitDirthroughdataFetcher.ts:274-276andstateDb.ts:151so compound states behave consistently is more thorough than #4648 strictly required, and avoids leaving state in an inconsistent place. Nice touch. - 🎉 Changeset is well-written and explicitly calls out the behavior shift — readers tracing the changelog will understand both the fix and the intent.
Things to address
🔴 [blocking] The new unit tests don't actually exercise Branch 1
This one's subtle and worth verifying yourself. I ran the new spec tests with verbose output:
$ pnpm vitest run packages/mermaid/src/rendering-util/layout-algorithms/dagre/mermaid-graphlib.spec.js -t "GLB-DIR"
...
WARN: Cluster without external connections, without a parent and with children C1 0 ← GLB-DIR1
WARN: Cluster without external connections, without a parent and with children C1 0 ← GLB-DIR2
WARN: Cluster without external connections, without a parent and with children B1 0 ← GLB-DIR3
WARN: Cluster without external connections, without a parent and with children B2 0
✓ all 3 pass
All three tests are landing in the old Branch 2 (!externalConnections && hasChildren), not the new Branch 1 (explicitDir && hasChildren). The reason: the test setup uses g.setNode('C1', { data: 3, dir: 'LR' }) — dir is set but explicitDir is not, and clusterData is just the node object (mermaid-graphlib.js:219: clusterDb.set(id, { id: ..., clusterData: graph.node(id) })). So clusterData?.explicitDir is undefined, Branch 1 is skipped, and the cluster gets extracted by the unchanged Branch 2 logic.
In other words, deleting Branch 1 entirely would not break these tests — they'd still pass. The new code path is unit-untested.
Fix: set explicitDir: true on each cluster node in the test setup, e.g.:
g.setNode('C1', { data: 3, dir: 'LR', explicitDir: true });Worth confirming with a quick TDD sanity check: temporarily comment out the new Branch 1, re-run the tests, and they should fail. If they still pass, the test still isn't exercising the new branch.
(The Cypress visual tests do exercise Branch 1 transitively — they go through flowDb.ts which sets explicitDir: !!subGraph.dir. But for a shared rendering-util change of this scope, unit-level coverage of the new branch is the right safety net.)
🔴 [blocking] Old test/4648.html is still in the diff
Round 1 flagged this one as 🔴 because the mermaid source was on a single line and wouldn't parse. You did the right thing creating the properly-formatted page at cypress/platform/regression/issue-4648.html, but the broken file is still being added at test/4648.html (line 674 of the diff). The test/ directory isn't an established location, the source still doesn't parse, and keeping it around will confuse future contributors who find it via grep. Please delete it.
🟡 [important] The 143-line dagre-d3-es@7.0.14.patch warrants a maintainer call
This is the part I'd most like a human to weigh in on, so I'm flagging it but not insisting on a particular outcome:
- Magnitude: The existing
patches/roughjs.patchis 4 lines of type-export tweak. This new patch is 143 lines of new layout logic (applyClusterDirections,getAllDescendants, sub-graph construction, position translation). That's a different kind of decision. - Two parallel cluster-layout systems:
mermaid-graphlib.js's extractor already builds a per-clusterclusterGraphwith its ownrankdir(your new Branch 1 lines 314-356 do exactly that). TheapplyClusterDirectionsfunction in the patch does conceptually similar work after the main layout pass. It's not obvious to me whether both layers are necessary, or whether one alone would suffice — what specifically breaks if the dagre-d3-es patch is dropped? - Maintenance: every upstream
dagre-d3-esversion bump now requires patch reconciliation. Worth either upstreaming (PR to dagre-d3-es) or having a clear "remove once X" target.
If the patch is genuinely needed, that's fine — but the architectural call ("are we comfortable taking on a 143-line third-party patch with parallel logic to existing code?") is bigger than I can resolve. @ashishjain0512 / @knsv / @aloisklink — worth your eyes.
🟡 [important] Cross-boundary edge rebinding in copy() lacks a unit test
mermaid-graphlib.js:282-307 introduces new logic that rebinds edges crossing the cluster boundary so they terminate at rootId in the outer graph (preventing the orphan-node crash you describe in the comment). The reasoning is sound, but it's a behavior change for any cluster hitting the copy() path. The Cypress tests cover it transitively via #4648 patterns, but a direct unit test that sets up a cross-boundary edge and asserts:
- the inner edge does NOT appear in
clusterGraph - the outer graph has a rebound edge from
rootIdto the external node
…would lock in the behavior and protect against future regressions. Could be added to the new GLB-DIR* describe block.
🟡 [important] Changeset description points to the wrong file
The changeset says:
Also normalises
direction TDtodirection TBinsideaddSubGraph…
But the diff puts the TD→TB normalization in mermaid-graphlib.js:322-325 (new Branch 1) and :372-373 (existing Branch 2), not in flowDb.ts addSubGraph (whose only change is the explicitDir flag). Round 1's review praised an addSubGraph normalization, so I'm guessing it lived there in an earlier revision and got moved. Either:
- (a) move the normalization back into
addSubGraph(single source of truth, matches the existingsetDirectionnormalization atflowDb.ts:451-453), or - (b) update the changeset to point to mermaid-graphlib.
(a) is probably cleaner — normalize once at parse-time rather than at every layout-extractor branch.
🟡 [important] Argos visual diff is the real safety net
This is a CI/maintainer thing rather than something you need to act on, but worth saying: with mermaid-graphlib.js + the dagre-d3-es patch both touched, the e2e Argos run on this PR is the most informative thing for landing it. If Argos comes back clean across flowchart/class/ER/state, the architectural concerns above mostly evaporate. If Argos shows shifts, those become the conversation.
Security
No XSS or injection concerns — the changes are entirely layout-positioning logic. No DOM sinks, no user-string interpolation, no DOMPurify config touched. The new manual HTML page is a standard mermaid-init demo (not loaded by the published library).
Summary
| Severity | Count |
|---|---|
| 🔴 blocking | 2 |
| 🟡 important | 4 |
| 🟢 nit | 0 |
| 💡 suggestion | 0 |
| 🎉 praise | 4 |
Verdict: REQUEST_CHANGES.
This is round 2 of automated review. Per project policy, if a third round is still requesting changes, the protocol is to escalate to a human reviewer rather than keep iterating with me — so the architectural call on the dagre-d3-es patch may end up landing there regardless. If you want to split this PR, dropping the patch and shipping just the explicitDir + tests + cleanup as a smaller fix would also be a sensible path forward — the patch can come as a follow-up once it's discussed.
Either way: thanks again for the patience and the depth of work here. Once the test-bug is fixed, test/4648.html is removed, and there's a path on the dagre-d3-es patch, this should land. 🙏
- Add explicitDir: true to GLB-DIR1/2/3 cluster node setups so tests actually exercise Branch 1 (clusterData.explicitDir check) rather than the unchanged Branch 2 path. Deleting Branch 1 would have left the previous tests green -- that loophole is now closed. - Add GLB-DIR4 unit test: cross-boundary edge from internal node to external node must NOT be copied into the clusterGraph; the outer graph must instead carry a rebound edge from the cluster root (C2) to the external node. Directly locks in the copy() rebinding logic. - Delete test/4648.html (broken: diagram source on one line, non-standard location). The correctly-formatted manual repro is already at cypress/platform/regression/issue-4648.html. - Move TD->TB normalisation from mermaid-graphlib.js (two sites) into flowDb.ts addSubGraph -- parse-time single source of truth, mirrors setDirection() and matches the changeset description. - Update changeset to reference clusterData?.explicitDir (actual check) rather than clusterData?.dir.
|
@ashishjain0512 any chance of another review? |
|
Hi @sjackson0109, [sisyphus-bot] What's Working Well 🎉 [praise] The explicitDir boolean is an elegant semantic solution. The root of the state diagram regressions in earlier commits was that checking clusterData?.dir was ambiguous — state compound 🎉 [praise] The cross-boundary edge rebinding in copy() is well-diagnosed and well-reasoned. The OR-logic bug in edgeInCluster causing orphan crash was subtle — tracing it through the crash 🎉 [praise] The state diagram fix in dataFetcher.ts (parsedItem.doc.some((s) => s.stmt === 'dir')) is precisely targeted. Rather than a broad change, it pinpoints exactly which state diagram nodes 🎉 [praise] The GLB-DIR4 test (cross-boundary edge not copied into clusterGraph) is especially valuable — it directly tests the copy() bug that was triggering the Cypress crash. Things to Address 🟡 [important] patches/dagre-d3-es@7.0.14.patch — The 143-line vendor patch adds applyClusterDirections to dagre's layout pipeline, but this function never fires in Mermaid's rendering pipeline. applyClusterDirections triggers when an individual graphlib node has node.rankdir set. In Mermaid's pipeline, rankdir is only ever set on graph metadata (graph.setGraph({ rankdir: dir })), never on The actual fix is Branch 1 in mermaid-graphlib.js — it correctly extracts clusters with explicit direction into their own subgraph regardless of external connections, and the recursive extractor + The patch as-is:
It would be great to verify the fix still works without the patch (remove patches/dagre-d3-es@7.0.14.patch, the package.json entry, and pnpm-lock.yaml changes, then rerun the Cypress tests). If the 🟡 [important] flowDb.ts:1226 — explicitDir: !!subGraph.dir has a false-positive case with flowchart.inheritDir: true. When inheritDir is enabled, addSubGraph() (lines 703–708) sets dir to the dir = dir ?? (flowchartConfig.inheritDir If inheritDir: true and dir is undefined (no explicit keyword), dir gets the parent direction. Then explicitDir: !!subGraph.dir becomes true, triggering Branch 1 for all subgraphs — including A more robust fix would track the flag before the inheritDir override: let dir = result.dir; Security No XSS or injection issues identified. The dir/explicitDir values are bounded by JISON grammar literals (TB, LR, RL, BT, TD) and flow only through dagre's numeric layout calculation — never into SVG Tests Good coverage:
One note: snapshot baselines for the new Cypress tests will be generated on first CI run. This is the expected workflow. |
…inheritDir Agent-Logs-Url: https://github.com/sjackson0109/mermaid/sessions/6e0b59e7-9b3a-4532-bdc3-7d7893b2c0da Co-authored-by: sjackson0109 <38080190+sjackson0109@users.noreply.github.com>
…e-consumption point Agent-Logs-Url: https://github.com/sjackson0109/mermaid/sessions/ed8547c3-2e55-4088-bbec-113a7976d0aa Co-authored-by: sjackson0109 <38080190+sjackson0109@users.noreply.github.com>
Agent-Logs-Url: https://github.com/sjackson0109/mermaid/sessions/ed8547c3-2e55-4088-bbec-113a7976d0aa Co-authored-by: sjackson0109 <38080190+sjackson0109@users.noreply.github.com>
Agent-Logs-Url: https://github.com/sjackson0109/mermaid/sessions/367892ea-7828-42ac-a114-e4e3119212ef Co-authored-by: sjackson0109 <38080190+sjackson0109@users.noreply.github.com>
…-handling fix(flowchart): preserve raw subgraph direction value; normalize TD→TB only for dagre
CI status —
|
| Failing test file | Failures | Root cause |
|---|---|---|
packages/parser/tests/treeView.test.ts |
19 | incomplete treeView parser feature (merged in #7527) |
packages/mermaid/src/diagrams/treeView/parser.spec.ts |
27 | same |
packages/mermaid/src/diagrams/cynefin/cynefin.integration.spec.ts |
15 | @mermaid-js/parser missing DomainBlock/Transition/Cynefin exports (merged in #7535) |
packages/mermaid/src/diagrams/wardley/wardleyParser.spec.ts |
7 | pre-existing |
packages/mermaid/src/diagrams/architecture/svgDraw.spec.ts |
1 | canvas unavailable in jsdom (pre-existing) |
packages/mermaid/src/mermaidAPI.spec.ts |
1 | same canvas issue |
| 2 others | 2 | canvas issue |
Verified locally against upstream/develop: both branches produce 8 failed | 129 passed, 72 individual test failures — identical counts, identical test names.
This PR introduces zero new test failures. All flowchart and graphlib tests pass:
mermaid-graphlib.spec.js→ 19/19 passed ✅ (including 4 new GLB-DIR tests)- All flowchart diagram tests → 971 passed, 3 skipped ✅
|
I could rerun the unit test, and then it worked. GitHub had a network glitch. |
|
Thanks for tackling this one, @sjackson0109 — #4648 has been open since 2023 and the diagnosis here is exactly right. The cluster-extraction predicate using !externalConnections was the wrong signal; pivoting to explicitDir cleanly separates the fix from the legacy default-behavior path. Really nice work, and the writeup in the PR description, (especially the behavior-change matrix) made this a much faster read. |
Summary
This PR fixes a long-standing bug (#4648, #6785) where subgraphs with an explicit
directionkeyword were silently ignored when those subgraphs had external connections.What changed
packages/mermaid/src/rendering-util/layout-algorithms/dagre/mermaid-graphlib.jsThe cluster-extraction predicate was changed from:
to:
The old check (
!externalConnections) meant that any subgraph with an edge to/from a node outside its boundary would never get its own cluster sub-layout — so the user'sdirectionkeyword was silently discarded. The new check uses theexplicitDirflag (set only when the user wrote an explicitdirection Xkeyword) as the trigger, which is the correct and intentional signal from the user.packages/mermaid/src/diagrams/flowchart/flowDb.tsaddSubGraphnow captureshasExplicitDir = rawDir !== undefinedbefore any inherited-direction fallback, ensuring the flag istrueonly when the user wrote an explicitdirectionkeyword.getDatamapsexplicitDir: subGraph.hasExplicitDirand normalisesdirection TD → TB, mirroring the existing normalisation insetDirectionfor the top-level graph.packages/mermaid/src/diagrams/flowchart/types.tsAdded
hasExplicitDir: booleanto theFlowSubGraphinterface to carry the parse-time flag through to layout.packages/mermaid/src/rendering-util/types.tsAdded
explicitDir?: booleanto theBaseNodeinterface so the flag is visible tomermaid-graphlib.js.Behavior change
direction, no external edgesdirection, has external edgesrankdir❌directionrankdirrankdir(no separate cluster)Tests
Two new regression tests added to
cypress/integration/rendering/flowchart/flowchart-v2.spec.js:Manual visual test page added at
cypress/platform/regression/issue-4648.htmlUnit tests in
mermaid-graphlib.spec.js: all 19 tests pass ✅ including 4 newGLB-DIRtests that specifically exercise theexplicitDir→ separate-cluster code path.Fixes #4648
See also #6785