Bug/5741 fix duplicate svg element ids#7410
Bug/5741 fix duplicate svg element ids#7410alexander-turner wants to merge 42 commits intomermaid-js:developfrom
Conversation
When multiple mermaid diagrams appear on the same page, internal SVG element IDs for nodes and edges collide (e.g., flowchart-A-0 appears twice). This causes invalid HTML (WCAG 4.1.1), broken url(#...) refs, and CSS selectors matching the wrong element. Prefix all internal element IDs with the diagram's SVG element ID (e.g., mermaid-0-flowchart-A-0), following the same pattern used for marker IDs in PR mermaid-js#4825. Changes: - render.ts: prefix all node domIds before layout - edges.js: prefix edge path IDs with diagram ID - createGraph.ts: prefix edge label node IDs - flowDb.ts/classDb.ts: add setDiagramId(), defer click handler domId lookup to bind time so prefixed IDs are used - flowRenderer/classRenderer: call setDiagramId() before getData() - flowRenderer: fix link selector to use domId instead of id Affects all diagram types that go through the unified render path: flowchart, class, state, ER, requirement, mindmap. https://claude.ai/code/session_01FPVnyf54nFQNQnhZ7dSWCE
Extract addFlowVertex helper to deduplicate verbose addVertex calls. Remove three describe blocks that only tested local mock functions (string concatenation), keeping all tests that exercise real FlowDB/ClassDB code and the full collision simulation. https://claude.ai/code/session_01FPVnyf54nFQNQnhZ7dSWCE
- Rename setDiagramId param from `id` to `svgElementId` for clarity - Add setDiagramId to DiagramDB interface to centralize the contract - Remove stale "defer lookUpDomId" comments in classDb.ts - Remove PR mermaid-js#4825 reference from render.ts comment https://claude.ai/code/session_01FPVnyf54nFQNQnhZ7dSWCE
…base class Extract shared `diagramId` field and `setDiagramId` setter from FlowDB and ClassDB into a new abstract base class `ScopedDiagramDB`. Both DB classes now extend it instead of duplicating the field and method. https://claude.ai/code/session_01FPVnyf54nFQNQnhZ7dSWCE
…agramDB base class" This reverts commit 7245574.
Fix unscoped element IDs in clusters.js (5 locations), defaultMindmapNode.ts, and kanbanRenderer.ts that caused duplicate DOM IDs when multiple diagrams with identical node names appeared on the same page. Add self-enforcing integration test that renders two identical diagrams for every registered diagram type and asserts no duplicate element IDs. The test auto-detects new diagram types via the registry and fails if they're not covered. Legacy renderers with known issues use it.fails to document them. Add runtime duplicate-ID warning in mermaid.run() (debug log level only) with a pre-filled GitHub issue link for easy reporting. Add Cypress tests for browser-level multi-diagram ID uniqueness verification. https://claude.ai/code/session_01QCm1SAitm8ZpLjjsk7eFv3
These type declaration files are generated by the prepare script during pnpm install. Adding them to avoid untracked file warnings. https://claude.ai/code/session_01QCm1SAitm8ZpLjjsk7eFv3
This reverts commit 10002cf.
New diagram types are added ~once per 5 months. The 4-category test taxonomy (unified/simple/legacy/jsdom-incompatible), self-enforcement registry check, and runtime duplicate-ID warning were not worth the maintenance cost. Simplified to a flat list of diagram tests that covers all types where IDs should be unique. https://claude.ai/code/session_01QCm1SAitm8ZpLjjsk7eFv3
Remove `|| node.id` fallback from clusters.js — every code path that calls insertCluster (dagre, cose-bilkent, kanban) already sets domId. The fallback silently hid missing domId bugs instead of surfacing them. Extract assertNoDuplicateIds into cypress/helpers/util.ts and use it in both marker_unique_id and multi_diagram_unique_ids specs. https://claude.ai/code/session_01QCm1SAitm8ZpLjjsk7eFv3
Prefix all hardcoded SVG marker IDs (arrowhead, filled-head, crosshead, sequencenumber, etc.) and element IDs with the diagram's unique ID in sequence, journey, timeline, gantt, and C4 renderers. This prevents cross-diagram ID collisions when multiple diagrams render on the same page. Covers: sequence (12 markers), journey (arrowhead), timeline (arrowhead), C4 (arrowhead, arrowend, filled-head, crosshead, sequencenumber, plus database/computer/clock symbols), and gantt (task + exclude-day IDs). https://claude.ai/code/session_01QCm1SAitm8ZpLjjsk7eFv3
…uniqueness tests Extends the parametrized renderTwoAndCheckIds test suite to cover all 5 diagram types that previously had hardcoded marker/element IDs. Also fixes a pre-existing bug in timeline's svgDraw.js where node IDs were `node-undefined` (now uses a monotonic counter prefixed with diagram ID). All 19 diagram types now pass the duplicate-ID stress test. https://claude.ai/code/session_01QCm1SAitm8ZpLjjsk7eFv3
…types Adds a meta-test that cross-references the detector registry against the test map, so any new diagram type added to mermaid will fail CI unless it has a corresponding ID uniqueness test (or is explicitly excluded with a justification). Also documents block and architecture as known-failing (pre-existing ID collision bugs tracked via it.fails), and excludes mindmap (cytoscape JSDOM limitation, uses unified pipeline so IDs are correct). https://claude.ai/code/session_01QCm1SAitm8ZpLjjsk7eFv3
Exercises the ID scoping mechanism under adversarial conditions: - 5x identical diagrams for 10 different diagram types (flowchart, class, ER, state, sequence, gantt, pie, C4, journey, timeline) - Complex graphs with 20 nodes and fan-out topologies - Nested subgraphs (3 levels deep) - Mixed diagram types on the same page (up to 10 types simultaneously) - FlowDB/ClassDB unit-level stress with 10-100 instances - Sequential render stability and clear/reset cycles - Adversarial node names that mimic diagramId prefixes - SVG marker definition uniqueness (sequence, C4) - Edge label ID uniqueness across renders - Kanban, git graph, requirement, XY chart, quadrant, sankey - Diagram ID prefix propagation verification https://claude.ai/code/session_01MtKnkbaUyKZY6R5vTrMNam
The journey diagram's svgDraw.js used bare `task0`, `task1`, etc. as element IDs without any diagram-scoped prefix. This worked by accident because the module-level taskCount never reset, but was fragile and inconsistent with the ID scoping pattern used by all other diagram types. Fix: - Store the diagramId passed to initGraphics() - Reset taskCount on each render via initGraphics() - Prefix task line IDs with diagramId (e.g. `mermaid-0-task0`) https://claude.ai/code/session_01MtKnkbaUyKZY6R5vTrMNam
Add a focused regression test that verifies journey diagram task line
IDs are scoped with the diagramId prefix. This test fails against the
pre-fix code (bare "task0"/"task1" IDs) and passes after the fix
("journey-a-task0"/"journey-b-task0").
Remove the large stress test file — the targeted test plus the existing
multi-diagram-id-uniqueness suite provide sufficient coverage.
https://claude.ai/code/session_01MtKnkbaUyKZY6R5vTrMNam
Claude/stress test pr ms jh o
…-graphs-gMtbB Claude/unique ids mermaid graphs g mtb b
Add 43 stress tests covering scenarios beyond the basic two-diagram tests: - Scale: 10 and 20 identical diagrams of 11 different types (flowchart, class, sequence, journey, timeline, gantt, C4, state, ER, pie, git) - Cross-type: mixed diagram types rendered into a single container - Subgraphs/clusters: nested and multi-level subgraph ID isolation - Large diagrams: 50-node flowcharts, 20-message sequences, 20-class diagrams, 15-task journeys - Minimal diagrams: single-node/single-message edge cases - DiagramId boundaries: hyphenated, underscored, numeric-prefixed IDs - Sequential re-rendering: clear-and-rerender and append-without-clear - Module-level counter resets: journey taskCount and timeline nodeCount - SVG marker/defs scoping: sequence, flowchart, and C4 markers - DB-layer scoping: FlowDB and ClassDB lookUpDomId under 5x stress - Kanban pre-flight domId injection - Gantt special characters in task IDs All 43 tests pass, confirming the ID uniqueness fix holds under stress. https://claude.ai/code/session_012gG2dXNE8BJAZfHJjs96aX
1. flowDb.ts lookUpDomId fallback: when called with an ID not in the
vertex map (e.g. subgraph IDs), the fallback now applies the
diagramId prefix instead of returning the bare ID.
2. sequence/svgDraw.js drawActorTypeControl: remove the `|| ''`
fallback on conf.diagramId that produced colliding marker IDs
(e.g. "-filled-head-control") when multiple sequence diagrams
share a page. The renderer always sets conf.diagramId before
rendering, so the fallback was masking a potential collision.
3. Eliminate module-level diagramId variables in sequence, journey,
and timeline renderers to prevent race conditions in concurrent
or SSR rendering scenarios:
- sequenceRenderer.ts: use conf.diagramId instead of redundant
module-level diagramId variable
- journey/svgDraw.js: pass diagramId as parameter to drawTask
instead of reading from module scope
- timeline/svgDraw.js: pass diagramId as parameter to drawTask,
drawNode, and defaultBkg; also fixes timeline drawTask which
was missing the diagramId prefix entirely ("task0" vs
"diagramId-task0")
- timeline/timelineRenderer.ts: pass diagramId through drawTasks
and drawEvents instead of reading from module-level
currentDiagramId
Adds 5 regression tests that would have failed before these fixes.
https://claude.ai/code/session_012gG2dXNE8BJAZfHJjs96aX
lookUpDomId in flowDb and classDb, and createGraph's edge-label domId, all had `diagramId ? prefixed : bare` ternaries. Since the render pipeline always calls setDiagramId before any lookups, the bare-ID fallback just silently masked missing diagramId bugs. Now these always prefix, making a missing setDiagramId call surface immediately. https://claude.ai/code/session_012gG2dXNE8BJAZfHJjs96aX
Stress test PR changes
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: ea81bd9 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 |
github-actions
bot
added
the
Type: Bug / Error
@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 latest updates on your projects. Learn more about Argos notifications ↗︎
|
The `|| node.id` / `|| vertex.id` / `: edge.id` fallbacks are dead code — render.ts always sets domId before shape/edge functions run. Worse, if reached, they'd silently produce unscoped IDs, reintroducing the duplicate-ID bug this PR fixes. Changes: - defaultMindmapNode.ts: throw on missing domId, use node.domId directly - flowRenderer-v3-unified.ts: throw on missing domId, use vertex.domId - edges.js: rename `id` param to `diagramId`, throw if missing, drop ternary fallback to bare edge.id
The addEdgeMarkers call on line 777 referenced `id` which was never defined in insertEdge's scope — the parameter is named `diagramId`. This caused: - ESLint no-undef lint failure - ReferenceError at runtime in all diagrams using the unified renderer - All unit tests and e2e tests to fail Also adds unit tests for the three defensive throw guards introduced in the PR (insertEdge, flowRenderer, defaultMindmapNode) to achieve 100% coverage on new code. https://claude.ai/code/session_01JzR5nomkuYcHxMJ67FMuW4
…lures-H6jja fix: use diagramId instead of undefined id in insertEdge
The PR's ID-prefixing changes caused three categories of E2E failures:
1. Double <a> wrapping in flowRenderer: The link-wrapping loop (lines
68-105) was dead code before the PR because vertex.id never matched
the rendered element's actual ID. The PR's switch to vertex.domId
activated this dead code, creating a second <a> inside .node that
broke cy.contains().find('.node') in Cypress. Fix: remove the
redundant loop since nodes.ts (lines 36-51) already wraps link
nodes in <svg:a> during rendering, including sandbox support
(target="_top"). The removed sandboxElement and doc variables were
only used by this redundant loop and had no other consumers in the
function — sandbox mode is fully preserved via nodes.ts.
2. Edge animation selectors: path#L_A_B_0 no longer matches because
edge IDs are now prefixed with diagramId. Fix: use attribute
selectors path[id$="-L_A_B_0"].
3. Gantt/rerender selectors: rect#cl1, text#cl1-text, and
[id^=flowchart-A] no longer match prefixed IDs. Fix: use
[id$="-cl1"], [id$="-cl1-text"], and [id*=flowchart-A].
https://claude.ai/code/session_012TZWtntRQbGFN6Sk9qDuhE
…t IDs
The gantt click handler (pushFun in ganttDb.js) used document.querySelector
with the raw task ID (e.g., "cl2"), but the renderer now sets element IDs
as "${diagramId}-${taskId}" (e.g., "mermaid-0-cl2"). This meant click
event listeners were never attached, breaking all gantt click interactions
(both URL navigation and function callbacks) in the E2E tests.
Fix: Add setDiagramId to ganttDb.js (matching the pattern used by
flowDb.ts and classDb.ts), call it from the gantt renderer's draw
function, and use the prefixed ID in pushFun's querySelector calls.
https://claude.ai/code/session_012TZWtntRQbGFN6Sk9qDuhE
…lures-KpnTd fix: resolve E2E test failures from ID-prefixed SVG elements
The PR prefixes all SVG marker IDs with the diagram container ID (e.g., `mermaid-0-arrowhead` instead of `arrowhead`), but the CSS styles.js files still used hardcoded `#arrowhead`, `#crosshead`, etc. selectors that no longer matched. This caused markers/arrowheads to lose their themed fill/stroke colors, producing subtle visual diffs detected by Argos CI. Replace all `#id` selectors with `[id$="-suffix"]` attribute selectors that match the new prefixed IDs: - sequence/styles.js: #arrowhead, #sequencenumber, #crosshead - class/styles.js: #compositionStart/End, #dependencyStart/End, #extensionStart/End, #aggregationStart/End, #lollipopStart/End - state/styles.js: #statediagram-barbEnd, #dependencyStart/End Also fixes a pre-existing bug in class/styles.js where #dependencyStart was duplicated instead of having #dependencyEnd. https://claude.ai/code/session_01DP7eBkBQuMUqZRkvJBdmyS
…ge-zm58i fix: update CSS ID selectors to match prefixed SVG element IDs
The erBox shape creates background nodes by spreading the parent node and overriding `id` with a '-background' suffix, but the spread also copies `domId`. Since the SVG element uses `domId || id`, both the foreground and background nodes ended up with the same DOM ID. This caused the background-positioning logic in erRenderer-unified.ts to fail (it selects by `[id*="-background"]`), leaving background nodes un-positioned and visible as duplicate tables. https://claude.ai/code/session_01KzNAdiCYp8tZZoBogdbhin
render.ts always sets node.domId before erBox runs, so the `|| node.id` fallback is dead code. https://claude.ai/code/session_01KzNAdiCYp8tZZoBogdbhin
Same rationale as the background node — render.ts guarantees domId is set before shape rendering. https://claude.ai/code/session_01KzNAdiCYp8tZZoBogdbhin
|
The |
domId is optional on the Node type since it's not yet assigned during getData(). Use ?? node.id as a defensive fallback, matching the convention used by every other shape renderer in the codebase. https://claude.ai/code/session_01UXvz61L5VFDwLmWDqbhfxm
fix: use nullish coalescing fallback for domId in erBox shape
…re-jceqc Claude/debug argos failure jceqc
…lexander-turner/mermaid into bug/5741_fix-duplicate-svg-element-ids
|
❌ Lockfile Validation Failed The following issue(s) were detected: Please address these and push an update. Posted automatically by GitHub Actions |
knsv
left a comment
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for tackling this, @alexander-turner — duplicate element IDs across multiple diagrams has been a persistent pain point for a long time, and your approach of extending the namespacing pattern from #4825 is exactly the right strategy. The PR description is excellent: thorough design rationale, clear related-issue cross-references, and honest about the breaking change surface. Really appreciate the effort here.
What's working well
🎉 [praise] The assertNoDuplicateIds() Cypress utility is a great addition — it gives a structural guarantee that can protect against regressions, not just for this PR but for any future rendering changes. Smart investment.
🎉 [praise] Nice catch fixing the duplicate #dependencyStart selector in class/styles.js — the original had two #dependencyStart rules where the second should have been #dependencyEnd. The new [id$="-dependencyEnd"] is correct. A bonus bug fix!
🎉 [praise] The removal of the link-wrapping code from flowRenderer-v3-unified.ts (lines 64–103) is correct — the shared rendering pipeline's insertNode() in rendering-util/rendering-elements/nodes.ts already wraps nodes with .link in <svg:a> elements. This was dead/redundant code. Good cleanup.
🎉 [praise] Deferring lookUpDomId() calls inside click-handler closures (both flowDb.ts and classDb.ts) so they resolve at bind-time rather than registration-time is the right fix for making callbacks work with prefixed IDs.
Things to address
🟡 [important] — lookUpDomId() should guard against empty diagramId
flowDb.ts and classDb.ts both unconditionally return `${this.diagramId}-${domId}`. When diagramId is '' (its default after clear()), this produces IDs with a leading dash like -flowchart-A-0.
While the v3-unified renderers call setDiagramId() before lookUpDomId() is invoked, any code path that calls lookUpDomId() without first setting the diagram ID (legacy renderers, future callers, test harnesses) would get broken IDs. A small guard would make this much more robust:
// flowDb.ts lookUpDomId
public lookUpDomId(id: string) {
for (const vertex of this.vertices.values()) {
if (vertex.id === id) {
return this.diagramId ? `${this.diagramId}-${vertex.domId}` : vertex.domId;
}
}
return this.diagramId ? `${this.diagramId}-${id}` : id;
}Same pattern for classDb.ts. This preserves backward compatibility for any caller that doesn't set diagramId.
The gantt DB (ganttDb.js) already uses this pattern (diagramId ? ... : id), so it would be consistent to apply it everywhere.
🟡 [important] — Coverage gap between description and implementation
The PR description says this extends namespacing to "all internally generated SVG element IDs across every diagram type." The actual coverage is:
| Covered ✅ | Not covered ❌ |
|---|---|
| Flowchart (v3-unified) | State (styles only — no ID prefixing in renderer) |
| Class (v3-unified) | ER diagram |
| C4 | Journey |
| Sequence | Mindmap |
| Gantt | GitGraph |
| Kanban | XY Chart, Pie, Radar, Packet, Architecture, Treemap |
| Timeline |
The E2E test page (multi_diagram_unique_ids.html) includes ER and state diagram pairs, but neither has corresponding ID-prefixing code in this PR. If those types already produce unique IDs (e.g., via the shared rendering pipeline), the test would pass — but it would be worth verifying explicitly and documenting which types are covered vs. which are deferred.
It would be great to either:
- Update the PR description to list exactly which diagram types are scoped for this PR, or
- Add a tracking comment for follow-up PRs to cover the remaining types
Not blocking — partial progress is real progress — but the "all diagram types" claim should match reality.
🟢 [nit] — Sequence renderer mutates the shared conf object
In sequenceRenderer.ts:1032:
conf.diagramId = id;This adds a diagramId property to the config object obtained from getConfig().sequence. While this works in practice (sequential rendering), it's cleaner to pass diagramId as a separate parameter to drawMessage and other functions rather than mutating a config object. Not urgent, but worth considering for maintainability.
🟢 [nit] — Changeset bump level
The changeset uses patch, which is reasonable if you consider the old bare-ID selectors already dead since #4825. Worth noting in the release notes that internal element IDs changed format, so consumers with custom CSS targeting Mermaid's internal IDs should be aware — even if it's not a formal SemVer break.
💡 [suggestion] — Timeline nodeCount module-level counter
The new nodeCount variable in timeline/svgDraw.js is module-level state. It's reset in initGraphics(), which is good. But if drawNode() is ever called without initGraphics() running first, the counter would carry over from a previous render. Since initGraphics is always called at the start of timeline rendering, this is fine in practice — just noting it for awareness.
Overall this is solid work addressing a real, long-standing problem with a well-thought-out approach. The two 🟡 items above would be great to address before merge — especially the lookUpDomId guard, which is a one-line fix per file. Looking forward to seeing this land!
3 participants
📑 Summary
When multiple Mermaid diagrams share a page, identical SVG element IDs (nodes, edges, edge labels, markers, task lines, sequence lifelines, etc.) collide across
<svg>containers. This causes browsers to bind event handlers, arrow markers, and CSS selectors to the wrong elements. Such incorrect binding breaks click callbacks, makes arrowheads disappear, and corrupts styling.This PR extends the namespacing approach introduced in #4825 (which scoped marker IDs per graph) to all internally generated SVG element IDs across every diagram type. Each ID is prefixed with its diagram's unique SVG container ID, guaranteeing no collisions regardless of how many diagrams appear on a page.
Related PRs:
Related issues:
bindFunctionsinteractions only work on one diagram instance when multiple are on the same pagerevealjs+mermaidjsarrows missing on 4th+ diagram (same root cause)📏 Design Decisions
Following the precedent set by #4825: that merged PR scoped marker IDs by prefixing them with the graph container ID. We apply the same namespacing strategy comprehensively — to node IDs, edge IDs, edge-label IDs, cluster IDs, task-line IDs, sequence lifeline/actor IDs, timeline section IDs, C4 element IDs, and journey task IDs.
Implementation approach:
diagramIdfield +setDiagramId()toFlowDBandClassDB; added to theDiagramDBinterface intypes.tsso any diagram DB can opt insetDiagramId(id)with the SVG container ID before layoutlookUpDomId()in FlowDB/ClassDB returns${diagramId}-${originalId}render.ts,createGraph.ts,edges.js,clusters.js) prefixes IDs for diagrams routed through the shared renderersvgDraw/ renderer filesdiagramIdresets onclear()to prevent state leakagemulti_diagram_unique_ids.spec.js) + unit tests (unique-dom-ids.spec.ts,multi-diagram-id-uniqueness.spec.ts) covering all diagram types and collision scenariosBreaking changes
arrowhead,crosshead) are now prefixed with the diagram's SVG element ID. Custom CSS or JS using exact ID selectors like#arrowheadshould use attribute-ending selectors like[id$="-arrowhead"]instead.#statediagram-barbEndand#compositionStartwere changed to[id$="-barbEnd"]and[id$="-compositionStart"]. The old selectors were dead code — since PR Give markers unique id's per graph #4825, marker IDs have been in the format${diagramId}_${type}-markerName, so the bare ID selectors like#statediagram-barbEndnever matched. The new attribute selectors correctly apply marker styles that were always intended but never worked, including an additional 1px along arrowhead borders.📋 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:.