fix(class): Self-referential class multiplicity labels rendered multi…#7578
fix(class): Self-referential class multiplicity labels rendered multi…#7578Gaston202 wants to merge 3 commits intomermaid-js:developfrom
Conversation
…ple times Fixes mermaid-js#7560 where cardinality labels (e.g. "1", "0..1") were displayed 3x on self-referential class diagram relationships. Root cause: The dagre layout splits self-loops into 3 edges but structuredClone copied cardinality labels to all of them. Now each segment only carries its relevant cardinality label. Also fix DOM hierarchy bug in edge label creation where labels were appended to the wrong parent element. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 1f98db8 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. |
@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 #7578 +/- ##
==========================================
- Coverage 3.34% 3.33% -0.01%
==========================================
Files 524 535 +11
Lines 55392 56232 +840
Branches 795 820 +25
==========================================
+ Hits 1853 1877 +24
- Misses 53539 54355 +816
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 ↗︎
|
The middle edge segment of a self-loop should preserve its label (e.g. "refers") — only the cardinality labels need to be cleared. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Review: fix(class): Self-referential class multiplicity labels rendered multiple times
Thanks for digging into this one, @Gaston202! Self-referential edge rendering is a tricky area with the 3-way edge splitting, and you've correctly identified both the root cause and the fix locations across the legacy and modern rendering paths. Nice work.
What's working well
🎉 [praise] Correct root cause identification — the structuredClone of the edge copies all label properties to all 3 sub-edges, causing 3x rendering. The fix in dagre/index.js correctly clears labels from the sub-edges that shouldn't carry them (edge1 keeps startLabelRight, edge2 keeps endLabelLeft, edgeMid gets neither).
🎉 [praise] Consistent fix across both dagre-wrapper/edges.js (deprecated path) and rendering-util/rendering-elements/edges.js (modern path) — good attention to ensuring both code paths are addressed.
🎉 [praise] The addition of edge2.arrowTypeStart = 'none' is a nice catch — without it, the intermediate-to-target sub-edge would render a spurious start arrowhead at the intermediate node.
Things to address
🟡 [important] Missing visual regression test. This is a rendering bug fix that affects shared layout code (rendering-util/). Per project conventions, renderer changes need E2E visual regression tests in cypress/integration/rendering/ using imgSnapshotTest(). A test like this would prevent future regressions:
imgSnapshotTest(
`classDiagram
class SelfReferential{
+int id
+SelfReferential referenced
}
SelfReferential "1" --> "0..1" SelfReferential : referenced
`
);There are currently no E2E tests covering self-referential class diagram edges with multiplicity labels, so this would be valuable new coverage. (cypress/integration/rendering/classDiagram*.spec.js would be the right home.)
🟡 [important] Missing changeset. This is a user-facing bug fix and needs a changeset for release notes. It would be great to run pnpm changeset, select @mermaid-js/mermaid, pick patch, and write something like fix: self-referential class multiplicity labels no longer rendered multiple times.
💡 [suggestion] Defensive clearing of all 4 label positions. The fix clears startLabelRight and endLabelLeft on the appropriate sub-edges, which covers class diagram multiplicity labels. But startLabelLeft and endLabelRight are not cleared — if any diagram type sets those on a self-referential edge, they'd still appear 3x. Worth defensively clearing all four label positions for completeness:
// dagre/index.js — cyclic edge splitting
edge1.endLabelLeft = '';
edge1.endLabelRight = ''; // defensive
edgeMid.startLabelRight = '';
edgeMid.startLabelLeft = ''; // defensive
edgeMid.endLabelLeft = '';
edgeMid.endLabelRight = ''; // defensive
edge2.startLabelRight = '';
edge2.startLabelLeft = ''; // defensive🟢 [nit] The PR description body could link to the issue it fixes (e.g., Resolves #7560) so GitHub auto-links and auto-closes on merge.
Security
No XSS or injection issues identified. The changes are purely structural (DOM node parenting and property clearing on cloned edge objects) — no new user input flows, no changes to sanitization paths.
Self-check
- At least one 🎉 praise item
- No duplicate comments
- Severity tally: 🔴 0 / 🟡 2 / 🟢 1 / 💡 1 / 🎉 3
- Verdict matches criteria: COMMENT (2 🟡 items, no 🔴)
- Not a draft → full review appropriate
- No inline comments used
- Tone check: collaborative and appreciative ✓
|
Good work @Gaston202, let's get it over the finishing line! |
Clear label props on split sub-edges to prevent multiplicity labels from rendering 3x after structuredClone during layout. - keep labels only on correct sub-edges - defensively clear all label positions - remove unintended arrow on edge2 - add visual regression test - add changeset
|
Thanks so much for the thorough and thoughtful review, @knsv! I've just addressed all the feedback: -Added visual regression test - Created a test in cypress/integration/rendering/classDiagram-v3.spec.js that specifically covers self-referential class diagrams with multiplicity labels to prevent future regressions. -Added changeset - Created .changeset/fix-7560-self-referential-multiplicity.md with a clear description for the release notes. -Defensive clearing - Extended the fix to clear all 4 label positions (startLabelLeft/Right, endLabelLeft/Right) on all three sub-edges in both rendering paths:
Really appreciate the positive feedback and the suggestion about defensive clearing. It's a small addition that makes the solution more robust for any future diagram types that might use the other label positions. Thanks again for the great review! 😊 |
📑 Summary
Fix a bug where cardinality/multiplicity labels (e.g.
"1","0..1") were rendered three times on self-referential class diagram relationships.Resolves #7560
📏 Design Decisions
The dagre layout algorithm handles self-loops by splitting them into 3 separate edge segments (outgoing, middle, returning). The existing code used
structuredCloneto copy all edge properties toeach segment, duplicating cardinality labels (
startLabelRight,endLabelLeft) across all 3 edges instead of distributing them correctly.The fix clears cardinality labels from cloned self-loop edges so each segment only carries its relevant label:
startLabelRight, clearsendLabelLeftstartLabelRight, keepsendLabelLeftAdditionally,
createLabelwas appending elements to the wrong parent container in bothdagre-wrapper/edges.jsandrendering-elements/edges.js, which was fixed by removing redundantappendChildcalls.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that addfeatures should be
minorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.