Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 48f388c 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 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughKaTeX-enabled flowchart labels now preserve ChangesKaTeX label line breaks
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 #7962 +/- ##
===========================================
+ Coverage 77.51% 77.63% +0.11%
===========================================
Files 564 564
Lines 74906 74906
Branches 14617 12667 -1950
===========================================
+ Hits 58065 58152 +87
+ Misses 15842 15758 -84
+ Partials 999 996 -3
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 ↗︎
|
The KaTeX branch in createText bypasses markdownToHTML/nonMarkdownToHTML, so hard line breaks in a label source never became <br/>, and renderKatexSanitized -- which splits its input on <br/> -- saw a single line. Apply the same \n -> <br/> rule markdownToHTML uses for plain-text tokens, rather than running the label through the markdown lexer, which would wrap it in <p> and leave the tags split across separate <div>s. Diagnosis of the inputForKatex path is due to @omkarht in mermaid-js#7202. Resolves mermaid-js#7194 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Extended to also cover #7194I've pushed a follow-up commit that extends this PR to fix #7194 as well. Some context on why the I measured the four relevant configurations with a
Why they're the same root cause.
Row 3 of the table is worth flagging: because of (1), the fix proposed in #7202 currently has no On the implementation. I used Multi-line math inside a single Two more unit tests cover the hard-line-break case and a math-then-hard-break case. Full unit |
<br/> line breaks in labels containing KaTeX math|
Hi @YBJ0000 Some input: [sisyphos-bot] What's working well 🎉 The fix is minimal and surgical — two lines changed, zero risk to non-math labels. 🎉 The four new unit tests cover exactly the reported failure modes: 🎉 The comment on the inputForKatex change clearly explains why the conversion is needed — "KaTeX labels bypass the markdown/non-markdown conversion above" — which is non-obvious and exactly the 🎉 Removing the unused common default import is clean housekeeping. Things to address 🟡 [important] — Missing Cypress visual regression test Per repo conventions, changes to rendering-util/ require E2E visual tests (cypress/integration/rendering/ with imgSnapshotTest()). The existing cypress/integration/rendering/katex.spec.js has no
It would be great to add 1–2 test cases to katex.spec.js: Security No XSS or injection issues introduced. The \n → |
📑 Summary
In flowchart labels, line breaks stopped working as soon as the label also contained a
$$…$$KaTeX expression.Input<br/>$$x$$rendered as a single lineInputx, and a hard-wrapped label such ascollapsed onto one line too — while the same labels without math broke into separate lines as expected.
Resolves #7873
Resolves #7194
📏 Design Decisions
renderKatexUnsanitizedalready handles multi-line labels: it splits the text on<br/>and wraps each line in its own<div>(this is how sequence diagrams render multi-line KaTeX labels today). Two separate things prevented labels from reaching it in that shape.<br/>was destroyed before rendering (#7873).addHtmlSpanreplaced every<br/>with\nbefore callingrenderKatexSanitized. After that replacement the split never finds anything, the whole label lands in one<div>, and the literal newline renders as nothing in HTML. Fixed by passing the label through unchanged.Hard line breaks never became
<br/>(#7194). The flowchart lexer (<string>[^"]+inflow.jison) returns node text verbatim, so a hard-wrapped label arrives atcreateTextwith literal\n. For labels without math,markdownToHTML/nonMarkdownToHTMLconvert those to<br/>, but the KaTeX branch bypasses both (label: hasKatex(text) ? inputForKatex : …). Fixed by applying the same\n→<br/>rule thatmarkdownToHTMLuses for plain-text tokens.The second half of this fix follows @omkarht's diagnosis in #7202 — credit to them for locating the
inputForKatexpath. I used the narrowreplace(/\n */g, '<br/>')rather than running the label throughmarkdownToHTML, because the latter wraps the label in<p>, and sincerenderKatexUnsanitizedthen splits on<br/>, the opening and closing<p>tags end up in different<div>s. Note also that #7202 cannot take effect on its own: on currentdevelopthe<br/>it produces are immediately converted back to\nbyaddHtmlSpan, so the two edits cancel out. Removing thatreplaceis the prerequisite.Multi-line math inside a single
$$…$$is unaffected —katexRegexhas nosflag, so such text is not detected as KaTeX and never enters this branch.Verified in the demo server that both forms now render on separate rows, matching the control diagrams without math, and that single-line math labels are unaffected.
Four unit tests cover: a text line + math line via
<br/>, two math lines via<br/>, a hard-wrapped label ending in math, and math followed by a hard line break.📋 Tasks
Make sure you
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Review Summary
🎉 Fix correctly preserves
<br/>breaks in KaTeX flowchart node and edge labels by passing the original label (with<br/>intact) to the existing KaTeX line-splitting/rendering logic.🎉 Added focused unit tests covering both text-plus-math (
Input<br/>$$x$$) and multiple math-only lines ($$x$$<br/>$$f(x)$$), verifying each<br/>-separated line renders as its own row.🎉 Included a changeset recording the behavior fix.
No blocking or important issues identified; sanitization is maintained via
renderKatexSanitized.Verdict: APPROVE