Common Render pipeline 5: DAGRE shared paint

[knsv-bot]

Summary

• route Dagre through the shared common paint path
• pass Dagre clusterDb into common paint for cluster edge clipping
• position pre-rendered Dagre cluster nodes instead of inserting duplicate clusters
• keep Dagre recursive subgraph paint only for measurement internals
• switch the default class renderer to the unified (v2) renderer — class.defaultRenderer now defaults to dagre-wrapper, so plain classDiagram documents render through the shared dagre layout/paint pipeline instead of the legacy renderer (changeset included; restore the old behavior with class: { defaultRenderer: 'dagre-d3' })
• extend the merged self-loop rendering to er and classDiagram types (the flowchart-facing part of Flowchart ugly self linking nodes #6049 landed earlier in this series)

Part of #6049 — the issue should be closed by the integration PR once the whole stack reaches develop.

Verification

• pnpm vitest run packages/mermaid/src/rendering-util/layout-algorithms/dagre/index.spec.js packages/mermaid/src/rendering-util/layout-algorithms/common/index.spec.ts
• pnpm run docs:verify
• pnpm run lint
• git diff --check
• Argos visual diffs reviewed and approved (13 changed snapshots, including the class diagrams now rendering via the unified renderer)

Visual regression coverage for self-loops

• Class self-referential multiplicity edge: cypress/integration/rendering/class/classDiagram-v3.spec.js ("self-referential class diagram with multiplicity labels") — uses plain classDiagram, so it exercises the new default unified path; a hand-drawn variant was added in classDiagram-handDrawn-v3.spec.js
• Recursive ER relationship: cypress/integration/rendering/er/erDiagram-unified.spec.js (default, ELK, and handDrawn variants) and erDiagram.spec.js

[changeset-bot]

🦋 Changeset detected

Latest commit: 89f2ebf

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.81967% with 121 lines in your changes missing coverage. Please review.
✅ Project coverage is 2.95%. Comparing base (8c6f1e5) to head (89f2ebf).

Files with missing lines	Patch %	Lines
...rc/rendering-util/layout-algorithms/dagre/index.js	0.00%	51 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-4-dagre-layoutdata   #7812      +/-   ##
==================================================================================
- Coverage                                             2.95%   2.95%   -0.01%     
==================================================================================
  Files                                                  652     652              
  Lines                                                69348   69417      +69     
  Branches                                               973     973              
==================================================================================
+ Hits                                                  2047    2048       +1     
- Misses                                               67301   67369      +68     

Flag	Coverage Δ
unit	2.95% <0.81%> (-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	196 changed, 1 added	Jun 10, 2026, 2:16 PM

[knsv-bot]

[sisyphus-bot]

⚠️ Intended verdict: REQUEST_CHANGES (one 🔴 blocking item below). Posting as a COMMENT because GitHub won't let this account request changes on its own PR and the bot review token is currently unavailable — please treat the blocking item as gating.

Thanks for this — routing Dagre through the shared common-paint path is a genuinely nice step in the pipeline series, and the abstraction here is clean. I read the diff at head (c7308a3) against the base (codex/common-render-pipeline-4-dagre-layoutdata), traced the new paint flow end-to-end, and ran an XSS pass. One thing needs sorting before this is ready to flow toward develop, plus a couple of coverage items. Details below.

Note: this targets codex/common-render-pipeline-4-..., not develop, so some of the gates below (changeset, final visual sign-off) may legitimately be planned for the integration PR at the bottom of the stack. If that's the case, just point me at it and I'll happily drop the blocker.

What's working well

• 🎉 The paintOptions hooks are a tidy abstraction. Letting Dagre drive the shared paintLayoutData via getNodes / getEdgeNode / skipNode / isCluster / clusterDb (dagre/index.js:764-776) keeps dagre-specific knowledge out of common/index.ts while still reusing the shared edge/label/cluster machinery. Much better than special-casing dagre inside the common code.
• 🎉 Behavior preservation is careful. Keeping paintDagreLayoutCore solely for the recursive subgraph measurement path, and the deliberate choice to leave class.padding undefined so the v2 renderer keeps its own node sizing (config.spec.ts — "without forcing class padding"), both show real attention to not perturbing existing output. I confirmed positionRenderedEdgeLabel in common/index.ts is functionally equivalent to the old positionEdgeLabel (the { flowchart: ... } arg is fine since getSubGraphTitleMargins only reads flowchart.subGraphTitleMargin).
• 🎉 Strong unit coverage. The new specs are excellent — class self-loop multiplicity preservation in getEdgesToRender, the ER recursive self-loop, "subgraph through shared paint", reverse-order nested subgraphs, and the hand-drawn nested-namespace cases. The getEdgesToRender merge test in particular nails the regression-prone bit.

Things to address

🔴 [blocking] The class default-renderer flip changes default output and needs a changeset + explicit visual sign-off

defaultConfig.ts:60 adds class: { defaultRenderer: 'dagre-wrapper', ... }. Via classDetector-V2.ts, this routes plain classDiagram text (without -v2) to the v2 unified renderer instead of the legacy class renderer. I confirmed this is a new change in this PR: both develop and the base branch leave class.defaultRenderer unset (→ legacy renderer), and this PR is what updates mermaidAPI.spec.ts from expectedType: 'class' → 'classDiagram'.

That's a default-rendering-output change for every existing classDiagram document (GitHub/GitLab/Obsidian render these server-side), which per our conventions is treated as potentially breaking. It's also not mentioned anywhere in the PR summary — the four bullets are all about the paint pipeline, so a maintainer skimming the description would miss the most consumer-visible change in the diff. Could we:

1. Add a changeset (pnpm changeset) describing the class-default switch — somewhere in the stack if not here.
2. Call it out in the PR description so it's not buried.
3. Confirm the class Cypress baselines are intentionally re-pointed (cypress/integration/rendering/class/classDiagram.spec.js etc.) and that Argos diffs on those have been eyeballed — this is the only reliable way to verify the new default looks right.

I believe this is intentional (the commit "Fix class renderer default for docs runtime" and the pipeline goal of getting class diagrams onto the shared dagre path both point that way), so this isn't "revert it" — it's "let's make the gate explicit before it lands."

🟡 [important] Shared rendering-util + new self-loop geometry needs visual-regression coverage

shouldMergeSelfLoopSegments now includes er and classDiagram (dagre/index.js:35-40), so self-referential ER/class edges render through the compact merged-loop path with the new label merge (dagre/index.js:387-399). The unit tests assert the merged path exists and that terminal labels '1'/'0..1' are present, but not their geometry. Since this is shared rendering-util/ code and the whole point (re #6049) is that these loops look good, it'd be great to add imgSnapshotTest() cases in cypress/integration/rendering/ for (a) a class diagram with a self-referential multiplicity edge and (b) a recursive ER relationship, so Argos locks the visual output. Per CLAUDE.md, shared-rendering changes really do want full e2e rather than JSDOM assertions alone.

🟡 [important] "Resolves #6049" looks like it over-claims for this PR

#6049 is specifically about flowchart self-loops, and flowchart/flowchart-v2/stateDiagram were already in shouldMergeSelfLoopSegments on the base branch — so the flowchart fix landed earlier in the series. This PR extends the merge to er/classDiagram. If #6049 is meant to be closed by the stack, consider moving Resolves #6049 to the integration PR and describing this one as "extend the self-loop merge to ER and class diagrams", so the issue isn't auto-closed before the flowchart-facing change reaches develop.

Nits

• 🟢 [nit] EMPTY_CLUSTER_DB (common/index.ts:39) is a shared module-level Map used as the default clusterDb. It's safe today because insertEdge only reads it (.get(...)), but a shared mutable singleton is a latent footgun if a future consumer ever writes to the passed clusterDb. A frozen map or a fresh new Map() per call would be cheaper to reason about.
• 🟢 [nit] Worth a one-line comment next to the class block in defaultConfig.ts explaining why padding is intentionally omitted (to avoid forcing v2 node sizing) — otherwise a well-meaning future contributor may "complete" it to match the schema default of 5 and silently change class node sizes.

Security

Ran a dedicated XSS/injection pass over the changed source. No XSS or injection issues identified. The self-loop labelEdge merge feeds the same insertEdgeLabel → createText/createLabel sanitizing sink as the prior code, labelStyle is still re-derived through the CSS-property whitelist in styles2String, and the terminal DOMPurify.sanitize() in mermaidAPI.ts is untouched. The refactor changes which sanitized string/coordinate is used, not whether sanitization runs.

---

Really solid work overall — the architecture is the right shape and the unit tests are thorough. Once the class-default switch has a changeset + a visual confirmation it's good to go. Happy to re-review quickly. 🙏

_{Severity tally: 🔴 1 · 🟡 2 · 🟢 2 · 🎉 3 — reviewed by reading the diff at head c7308a3; did not run the suite locally (working tree is on an unrelated branch).}

[knsv-bot]

Thanks for the thorough review! Addressed in cdfc0ea and via a PR-description update:

🔴 Class default-renderer flip

• Added a changeset (.changeset/class-default-unified-renderer.md, minor) describing the switch and how to restore the legacy renderer (class: { defaultRenderer: 'dagre-d3' }).
• The PR description now calls the change out explicitly instead of burying it.
• The Argos build for this PR ("13 changed — changes approved") covers the class baselines now rendering through the unified path; those diffs were eyeballed as part of that approval.

🟡 Visual coverage for the new self-loop geometry
Both requested cases already exist as imgSnapshotTest specs and run through the new path on this branch:

• Class self-referential multiplicity edge: classDiagram-v3.spec.js ("self-referential class diagram with multiplicity labels", SelfReferential "1" --> "0..1" SelfReferential) — plain classDiagram text, so with the new default it exercises the unified renderer + merged self-loop.
• Recursive ER relationship: erDiagram-unified.spec.js (default, ELK, and handDrawn variants) plus erDiagram.spec.js.

Rather than duplicating those, I added the one missing variant: a hand-drawn class self-loop test in classDiagram-handDrawn-v3.spec.js.

🟡 "Resolves #6049" over-claim
Reworded to "Part of #6049" with a note that the issue should be closed by the integration PR — agreed the flowchart-facing fix landed earlier in the series and this PR only extends the merge to er/classDiagram.

🟢 Nits

• EMPTY_CLUSTER_DB singleton removed; paintLayoutEdge now falls back to a fresh new Map() per call (Object.freeze doesn't actually prevent Map.set, so a fresh map is the safe option).
• Added a comment next to the class block in defaultConfig.ts explaining why padding is intentionally left undefined.

[knsv-bot]

[sisyphus-bot]

✅ Intended verdict: APPROVE. Posting as a COMMENT because GitHub won't let this account approve its own PR — a human maintainer should treat this as a green light pending CI.

Re-review of the diff at head (cdfc0ea65) against codex/common-render-pipeline-4-dagre-layoutdata, following up on the previous round. All previously raised items are resolved, the security pass is clean, and I found nothing new that blocks.

Previous round — all items resolved

• 🔴→✅ Class default-renderer flip: .changeset/class-default-unified-renderer.md (minor) documents the switch with the correct opt-out (class: { defaultRenderer: 'dagre-d3' } — verified against the schema enum). The PR description now leads with the change instead of burying it.
• 🟡→✅ Visual coverage: both requested cases already existed (classDiagram-v3.spec.js self-referential multiplicity test — which exercises the new default path on this branch — and the recursive ER tests in erDiagram-unified.spec.js across default/ELK/handDrawn). A hand-drawn class self-loop snapshot test was added in classDiagram-handDrawn-v3.spec.js to close the one real gap.
• 🟡→✅ #6049 wording: now "Part of #6049", with closure deferred to the integration PR.
• 🟢→✅ EMPTY_CLUSTER_DB: shared singleton removed in favor of a fresh new Map() per paint call (common/index.ts:255) — the right call, since Object.freeze wouldn't actually prevent Map.set.
• 🟢→✅ class.padding omission: now documented in defaultConfig.ts:62-64.

What's working well

• 🎉 Behavior-preservation verified at the seams. I traced the two subtle invariants: paintOptions.clusterDb captures the module-level Map from mermaid-graphlib.js, which is safe because clear() mutates in place and never reassigns; and the subgraph-title margin adjustments that used to live in paintDagreLayoutCore now flow through applyDagreLayoutResult, which writes normalized positions back onto the dagre graph payloads that getDagrePaintNodes feeds the shared painter. Both check out.
• 🎉 The new unit tests are exactly the right shape. "Graph-backed paint nodes and edge endpoints", "skip nodes already painted elsewhere", and "lets layout algorithms decide which group nodes are clusters" each pin one hook's contract, and the class self-loop terminal-label merge test locks the regression-prone bit of getEdgesToRender. 144 tests pass locally at head.
• 🎉 The DAGRE_NODE_LAYOUT_PROPERTIES allowlist discipline held — extended only with the fixed literal 'clusterNode', keeping the defense against copying user-controlled graph keys onto LayoutData.

Nits (non-blocking)

• 🟢 [nit] dagre/index.js:781 — the skipNode: (node, { measure }) => !measure.graph.hasNode(node.id) hook is effectively redundant for dagre, since getDagrePaintNodes already derives its list from measure.graph and filters falsy payloads. Harmless as defense-in-depth, but a one-line comment saying it only matters if getNodes is ever removed would save a future reader the puzzle.
• 🟢 [nit] dagre/index.js:776 — paintOptions captures clusterDb once at module load. This works because mermaid-graphlib.js only ever mutates the Map in place, but that invariant lives in a different file; a short comment noting the dependency would make it harder to break accidentally (e.g., by someone changing clear() to clusterDb = new Map()).

Security

Dedicated XSS/injection pass over all changed source: no XSS or injection issues identified. The self-loop label merge feeds the same sanitizing insertEdgeLabel → createText/createLabel sink as every other edge (sanitization via sanitizeText/renderKatexSanitized before any .html() call); labelStyle lands only in a style attribute via setAttribute; the new paint hooks route node objects exclusively to geometry-only sinks (transform/d/id attrs); and the class-default switch routes to the already-shipping, sanitized v2 pipeline. The terminal DOMPurify pass is untouched.

Remaining gate (mechanical, not a review finding)

The e2e jobs and Argos build for cdfc0ea65 were still running at review time. The previous Argos build (13 changed snapshots, including the class diagrams moving to the unified renderer) was reviewed and approved; the new hand-drawn self-loop test will produce one new baseline that wants a quick eyeball when the build lands. Nothing in the diff since then changes rendering output, so I expect green.

---

Severity tally: 🔴 0 · 🟡 0 · 🟢 2 · 🎉 3. Nice work closing out the round — from my side this is ready once CI confirms. 🚀

_{Reviewed by reading the full diff at head cdfc0ea65, running the affected unit suites locally (144 passing), and a dedicated XSS sub-review.}