Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 8eb324e 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 site configuration. |
@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 #6119 +/- ##
==========================================
- Coverage 3.55% 3.55% -0.01%
==========================================
Files 489 489
Lines 48744 48774 +30
Branches 765 765
==========================================
Hits 1734 1734
- Misses 47010 47040 +30
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 ↗︎
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Hey @NealGooch — first of all, sorry for the long wait on this review. This PR has been sitting for over a year and that's not the experience we want for contributors. Let's get this across the finish line.
What's working well
🎉 [praise] Great bug catch! The fix is surgically precise — block.widthInColumns → child.widthInColumns on line 64 of layout.ts is clearly correct. When normalizing a child's rendered width to find the per-column unit width, you need to divide by that child's column span, not the parent's. Using the parent's span was causing completely wrong size calculations for nested blocks with multi-column children, which explains the overlapping and mispositioned elements in issue #5706.
🎉 [praise] One-line fix, right at the root cause. This is the kind of change that's easy to get right and hard to argue with.
Things to address
🟡 [important] Missing E2E visual regression test. Block diagram layout changes must have a Cypress snapshot test to prevent future regressions. Please add a test case in cypress/integration/rendering/block.spec.js (or similar) using imgSnapshotTest() with the reproduction case from issue #5706:
block-beta
columns 4
block:0_0:4
columns 6
Example:2
space:2
ExampleOther:2
ExampleOther:1
block:0_0_0:6
a b c d e f g h
end
end
This is especially important because this is shared layout code — a future refactor could easily re-introduce the same bug without visual coverage.
🟡 [important] Missing unit test. It would be great to add a unit test for getMaxChildSize() that covers multi-column children. The existing test file (layout.spec.ts) only tests calculateBlockPosition. A test that constructs a mock Block with children having different widthInColumns values and verifies the returned max width would nail this down. This provides faster feedback than E2E alone.
🟡 [important] Missing changeset. Since this is a bug fix, please generate one:
pnpm changesetSelect packages/mermaid, patch bump, and prefix the description with fix: (e.g., fix: correct block positioning when nested blocks span multiple columns).
Self-check
- At least one 🎉 praise item
- No duplicate comments
- Severity tally: 0 🔴 / 3 🟡 / 0 🟢 / 0 💡 / 2 🎉
- Verdict: Approved
- Tone check: collaborative and appreciative
Thanks for the contribution — the fix itself is spot-on. Once the test and changeset are added, this should be ready to go. Let me know if you need any help with the Cypress test setup!
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Here's why that breaks with the PR fix:
group2.y = parent.y - parent.h/2 + 2 * (group2.height + padding) + group2.height/2 + padding It computes the offset for rows 0 and 1 using group2.height — but row 1 actually contains group1, which is taller than group2. The formula underestimates the space taken by row
height = siblingHeight; // line 147 — forces uniform height This masks the y-positioning bug by keeping all children at the same height. The PR fix produces correct per-column widths, so children keep their natural heights — and the In short: the fix is correct for width calculation, but it exposes a latent bug in layoutBlocks where the y-positioning formula assumes uniform row heights. The real fix needs |
The y-position formula in layoutBlocks assumed all rows have the same height (the current child's height). When nested blocks have different internal layouts (e.g. group1 with 2 rows vs group2 with 1 row), this caused later rows to overlap earlier taller rows. Fix: pre-compute per-row max heights and use cumulative row offsets instead of py * (child.height + padding). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ights BL32: reproduces issue mermaid-js#5706 — nested blocks spanning multiple columns BL33: verifies rows with different heights don't overlap Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
❌ Lockfile Validation Failed The following issue(s) were detected: Please address these and push an update. Posted automatically by GitHub Actions |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
knsv
left a comment
There was a problem hiding this comment.
Ready to approve this one now!
|
@NealGooch, Thank you for the contribution! |

📑 Summary
Block diagrams positioning goes wrong when nesting blocks and blocks span columns
Resolves #5706
📏 Design Decisions
Fix
📋 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:.