Repository navigation
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 9f49de6 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: |
|
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:
📝 WalkthroughWalkthroughChangesFlowchart shape expansion
Argos screenshot grouping
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Flowchart
participant ShapeRegistry
participant ShapeRenderer
participant SVG
Flowchart->>ShapeRegistry: Resolve folder, bucket, console, or browser
ShapeRegistry->>ShapeRenderer: Invoke registered renderer
ShapeRenderer->>SVG: Draw shape and label
ShapeRenderer->>Flowchart: Set bounds and intersection handler
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7970 +/- ##
===========================================
+ Coverage 77.61% 77.81% +0.19%
===========================================
Files 567 572 +5
Lines 74906 75515 +609
Branches 14619 14811 +192
===========================================
+ Hits 58141 58760 +619
+ Misses 15768 15704 -64
- Partials 997 1051 +54
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/mermaid/src/rendering-util/rendering-elements/shapes/folder.ts (1)
60-63: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize measure phase by passing exact pre-computed bounds.
All four new shape renderers compute exact analytic dimensions but omit the
knownBoundsparameter when dispatching toupdateNodeBounds. As noted in the upstreamutil.tscontract, relying ongetBBox()triggers a synchronous layout reflow that drastically impacts performance on larger diagrams.Because standard shapes have strictly known geometry, you can completely avoid the
getBBox()penalty by passing the calculated dimensions conditionally (falling back to undefined only forhandDrawnwhereroughjsshapes organically overflow their bounds).
packages/mermaid/src/rendering-util/rendering-elements/shapes/folder.ts#L60-L63: Replace the deadnode.width/node.heightassignments (which are immediately overwritten) and pass bounds to the update call:
updateNodeBounds(node, folderShape, node.look !== 'handDrawn' ? { width: w, height: totalHeight } : undefined);packages/mermaid/src/rendering-util/rendering-elements/shapes/bucket.ts#L60-L60: Pass the conditionally known box:
updateNodeBounds(node, group, node.look !== 'handDrawn' ? { width: w, height: totalHeight } : undefined);packages/mermaid/src/rendering-util/rendering-elements/shapes/console.ts#L62-L62: Pass the conditionally known box:
updateNodeBounds(node, group, node.look !== 'handDrawn' ? { width: w, height: h } : undefined);packages/mermaid/src/rendering-util/rendering-elements/shapes/browser.ts#L82-L82: Pass the conditionally known box:
updateNodeBounds(node, group, node.look !== 'handDrawn' ? { width: w, height: h } : undefined);🤖 Prompt for 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. In `@packages/mermaid/src/rendering-util/rendering-elements/shapes/folder.ts` around lines 60 - 63, Update updateNodeBounds calls in packages/mermaid/src/rendering-util/rendering-elements/shapes/folder.ts:60-63, packages/mermaid/src/rendering-util/rendering-elements/shapes/bucket.ts:60-60, packages/mermaid/src/rendering-util/rendering-elements/shapes/console.ts:62-62, and packages/mermaid/src/rendering-util/rendering-elements/shapes/browser.ts:82-82 to pass the precomputed width and height when node.look is not handDrawn, and undefined otherwise. In folder.ts, remove the redundant node.width and node.height assignments; use totalHeight for folder and bucket, and h for console and browser.
🤖 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/rendering-elements/shapes/bucket.ts`:
- Around line 28-35: Update the top arc command in the bucket body path within
the body construction to use sweep-flag 0 instead of 1, making the filled body
extend to the back rim and occlude background elements while preserving the
unfilled top ellipse.
---
Nitpick comments:
In `@packages/mermaid/src/rendering-util/rendering-elements/shapes/folder.ts`:
- Around line 60-63: Update updateNodeBounds calls in
packages/mermaid/src/rendering-util/rendering-elements/shapes/folder.ts:60-63,
packages/mermaid/src/rendering-util/rendering-elements/shapes/bucket.ts:60-60,
packages/mermaid/src/rendering-util/rendering-elements/shapes/console.ts:62-62,
and
packages/mermaid/src/rendering-util/rendering-elements/shapes/browser.ts:82-82
to pass the precomputed width and height when node.look is not handDrawn, and
undefined otherwise. In folder.ts, remove the redundant node.width and
node.height assignments; use totalHeight for folder and bucket, and h for
console and browser.
🪄 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: 3778e438-0f93-4562-9ec8-3d53c99e0b30
📒 Files selected for processing (11)
.changeset/general-shapes.mdcypress/integration/rendering/flowchart/flowchart-shape-alias.spec.tscypress/integration/rendering/newShapes.spec.tsdocs/syntax/flowchart.mdpackages/mermaid/scripts/docs.spec.tspackages/mermaid/src/rendering-util/rendering-elements/nodes.spec.tspackages/mermaid/src/rendering-util/rendering-elements/shapes.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/browser.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/bucket.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/console.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/folder.ts
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
About the scary-looking Argos diff on the first run (50 changed sheets, whole pages flipping between light and dark): none of it is a rendering change. All specs sitting directly under The latest commit fixes the batching itself: a spec directly under the top-level category now gets its own sheet group (subfolder groups like |
|
Hi @filipsajdak ,
|
|
Note: There is some ongoing work with solving the problems with grid ordering when adding new tests, #7905. [sisyphos-bot] What's working well 🎉 Bucket body fill fix (PATCH 6/7) caught and fixed within the PR. Changing the top arc sweep from 0 0 1 to 0 0 0 makes the bucket opaque — the back-rim arc now closes the fill rather than the 🎉 Folder uses polygon intersection, not rect. Using intersect.polygon(node, points, point) means edges connect to the actual L-shaped outline rather than the bounding box. The points array exactly 🎉 Decoration colors from theme variables. All three shapes that have visible decorations (bucket rim, console glyph, browser chrome dots/bar) use themeVariables?.nodeBorder ?? 🎉 Visual regression coverage is comprehensive. newShapesSet7 exercises all 4 shapes across classic + handDrawn looks, default/long/markdown labels, and with styles. The alias pair folder/directory Things to address 🟢 folder.ts — node.width and node.height pre-assignments are dead code (folder.ts:62-63) // Current: updateNodeBounds without knownBounds always calls getBBox() and overwrites the node dimensions — so the two lines above are unreachable in effect. For the non-handDrawn path, getBBox() returns the if (node.look === 'handDrawn') { This skips the forced reflow on the non-handDrawn path (consistent with the optimization from PR #7951) and removes the confusing dead assignments. The other 3 shapes (bucket, console, browser) Pre-existing gap worth noting (out of scope for this PR) 💡 aliasSet40 (collate/hourglass) is declared but never added to aliasSets in flowchart-shape-alias.spec.ts. This predates this PR — aliasSet40 is defined on line ~88 but is absent from the Security XSS sub-agent review is in progress — will be posted as a follow-up once it completes. From direct code review: no .html() calls, no innerHTML, the only .text() call is hardcoded '>_' in console.ts, |
|
Thanks @pbrolin47! Fixed in af249fa - the bucket now computes edge intersections against its drawn outline (the rim's upper arc and the base's lower arc sampled into a polygon, joined by the tapered sides) instead of the bounding box; the same Before / after (zoomed on the rim - note the left and right arrowheads):
Full-diagram before/after plus the LR and hand-drawn looks are in this gallery. Note: the push resets the pending Argos decision - the bucket sheets will show the expected edge-endpoint diffs. |
|
Thanks for the detailed review!
|
|
Gentle nudge (no rush) - @pbrolin47, whenever you have a moment: your review points here are addressed (bucket edges meet the drawn outline via polygon intersection, and the folder passes its known bounds to skip a reflow), and it's green on the required checks. A re-review would be very welcome. The |
|
@pbrolin47 - when you have a moment, this is ready for another look. Both points from your review were addressed on 17 July:
The alias-suite gap you spotted alongside the review is #7972 (one line, independent of this PR). One note on |
|
Merged current Worth a word on why this one needed it when the other open PRs did not. #7972 added After the merge the array carries all three, and the diff against No change to the shapes themselves. 643 unit tests pass, Still outstanding here and unchanged: once @sidharthv96's #7905 lands, we drop the Argos sheet-batching commit ( |
A folder or directory: a rectangle with a tab along the top-left edge. Sizes from its label, supports the handdrawn look, and passes its known bounds to updateNodeBounds so the node is not measured twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ftYK49bbNLySKsY1Pg1Rg
An object-storage bucket: a tapered body under an elliptical rim. The body is filled up to the back of the rim so the fill does not show a seam, and edges meet the drawn outline through a sampled polygon intersection rather than the bounding box. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ftYK49bbNLySKsY1Pg1Rg
A terminal window: a rounded rectangle with a title bar and a prompt glyph. Named console because terminal is already an alias of the stadium shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ftYK49bbNLySKsY1Pg1Rg
A browser window: a rounded rectangle with a title bar, window controls and an address bar above the label area. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ftYK49bbNLySKsY1Pg1Rg
Adds the four shapes to the registry with folder aliased as directory, so they
are usable from flowcharts via A@{ shape: ... }, and covers them with unit,
alias and rendering tests. The generated shape table in the flowchart docs and
its snapshot pick up the new rows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ftYK49bbNLySKsY1Pg1Rg
Specs that sit directly in the rendering directory shared a composite sheet with whatever leaf directory sorted next to them, so adding a spec reshuffled unrelated screenshots. They now get their own group. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ftYK49bbNLySKsY1Pg1Rg
8bb6961 to
9f49de6
Compare
|
Rebased onto current The two develop merges are gone, and the three follow-up fixes are folded into the shapes they fix - so each shape is now one commit that stands on its own, rather than "add bucket ... two commits later, fix bucket": The Argos sheet-group commit is deliberately kept last and separate, so it stays trivial to drop once @sidharthv96's #7905 lands - that commitment is unchanged. The tree is byte-identical to the previous head: the series was rebuilt from that exact tree rather than replayed, and |
|
@pbrolin47 your 2026-07-17 points were addressed the same day: bucket edges now meet the drawn outline via polygon intersection, and folder passes known bounds so the measure pass skips a reflow. This has been waiting on a re-review since 2026-07-24. The standing commitment is unchanged: the Argos sheet-batching commit carried here gets dropped as soon as #7905 lands, so it will not collide with @sidharthv96's append-only manifest. #7905 is still open, so it stays for now. As with the other rendering PRs, the Full stack status: #7849 (comment) |
|
Thanks for the review! One thing worth flagging before this merges, because it affects someone else's PR rather than this one. The top commit here, It is deliberately the last commit on the branch, so removing it is a one-commit rebase. Either way suits me:
Happy to do whichever you and @sidharthv96 prefer - just say which and I will push it. If I hear nothing I will leave the commit in place rather than change an approved PR under you. |
…orktrees These cases were written ad hoc while answering reviews and only ever existed in the pr02 (element shapes) and shapes (folder/bucket) worktrees, untracked. Those worktrees are being removed, and the files are in no git ref anywhere, so they are collected here: the colour/wrap/z-order and person-edge repros for the element-shape review, the bucket-angle and folder repros for mermaid-js#7970, and the two DOM assertion scripts used to check computed styles rather than eyeball a render.
Upstream release: https://github.com/mermaid-js/mermaid/releases/tag/mermaid%4011.17.0 Release notes: ### Minor Changes - [#7842](mermaid-js/mermaid#7842) [`3670b4e`](mermaid-js/mermaid@3670b4e) Thanks [@filipsajdak](https://github.com/filipsajdak)! - feat(c4): render C4 elements through the unified shape system, using the new person shape - [#7812](mermaid-js/mermaid#7812) [`cdfc0ea`](mermaid-js/mermaid@cdfc0ea) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(class): route `classDiagram` to the unified (v2) renderer by default Set `class: { defaultRenderer: 'dagre-d3' }` in the config to restore the legacy renderer. - [#7785](mermaid-js/mermaid#7785) [`c45cde9`](mermaid-js/mermaid@c45cde9) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(flowchart): add collapsible flowchart subgraphs via `subgraphId@{ view: collapsed }` - [#7828](mermaid-js/mermaid#7828) [`8eb3afc`](mermaid-js/mermaid@8eb3afc) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(elk): add `elk.keepEntryNodeOnTop` config option to keep a recursive flow's entry node on top - [#7803](mermaid-js/mermaid#7803) [`74e44eb`](mermaid-js/mermaid@74e44eb) Thanks [@knsv-bot](https://github.com/knsv-bot)! - feat(elk): add `elk.nodePlacementAlignment` config option - [#7792](mermaid-js/mermaid#7792) [`ea55b31`](mermaid-js/mermaid@ea55b31) Thanks [@RodrigojndSantos](https://github.com/RodrigojndSantos)! - feat(er): add subgraph support to ER diagrams. - [#7970](mermaid-js/mermaid#7970) [`a2c0fb6`](mermaid-js/mermaid@a2c0fb6) Thanks [@filipsajdak](https://github.com/filipsajdak)! - feat(flowchart): add `folder`, `bucket`, `console` (terminal window) and `browser` shapes - [#7842](mermaid-js/mermaid#7842) [`ae3e115`](mermaid-js/mermaid@ae3e115) Thanks [@filipsajdak](https://github.com/filipsajdak)! - feat(flowchart): add `person` shape (circular head above a rounded body), usable in flowcharts via `A@{ shape: person }` - [#7724](mermaid-js/mermaid#7724) [`0fd7a9f`](mermaid-js/mermaid@0fd7a9f) Thanks [@xdumaine](https://github.com/xdumaine)! - feat(xyChart): add legends for named line and bar series ### Patch Changes - [#7847](mermaid-js/mermaid#7847) [`215fe89`](mermaid-js/mermaid@215fe89) Thanks [@filipsajdak](https://github.com/filipsajdak)! - fix(c4): named attributes such as `$tags`, `$link` and `$sprite` are no longer clobbered to undefined when they arrive in an earlier positional slot of Person/System/Container/Component/Boundary/Rel statements. - [#7871](mermaid-js/mermaid#7871) [`8d874c4`](mermaid-js/mermaid@8d874c4) Thanks [@knsv-bot](https://github.com/knsv-bot)! - fix(flowchart): stop dagre layout from spamming `warn`-level logs on every node/edge/cluster - [#8071](mermaid-js/mermaid#8071) [`b3d1f63`](mermaid-js/mermaid@b3d1f63) Thanks [@pbrolin47](https://github.com/pbrolin47)! - fix(block): sibling blocks overlapping in block diagrams when one has a label wider than 200px - [#7870](mermaid-js/mermaid#7870) [`71b8843`](mermaid-js/mermaid@71b8843) Thanks [@knsv-bot](https://github.com/knsv-bot)! - fix: a `RangeError: Invalid array length` crash when rendering certain edges. - [#7924](mermaid-js/mermaid#7924) [`9cbef5d`](mermaid-js/mermaid@9cbef5d) Thanks [@nightt5879](https://github.com/nightt5879)! - fix(treeView): icons disappearing after strict security sanitization. - [#7850](mermaid-js/mermaid#7850) [`a34cbf0`](mermaid-js/mermaid@a34cbf0) Thanks [@aloisklink](https://github.com/aloisklink)! - fix(block): allow classdefs to update text color - [#7937](mermaid-js/mermaid#7937) [`f9cbe1e`](mermaid-js/mermaid@f9cbe1e) Thanks [@filipsajdak](https://github.com/filipsajdak)! - fix(dagre): let a diagram's own nodeSpacing/rankSpacing take effect in the unified dagre layout - [#8005](mermaid-js/mermaid#8005) [`90eeece`](mermaid-js/mermaid@90eeece) Thanks [@pbrolin47](https://github.com/pbrolin47)! - fix(flowchart): reverts the behavior change from #7672 (fix/4648-directions), since arrows between subgraphs are broken - [#7951](mermaid-js/mermaid#7951) [`afa2f80`](mermaid-js/mermaid@afa2f80) Thanks [@aloisklink](https://github.com/aloisklink)! - perf: use `fastdom` to batch DOM measurements (up to 25% speedup) - Updated dependencies \[[`e848423`](mermaid-js/mermaid@e848423)]: - @mermaid-js/parser@1.2.1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: give me a 98K <240642031+inhuman-0@users.noreply.github.com>





Summary
Adds four general-purpose flowchart shapes -
folder(aliasdirectory),bucket,console(a terminal window) andbrowser- usable viaA@{ shape: folder }etc., in both the classic and handDrawn looks.These started as C4-specific shapes in #7842. Review feedback there asked for them to move out of that PR and come back as first-class shapes usable from flowcharts and other diagrams, so that is what this PR does. It is part of the C4 modernization plan tracked in #7849 but fully independent of the C4 stack; once this and #7842 both land, a small follow-up rewires the C4
$shape/$spritekeywords to these shapes.Naming question
terminalis already an alias ofstadium, so the terminal-window shape is registered asconsole. Happy to rename if you would prefer something else (e.g.terminal-window).What each shape gets
folderuses a polygon intersect matching its outline; the others use rectnodes.spec.tsandflowchart-shape-alias.spec.ts, plus anewShapes.spec.tsset (classic + handDrawn, labels, long labels, markdown, styles)minorchangesetReview by commit: one commit per shape, then a test + docs commit.
Prepared with assistance from Claude Code; all changes reviewed and tested by a human.
Review summary
🎉 Adds four new Mermaid flowchart node shapes usable via
A@{ shape: ... }—folder(aliasdirectory),bucket,console(terminal window), andbrowser—including shape registration/aliasing, classic +handDrawnrendering, theme-variable–driven decoration colors, and shape-specific geometry (folder/bucket use polygon-style outline intersection; console/browser use rectangle intersection). Includes updated syntax/docs, a minor changeset, and Cypress + unit tests covering aliasing, label variants (including long labels and markdown), and multiple looks/directions.Findings
No blocking or important issues identified. The Cypress suite explicitly adds
aliasSet42 = ['folder','directory']and coversnewShapesSet7 = ['folder','bucket','console','browser'], including long-label and markdown htmlLabels cases.Security
A targeted scan of the new/modified shape renderer files (
folder.ts,bucket.ts,console.ts,browser.ts) did not surface common XSS/injection sink patterns (e.g.,innerHTML/outerHTML,foreignObject,on*handlers,<script>,eval/Function, orjavascript:URLs).Notes on test stability
The PR also includes Argos sheet-batching/grouping adjustments (
scripts/argos-batch-sheets*.ts) to prevent unrelated snapshot churn when adding new rendering specs.Verdict
APPROVE