Skip to content

fix(flowchart): use rounded right-angle edges instead of smooth curves#7425

Merged
knsv merged 4 commits intodevelopfrom
bug/7213_elk-edges-right-angles
Mar 1, 2026
Merged

fix(flowchart): use rounded right-angle edges instead of smooth curves#7425
knsv merged 4 commits intodevelopfrom
bug/7213_elk-edges-right-angles

Conversation

@knsv
Copy link
Collaborator

@knsv knsv commented Feb 28, 2026

Summary

Resolves #7213

  • Replace the manual fixCorners corner-rounding logic with a proper 'rounded' D3 curve type
  • Add 'rounded' to the flowchart curve config enum and set it as the new default
  • Remove dead code: fixCorners, extractCornerPoints, findAdjacentPoint from edges.js
  • Add e2e snapshot tests for ELK right-angle edge routing and the new rounded curve option

Test plan

  • Flowchart unit tests pass (969 tests)
  • Visual spot-check via Playwright verify-diagram — ELK edges route orthogonally with rounded corners
  • Visual spot-check — curve: 'rounded' renders right-angle edges with rounded bends
  • Cypress e2e baseline snapshots generated
  • CI passes (lint, unit, e2e, type-check)

🤖 Generated with Claude Code

#7213)

Replace the manual fixCorners corner-rounding logic with a proper
'rounded' D3 curve type. Add 'rounded' to the flowchart curve enum
and make it the new default. This fixes ELK edges that were curving
instead of routing at right angles with rounded corners.

- Remove fixCorners, extractCornerPoints, findAdjacentPoint from edges.js
- Add 'rounded' to config.schema.yaml curve enum, set as default
- Regenerate config.type.ts
- Add e2e snapshot tests for ELK right-angle edges and rounded curve

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@netlify
Copy link

netlify bot commented Feb 28, 2026

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 84d3743
🔍 Latest deploy log https://app.netlify.com/projects/mermaid-js/deploys/69a3e2b7b9ac1600071d72bd
😎 Deploy Preview https://deploy-preview-7425--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2026

🦋 Changeset detected

Latest commit: 84d3743

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

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Feb 28, 2026
@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 28, 2026

Open in StackBlitz

@mermaid-js/examples

npm i https://pkg.pr.new/@mermaid-js/examples@7425

mermaid

npm i https://pkg.pr.new/mermaid@7425

@mermaid-js/layout-elk

npm i https://pkg.pr.new/@mermaid-js/layout-elk@7425

@mermaid-js/layout-tidy-tree

npm i https://pkg.pr.new/@mermaid-js/layout-tidy-tree@7425

@mermaid-js/mermaid-zenuml

npm i https://pkg.pr.new/@mermaid-js/mermaid-zenuml@7425

@mermaid-js/parser

npm i https://pkg.pr.new/@mermaid-js/parser@7425

@mermaid-js/tiny

npm i https://pkg.pr.new/@mermaid-js/tiny@7425

commit: bdcc76e

@codecov
Copy link

codecov bot commented Feb 28, 2026

Codecov Report

❌ Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 3.55%. Comparing base (33c7c72) to head (84d3743).
⚠️ Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
...aid/src/rendering-util/rendering-elements/edges.js 0.00% 23 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #7425      +/-   ##
==========================================
- Coverage     3.55%   3.55%   -0.01%     
==========================================
  Files          489     490       +1     
  Lines        48729   48755      +26     
  Branches       765     765              
==========================================
  Hits          1734    1734              
- Misses       46995   47021      +26     
Flag Coverage Δ
unit 3.55% <0.00%> (-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/config.type.ts 100.00% <ø> (ø)
...aid/src/rendering-util/rendering-elements/edges.js 0.00% <0.00%> (ø)

... and 1 file 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
Copy link

argos-ci bot commented Feb 28, 2026

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) 👍 Changes approved 441 changed, 2 added Mar 1, 2026, 7:05 AM

When edge.curve is not a string (undefined for class diagrams, D3
CurveFactory for others), fall back to getConfig().flowchart.curve.
This makes the 'rounded' curve work for class diagrams, state
diagrams, and all diagram types using the rendering-util edge path
— not just flowcharts.

- Add resolveEdgeCurveType() helper that returns edge.curve if string,
  otherwise falls back to config
- Add 'rounded' case to the curve switch statement
- Add unit tests for the resolution logic

Resolves #7213

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Collaborator Author

@knsv knsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sisyphus-bot]

Early-stage review — heads up for when this is ready

This is looking like a solid approach. The two-commit structure is clean: first commit replaces fixCorners with the 'rounded' curve and generateRoundedPath, second commit fixes the curve resolution so it works across all diagram types. Some items to consider before marking ready for review.


What's working well

🎉 [praise] The resolveEdgeCurveType() helper (edges.js:34-43) is elegant. A single exported function, well-documented, tested in isolation — it solves the "class diagrams don't get rounded edges" problem at exactly the right level in the pipeline instead of requiring changes in every diagram DB.

🎉 [praise] Removing fixCorners (86 lines of manual geometry with magic numbers like Math.sqrt(2) * 2) in favor of generateRoundedPath (which was already in the file) is a clean improvement. The new function handles edge cases better (collinear points, marker offsets) and is easier to reason about.


Things to address

🟡 [important] Breaking change not flagged. The config default changes from 'basis' to 'rounded' (config.schema.yaml:2158-2159). Per CLAUDE.md, "changing default rendering output in ways visible to consumers" constitutes a breaking change. Every flowchart that doesn't explicitly set curve: will switch from smooth splines to right-angle-with-rounded-corners. This is an intentional design improvement, but it should be:

  • Called out in the PR description as a breaking/visual change
  • Reflected in the changeset semver (at least minor, arguably major)
  • Noted for the release notes so consumers aren't surprised

🟡 [important] E2E coverage gap for non-flowchart diagrams. The fixCorners removal + default change affects ALL diagram types using the rendering-util edge path — class diagrams, state diagrams, etc. The e2e tests added only cover flowcharts (ELK and v2). Since rendering-util/ is shared code, the CLAUDE.md convention is: "Always run full e2e tests after modifying shared rendering code — unit tests alone are insufficient." Worth adding at least one e2e test for a non-flowchart diagram (class or state with ELK) to confirm the resolveEdgeCurveType fallback works visually.

🟡 [important] Missing changeset. No .changeset/*.md file in the diff. This is a user-facing rendering change that needs pnpm changeset — patch bump at minimum (since it's a bug fix), but given the default change, consider minor.

💡 [suggestion] Dead code on lines 510-511. let curve = curveBasis; curve = curveLinear; — the first assignment is immediately overwritten. This is pre-existing, but since you're already touching these lines, could clean it up to just let curve = curveLinear;.

💡 [suggestion] Config namespace consideration. resolveEdgeCurveType falls back to getConfig()?.flowchart?.curve — this means all diagram types (class, state, etc.) inherit the flowchart curve setting. This works pragmatically today since flowchart.curve is the only curve config. If this becomes a pattern, a top-level curve config key might be cleaner long-term. Not blocking — just noting the architectural direction.


Summary

Severity Count
🔴 blocking 0
🟡 important 3
🟢 nit 0
💡 suggestion 2
🎉 praise 2

Verdict: COMMENT (draft PR — all items are heads-up, not demands). The core approach is right. The main gap is confirming the visual impact across diagram types with e2e tests and adding a changeset before this moves out of draft.

- Add minor changeset documenting the default curve change from
  'basis' to 'rounded', with migration note
- Remove dead code: `let curve = curveBasis; curve = curveLinear;`
  collapsed to `let curve = curveLinear;`

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Collaborator Author

@knsv knsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sisyphus-bot]

Nice work on this — the approach is clean and directly addresses #7213. A few heads-ups for when you're ready to finalize.


What's working well

🎉 [praise] The removal of fixCorners, extractCornerPoints, and findAdjacentPoint (~90 lines) in favor of generateRoundedPath is a great cleanup. The quadratic Bezier approach is more mathematically sound and handles edge cases (zero-length segments, near-collinear points) gracefully. Much nicer than the old manual offset logic.

🎉 [praise] The resolveEdgeCurveType helper is a smart abstraction — it cleanly separates the "what curve type are we using?" question from the D3 curve factory selection. The unit tests covering string, undefined, null, and function inputs give good confidence in the resolution logic.

🎉 [praise] Test coverage is solid: the ELK e2e test reproduces the exact scenario from #7213, the flowchart-v2 test exercises the new rounded curve option explicitly, and the unit tests verify the config fallback logic. This is the right combination of unit + visual regression tests for a rendering-util change.


Things to consider

🟡 [important]Default change is a visual breaking change. Changing the default flowchart.curve from 'basis' to 'rounded' will alter every existing flowchart rendering across GitHub, GitLab, Obsidian, and all other consumers. The project's own guidelines list "changing default rendering output in ways visible to consumers (edge paths, spacing)" as potentially breaking. The changeset uses a minor bump — worth considering whether this should be major, or at minimum confirming this is an intentional trade-off you're comfortable with. The migration note in the changeset ("set flowchart.curve: 'basis'") is great — just want to flag the semver question. (config.schema.yaml:2160)

🟡 [important]Cross-diagram regression risk for non-flowchart types. When edge.curve is a D3 CurveFactory function (as class diagrams, state diagrams, and others may pass), the old code would fall through the switch to default: curveBasis. The new resolveEdgeCurveType resolves those to the flowchart config default ('rounded'), which then uses curveLinear + generateRoundedPath instead of curveBasis. This could change edge rendering for non-flowchart diagram types unintentionally. Worth a quick visual check on a class diagram and state diagram to confirm they still look right, or adding a guard so non-string edge.curve values that are actual D3 functions get applied directly rather than falling through to config. (edges.js:41-42)

The previous commit removed fixCorners entirely, but it is still needed
by diagram types that use curve: 'basis' (e.g. ER diagrams). Without it,
right-angle corners from ELK layout lose their smooth rounding when
rendered with curveBasis. Now fixCorners is conditionally applied for all
curve types except 'rounded', which uses generateRoundedPath instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@knsv knsv marked this pull request as ready for review March 1, 2026 11:47
@knsv knsv merged commit 4306d6d into develop Mar 1, 2026
28 checks passed
@knsv knsv deleted the bug/7213_elk-edges-right-angles branch March 1, 2026 11:47
@github-actions github-actions bot mentioned this pull request Mar 3, 2026
@github-actions github-actions bot mentioned this pull request Mar 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug / Error Something isn't working or is incorrect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ELK flowchart edge curving instead of right angle

1 participant