Bug/4912 GitGraph routing and colouring for merges and cherry-picks #4927

guypursey · 2023-10-07T18:10:46Z

📑 Summary

Fixes issues with colouring and routing of merges in GitGraph.

Resolves #4912

📏 Design Decisions

Changed commit variable names commit1 and commit2 to commitA and commitB respectively, to prevent confusion with seq property values (which are also numbers and used to ID the commits).
Refactored hasOverlappingCommits function to be easier to read and renamed it shouldRerouteArrow:
- Used Array.some() instead of Array.filter() (should increase performance with larger graphs, if needed)
- Swapped out Object.keys() for Object.values()
- It takes a new argument: the name of the branch which would receive the arrow's curve and thereby causing issues for overlapping commits.
Introduced branch colour assignments to each conditional check for routing.
New e2e tests (many based on reviews by @nirname and @mathbraga)
Added more sample diagrams to demo folder show how the graphs should look in different situations.

📋 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.
🔖 targeted develop branch

netlify · 2023-10-07T18:10:50Z

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	`3df7cf2`
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/65552da1a1ac44000882c681
😎 Deploy Preview	https://deploy-preview-4927--mermaid-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

codecov · 2023-10-07T18:14:26Z

Codecov Report

Merging #4927 (3df7cf2) into develop (e8ee5f5) will decrease coverage by 0.77%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4927      +/-   ##
===========================================
- Coverage    80.13%   79.37%   -0.77%     
===========================================
  Files          164      164              
  Lines        13864    13869       +5     
  Branches       698      698              
===========================================
- Hits         11110    11008     -102     
- Misses        2604     2711     +107     
  Partials       150      150

Flag	Coverage Δ
e2e	`85.28% <100.00%> (-0.97%)`	⬇️
unit	`42.91% <ø> (ø)`

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

Files	Coverage Δ
...kages/mermaid/src/diagrams/git/gitGraphRenderer.js	`98.18% <100.00%> (+0.03%)`	⬆️

... and 9 files with indirect coverage changes

guypursey · 2023-10-07T21:58:21Z

E2E tests passing locally but just one test is failing remotely (other/interactive.spec).

Took the opportunity to reword some commit messages via rebase interactive and force pushed changes to retrigger test run.

guypursey · 2023-10-09T17:16:46Z

@nirname Not sure of etiquette re requesting approval but this one is ready for review now 🙂

nirname · 2023-10-11T10:07:07Z

@guypursey No problem at all, ask for review as many times as needed.

I saw you have added demo page. That is good, but not enough. We also need to ensure that the following changes won't break this one. Code Coverage complains about that line you've added. Perhaps you need to add another end-2-end test on top of the existing ones. Integration tests basically make a screenshot and compare it when you do the changes. A couple of samples to cover both p1.y > p2.y and the opposite p1.y <= p2.y would be enough. Please check if this issue is reproduced for vertical orientation (TB, top-bottom) of gitGraph also.

nirname

We also need to check this solution against TB graph

packages/mermaid/src/diagrams/git/gitGraphRenderer.js

guypursey · 2023-10-13T09:39:55Z

@nirname @mathbraga I have made a few changes since your last reviews and updated the PR description at the top.

It looks like code coverage has now increased slightly from the develop branch since my recent changes -- I think the existing integration tests should cover what's needed anyway. When I run Cypress locally, the only changes I see seem to be what I was trying to fix. Let me know if you spot anything else though.

mathbraga · 2023-10-13T19:47:05Z

Hey @guypursey, all looks good for my reviews. I think only @nirname can resolve conversations though.

nirname · 2023-10-15T00:28:30Z

Your code breaks something. I have faced this rendering issue

While current version works fine

gitGraph
      commit
      commit
      branch feature-001
      commit
      commit
      checkout main
      branch feature-002
      commit
      checkout feature-001
      merge feature-002

guypursey · 2023-10-15T12:59:53Z

@nirname Good catch.

I've added 2x e2e tests (and updated demos) as the scenario you mentioned wasn't being tested.
I've removed a line of code and the condition that was causing the issue.

Ready for review 👍

mathbraga · 2023-10-16T22:12:05Z

@guypursey You missed the gitGraph TB: on the first line of your second test body (test 35) for the vertical render.

guypursey · 2023-10-17T07:52:53Z

You missed the gitGraph TB: on the first line of your second test body (test 35) for the vertical render.

Ah, thanks @mathbraga. Well spotted. I've updated that now.

In looking again, I realised I hadn't specified the commit IDs in each new test. This meant the IDs would show up in diff output as they would change between Cypress runs.

So I've now followed the practice of other graphs and added an ID for each commit 👍

guypursey · 2023-10-19T08:01:19Z

@nirname I've just rebased this PR from develop branch to ensure coverage is still okay.

I think I've addressed your request now, so ready for review again when you have time 👍

nirname

@guypursey Sorry for long response. I have discovered that unnecessary routing still appears in your branch, while it does not appear in current release

Expected behaviour (and current release behavior)

Current behaviour (in this branch only)

I know it seems to be minor, nevertheless users rely on the layout, and if it changes suddenly their docs may be ruined. We have measure thrice. Try this code to reproduce:

gitGraph:
  commit
  branch x
  checkout main
  commit
  checkout x
  commit
  checkout main
  merge x

Colors works as expected, by the way. I am not sure why it changes layout. May be this happens because you have changed shouldRerouteArrow function (formerly hasOverlappingCommits).

Try investigating it further.

packages/mermaid/src/diagrams/git/gitGraphRenderer.js

nirname · 2023-10-22T15:37:25Z

We are almost there. Good to go after fixing some layout problems. Great job.

@nirname

In these tests, a new branch is created but then a commit is made on the main branch before the new branch gets a commit. This important to see what happens with rerouting of arrows. Suggested by @nirname in PR review of mermaid-js#4927.

@nirname

In these tests, a new branch is created but then a commit is made on the main branch before the new branch gets a commit. This important to see what happens with rerouting of arrows. Suggested by @nirname in PR review of mermaid-js#4927.

On previous rewrite, I had created new functions within the overlapping functions but these were being recreated on each iteration of Object.some(). I moved them outside this for clarity and so they're not recreated each iteration.

The function also now does an additional check to see if source branch in overlap check is on main. As we're no longer purely checking for an overlap and the only use of this function is to reroute the arrows to avoid running over commits, this more literal name should be clearer.

@nirname

Checking if branch was same as main turned out to be redundant for now, since there don't seem to be any cases where routing curves into main. This fixes issue found in review by @nirname and avoids a situation where branching from the same commit results in unnecessary rerouting.

@mathbraga

Fix for error spotted by @mathbraga

Followed practice of other tests so that commit IDs are stabilised (i.e., not randomly generated) and therefore don't show repeatedly in Cypress diff output screenshots

@nirname

In these tests, a new branch is created but then a commit is made on the main branch before the new branch gets a commit. This important to see what happens with rerouting of arrows. Suggested by @nirname in PR review of mermaid-js#4927.

Hypothesised that working out which branch needed checking for overlapping commits might be missing, so added that as a nested ternary and passed result as new argument to rerouting check. If commits are found on the branch which will be getting the curve (whichever branch is lower or more to the right of main than the other, for now), then the arrow will be rerouted. I may refactor in a follow-up commit and I think there's scope to simplify the logic but this is a test for now.

My focus earlier on had been on relationship to `main` branch so this is to ensure that we have some tests that cover relationship between a pair of branches that doesn't include `main`.

@nirname

Based on review by @nirname. I had originally been trying to minimise number of new arguments being passed to rerouting check but as the branch curve check is not used elsewhere and is part of the same rerouting check it makes sense for them to be together. Position information now passed to rerouting fn instead.

Should stop randomised commit IDs from showing arbitrary differences between test runs.

Wanted to avoid repetition given that the originally nested ternaries had the same structure

Pre-commit lint hook had made the use of ternaries harder to read than I'd originally intended so I introduced an additional variable which explains what is being checked and keeps ternaries from becoming obscured.

sidharthv96

@guypursey PR looks wonderful!

@nirname can you resolve the conversations if you're satisfied with the responses?

guypursey force-pushed the bug/4912_gitgraph-merge-routing-colouring branch from c4843e0 to 2a564f0 Compare October 7, 2023 18:25

guypursey force-pushed the bug/4912_gitgraph-merge-routing-colouring branch from 2a564f0 to d601a88 Compare October 7, 2023 21:58

guypursey mentioned this pull request Oct 9, 2023

GitGraph - line routing and colour for adjacent merges #4912

Closed

guypursey mentioned this pull request Oct 11, 2023

GitGraph: line routing and colouring for branches above main #4932

Open

nirname requested changes Oct 11, 2023

View reviewed changes

packages/mermaid/src/diagrams/git/gitGraphRenderer.js Outdated Show resolved Hide resolved

packages/mermaid/src/diagrams/git/gitGraphRenderer.js Outdated Show resolved Hide resolved

mathbraga reviewed Oct 11, 2023

View reviewed changes

packages/mermaid/src/diagrams/git/gitGraphRenderer.js Outdated Show resolved Hide resolved

packages/mermaid/src/diagrams/git/gitGraphRenderer.js Outdated Show resolved Hide resolved

guypursey force-pushed the bug/4912_gitgraph-merge-routing-colouring branch 2 times, most recently from 8e1de27 to 14d585e Compare October 12, 2023 20:39

guypursey force-pushed the bug/4912_gitgraph-merge-routing-colouring branch from 7922dc9 to 39ae754 Compare October 19, 2023 07:18

guypursey requested a review from nirname October 19, 2023 08:02

guypursey changed the title ~~Bug/4912 gitgraph merge routing colouring~~ Bug/4912 GitGraph routing and colouring for merges and cherry-picks Oct 19, 2023

nirname requested changes Oct 22, 2023

View reviewed changes

packages/mermaid/src/diagrams/git/gitGraphRenderer.js Show resolved Hide resolved

guypursey force-pushed the bug/4912_gitgraph-merge-routing-colouring branch from 39ae754 to a8e544f Compare October 24, 2023 20:30

guypursey requested a review from nirname October 25, 2023 08:59

guypursey force-pushed the bug/4912_gitgraph-merge-routing-colouring branch from abd00b4 to dc2fa29 Compare October 26, 2023 08:36

guypursey added 19 commits October 31, 2023 17:24

GitGraph: added destination branch check into overlapping fn.

8c43d22

GitGraph: Rewrote overlap fn to make main branch exception to rule.

9f8457d

GitGraph: added and updated demos to align with some of e2e testing

902a9dd

GitGraph: simplified overlapping check fn

dba7197

GitGraph: added more to demos

3ba33c8

GitGraph: added e2e tests for multiple branches from one commit

25a678c

GitGraph: fixed an e2e branch for vertical branch

b0b2768

Fix for error spotted by @mathbraga

GitGraph: e2e tests, added commit IDs to test graphs

2acad55

Followed practice of other tests so that commit IDs are stabilised (i.e., not randomly generated) and therefore don't show repeatedly in Cypress diff output screenshots

GitGraph: added 2x e2e tests for branches not used immediately

f7d83e2

My focus earlier on had been on relationship to `main` branch so this is to ensure that we have some tests that cover relationship between a pair of branches that doesn't include `main`.

GitGraph: corrected minor typo in comment.

31a8e04

GitGraph: added commit IDs to e2e test to remove false positives

d80e1a2

Should stop randomised commit IDs from showing arbitrary differences between test runs.

GitGraph: simplified branch check in arrow rerouting fn

a9c5d90

Wanted to avoid repetition given that the originally nested ternaries had the same structure

GitGraph: made reroute fn more readable

57a9d73

Pre-commit lint hook had made the use of ternaries harder to read than I'd originally intended so I introduced an additional variable which explains what is being checked and keeps ternaries from becoming obscured.

guypursey force-pushed the bug/4912_gitgraph-merge-routing-colouring branch from 22fd12d to 57a9d73 Compare October 31, 2023 17:24

Merge branch 'develop' into bug/4912_gitgraph-merge-routing-colouring

fb031ee

nirname added the Type: Bug / Error Something isn't working or is incorrect label Nov 13, 2023

nirname requested review from sidharthv96 and aloisklink November 13, 2023 20:23

Merge branch 'develop' into bug/4912_gitgraph-merge-routing-colouring

52603d7

sidharthv96 approved these changes Nov 15, 2023

View reviewed changes

Merge branch 'develop' into bug/4912_gitgraph-merge-routing-colouring

3df7cf2

nirname added this pull request to the merge queue Nov 15, 2023

Merged via the queue into mermaid-js:develop with commit d7f59c1 Nov 15, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/4912 GitGraph routing and colouring for merges and cherry-picks #4927

Bug/4912 GitGraph routing and colouring for merges and cherry-picks #4927

guypursey commented Oct 7, 2023 •

edited

Loading

netlify bot commented Oct 7, 2023 •

edited

Loading

codecov bot commented Oct 7, 2023 •

edited

Loading

guypursey commented Oct 7, 2023

guypursey commented Oct 9, 2023

nirname commented Oct 11, 2023

nirname left a comment

guypursey commented Oct 13, 2023 •

edited

Loading

mathbraga commented Oct 13, 2023

nirname commented Oct 15, 2023 •

edited

Loading

guypursey commented Oct 15, 2023 •

edited

Loading

mathbraga commented Oct 16, 2023

guypursey commented Oct 17, 2023 •

edited

Loading

guypursey commented Oct 19, 2023 •

edited

Loading

nirname left a comment

nirname commented Oct 22, 2023

sidharthv96 left a comment

Bug/4912 GitGraph routing and colouring for merges and cherry-picks #4927

Bug/4912 GitGraph routing and colouring for merges and cherry-picks #4927

Conversation

guypursey commented Oct 7, 2023 • edited Loading

📑 Summary

📏 Design Decisions

📋 Tasks

netlify bot commented Oct 7, 2023 • edited Loading

✅ Deploy Preview for mermaid-js ready!

codecov bot commented Oct 7, 2023 • edited Loading

Codecov Report

guypursey commented Oct 7, 2023

guypursey commented Oct 9, 2023

nirname commented Oct 11, 2023

nirname left a comment

Choose a reason for hiding this comment

guypursey commented Oct 13, 2023 • edited Loading

mathbraga commented Oct 13, 2023

nirname commented Oct 15, 2023 • edited Loading

guypursey commented Oct 15, 2023 • edited Loading

mathbraga commented Oct 16, 2023

guypursey commented Oct 17, 2023 • edited Loading

guypursey commented Oct 19, 2023 • edited Loading

nirname left a comment

Choose a reason for hiding this comment

nirname commented Oct 22, 2023

sidharthv96 left a comment

Choose a reason for hiding this comment

guypursey commented Oct 7, 2023 •

edited

Loading

netlify bot commented Oct 7, 2023 •

edited

Loading

codecov bot commented Oct 7, 2023 •

edited

Loading

guypursey commented Oct 13, 2023 •

edited

Loading

nirname commented Oct 15, 2023 •

edited

Loading

guypursey commented Oct 15, 2023 •

edited

Loading

guypursey commented Oct 17, 2023 •

edited

Loading

guypursey commented Oct 19, 2023 •

edited

Loading