Conversation
|
github-actions
Bot
added
the
Type: Bug / Error
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5756 +/- ##
==========================================
- Coverage 5.00% 3.53% -1.47%
==========================================
Files 337 491 +154
Lines 48213 49003 +790
Branches 576 766 +190
==========================================
- Hits 2413 1733 -680
- Misses 45800 47270 +1470
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 ↗︎
|
|
I don't know why Lint fails here on GitHub (the “Some checks were not successful”). It works fine on my machine. Of note too: |
There was a problem hiding this comment.
Can you remove this file from the PR?
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
|
What's the status of this PR? @Bunker-D |
|
@Bunker-D can you please sync with latest develop? @shubhamparikh2704 please add this to our agenda for next meeting. |
|
@sidharthv96 @shubhamparikh2704 Any news? This would resolve a11y issues on my site. |
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I updated the PR with the current version of Mermaid. I removed my changed to Entity Relation (er) diagrams has the issue had been solved since my original PR. But I kept the dedicated Cypress test. Edit: And yes, the results of the new tests in argos are what is expected, i.e., the markers (arrows etc) are visible even though there is a hidden version of the graph ( |
|
I will work with my code assistant to review the PR, will be reviewing the reviewer at the same time. |
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thank you for this contribution, @Bunker-D — this is an important quality-of-life fix that affects a lot of users, and you've done the hard work of tracking down all the affected diagram types. The two-diagram test pattern (one hidden, one visible on the same page) is exactly the right approach to catch marker ID collisions. Let's get this across the finish line!
What's working well
🎉 The two-diagram test pattern in the cypress HTML fixtures is spot-on. Rendering a hidden diagram first and then a visible one is the correct way to expose duplicate-ID issues — well designed.
🎉 Adding CSS classes (mermaid-marker-*) alongside the ID fix is a great forward-looking addition, giving diagram consumers a stable hook for custom styling.
🎉 The fix mechanism itself — threading id as a parameter to all marker-insertion functions — is clean and consistent with how PR #4825 handled the same problem for flowcharts, class diagrams, and state diagrams.
🎉 Good JSDoc updates on the modified functions.
Things to address
🔴 [blocking] ER diagram markers not fixed — but Cypress test was added
The PR includes cypress/integration/rendering/marker_unique_id_er.spec.js and cypress/platform/marker_unique_id_er.html, which correctly puts two ER diagrams on the same page to expose the duplicate-ID bug. However, the actual fix for ER markers was not implemented.
packages/mermaid/src/diagrams/er/erMarkers.js still defines all ten markers using global string IDs (ONLY_ONE_START, ZERO_OR_ONE_END, etc.) via its insertMarkers(elem, conf) function, and erRenderer.js:581 calls it without passing id. The marker references at erRenderer.js:463–502 use url(#ZERO_OR_ONE_END) etc. — none prefixed with the diagram's id.
The Cypress test will show the broken rendering (disappearing markers on the second diagram) unless erMarkers.js:insertMarkers() is updated to accept id and prefix each .attr('id', ...) call, and erRenderer.js is updated to pass id both to insertMarkers and to each url(#...) reference.
🔴 [blocking] Sequence diagram: four arrowhead markers still use global IDs
The PR updates insertArrowHead, insertArrowFilledHead, insertArrowCrossHead, and insertSequenceNumber in sequence/svgDraw.js. But there are four more marker-insertion functions in the same file that were missed:
insertSolidTopArrowHead—id: 'solidTopArrowHead'(line 1717)insertSolidBottomArrowHead—id: 'solidBottomArrowHead'(line 1732)insertStickTopArrowHead—id: 'stickTopArrowHead'(line 1747)insertStickBottomArrowHead—id: 'stickBottomArrowHead'(line 1765)
These are called in sequenceRenderer.ts at lines 1060–1063 without the id parameter, and referenced at sequenceRenderer.ts:566–607 without the id prefix (e.g., url(#solidTopArrowHead)). All four have the same collision risk.
There is also a filled-head-control marker defined inline inside the actor-drawing path (sequence/svgDraw.js:690) that is never prefixed. Since its definition and reference are co-located within the same actor draw call, the collision risk is lower in practice — but it's still a global ID that should be prefixed for consistency.
The current Cypress test for sequence only exercises arrowhead, filled-head, and crosshead. Adding a test case with SOLID_TOP, SOLID_BOTTOM, STICK_TOP, and STICK_BOTTOM message types would catch the four missed markers above.
🟡 [important] Missing changeset
This is a user-facing bug fix that resolves long-standing issues (#1318, #3267, #5741) affecting C4, ER, sequence, timeline, and journey diagrams. It needs a changeset for the mermaid package:
pnpm changeset
# select: mermaid, patch bump
# description: fix: ensure unique marker IDs per diagram to prevent arrow rendering issuesSecurity
No XSS or injection issues were found. The id values being concatenated into marker .attr('id', ...) calls are generated internally by Mermaid, not from user input. CSS class names are hardcoded strings. All user-provided text goes through .text() (not .html()), so it is properly escaped.
Summary: Two blocking issues: ER markers not yet fixed despite the test being added, and four sequence arrowhead markers missed. Once those are addressed and a changeset is added, this should be in great shape!
|
Well. Pull #7410 solved the issue, and pass the tests I added. An AI produced basically the same solution as me. That being said, the AI didn't provide any tests. I also added classes for the markers whose IDs where modified, so that user can still relatively easily style said markers with CSS. @knsv, do you want me to update my PR to include those elements? Or should we just close it? |
|
Hi @Bunker-D. Thank you for your patience, and I'm sorry this sat for so long before being reviewed. You were the first to identify, report, and start working on this fix, and you absolutely deserve credit for that. PR #7410 has been merged and covers the same core bug (and then some), so the duplicate-marker issue should now be resolved in develop. This PR is no longer needed to fix the regression — feel free to close it. That said, the CSS classes you added (mermaid-marker-*) are a nice bonus that #7410 doesn't include. If you'd like to contribute those as a standalone follow-up PR, we'd welcome that. It would be a small, clean, focused change and should be straightforward to merge. It would also in that case get full focus from us. Thank you again for doing the groundwork here — this was a long-standing pain point for many users. |
|
Hi @Bunker-D — closing this one out, as the underlying issue (#5741, duplicate marker/SVG element ids) was fixed and merged in #7410. As you noted, that landed essentially the same solution you'd identified, and your tests held up against it. Credit where it's due: you were the first to identify, report, and fix this — thank you for the careful work and for your patience while it sat. The fix being in Really appreciate the contribution. 🙇 |
6 participants
📑 Summary
Ensures unique IDs for the markers. This extends merge #4825, to cover C4 context diagrams, Entity Relationship diagrams, Requirement diagram, Sequence diagrams, Timelines, and User Journeys.
Resolves #5741, #1318 and #3267 (that were closed while only partially resolved), mjbvz/vscode-markdown-mermaid/#270.
Also, adds classes to the markers for future and/or custom styling purposes.
📏 Design Decisions
Markers IDs are replaced by
<svg_id>-<marker_id>, where:<svg_id>is the ID of the SVG element (the Mermaid diagram)<marker_id>is the previously used id.The added classes use the format:
mermaid-marker-<diagram_type>-<marker_id>(e.g.,mermaid-marker-er-ONLY_ONE_END), where<diagram_type>is a shorthand for the diagram type:C4Context) →c4erDiagram) →erjourney) →journeyrequirementDiagram) →reqsequenceDiagram) →seqtimeline) →tlThose changes have also been implemented for seemingly unused markers found in the code.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.