Repository navigation
Conversation
lineBreakRegex is global so that sanitizeText and splitBreaks can use it, but hasBreaks and wrapLabel checked it with .test(), which advances and keeps lastIndex. The next check therefore started mid-string and missed a <br/> placed before that offset, so wrapLabel re-wrapped a label whose line breaks the author had written by hand. Closes mermaid-js#8037
🦋 Changeset detectedLatest commit: 709e383 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
❌ Deploy Preview for mermaid-js failed.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMermaid now detects existing ChangesLabel break preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8038 +/- ##
===========================================
+ Coverage 77.61% 77.68% +0.07%
===========================================
Files 568 568
Lines 74992 74992
Branches 12690 12691 +1
===========================================
+ Hits 58202 58255 +53
+ Misses 15796 15744 -52
+ Partials 994 993 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
Superseded — a6ec7ff ( I checked out |
📑 Summary
Resolves #8037
lineBreakRegexis declared global (/<br\s*\/?>/gi) becausesanitizeTextandsplitBreaksneed it that way, but two call sites used it with.test():hasBreaks()inpackages/mermaid/src/diagrams/common/common.tswrapLabel()inpackages/mermaid/src/utils.ts, in the early return that leaves an already-broken label aloneRegExp.prototype.teston a global regex advanceslastIndexand leaves it set. The next check then starts mid-string, so a<br/>sitting before that offset is not found,wrapLabelfalls through to its word-wrapping branch, and it rewrites a label the author had already broken by hand.In a sequence diagram this means the message after a message containing a
<br/>loses its line break and is re-flowed:aaa bbb ccc ddd eee fff ggg hhh iii jjj/kkklll/mmm nnn ooo ppp/qqq rrr sss ttt uuu/vvvlll/mmm nnn ooo ppp qqq rrr sss ttt uuu vvv📏 Design Decisions
hasBreaksnow usesString.prototype.search, which ignoreslastIndexand restores it, so the shared regex keeps working for thereplace/splitcallers that rely on the global flag.wrapLabelcallscommon.hasBreaks(label)instead of reaching for the regex itself, so there is one place left that decides whether a label already has breaks.The alternative — a second, non-global copy of the pattern — would put the
<br>pattern in two places, and clearinglastIndexbefore eachtestwould still leave it dirty for the next caller.📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Documentation is unchanged — this restores the documented behaviour of
<br/>rather than adding to it.Tests added, each failing on
developand passing here:hasBreaksgives the same answer on repeated calls (common.spec.ts)wrapLabelleaves every label with line breaks untouched, not just the first one (utils.spec.ts)vitest runis green: 226 files, 5091 passed / 16 skipped / 2 todo.pnpm lintis clean.What's working well
.test()calls withsearch()inhasBreaks().wrapLabel()usecommon.hasBreaks().<br/>breaks.Review result
✅ Approve
The fix addresses stale
lastIndexstate and preserves explicit breaks across wrapped labels. No API or documentation changes are introduced.