Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(flow): fix invalid ellipseText regex #5016

Merged

Conversation

aloisklink
Copy link
Member

📑 Summary

Fixes an invalid regex that was causing flowcharts to freeze if there was an ellipse node that had a ( character in the text.

E.g.

The following diagram freezes in Mermaid v10.6.0

```mermaid
flowchart
  X(- My Text (
```

Resolves #4964

📏 Design Decisions

I'm not 100% sure why this was causing a freeze (shouldn't JISON throw an error instead?), but this seems to fix the issue, so 🤷.

As a side-note, if this test freezes, it completely locks up Vitest (setting a timeout parameter doesn't seem to help). We could instead run this test in a child_process, but since creating a child process is pretty slow normally, so it's probably not worth slowing down our unit tests for it.

📋 Tasks

Make sure you

This invalid regex was causing Mermaid to freeze.
Copy link

netlify bot commented Nov 6, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 172d90e
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/6548be1771ef730008ccffc5
😎 Deploy Preview https://deploy-preview-5016--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 site configuration.

@github-actions github-actions bot added the fix label Nov 6, 2023
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #5016 (172d90e) into develop (ff4d68f) will increase coverage by 0.34%.
Report is 17 commits behind head on develop.
The diff coverage is 66.66%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5016      +/-   ##
===========================================
+ Coverage    79.37%   79.72%   +0.34%     
===========================================
  Files          164      164              
  Lines        13815    13820       +5     
  Branches       693      693              
===========================================
+ Hits         10966    11018      +52     
+ Misses        2700     2653      -47     
  Partials       149      149              
Flag Coverage Δ
e2e 85.67% <66.66%> (+0.43%) ⬆️
unit 42.94% <ø> (ø)

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

Files Coverage Δ
packages/mermaid/src/diagrams/flowchart/flowDb.js 83.92% <66.66%> (+1.87%) ⬆️

... and 6 files with indirect coverage changes

Copy link
Collaborator

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

LGTM, Thanks for adding the test

@knsv knsv added this pull request to the merge queue Nov 6, 2023
Merged via the queue into mermaid-js:develop with commit 7c7f3dd Nov 6, 2023
16 of 17 checks passed
@aloisklink aloisklink deleted the fix/4964-fix-invalid-ellipseText-regex branch November 6, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application Crash due to (--> Syntax
2 participants