Repository navigation
Fix non-markdown labels in flowcharts being treated like markdown - #7276
Conversation
🦋 Changeset detectedLatest commit: dc7e7d8 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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
…anges. on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
78264fc to
105a43d
Compare
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
105a43d to
8491523
Compare
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
…tion on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…ext variable on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…d-js/mermaid into markdown-specific-changes
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
This reverts commit 99be550.
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…dering" This reverts commit 443ff41.
… width handling on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…tion on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…onfiguration" This reverts commit 9bbb075.
…nd text on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot] Review of PR #7276
What's working well
🎉 [praise] Great architectural approach — switching from "everything is markdown by default" to explicit opt-in via labelType is the right direction. The strategy of adding labelType: 'markdown' to all other diagram DBs (class, ER, kanban, mindmap, requirement, state) preserves their existing behavior while letting flowcharts handle label types correctly. This is a clean, defensive change.
🎉 [praise] Excellent PR description. The "Why This Is An Issue" section with concrete examples of how markdown misinterprets plain text (> something → blockquote, - x → list item, 1. → ordered list) makes the regression immediately clear. This is the kind of write-up that makes reviews much easier.
🎉 [praise] Good scoping discipline — limiting the fix to flowcharts with an explicit note about future work keeps this PR reviewable and reduces regression risk.
🎉 [praise] The new e2e test for edge label auto-wrapping (flowchart-v2.spec.js) testing multiple config combinations (markdownAutoWrap × htmlLabels) is thorough. The flowchart.spec.js additions covering mixed string/markdown labels and subgraph headings with list-like text ("1. first", "2. second") directly target the reported regression.
Things to address
🟡 [important] — handle-markdown-text.spec.ts:205: The skipped unit test ("No auto wrapping") and the commented-out e2e config {markdownAutoWrap: false, htmlLabels: false} in flowchart-v2.spec.js:82 both have TODO comments but no tracking issues. These tend to get lost without a filed issue. Would you be open to filing a quick follow-up issue to track the markdownAutoWrap: false, htmlLabels: false bug? The code comment in handle-markdown-text.ts:20-22 explains the root cause (splitWordToFitWidth splits even when spaces should be preserved), which is great context for whoever picks it up.
🟡 [important] — Two separate changesets (curvy-cases-battle.md and swift-cloths-run.md) for a single PR is a bit unusual. The first describes the markdownAutoWrap fix (which is really a side effect of the main fix), and the second describes the main fix. Since they'll both produce separate changelog entries for the same patch bump, it might be cleaner to consolidate into one changeset that covers both aspects. Not blocking, but worth considering.
🟢 [nit] — Agreeing with @aloisklink's earlier suggestion on sanitizeNodeLabelType (flowDb.ts:89). The name implies security sanitization, but it's really type validation/coercion. castLabelType or validateLabelType would communicate the intent more clearly. Fine as a follow-up.
💡 [suggestion] — nonMarkdownToHTML (handle-markdown-text.ts:96) wraps output in <p> tags, and block/styles.ts adds .edgeLabel p { display: inline } to compensate. The flowchart styles.ts doesn't have this rule — if non-markdown edge labels in flowcharts render with unexpected block-level spacing, this could be why. The Argos screenshots should catch it, so this may already be fine, but worth a quick check.
Summary
Solid fix for a significant v10 → v11 regression. The approach is architecturally sound and well-tested. The shared rendering-util changes (createText.ts, handle-markdown-text.ts, clusters.js, edges.js, shapes/util.ts) are the highest-risk area, but the explicit labelType: 'markdown' additions to other diagram types provide a good safety net. The Argos visual diffs (5 changed, 4 added) are the key signal for cross-diagram regression — as long as those look expected, this is in great shape.
…width configuration
…s tests on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
aloisklink
left a comment
There was a problem hiding this comment.
Your latest changes, where we autowrap plain text labels in flowcharts, but not plain text labels in other diagram types, looks okay to me too.
I'm a bit worried that some v10 users might complain about it--especially since we don't have an equivalent markdownAutoWrap: false setting to disable this--but I do agree with you that it'll probably prevent less complaints with people that are used to the post-v11 system of linewrapping by default, and we can always considering changing it later if there are complaints about it.
|
@darshanr0107 I think this PR is a step in the right direction, and we should follow-up markdown & autowrap handling for other diagrams in separate PR. I will merge this now. |
📑 Summary
This PR fixes a regression introduced in #5604 (Mermaid
v11.0.0) where all flowchart labels were being treated as markdown by default, causing rendering issues and breaking backwards compatibility with Mermaidv10diagrams. The fix ensures that only labels explicitly marked as markdown (e.g.node["`_markdown_ **text**`"]) are processed as markdown, while plain text labels are rendered as regular text.Note
This PR currently focuses only on flowcharts. Other diagram types will be handled in future work.
Tip
If you want markdown formatting, you can still use markdown in your flowchart labels by using the proper markdown syntax.
Wrap your markdown text with double quotes and backticks:
node["`_markdown_ **text**`"]Example:
Resolves #5824
Related to #6048(fixes the issue for flowcharts only)
Related to #6275(fixes the issue for flowcharts only)
Related PR's #6345 , #6087
Why This Is An Issue
When all labels are treated as markdown by default, several critical problems arise:
1. Line Wrapping Issues
2. Text Interpretation Errors
> somethingand- somethingcan be valid math expressions (greater-than or minus),but markdown would instead render them as a quote/unordered list item.50. xand100. yare often numeric expressions, but Markdown will auto-renumber them (e.g.,50. x→50. x,100. y→51. y).3. Backwards Compatibility Broken
v10diagrams that worked perfectly now fail or render incorrectlyv11: https://gitlab.com/gitlab-org/gitlab/-/issues/491514📏 Design Decisions
1. Label Type Propagation Through the Rendering Pipeline
labelTypefrom parser → database → rendering functions2. Dual Rendering Paths Based on Label Type
createText()(markdown) andcreateLabel()(plain text)createText()handles markdown processing with wrapping and parsingcreateLabel()handles plain text with proper multiline support3. Markdown Flag in createText()
markdownboolean parameter with defaulttruelabelType4. Automatic Line Wrapping for plain text
Flowchart-Specific Fix (Why Not All Diagram Types)
As flowcharts are the most common mermaid diagram, fixing this for flowcharts first would have the highest amount of impact. And additionally, since the grammar and documentation already distinguish between markdown
("`markdown`")and plain text"plain text", it's also the least amount of effort for both us and users to fix.I've made sure that all other diagram types remain using their current behaviour, so we can fix them in separate PRs.
📋 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:.