Skip to content

Conversation

@lmac-1
Copy link
Collaborator

@lmac-1 lmac-1 commented Jan 23, 2026

Description

This PR fixes a bug where converging edges would swap positions when selected on the diagram.

The issue was caused by sortOrderForSvg reordering edges based on selection state, which affected Dagre's layout decisions. The fix preserves stable edge order regardless of selection.

Before:
image

After:
image

See the bug in action: https://www.loom.com/share/302055f6f3b748f1a4523ef7ccef67af

Closes #4328

Validation steps

  1. Open workflow with multiple branching points
  2. Make sure your diagram is on auto layout mode
  3. Click edges from different parent nodes
  4. Click edges that lead to converging paths
  5. Expected: All paths maintain their positions

Additional notes for the reviewer

  1. If you have bigger workflows - perhaps click around on the paths and make sure that everything behaves as expected
  2. I suspected that this sorting algorithm was to ensure that the edges stayed on top in terms of z-index. I've verified both with and without the fix that the behaviour remains the same when there are edges that are on top of each other.

AI Usage

Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):

  • I have used Claude Code
  • I have used another model
  • I have not used AI

You can read more details in our
Responsible AI Policy

Pre-submission checklist

  • I have performed an AI review of my code (we recommend using /review
    with Claude Code)
  • I have implemented and tested all related authorization policies.
    (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

Fixes #4328

When clicking different edges in auto-layout mode, paths would swap
positions because sortOrderForSvg was reordering edges based on
selection state. Dagre's layout algorithm uses edge order to determine
positioning when disableOptimalOrderHeuristic is enabled.

Fix by removing selection-based sorting and preserving stable edge order.
Only disabled/enabled status affects sort order now.
@github-project-automation github-project-automation bot moved this to New Issues in v2 Jan 23, 2026
@codecov
Copy link

codecov bot commented Jan 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.33%. Comparing base (549de66) to head (b398d4e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4348      +/-   ##
==========================================
+ Coverage   89.29%   89.33%   +0.04%     
==========================================
  Files         425      425              
  Lines       19994    19994              
==========================================
+ Hits        17854    17862       +8     
+ Misses       2140     2132       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

Copy link
Contributor

@doc-han doc-han left a comment

Choose a reason for hiding this comment

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

Works perfectly 🎉
Can we remove the comments with the PR number in them?

Also, I'm yet to understand why the sort function was initially made to work in that manner(pushing the selected edge to the bottom).

Copy link
Contributor

@doc-han doc-han left a comment

Choose a reason for hiding this comment

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

Can we remove the comments with PR numbers? Thanks.

@github-project-automation github-project-automation bot moved this from New Issues to In review in v2 Jan 26, 2026
@lmac-1
Copy link
Collaborator Author

lmac-1 commented Jan 26, 2026

@doc-han thanks for the review. I've fixed the comments referring to the PR. Can you check it again?

Also re the sorting function, I asked my good friend Claude and it seems to think that it "ensures disabled edges render behind enabled edges in the SVG." So I think it's best if we keep it in!

@lmac-1 lmac-1 requested a review from doc-han January 26, 2026 21:43
@doc-han
Copy link
Contributor

doc-han commented Jan 27, 2026

@doc-han thanks for the review. I've fixed the comments referring to the PR. Can you check it again?

Also re the sorting function, I asked my good friend Claude and it seems to think that it "ensures disabled edges render behind enabled edges in the SVG." So I think it's best if we keep it in!

Yeah makes sense. I was more concerned as to why the line had return a.selected - b.selected; because with the sort function, getting there meant the two values being compared are equal, so I'm wondering why we needed to sort based on another criteria which is .selected

@lmac-1
Copy link
Collaborator Author

lmac-1 commented Jan 27, 2026

@doc-han are there any further changes needed here or can this be approved?

@doc-han
Copy link
Contributor

doc-han commented Jan 27, 2026

Hi @lmac-1 sorry for the delay. I was doing a deep dive into this.

Why sortOrderForSvg

  1. React flow assigns z-index to edges based on their order in the edges array. eg. [edge1, edge2] edge2 will have a higher z-index
  2. Hence, the sort function sortOrderForSvg existed to make sure that the selected edge will always be at the bottom of the edges array. so that when two edges overlap, selecting either will always put it above the other.

Side Effect of sortOrderForSvg with Dagre

We then had a side-effect, Dagre(a library we use for calculating layouts) also depends on the order of edges to determine which edge/node shows left-most when a parent has more than one children.

So the function sortOrderForSvg actually existing for solving z-index but had a side-effect of altering how Dagre also calculated its positioning.

This solution

The solution in this PR tries to make Dagre satisfied by ensuring that edges always reach Dagre in the same order but then breaks the z-indexing reason why sortOrderForSvg existed.

Regression on this branch

When two edge labels overlap on this branch, the selected one doesn't get z-index precedence.

Existing Regression on main

Looking at the logic in sortOrderForSvg, it tells me the problem also exists when you enable/disable an edge, the nodes will shift. Hence, that's something we also need to take care of.

Proposed Solution

We handle the z-index well and get rid of sortOrderForSvg. I think this is a problem react-flow is also realizing recently, so they just added a new way of handling z-indexing. This will require updating our version of react-flow and using the new z-index feature they've introduced. This will solve both problems.

zIndexMode in ReactFlow

@doc-han
Copy link
Contributor

doc-han commented Jan 28, 2026

In short, this change swaps out the bug for another one by breaking the below feature.

CPT2601281336-539x301 (1)

It also doesn't handle the case for when the edge is being enabled/disabled which also gets us to the same issue.

CPT2601281340-826x776

@lmac-1 let me know whether you'll want to take on this, else I could pick it up.

@lmac-1
Copy link
Collaborator Author

lmac-1 commented Jan 28, 2026

@doc-han great catch! I think it would be better if you could pick this up if you have time. I'm flying to the UK tonight and won't be online much more this week.

@doc-han
Copy link
Contributor

doc-han commented Jan 28, 2026

@doc-han great catch! I think it would be better if you could pick this up if you have time. I'm flying to the UK tonight and won't be online much more this week.

Safe flight. I'll pick it up then

@lmac-1
Copy link
Collaborator Author

lmac-1 commented Jan 28, 2026

Thanks a lot @doc-han - I appreciate the deep dive on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

Shifty Path and Step Behavior on Auto-Layout Mode

3 participants