Conversation
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
Type: Bug / Error
knsv
left a comment
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Hey @EmnaBarbouch — thanks for taking the time to look into #7492 and submitting a PR! Appreciated. 🙏
What's working well
🎉 [praise] The simplified diagram text is actually quite readable and well-structured — shorter labels make it easy to follow the architecture at a glance.
🎉 [praise] Good instinct to look at the demo file first to understand the problem.
Things to address
🔴 [blocking] — This doesn't fix the underlying rendering bug
The core issue in #7492 is that the C4 layout engine doesn't properly handle label sizing, text overflow, and edge routing — causing overlapping when labels are long. The issue's suggested fixes point to c4Renderer.js and c4.css.
This PR works around the symptom by shortening the demo text, but any user with longer labels will still hit the same overlapping problem. A demo change masks the bug rather than fixing it. To actually resolve #7492, the rendering code needs to be modified (better label offset calculations, text wrapping/ellipsis, or improved layout spacing).
🟡 [important] — Demo loses valuable syntax examples
The original demo served as documentation for C4 diagram features. The rewrite removes examples of:
UpdateRelStyle()with offset parameters — this is the primary mechanism users have to fix label placement$tagsparameter usageContainer_Ext()andContainerDb_Ext()(external container variants)Rel_Back()(reverse relationship)- Protocol labels on relationships (e.g.,
"HTTPS","async, JSON/HTTPS","sync, JDBC")
These are important reference patterns for contributors and users testing C4 diagrams locally. If the demo is simplified, these examples should be preserved elsewhere or in a separate demo block.
🟡 [important] — Broken HTML closing tag (demos/c4context.html)
The patch introduces a split closing tag:
</pre
>This should be </pre> on a single line. While browsers may tolerate this, it's malformed HTML and inconsistent with the rest of the file.
🟡 [important] — No tests or changeset
Even for a demo-only change, if the intent is to close a bug, there should be some verification that the fix works. Since this is a layout issue, a visual regression test (imgSnapshotTest) showing the C4 container diagram renders without overlapping would be appropriate. A changeset (pnpm changeset) is also expected for bug fixes.
🟢 [nit] — Unrelated .gitignore entry
The addition of 7492.bundle to .gitignore looks like a local build artifact. This shouldn't be committed — .gitignore entries should be for patterns that any contributor might encounter, not one-off local files.
🟢 [nit] — Inconsistent indentation
The original demo content was indented within the <pre> tags to match the HTML structure. The new content removes indentation for the diagram text but keeps it for the closing </pre> tag split, creating an inconsistency.
Recommendation
To actually fix #7492, the approach would need to address the rendering layer — for example:
- Improving label offset defaults in the C4 renderer so they don't overlap nodes
- Adding text truncation or wrapping for long descriptions
- Adjusting node padding to accommodate longer text
If you'd like to pursue a rendering fix, packages/mermaid/src/diagrams/c4/c4Renderer.ts and the C4 styles would be the right place to start. The issue itself has a helpful table of which files to check. Happy to help guide you through the C4 rendering code if you'd like to take another pass at this!
Self-check:
- At least one 🎉 [praise] item exists
- No duplicate comments
- Severity tally: 1 🔴 blocking / 3 🟡 important / 2 🟢 nit / 0 💡 suggestion / 2 🎉 praise
- Verdict matches criteria: REQUEST_CHANGES (1 🔴)
- Not a draft PR — REQUEST_CHANGES is appropriate
- No inline comments used
- Tone check: constructive, appreciative, offers guidance
@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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7583 +/- ##
=======================================
Coverage 3.33% 3.33%
=======================================
Files 536 535 -1
Lines 56249 56238 -11
Branches 820 820
=======================================
Hits 1876 1876
+ Misses 54373 54362 -11
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 @knsv Thank you for the detailed review. I've taken your feedback into account and pushed a new update to the branch. Instead of just shortening the demo text, I've now addressed the underlying rendering issue in the C4 layout engine so that overlapping labels are properly handled regardless of text length. Here's a summary of what was fixed in the new push:
Could you please take another look when you get a chance? Happy to make any further adjustments based on your feedback! |
|
Hey @EmnaBarbouch I'm looking at 1996c89 and I don't see the renderer changes, restored syntax examples, or .gitignore/ fixes you described. Could you double-check that your latest commit was pushed? |
3 participants
📑 Summary
Shorten node and relationship labels in the C4 Container demo (
demos/c4context.html) to fix overlapping text in rendered diagrams.Resolves #7492
📏 Design Decisions
Excessively long description strings in
Person,System_Ext,Container,ContainerDb, andRelnodes were causing label overflow and visual overlap in the C4 context and container diagrams. Labels were trimmed to their essential meaning without losing semantic clarity.UpdateRelStyleoffset overrides were also removed since they are no longer needed with the shorter labels. One line was added to.gitignoreto exclude auto-generated Langium CLI output.📋 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:.