Common Render pipeline 4: DAGRE LayoutData normalization

[knsv-bot]

Summary

• normalize Dagre graphlib layout results back onto LayoutData
• copy node positions and renderable edge geometry after the Dagre layout core runs
• add focused coverage for normal edges, merged self-loops, and the phase contract

Verification

• pnpm vitest run packages/mermaid/src/rendering-util/layout-algorithms/dagre/index.spec.js
• pnpm exec eslint --quiet --cache --cache-strategy content packages/mermaid/src/rendering-util/layout-algorithms/dagre/index.js packages/mermaid/src/rendering-util/layout-algorithms/dagre/index.spec.js
• pnpm run lint
• git diff --check

[changeset-bot]

🦋 Changeset detected

Latest commit: 9841d26

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

This PR includes changesets to release 1 package

Name	Type
mermaid	Minor

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

[codecov]

Codecov Report

❌ Patch coverage is 0.50505% with 197 lines in your changes missing coverage. Please review.
✅ Project coverage is 2.95%. Comparing base (90a35ca) to head (9841d26).

Files with missing lines	Patch %	Lines
...rc/rendering-util/layout-algorithms/dagre/index.js	0.00%	127 Missing ⚠️
packages/mermaid/src/rendering-util/createGraph.ts	0.00%	40 Missing ⚠️
...c/rendering-util/layout-algorithms/common/index.ts	0.00%	30 Missing ⚠️

Additional details and impacted files

@@                              Coverage Diff                               @@
##           codex/common-render-pipeline-3-dagre-phases   #7811      +/-   ##
==============================================================================
- Coverage                                         2.95%   2.95%   -0.01%     
==============================================================================
  Files                                              651     651              
  Lines                                            69258   69406     +148     
  Branches                                           973     973              
==============================================================================
+ Hits                                              2047    2048       +1     
- Misses                                           67211   67358     +147     

Flag	Coverage Δ
unit	2.95% <0.50%> (-0.01%)	⬇️

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

Files with missing lines	Coverage Δ
packages/mermaid/src/defaultConfig.ts	49.18% <100.00%> (+0.20%)	⬆️
...c/rendering-util/layout-algorithms/common/index.ts	0.43% <0.00%> (-0.04%)	⬇️
packages/mermaid/src/rendering-util/createGraph.ts	1.14% <0.00%> (-0.39%)	⬇️
...rc/rendering-util/layout-algorithms/dagre/index.js	0.00% <0.00%> (ø)

🚀 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]

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

Build	Status	Details	Updated (UTC)
default (Inspect)	👍 Changes approved	155 changed, 1 added	Jun 10, 2026, 2:35 PM

[knsv-bot]

[sisyphus-bot]

Review of Common Render pipeline 4: DAGRE LayoutData normalization (stacked on codex/common-render-pipeline-3-dagre-phases).

Verdict: COMMENT — one important question about normalization completeness, nothing blocking.

What's working well

• 🎉 [praise] normalizeDagreNode mirrors the paint-phase offset rules exactly (clusterNode → y + margin, pure cluster → height + margin, regular → y + margin/2), and works on copies so the graph the paint phase still consumes is untouched. I traced both paths and found no double-application of offsets — including the subtle self-loop case where getSelfLoopPoints subtracts yOffset and the normalizer adds it back, matching the existing paint behavior.
• 🎉 [praise] The tests hit the right cases: plain edge geometry with margin offsets, merged self-loop collapsing back to one logical edge with selfLoop stripped, and extending the phase-contract test to assert LayoutData is populated after the core phase. Focused and readable.
• 🎉 [praise] nodeById as a Map keeps the write-back linear, and DAGRE_NODE_LAYOUT_PROPERTIES as a fixed allowlist (rather than Object.assign) is the right call — it also keeps user-influenced graph keys from leaking onto LayoutData.

Security

XSS sub-agent review: no XSS or injection issues identified. The change is a pure data transformation with no DOM sinks; the property allowlist is hard-coded (no prototype-pollution surface, and the Map lookup is immune to __proto__-keyed ids); no sanitization boundary moves.

Things to address

• 🟡 [important] applyDagreLayoutResult is silently lossy for anything beyond a flat top-level graph (dagre/index.js:458-481). Two facets:

  1. Recursive clusters: adjustClustersAndEdges moves cluster children and intra-cluster edges into the nested node.graph (mermaid-graphlib.js:137,311), so sortNodesByHierarchy(graph) never sees those nodes — their LayoutData entries keep stale/no positions — and data4Layout.edges = getEdgesToRender(graph, ...) drops intra-cluster edges from LayoutData entirely.
  2. Non-merged self-loops: for diagram types where mergeSelfLoops is false (e.g., class diagrams), the three *-cyclic-special-* dummy segments replace the original self-edge in data4Layout.edges, leaking synthetic layout-internal edges into the public structure.

  Nothing consumes the normalized LayoutData yet (paint still reads the graph), so nothing breaks today — but phase 5 presumably will consume it, and these gaps will surface there as missing clusters and phantom edges. It would be great to either handle the recursive case now (walk into node.graph and translate to the parent coordinate space), or — if that's deliberately deferred — add a code comment plus a test documenting the current limitation so the next phase doesn't build on an assumption the data doesn't satisfy.

• 🟢 [nit] runDagreLayoutCore's first parameter is still named _data4Layout (dagre/index.js:730,738) — the underscore signalled "unused", but this PR now passes it to applyDagreLayoutResult. Renaming to data4Layout keeps the convention honest.

• 🟢 [nit] dagre/index.js:40-49 — worth a one-line comment on DAGRE_NODE_LAYOUT_PROPERTIES stating the keys must remain a fixed literal allowlist (never derived from graph data); the safety of applyDagreNodeLayout depends on it, and a future "simplify to Object.assign" refactor would reopen a property-injection surface.

• 💡 [suggestion] Copying live function references (intersect, calcIntersect) onto LayoutData nodes mixes behavior into what downstream consumers (DDLT fixtures, serialization) treat as plain data — and those closures may capture the dagre-graph node rather than the LayoutData node. If the painter is the only consumer, consider keeping them dagre-internal and passing them alongside instead. Fine to defer to the phase where they're actually consumed.

No changeset is correct for an internal refactor with no user-facing change. All CI green including the five e2e shards and Argos.

Solid continuation of the pipeline series — the normalization direction (dagre converging on the same LayoutData contract ELK already uses) is exactly right. 🚀

[knsv-bot]

[sisyphus-bot]

Review follow-up — the findings from the review have been addressed in 8c6f1e5 (merged up the stack into #7812 and #7813):

• 🟡 Lossy normalization: took the document-and-pin option from the review, since Common Render pipeline 5: DAGRE shared paint #7812's shared paint walks the measure graph and doesn't depend on LayoutData completeness for nested content. applyDagreLayoutResult now carries a doc comment spelling out both limitations (recursive cluster children/intra-cluster edges stay with the recursive paint path; unmerged self-loop segments replace the original self-edge), and a new unit test pins the recursive-cluster behavior so phase 5+ can't silently build on assumptions the data doesn't satisfy. The self-loop facet is additionally resolved for er/classDiagram by Common Render pipeline 5: DAGRE shared paint #7812 extending segment merging.
• 🟢 _data4Layout → data4Layout in runDagreLayoutCore.
• 🟢 DAGRE_NODE_LAYOUT_PROPERTIES now documents the fixed-literal-allowlist requirement and why the intersect/calcIntersect function references are copied.

Stack propagation: merged into codex/common-render-pipeline-5-dagre-shared-paint (c46198a) and codex/common-render-pipeline-6-dagre-shared-nodes (6249755), both clean merges with dagre + common specs passing.

[knsv-bot]

[sisyphus-bot]

Re-review of Common Render pipeline 4: DAGRE LayoutData normalization — follow-up to the first review round.

Verdict: APPROVE — all findings from the previous round are resolved; no new findings. (Posted as a comment because the authoring account cannot formally approve its own PR.)

Transparency note: the remediation commit (8c6f1e5) was authored by the same automation pipeline that produced this review, so this is automation verifying automation — a human maintainer should give the delta a quick glance before merge.

Previous findings — all resolved in 8c6f1e5

• 🟡 Lossy normalization → resolved via the document-and-pin option the review offered: applyDagreLayoutResult now carries a doc comment spelling out both limitations (recursive cluster children and intra-cluster edges stay with the recursive paint path; unmerged self-loop segments replace the original self-edge), and a new unit test pins the recursive-cluster behavior (top-level nodes normalized, cluster children untouched, intra-cluster edges absent). This is the right scope given #7812's shared paint walks the measure graph and never depends on LayoutData completeness for nested content — and #7812 separately resolves the self-loop facet for er/classDiagram.
• 🟢 _data4Layout → data4Layout in runDagreLayoutCore — done.
• 🟢 Fixed-literal-allowlist comment on DAGRE_NODE_LAYOUT_PROPERTIES — done, and it also covers the 💡 about the intersect/calcIntersect function references.

Verification

• 🎉 [praise] The pinning test asserts the limitation from three angles (normalized top-level nodes, untouched cluster child, dropped intra-cluster edge) — exactly the guard the next pipeline phase needs.
• 🎉 [praise] Full CI green on the new head: unit tests, all five e2e shards, and Argos reports no visual changes — consistent with a comments/test/rename-only delta.

Security

XSS sub-agent re-checked the delta: no XSS or injection issues identified. The commit contains only comments, a parameter rename, and a unit test with static fixture data; no DOM sinks or sanitization boundary changes.

Nothing further from this side — over to a human for the final merge call. 🚀