fix(c4): break rows on c4ShapeInRow instead of the display width - #8056
Open
filipsajdak wants to merge 2 commits into
Open
fix(c4): break rows on c4ShapeInRow instead of the display width#8056filipsajdak wants to merge 2 commits into
filipsajdak wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 8da7777 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 failed.
|
github-actions
Bot
added
the
Type: Bug / Error
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughC4 rendering now determines row breaks only from ChangesC4 layout determinism
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
🧹 Nitpick comments (1)
eslint.config.js (1)
235-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnable
checkGlobalObjectforno-restricted-globals.Set
checkGlobalObject: trueto reportwindow.screen,globalThis.screen, andself.screen. Exclude**/*.spec.{ts,js}or add a narrow exception becausec4Renderer.spec.tsaccessesglobalThis.screen.🤖 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 `@eslint.config.js` around lines 235 - 247, Update the no-restricted-globals configuration for the screen entry in the ESLint config to set checkGlobalObject: true, so qualified global references are reported. Exclude **/*.spec.{ts,js} from this restriction or add a targeted exception for c4Renderer.spec.ts to preserve its globalThis.screen access.
🤖 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/c4/c4Renderer.spec.ts`:
- Line 33: Update the test setup around the global MutationObserver assignment
to save its original value before setting it to undefined, then restore that
saved value in the rendering test’s finally block so later tests retain the
prior global state.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 235-247: Update the no-restricted-globals configuration for the
screen entry in the ESLint config to set checkGlobalObject: true, so qualified
global references are reported. Exclude **/*.spec.{ts,js} from this restriction
or add a targeted exception for c4Renderer.spec.ts to preserve its
globalThis.screen access.
🪄 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: e432cc5a-7095-4145-b68e-c4927f69391a
📒 Files selected for processing (4)
.changeset/c4-row-break-determinism.mdeslint.config.jspackages/mermaid/src/diagrams/c4/c4Renderer.spec.tspackages/mermaid/src/diagrams/c4/c4Renderer.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8056 +/- ##
===========================================
+ Coverage 77.56% 77.65% +0.09%
===========================================
Files 572 572
Lines 75278 75273 -5
Branches 14685 14695 +10
===========================================
+ Hits 58389 58454 +65
+ Misses 15885 15825 -60
+ Partials 1004 994 -10
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 ↗︎
|
A row ends after `c4ShapeInRow` elements, however wide those elements measure. The row budget used to be seeded from `screen.availWidth`, so the same diagram laid out differently depending on the monitor it was rendered on, and stacked into a single column under jsdom, where `availWidth` is 0. Nesting divided that budget once per level, which is why elements inside boundaries ended up one per row on a narrow display. Boundary label, type and description text wrap to the widest row the grid can produce - `c4ShapeInRow` elements plus their margins - rather than to the display width, so wrapping does not vary by machine either.
filipsajdak
force-pushed
the
fix/c4-row-break-determinism
branch
from
August 12, 2026 16:47
1071adf to
1422198
Compare
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/c4/c4Renderer.spec.ts`:
- Around line 117-123: Constrain the display width in both layout tests by
setting globalThis.screen.availWidth below the width needed for the tested
elements before rendering:
packages/mermaid/src/diagrams/c4/c4Renderer.spec.ts:117-123 for six elements and
packages/mermaid/src/diagrams/c4/c4Renderer.spec.ts:126-135 for three boundary
elements. Use the existing afterEach cleanup to restore the environment.
🪄 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: 83dcb96b-f18f-46c0-87b6-be06ee41a90a
📒 Files selected for processing (2)
eslint.config.jspackages/mermaid/src/diagrams/c4/c4Renderer.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- eslint.config.js
`no-restricted-globals` rejects `screen` under `packages/mermaid/src`, so rendered geometry cannot start depending on the size of the viewer's monitor again.
filipsajdak
force-pushed
the
fix/c4-row-break-determinism
branch
from
August 12, 2026 17:27
1422198 to
8da7777
Compare
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C4 row breaking depends on the size of the monitor the diagram is rendered on. This removes that dependency, so a C4 diagram lays out the same way everywhere.
Found while answering @pbrolin47's question on #7842 about a C4 layout difference in the 11.17.0 release-candidate image tests that he could not reproduce on a local dev server (diagnosis, measured numbers). This is the cause of the "cannot reproduce locally" half of that report. The width change he was looking at is separate and expected - it is #7842's reviewed change.
The defect
c4Renderer.tsseeded the row budget from the display:That was the only place in
packages/mermaid/srcreading the physical display, so C4 was the only diagram type in mermaid whose geometry depended on the machine. A row ended when the next element would cross that budget, so with default config (width216,c4ShapeMargin50,diagramMarginX50) the fourth element of a row lands at450 + 4wand the row breaks oncew >= (availWidth - 450) / 4:screen.availWidthe2ejob) - Chrome's Linux new-headless hard-codes an 800x600 virtual screen-screen 0 1280x1024x24, the Applitools job)Screen-impl.jshard-codesavailWidth = 0)Nesting made it worse: the budget was divided by
min(c4BoundaryInRow, siblings)at every level, so a boundary two levels deep got a quarter of it and its elements stacked one per row. And because the test compared an absolute x coordinate against a width budget, the effective threshold varied with nesting depth.One user-visible consequence:
UpdateLayoutConfig($c4ShapeInRow="6")was silently capped, because the 5th element already crossed the budget on most displays.The change
A row ends after
c4ShapeInRowelements, however wide they measure:c4ShapeInRowalready existed as the count-based break, so this removes the pixel test rather than replacing it. Row topology is now a function of the diagram source and its config alone. Element widths still depend on the font, but only box sizes vary now - not which row an element lands in.widthLimithad a second, non-obvious role: it was also the wrap budget for boundary label/type/description text. That is now the widest row the grid can produce (c4ShapeInRowelements plus their margins), so boundary text wrapping stops varying by machine too, instead of silently losing wrapping.Evidence
c4Renderer.spec.tsrenders through the public API withgetBBoxstubbed, so every element self-sizes to thec4.widthfloor and the grid coordinates are exact. Four cases: a row fills toc4ShapeInRowbefore the next starts; the same source places identically whatever the display reports;$c4ShapeInRow="6"is honoured; a boundary's elements stay on one row.All four are red on
developand green here. Ondevelopjsdom reportsavailWidth = 0, so the five elements stack in a single column at x 258 (y 403, 643, 883, 1123, 1363) instead of laying out 4 + 1. Worth knowing on its own: no unit test could previously exercise C4 layout at all, because every element landed on its own row regardless of the diagram.The 130 existing C4 unit tests still pass; they are parser and db tests and never asserted geometry.
eslint.config.jsgainsno-restricted-globalsforscreenunderpackages/mermaid/src, so this cannot regress. Verified it fires by re-adding ascreen.availWidthread.This moves screenshots, deliberately
Argos will show a large C4 diff, and it is the point of the change rather than a side effect. Diagrams get wider and shorter: where a narrow CI display previously forced 2-3 elements per row, a row now holds up to 4, and elements inside boundaries are no longer stacked one per row. The changeset says so.
Flagging the timing explicitly: 11.17.0 is being prepared right now, and this alters the C4 baseline it is cut from. @pbrolin47 - happy to hold this until the release is out if that is easier; the defect is not new and is not getting worse.
Not fixed here
c4BoundaryInRowby index, unchanged.c4.wrapis still ignored in favour of the rootwrap(fix(c4): wrap config never takes effect (parse-time .wrap frozen to false before conf.wrap applies) #7949).All of this disappears with #8042, which replaces this grid with the unified layout pipeline and deletes
c4Renderer.ts. This is deliberately a small change to a file with a limited remaining life, chosen over re-designing the row-break rule.Summary
c4ShapeInRowinstead of display width.screen.availWidthfrom C4 layout calculations.screen-based layout calculations in Mermaid source.Testing