Fix: ER diagram edge label positioning

[darshanr0107]

📑 Summary

This PR fixes an issue with edge label positioning in ER diagrams when the htmlLabels configuration is set to false.
Previously, edge labels were not positioned correctly when HTML labels were disabled, leading to misaligned or incorrectly rendered labels.

📏 Design Decisions

Describe the way your implementation works or what design decisions you made if applicable.

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added necessary unit/e2e tests.
• 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
• 🦋 If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: d73bdbf

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

This PR includes changesets to release 1 package

Name	Type
mermaid	Patch

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 2.00000% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 3.53%. Comparing base (547e6d5) to head (d73bdbf).
⚠️ Report is 452 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/mermaid/src/rendering-util/createText.ts	0.00%	14 Missing ⚠️
...aid/src/rendering-util/rendering-elements/edges.js	0.00%	14 Missing ⚠️
packages/mermaid/src/dagre-wrapper/edges.js	0.00%	13 Missing ⚠️
...kages/mermaid/src/rendering-util/labelTransform.ts	11.11%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #7453      +/-   ##
=========================================
- Coverage    3.59%   3.53%   -0.06%     
=========================================
  Files         475     496      +21     
  Lines       47191   49113    +1922     
  Branches      735     771      +36     
=========================================
+ Hits         1696    1736      +40     
- Misses      45495   47377    +1882     

Flag	Coverage Δ
unit	3.53% <2.00%> (-0.06%)	⬇️

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

Files with missing lines	Coverage Δ
...kages/mermaid/src/rendering-util/labelTransform.ts	11.11% <11.11%> (ø)
packages/mermaid/src/dagre-wrapper/edges.js	0.00% <0.00%> (ø)
packages/mermaid/src/rendering-util/createText.ts	0.41% <0.00%> (-0.04%)	⬇️
...aid/src/rendering-util/rendering-elements/edges.js	0.00% <0.00%> (ø)

... and 6 files with indirect coverage changes

🚀 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	110 changed, 6 added	Mar 6, 2026, 12:07 PM

[ashishjain0512]

PR #7453 Review: Fix ER diagram edge label positioning
Summary
The PR fixes edge label positioning when htmlLabels: false by accounting for SVG getBBox() x/y offsets in the centering transform. The change is in insertEdgeLabel in edges.js.

✅ Correctness of the Core Fix
The fix is correct. Here's why:

SVG getBBox() returns {x, y, width, height} where x and y can be non-zero (they represent the origin of the bounding box relative to the element's coordinate system)
HTML getBoundingClientRect() returns viewport-relative coordinates, and the old centering translate(-width/2, -height/2) worked fine for HTML because the foreignObject content starts at (0,0)
For SVG text, the origin of the bbox may not be (0,0) — the text baseline offset and tspan positioning means bbox.x and bbox.y are often negative
The formula -(bbox.x + bbox.width / 2) correctly computes the centering offset as: "negate the distance from origin to the center of the bbox"
Precedent: labelRect.ts (line 36) already uses a similar bbox-offset-aware centering formula:

labelRect.ts
packages/mermaid/src/rendering-util/rendering-elements/shapes
⚠️ Issues Found

1. Terminal labels have the same bug (not fixed)
   The PR only fixes the center label (lines 97-108). Four terminal label blocks at lines 134, 162, 190, 221 have the identical issue:

edges.js
packages/mermaid/src/rendering-util/rendering-elements
These terminal labels use createLabel (which calls createText with markdown: false), and the same SVG BBox offset issue applies. ER diagrams don't use terminal labels, so this won't affect the immediate use case, but it's an incomplete fix for any diagram type that uses terminal edge labels with htmlLabels: false.

Recommendation: Apply the same if/else pattern to the 4 terminal label blocks for consistency, or document it as a known limitation for a follow-up.

⚠️ No unit tests
Codecov reports 0% patch coverage with 6 lines uncovered. While e2e visual tests (Argos) provide visual regression coverage, the centering logic is pure math and would benefit from a unit test to prevent future regressions. A test could verify that for a mock SVG bbox with non-zero x/y, the computed transform correctly centers the element.

[ashishjain0512]

[sisyphus-bot]

Thanks for sticking with this, @darshanr0107! Great to see the follow-up commits addressing the terminal labels and adding unit tests — that shows real attentiveness to feedback. Let's get the remaining items sorted so we can land this.

---

What's working well

🎉 [praise] The core fix is mathematically correct. SVG getBBox() returns {x, y, width, height} where x/y can be non-zero (e.g., negative when addSvgBackground adds padding), and the formula -(bbox.x + bbox.width/2) correctly computes the center offset. This matches the existing pattern in labelRect.ts:36.

🎉 [praise] All 5 label positions (center + 4 terminal labels) are now fixed consistently across all commits. The changeset is present and correctly scoped as a patch.

---

Things to address

🟡 [important] Unit tests don't exercise the actual production code

The unit tests in edges.spec.js import and test computeLabelTransform from src/tests/util.ts — a standalone helper created purely for testing. However, the production code in edges.js does not use this helper; it has its own inline transform calculations repeated 5 times.

This means the tests verify the helper's math is correct, but they don't verify that edges.js actually applies the same formula. If someone later changes the formula in edges.js without updating the helper (or vice versa), the tests would still pass while the production behavior is wrong.

Two paths forward:

1. Preferred: Extract computeLabelTransform into a shared utility (e.g., in rendering-util/) and use it in both edges.js and the tests. This DRYs up the 5 repeated if/else blocks and makes the tests meaningful.
2. Acceptable: Keep the current structure but add an E2E visual test (see next item) that covers the actual rendering path.

---

🟡 [important] Missing E2E visual regression test

This change modifies rendering-util/rendering-elements/edges.js, which is shared code used by all diagram types that render edges. Per project conventions, renderer changes in shared code need E2E snapshot tests to catch visual regressions across diagram types.

It would be great to add at least one imgSnapshotTest in cypress/integration/rendering/ that renders an ER diagram with htmlLabels: false to confirm the labels are correctly positioned. This is the primary safety net for visual correctness.

---

💡 [suggestion] DRY up the repeated transform logic

The same if/else pattern appears 5 times in insertEdgeLabel. Consider extracting a helper function (the computeLabelTransform from tests/util.ts is already a good candidate) and calling it from all 5 locations. This would reduce ~50 lines to ~10 and make future maintenance easier.

---

Security

No XSS or injection issues identified. The transform values come exclusively from getBBox()/getBoundingClientRect() numeric measurements — no user-controlled strings flow into the attr('transform', ...) calls.

---

Summary: The fix itself is correct and well-scoped. At minimum, an E2E test is added; this should be good to go.

[ashishjain0512]

[sisyphus-bot]

Thanks for the follow-up work, @darshanr0107! Really appreciate you addressing the previous review feedback — extracting computeLabelTransform into a shared utility, adding unit tests, and adding the E2E snapshot test. That's exactly what was asked for, and it shows great responsiveness. Let's get the last few items sorted so we can land this.

---

What's working well

🎉 [praise] The extraction of computeLabelTransform into rendering-util/labelTransform.js is clean and well-documented. The JSDoc clearly explains why HTML and SVG labels need different treatment — that's the kind of comment that saves future maintainers real debugging time.

🎉 [praise] The unit tests in edges.spec.js are excellent — they cover the three key scenarios (SVG with background offset, SVG without offset, and HTML labels) and the inline comments showing the math make them self-documenting. This directly addresses the previous review's concern about tests exercising production code.

🎉 [praise] The E2E snapshot test for ER diagrams with htmlLabels: false is exactly the right safety net. Well done.

🎉 [praise] The changeset is present and correctly scoped as patch.

---

Things to address

🔴 [blocking] PR targets master — should target develop

The PR base branch is master, which contains the live release. Per project conventions, all PRs should target develop. This will need to be retargeted before it can be merged.

---

🔴 [blocking] createText.ts change breaks edge labels in dagre-wrapper/edges.js

The change to createText.ts passes centerText = !isNode to createFormattedText, which applies text-anchor: middle to all SVG edge labels across the entire codebase — not just ER diagrams.

The problem: dagre-wrapper/edges.js:61 also calls createText with isNode: false, but still uses the old centering formula:

label.attr('transform', 'translate(' + -bbox.width / 2 + ', ' + -bbox.height / 2 + ')');

With text-anchor: middle, text is now centered at x=0 (extending from -w/2 to +w/2). The old transform then shifts it left by another w/2, resulting in the label being visually offset to the left by half its width. The dagre-wrapper is deprecated but still active for block diagrams.

Two paths forward:

1. Preferred: Don't add text-anchor: middle globally in createText.ts. Instead, handle the centering entirely in edges.js via computeLabelTransform — which already accounts for bbox.x/y offsets. This keeps the fix scoped to where the transform is applied, avoiding side effects on other code paths that call createText.

2. Acceptable: Keep the createText.ts change but also update dagre-wrapper/edges.js to use computeLabelTransform. However, modifying both shared rendering utilities increases regression risk and testing scope significantly.

Ref: packages/mermaid/src/dagre-wrapper/edges.js:61

---

🟡 [important] Broader E2E coverage needed for shared code change

rendering-util/rendering-elements/edges.js and rendering-util/createText.ts are shared by all diagram types. The current E2E test only covers ER diagrams with htmlLabels: false. Since the createText.ts change applies text-anchor: middle to every SVG edge label, it would be great to add at least one more snapshot test for another diagram type that uses edge labels (e.g., flowchart with htmlLabels: false) to verify no regression.

If you go with path 1 above (removing the createText.ts change), this becomes less critical since the fix would be scoped to edges.js only — but still a good idea given that computeLabelTransform changes the transform formula for all edge labels.

---

🟡 [important] New file labelTransform.js should be TypeScript

The codebase convention is TypeScript for new files. labelTransform.js should be labelTransform.ts with proper type annotations for the bbox parameter and return type. Minor but keeps the codebase consistent.

---

Security

No XSS or injection issues identified. The transform values in computeLabelTransform come exclusively from getBBox()/getBoundingClientRect() numeric measurements — no user-controlled strings flow into the attr('transform', ...) calls. The text-anchor: middle addition is a hardcoded literal, not user-derived.

---

Self-Check

• At least one 🎉 [praise] item exists
• No duplicate comments
• Severity tally: 2 🔴 blocking / 2 🟡 important / 0 🟢 nit / 0 💡 suggestion / 4 🎉 praise
• Verdict matches criteria: REQUEST_CHANGES (🔴 present)
• Tone check: collaborative and appreciative

[ashishjain0512]

[sisyphus-bot]

This PR has been through 2 rounds of automated review with requested changes. Escalating to a human reviewer for further guidance — automated reviews are most useful in the first couple of rounds, and after that a human eye is better placed to navigate the remaining tradeoffs.

Great news: the contributor has addressed all code-level feedback from both rounds. @darshanr0107 has been extremely responsive throughout — every review item was tackled thoughtfully. Here's the status:

Resolved items

• computeLabelTransform extracted as shared utility — now in rendering-util/labelTransform.ts with proper TypeScript types (LabelBBox interface) and thorough JSDoc. Used by both rendering-util/rendering-elements/edges.js and dagre-wrapper/edges.js.
• dagre-wrapper updated — dagre-wrapper/edges.js now uses computeLabelTransform, preventing the centering regression identified in round 2.
• Unit tests exercise the actual production computeLabelTransform function with 3 well-documented scenarios.
• E2E coverage includes both ER diagram (erDiagram-unified.spec.js) and flowchart (flowchart-v2.spec.js) with htmlLabels: false.
• Changeset present and correctly scoped as patch.

Assessment

The code changes look solid. The fix correctly accounts for getBBox() x/y offsets when centering SVG edge labels, the text-anchor: middle approach in createText.ts is applied consistently across both edge code paths (rendering-util and dagre-wrapper), and test coverage is comprehensive. Once the base branch is retargeted to develop, this should be ready for a final human review and merge.