Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 04b2ee0 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFlowchart node metadata now has shared optional types, is stored on vertices, and is copied into rendered node data for existing and new nodes. JSDOM tests validate metadata handling across flowchart shapes and subgraphs. ChangesFlowchart metadata propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mermaid/src/diagrams/flowchart/flowDb.ts`:
- Line 1075: Update the existing-node branch in the node registration logic to
preserve metadata by copying vertex.metadata onto the already-registered node,
alongside its existing style/class updates. Ensure repeated IDs retain the
latest subgraph metadata instead of dropping it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6619358b-e8c4-44ad-bbee-5afdef661579
📒 Files selected for processing (3)
packages/mermaid/src/diagrams/flowchart/flowDb.tspackages/mermaid/src/diagrams/flowchart/types.tspackages/mermaid/src/rendering-util/types.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8016 +/- ##
===========================================
+ Coverage 77.61% 78.76% +1.14%
===========================================
Files 567 568 +1
Lines 74906 74999 +93
Branches 14619 12750 -1869
===========================================
+ Hits 58141 59070 +929
+ Misses 15768 15074 -694
+ Partials 997 855 -142
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 ↗︎
|
|
Hi @strowk, The metadata is a new field that nothing in the existing codebase reads. It gets stored on layout nodes but no existing layout algorithm touches it — so the raw user values sit there inert. There's no current code path from metadata to any DOM/SVG sink. The theoretical future risk: a layout algorithm written later could naively read node.metadata.someKey and stick it into an .attr() or .html() call without sanitizing. That would be a problem at that point, not today. So, I recommend:
[sisyphos-bot] What's working well 🎉 Exactly follows the established pattern. The PR mirrors how subgraphs already pass metadata (same metadata field name, same spread approach), which makes the implementation immediately 🎉 Both vertex-to-node branches are covered. The existing-node mutation path (node.metadata = vertex.metadata, flowDb.ts:1043) and the new-node object literal path (metadata: vertex.metadata, 🎉 Optional typing is correct. metadata?: Record<string, unknown> keeps all existing nodes valid without any migration burden. Things to address 🟡 [important] Missing changeset metadata is added to BaseNode → Node, which is exported as part of the public LayoutData.Node API from mermaid.ts. Optional additions to public types are additive and non-breaking, but they still Run pnpm changeset, select mermaid, bump minor, and prefix the description with feat:. 🟡 [important] No unit test The behavioral change — that @{ key: value } properties in flowchart syntax now propagate to the layout-node's metadata field — has no test. There's already a flowDb test infrastructure (e.g., it('should pass @{} metadata to layout nodes', () => { }); (Adjust to match the actual FlowDB method signatures — the key thing is coverage for the vertex.metadata accumulation path.) Nit / Suggestion 💡 metadata contains all @{} properties, including the ones Mermaid handles explicitly (shape, label, w, h, icon, pos, img, constraint). vertex.metadata = { ...vertex.metadata, ...doc }; This works fine for the stated use case, and it's simpler than maintaining an exclusion list. Worth being aware of: layout algorithms reading metadata will see both Mermaid-controlled keys and truly |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mermaid/src/rendering-util/metadata-escaping.spec.ts`:
- Around line 68-100: Update the render test setup in the metadata-escaping spec
so the temporary global MutationObserver override is saved before being set to
undefined and restored in the finally block alongside window and document. Use
the existing render helper around mermaidAPI.initialize and mermaidAPI.render as
the scope for the fix, and keep the current DOM cleanup behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0f91c08-0a81-4976-b2cf-79aa8d2a53c1
📒 Files selected for processing (5)
.changeset/node-pass-metadata-to-layout.mdpackages/mermaid/src/diagrams/flowchart/flowDb.spec.tspackages/mermaid/src/diagrams/flowchart/types.tspackages/mermaid/src/rendering-util/metadata-escaping.spec.tspackages/mermaid/src/rendering-util/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/mermaid/src/diagrams/flowchart/types.ts
- packages/mermaid/src/rendering-util/types.ts
|
@pbrolin47 , hey, thanks for reviewing! Added comments: I decided to put test validating that html content can't reach output via metadata unescaped in a separate While trying to make these tests fail to check whether they test what they should, I have noticed that subgraph metadata is not actually passed down all the way, so fixed that as well with same comments. Both cases now also have unit tests (for node and for subgraph) and I have added a changeset too. Not quite sure if I shall change anything in docs, not seeing anything in particular about layout algo development there. |
|
Hi @strowk, [sisyphos-bot] What changed since round 1 🎉 Issue 1 fixed: Missing changeset 🎉 Issue 2 fixed: Missing unit test
🎉 JSDoc added everywhere it matters Security The XSS sub-agent confirmed:
The "tripwire, not a fix verification" framing in the spec header is exactly right. |
|
Anything else I should fix here, @pbrolin47 ? |
|
Hi @strowk , |
This branch has not been deployed
2 participants
📑 Allow layout algo to access complete node shape data
In the same way, that subgraphs partly do, this change passes complete node shape data in metadata field.
Resolves #8004
📏 Design Decisions
I used same word 'metadata' there since subgraphs pass it with same name.
Happy to change if needed.
📋 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:.Summary
Passes complete regular node shape metadata into the layout pipeline via the
metadatafield, matching subgraph metadata behavior, and adds coverage for metadata escaping and propagation.Changes
FlowVertex.metadata?: Record<string, unknown>to carry raw user-input shape metadata.metadatarecord so nodes can transport arbitrary key/value metadata.@{}-parsed metadata is stored on vertices and propagated to layout nodes for both existing and newly created nodes.addVertexparses@{}metadata and exposes it on resulting layout nodes.