feat: add support for inline markdown links in text rendering#7470
feat: add support for inline markdown links in text rendering#7470taoqf wants to merge 19 commits intomermaid-js:developfrom
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 72441f4 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 |
@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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7470 +/- ##
==========================================
- Coverage 3.45% 3.44% -0.02%
==========================================
Files 518 518
Lines 51036 51203 +167
Branches 795 795
==========================================
Hits 1762 1762
- Misses 49274 49441 +167
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 ↗︎
|
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this contribution, @taoqf! Adding inline markdown link support is a useful feature. I have some concerns about security, theming, and test coverage that I'd like to work through before this can land — especially since this touches shared rendering-util/ code that affects all diagram types.
File Triage
| Tier | Count | Files |
|---|---|---|
| Tier 1 (full read) | 5 | createText.ts, handle-markdown-text.ts, handle-markdown-text.spec.ts, splitText.ts, types.ts |
| Tier 2 (diff + context) | 2 | svgDraw.js, sequenceDiagram.spec.js |
| Tier 3 (diff only) | 2 | changeset, src/docs/syntax/sequenceDiagram.md |
| Skip (generated) | 2 | docs/config/setup/mermaid/interfaces/LayoutData.md, docs/syntax/sequenceDiagram.md |
What's working well
🎉 [praise] Good use of @braintree/sanitize-url for URL sanitization, and the tests explicitly verify that javascript: and data: URLs are replaced with about:blank. This is the right library for the job.
🎉 [praise] Thorough unit tests in handle-markdown-text.spec.ts — the markdownToLines and markdownToHTML test cases cover single links, multiple links, mixed formatting, and malicious URLs. Nice coverage.
🎉 [praise] The splitText.ts changes correctly propagate the href field through the word-splitting recursion, preserving link context when text wraps.
Things to address
Blocking
🔴 [blocking] No E2E visual regression tests. This PR changes shared rendering code in rendering-util/ (createText.ts, handle-markdown-text.ts, splitText.ts, types.ts) which affects all diagram types that use markdown text — not just sequence diagrams. Additionally, the sequence diagram renderer (svgDraw.js) now renders clickable <a> elements, which is a visual change. Per project conventions, renderer and shared rendering-util changes require E2E tests using imgSnapshotTest() in cypress/integration/rendering/. At minimum, add a sequence diagram E2E test showing links rendering correctly.
Security
🟡 [important] Unescaped href in markdownToHTML string interpolation. In handle-markdown-text.ts, the new link handling builds HTML via template literal:
return `<a href="${href}" target="_blank">${text}</a>`;While sanitizeUrl() blocks dangerous URL schemes (javascript:, data:, etc. — including mixed-case, entity-encoded, and whitespace-padded variants), it does not HTML-encode special characters. A URL containing " could theoretically break out of the href attribute and inject additional attributes. DOMPurify sanitizes the final output (which mitigates this), but building malformed HTML before sanitization is a fragile pattern — it relies on DOMPurify parsing the broken HTML exactly the same way a browser would (mXSS risk).
Suggested fix: HTML-encode the href before interpolation:
const safeHref = href.replace(/&/g, '&').replace(/"/g, '"');
return `<a href="${safeHref}" target="_blank">${text}</a>`;Note: The D3 paths in createText.ts and svgDraw.js are safe — D3's .attr() properly encodes attribute values.
Important
🟡 [important] Hardcoded link color #0d47a1. This hex value appears in two places:
createText.ts(the diff'supdateTextContentAndStylesfunction):.attr('fill', '#0d47a1')svgDraw.js(thedrawTextfunction):.attr('fill', '#0d47a1')
This hardcoded color will look wrong on dark themes and won't respond to theme customization. It should use a theme variable instead. Look at how other diagrams reference themeVariables.textColor or similar — the link color could map to an existing variable or a new one. At minimum, the color should be consistent with the theme's color palette.
🟡 [important] Broad scope with narrow testing. The changes to handle-markdown-text.ts, createText.ts, splitText.ts, and types.ts affect ALL diagram types that render markdown labels (flowchart, class, state, mindmap, etc.), but the tests only exercise sequence diagrams. A link in a flowchart node label (node["\Check docs`"]) would also go through the modified markdownToLines/createText` pipeline. Even if you don't add tests for every diagram, it would be great to acknowledge this scope in the PR description and verify manually that a few other diagram types don't regress.
🟡 [important] Duplicate link rendering logic. Links are rendered in two independent places:
handle-markdown-text.ts+createText.ts— the shared markdown pipeline (used by all diagram types with markdown labels)svgDraw.js— sequence-diagram-specific regex-based rendering
This duplication means link rendering behavior could diverge (different regex patterns, different sanitization, different styling). Is the svgDraw.js path needed, or does the shared pipeline already handle sequence diagram messages? If sequence diagrams use a separate text rendering path that bypasses createText.ts, this is understandable — but it should be documented in the PR description.
Nit
🟢 [nit] The changeset says minor bump, which is correct for a new feature.
Summary
| Severity | Count |
|---|---|
| 🔴 blocking | 1 |
| 🟡 important | 4 |
| 🟢 nit | 1 |
| 💡 suggestion | 0 |
| 🎉 praise | 3 |
The core feature is well-implemented — the markdown parsing, URL sanitization, and word splitting are solid. The main concerns are: (1) the missing E2E tests for a shared rendering change, (2) the hardcoded link color bypassing the theme system, (3) the HTML interpolation pattern in markdownToHTML, and (4) explaining the dual rendering paths. Looking forward to seeing this updated!
|
@aloisklink Is this pr ready to merge? I had added link support for flowchart too. But it need a ` surround whole text(make them markdown format) sequenceDiagram do not need ` |
|
@aloisklink @knsv-bot @tobie @xuanxu @dbussink Will anyone review this? |
|
Thanks, I will do another round or reviews! |
|
Thanks for the thorough update, @taoqf! You've addressed several items from the first review — the hardcoded link color is gone, D3 paths now HTML-encode hrefs, and there's significantly more test coverage across sequence diagrams, flowcharts, and the markdown pipeline. Let me walk through what's resolved and what remains. What's improved since the last review 🎉 [praise] The hardcoded #0d47a1 link color is completely removed. Links now use text-decoration: underline and inherit theme-based text colors. The styles.js CSS selector change from text.actor > tspan to 🎉 [praise] The D3 rendering paths in both createText.ts and svgDraw.js now HTML-encode hrefs before interpolation: 🎉 [praise] Excellent test coverage added — 95 lines of handle-markdown-text.spec.ts tests (link formatting, multi-word links, multiple links, bold/italic mixed with links, malicious URL sanitization for both 🎉 [praise] Extending the feature to flowcharts (via flowDb.ts, shapes/util.ts, flowchart/types.ts) broadens the value of this PR and addresses the "narrow scope" concern from the first review. Things to address 🟡 [important] Contradictory sanitization bypass between flowDb.ts and shapes/util.ts. Both files skip sanitizeText() for markdown content, but each claims the other handles it:
Neither actually sanitizes. The text IS handled safely downstream — markdownToLines parses it via marked, text content goes through D3's .text() (sets textContent, not innerHTML), and URLs go through 🟡 [important] markdownToHTML still interpolates href without HTML encoding. handle-markdown-text.ts:143: The D3 rendering paths were fixed with replace(/"/g, '"'), but markdownToHTML (used for HTML label mode) still interpolates the raw sanitizeUrl() output. DOMPurify mitigates this since it sanitizes the There are still no e2e tests for this, but thanks to the thorough unit tests, that is not a blocker. So no blockers from me, but I would still want @aloisklink's eyes on the security aspects, just in case. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I did not change architecture-beta's behavior, no idea about argos check |
3 participants
📑 Summary
Brief description about the content of your PR.
Resolves #
📏 Design Decisions
Describe the way your implementation works or what design decisions you made if applicable.
📋 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:.