Repository navigation
feat(c4): migrate C4 relationships to the unified edge rendering - #7883
filipsajdak wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 05df8c2 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. |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
a54560f to
0673951
Compare
|
Both unit-test failures are fixed (the fixes live in the underlying #7842 commits, since both rooted in the shapes slice, and this branch carries them):
Both PRs were force-pushed (fixes squashed into the relevant commits, no fix-on-top noise); #7883 was rebased onto the new #7842 tip. Verified locally: full c4 unit suite (161 passing), |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7883 +/- ##
===========================================
- Coverage 80.73% 78.37% -2.37%
===========================================
Files 619 619
Lines 84970 85049 +79
Branches 18596 18536 -60
===========================================
- Hits 68601 66656 -1945
- Misses 15350 17500 +2150
+ Partials 1019 893 -126
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
655d903 to
b7d0efd
Compare
|
@filipsajdak please refrain from force pushing across 15+ PRs, as it blocks our CI for everyone else. We're also at 97% use for screenshot tests for this month, so just trying to be conservative. Thank you. 
|
|
@sidharthv96 sorry, I was trying to keep the PRs in easy to merge state. |
No worries, I was just letting you know of the problem. You've been doing amazing work with C4! 🚀 It seems like the workflow of having multiple stacked PRs from a fork is poorly done in GitHub, as we cannot point to the previous branch as base. |
|
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:
📝 WalkthroughWalkthroughC4 rendering now uses unified shapes, SVG labels, asynchronous layout, and unified relationship edges. A new Person shape is registered for flowcharts and C4 elements. Typography, wrapping, documentation, release metadata, and rendering tests are updated. ChangesC4 unified renderer migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant draw
participant drawInsideBoundary
participant drawC4ShapeArray
participant buildC4Node
participant drawRels
participant insertEdge
draw->>drawInsideBoundary: render root boundary
drawInsideBoundary->>drawC4ShapeArray: render and measure C4 nodes
drawC4ShapeArray->>buildC4Node: adapt legacy C4 shape
draw->>drawRels: render relationships
drawRels->>insertEdge: insert edge and label
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/mermaid/src/rendering-util/rendering-elements/shapes/c4Person.ts (2)
51-51: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPass
knownBoundstoupdateNodeBoundsto skip the reflow-causinggetBBox().
w/totalHeightare already computed analytically here (body rect ∪ head circle exactly bounds tow × totalHeight), soupdateNodeBoundsdoesn't need to fall back togetBBox(). Perutil.ts's own docstring, this measurement is explicitly the costly path on large diagrams. The same applies toc4Terminal.ts,c4Browser.ts,c4Database.ts, andc4Queue.ts, which also compute exactw/hbefore drawing.⚡ Proposed fix
- updateNodeBounds(node, group); + updateNodeBounds(node, group, { width: w, height: totalHeight });Also applies to: 17-29
🤖 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/c4Person.ts` at line 51, Pass the analytically computed bounds into updateNodeBounds instead of relying on the fallback getBBox() path. In c4Person and the related shape renderers c4Terminal, c4Browser, c4Database, and c4Queue, use the already-known w/totalHeight (or w/h) as knownBounds when calling updateNodeBounds so the layout avoids the costly reflow-triggering measurement.
11-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShared boilerplate duplicated across all new C4 shapes.
The setup (
styles2String/labelHelper/group insert), the label-centering transform formula, and thenode.intersect = intersect.rectassignment are copy-pasted verbatim acrossc4Person.ts,c4Terminal.ts,c4Browser.ts,c4Database.ts,c4Queue.ts(and reportedlyc4Folder.ts/c4Bucket.ts). Only the geometry/path differs per shape. TheaccentColorregex helper is also duplicated verbatim betweenc4Terminal.tsandc4Browser.ts.Consider extracting a shared helper (e.g. in
util.ts) that takes the computed body geometry and returns the positioned label transform, plus a single exportedaccentColorutil, to avoid the 5-7x duplication and reduce future drift.♻️ Sketch of a shared helper
// util.ts export const centerLabel = ( label: D3Selection<SVGGElement>, bbox: DOMRect, centerY: number ) => { label.attr( 'transform', `translate(${-(bbox.width / 2) - (bbox.x - (bbox.left ?? 0))}, ${centerY - bbox.height / 2 - (bbox.y - (bbox.top ?? 0))})` ); };🤖 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/c4Person.ts` around lines 11 - 64, The C4 shape implementations are repeating the same setup and label-positioning logic across c4Person and the other new shapes. Extract the shared boilerplate from c4Person’s styles2String/labelHelper/group insert flow, the label transform calculation, and the node.intersect = intersect.rect assignment into a common helper (for example in util.ts) that each shape can call with its own geometry values. Also move the duplicated accentColor regex logic into a single shared utility and import it from c4Terminal and c4Browser to keep the shape files focused only on their unique drawing code.packages/mermaid/src/rendering-util/rendering-elements/shapes/c4Bucket.ts (1)
43-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSame forced-reflow concern as
c4Folder.ts.
wandtotalHeightare already known analytically (the path + rim ellipse bounds are exactlyw×totalHeight). PassingknownBoundsavoids thegetBBox()reflow, which matters more here since this shape is rendered twice per node in the two-passdrawC4ShapeArrayflow.♻️ Proposed fix
- updateNodeBounds(node, group); + updateNodeBounds(node, group, { width: w, height: totalHeight });🤖 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/c4Bucket.ts` at line 43, The C4 bucket shape is still forcing a reflow by calling updateNodeBounds after rendering, even though its bounds are already known analytically. Update the c4Bucket rendering path to pass knownBounds (using w and totalHeight) into the node bounds update logic instead of relying on getBBox(), and make sure the change is applied in the same render flow used by drawC4ShapeArray so the two-pass rendering path avoids the extra layout work.packages/mermaid/src/diagrams/c4/c4Renderer.ts (1)
492-501: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the exported
edgeLabelsmap instead of DOM traversal for label positioning.Locating the just-inserted label via
edgeLabels.node()?.lastElementChildworks today because C4 edges never populatestartLabelLeft/Right/endLabelLeft/Right(whichinsertEdgeLabelwould otherwise append as additional siblings into the same container, breaking this assumption).edges.jsalready exports anedgeLabelsMap keyed byedge.idfor exactly this lookup purpose (edgeLabels.set(edge.id, edgeLabel)insideinsertEdgeLabel), which would be more robust than relying on DOM child order.♻️ Proposed fix
-import { insertEdgeLabel, insertEdge } from '../../rendering-util/rendering-elements/edges.js'; +import { insertEdgeLabel, insertEdge, edgeLabels as edgeLabelRegistry } from '../../rendering-util/rendering-elements/edges.js'; ... if (edge.label) { await insertEdgeLabel(edgeLabels, edge); - const labelGroup = edgeLabels.node()?.lastElementChild; - if (labelGroup) { - labelGroup.setAttribute('transform', `translate(${labelX}, ${labelY})`); + const labelGroup = edgeLabelRegistry.get(edge.id); + if (labelGroup) { + labelGroup.attr('transform', `translate(${labelX}, ${labelY})`); } }🤖 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/diagrams/c4/c4Renderer.ts` around lines 492 - 501, The label positioning in c4Renderer’s edge-label handling is using DOM traversal via edgeLabels.node()?.lastElementChild, which is brittle when insertEdgeLabel adds extra sibling labels. Update the logic in the edge-label block to retrieve the newly created label from the exported edgeLabels Map keyed by edge.id, matching how insertEdgeLabel stores it, and then apply the transform to that specific element instead of assuming the last child is the target.packages/mermaid/src/rendering-util/rendering-elements/shapes/c4Folder.ts (1)
36-36: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid forced reflow: pass known bounds to
updateNodeBounds.
w/totalHeightare already computed analytically above; callingupdateNodeBounds(node, group)withoutknownBoundsforces a synchronousgetBBox()read. Sincec4Renderer's two-pass flow (measure + draw) invokes this shape function twice per node, that's two avoidable reflows per shape.util.ts's own docs recommend supplyingknownBoundswhen the value exactly matches whatgetBBox()would return, which is the case here (the path bounds are exactlyw×totalHeight).♻️ Proposed fix
- updateNodeBounds(node, group); + updateNodeBounds(node, group, { width: w, height: totalHeight });🤖 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/c4Folder.ts` at line 36, The c4Folder shape currently calls updateNodeBounds(node, group) without supplying knownBounds, which causes an unnecessary getBBox() reflow. In c4Folder.ts, use the already computed w and totalHeight to pass knownBounds into updateNodeBounds so the measured bounds match the analytic path size and avoid the extra synchronous layout read in the c4Renderer two-pass flow.
🤖 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.
Nitpick comments:
In `@packages/mermaid/src/diagrams/c4/c4Renderer.ts`:
- Around line 492-501: The label positioning in c4Renderer’s edge-label handling
is using DOM traversal via edgeLabels.node()?.lastElementChild, which is brittle
when insertEdgeLabel adds extra sibling labels. Update the logic in the
edge-label block to retrieve the newly created label from the exported
edgeLabels Map keyed by edge.id, matching how insertEdgeLabel stores it, and
then apply the transform to that specific element instead of assuming the last
child is the target.
In `@packages/mermaid/src/rendering-util/rendering-elements/shapes/c4Bucket.ts`:
- Line 43: The C4 bucket shape is still forcing a reflow by calling
updateNodeBounds after rendering, even though its bounds are already known
analytically. Update the c4Bucket rendering path to pass knownBounds (using w
and totalHeight) into the node bounds update logic instead of relying on
getBBox(), and make sure the change is applied in the same render flow used by
drawC4ShapeArray so the two-pass rendering path avoids the extra layout work.
In `@packages/mermaid/src/rendering-util/rendering-elements/shapes/c4Folder.ts`:
- Line 36: The c4Folder shape currently calls updateNodeBounds(node, group)
without supplying knownBounds, which causes an unnecessary getBBox() reflow. In
c4Folder.ts, use the already computed w and totalHeight to pass knownBounds into
updateNodeBounds so the measured bounds match the analytic path size and avoid
the extra synchronous layout read in the c4Renderer two-pass flow.
In `@packages/mermaid/src/rendering-util/rendering-elements/shapes/c4Person.ts`:
- Line 51: Pass the analytically computed bounds into updateNodeBounds instead
of relying on the fallback getBBox() path. In c4Person and the related shape
renderers c4Terminal, c4Browser, c4Database, and c4Queue, use the already-known
w/totalHeight (or w/h) as knownBounds when calling updateNodeBounds so the
layout avoids the costly reflow-triggering measurement.
- Around line 11-64: The C4 shape implementations are repeating the same setup
and label-positioning logic across c4Person and the other new shapes. Extract
the shared boilerplate from c4Person’s styles2String/labelHelper/group insert
flow, the label transform calculation, and the node.intersect = intersect.rect
assignment into a common helper (for example in util.ts) that each shape can
call with its own geometry values. Also move the duplicated accentColor regex
logic into a single shared utility and import it from c4Terminal and c4Browser
to keep the shape files focused only on their unique drawing code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a96b4b07-ba16-42a2-80b7-709a79d9bd66
📒 Files selected for processing (14)
.cspell/mermaid-terms.txtdocs/syntax/flowchart.mdpackages/mermaid/scripts/docs.spec.tspackages/mermaid/src/diagrams/c4/c4Renderer.tspackages/mermaid/src/diagrams/c4/c4ShapeAdapter.tspackages/mermaid/src/diagrams/c4/styles.jspackages/mermaid/src/rendering-util/rendering-elements/shapes.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4Browser.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4Bucket.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4Database.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4Folder.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4Person.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4Queue.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4Terminal.ts
|
Re the CodeRabbit nitpicks: the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cypress/integration/rendering/c4/c4-characterization.spec.js (1)
127-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelationship characterization tests only assert on snapshots, not the new edge structure.
This PR's core change is migrating
Rel/BiReltoinsertEdge/insertEdgeLabel(dashed stroke, arrowhead, double-arrowhead forBiRel, HTML edge labels). None of the "relationships" tests add DOM assertions for this (e.g.,stroke-dasharray,marker-endon the path, or.edgeLabelpresence), unlike the shape/attribute tests in this same file which do add explicitcy.get(...)checks. Relying solely on image snapshots means a regression in edge rendering (e.g., missing arrowhead, wrong dash pattern) could pass unnoticed if baselines drift or if screenshots aren't diffed carefully in CI.Consider adding at least one structural assertion per relationship test, e.g.:
cy.get('path.relation').should('have.attr', 'marker-end'); cy.get('path.relation').should('have.css', 'stroke-dasharray');🤖 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 `@cypress/integration/rendering/c4/c4-characterization.spec.js` around lines 127 - 182, The relationship characterization tests in the relationships describe block rely only on imgSnapshotTest, so they do not verify the new edge DOM structure produced by insertEdge and insertEdgeLabel. Update the CHAR.rel-directions, CHAR.rel-bidirectional, CHAR.rel-back, and CHAR.rel-techn-descr cases to add structural Cypress assertions against the rendered relationship elements (for example checking the relation path for stroke-dasharray and marker-end, and asserting the edge label node exists) in addition to the snapshots. Use the existing imgSnapshotTest calls and nearby cy.get assertions in this spec as the pattern to locate where to add the new checks.
🤖 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.
Nitpick comments:
In `@cypress/integration/rendering/c4/c4-characterization.spec.js`:
- Around line 127-182: The relationship characterization tests in the
relationships describe block rely only on imgSnapshotTest, so they do not verify
the new edge DOM structure produced by insertEdge and insertEdgeLabel. Update
the CHAR.rel-directions, CHAR.rel-bidirectional, CHAR.rel-back, and
CHAR.rel-techn-descr cases to add structural Cypress assertions against the
rendered relationship elements (for example checking the relation path for
stroke-dasharray and marker-end, and asserting the edge label node exists) in
addition to the snapshots. Use the existing imgSnapshotTest calls and nearby
cy.get assertions in this spec as the pattern to locate where to add the new
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6512ae46-3cba-4bc1-8547-ff98f1fa8b56
📒 Files selected for processing (1)
cypress/integration/rendering/c4/c4-characterization.spec.js
Visual proof for this PR's actual diff (not the converged end-state)Same context as my note on #7842: the #7849 tracker's gallery is a preview of the converged end-state combining several future phases (not any single open PR's current diff). This PR had no dedicated visual proof of its own either, so here is one via the same local Note: #7883 is stacked on #7842, so this diff includes #7842's shape changes too, plus #7883's own relationship/edge migration on top - that's expected, not double-counting. 25 of 25 corpus tiles changed (1.8%-14.5%). A few representative tiles, chosen for relationship coverage:
|
| before (develop) | after (this PR, stacked on #7842) | diff |
|---|---|---|
 |
 |
 |
CHAR.rel-directions (6.31%) - every directional Rel
| before | after | diff |
|---|---|---|
 |
 |
 |
CHAR.rel-back (4.72%) - Rel_Back
| before | after | diff |
|---|---|---|
 |
 |
 |
Full 25-tile before/after/diff gallery + summary table: summary.md (rendered HTML at index.html, download to view locally since GitHub serves raw HTML as text).
Scope reminder: boundaries and the legacy grid layout are still untouched by this stack - that's the next slice (layout migration, opens once this lands).
|
Hi @aloisklink - friendly nudge on this one too, since it's stacked on #7842. Same CI pattern: green aside from the expected The full staged plan - what's landed, what's open, and the recommended merge order - is kept up to date in #7849. Thanks for your time! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/c4ShapeAdapter.ts`:
- Around line 116-117: Update the escapeHtml function to encode both
double-quote and single-quote characters in addition to &, <, and >, preserving
the existing escaping behavior. Prefer the vetted sanitization approach already
used by Mermaid if appropriate, otherwise complete the manual entity encoding
within escapeHtml.
In
`@packages/mermaid/src/rendering-util/rendering-elements/shapes/c4LabelHelper.ts`:
- Line 46: Update the wrapWidth calculation in the C4 label helper to use the
configured wrapping width when node.width is undefined, while retaining padding
subtraction and MIN_WRAP_WIDTH clamping. Follow the fallback behavior used by
the standard labelHelper and preserve the existing width-based calculation for
explicitly sized nodes.
🪄 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: eab4e03f-2fae-43bb-a1fc-ca562a88a8c9
📒 Files selected for processing (19)
.changeset/c4-unified-shapes.md.changeset/person-shape.mdcypress/integration/rendering/c4/c4-characterization.spec.jscypress/integration/rendering/c4/c4.spec.jscypress/integration/rendering/newShapes.spec.tsdocs/config/setup/mermaid/interfaces/LayoutData.mddocs/syntax/c4.mddocs/syntax/flowchart.mdpackages/mermaid/scripts/docs.spec.tspackages/mermaid/src/diagrams/c4/c4ShapeAdapter.tspackages/mermaid/src/diagrams/c4/styles.jspackages/mermaid/src/docs/syntax/c4.mdpackages/mermaid/src/rendering-util/rendering-elements/shapes.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/c4LabelHelper.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/cylinder.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/person.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/tiltedCylinder.tspackages/mermaid/src/rendering-util/rendering-elements/shapes/util.tspackages/mermaid/src/rendering-util/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/syntax/flowchart.md
|
CI note: the e2e (4) failure on the latest push is an unrelated flake - Cypress reports the Chrome renderer process crashed while starting rendering/iconShape-square.spec.ts (0 assertions failed; the other specs in the shard passed 332/332). I cannot re-run the job from a fork; a maintainer re-run of the failed job should bring e2e-required back to green. |
|
Reopened - this was closed in error a few minutes ago, not intentionally. Nothing about the PR changed; sorry for the noise on your notifications, @aloisklink @sidharthv96. While I am here, the useful update: now that #7842 has merged, this PR stands on its own. It no longer carries the 20 shape-migration commits, and the diff is down from 23 files to 3: That is the whole relationship migration: relationships render through the unified edge renderer (dashed lines, arrowheads, This is the last slice before the layout migration, which replaces the legacy |
c655077 to
4078c97
Compare
|
Rebased onto current The tree is byte-identical to the previous head - the series was rebuilt from that exact tree rather than replayed, and Still 3 files, +113/-42. |
|
This PR has not had a human review yet. Since #7842 merged it stands alone at 3 files, +113/-42, and it is the last dependency of the C4 layout slice. One thing to know before opening the checks: the Full stack status: #7849 (comment) |
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this, @filipsajdak — the edges slice of the C4 migration is a pleasure to read. Now that #7842 has merged, the diff is a tidy 3 files and the intent is very easy to follow. I did a close read of the renderer changes plus a full trace through the shared edge-label pipeline (and an independent XSS pass). A few things to tighten up before merge, but this is close.
🎉 What's working well
- 🎉 [praise] The migration is genuinely well-documented. The comments explaining why — drawing edges/labels on top of shapes so the line is never hidden, the
skipIntersectrationale for the node stubs, and the note on why you set the labeltransformdirectly instead of viapositionEdgeLabel— are exactly the kind of context that saves the next maintainer an hour. Thank you for writing those. - 🎉 [praise] Security-conscious label building:
escapeHtmlis applied to every user-controlled field (label.text,techn.text,descr.text) before it's wrapped in markup, and the values only ever land in element-content position. I traced the full pipeline (buildEdgeLabel→insertEdgeLabel→createText→nonMarkdownToHTML→decodeEntities(markers-only, doesn't decode<) →sanitizeText/DOMPurify →span.html()), plus the final whole-SVG sanitize, and an independent security review agreed: no XSS/injection vector. Nicely done. - 🎉 [praise] Good housekeeping: removing the now-dead
messageFonthelper and the four legacysvgDraw.insertArrow*defs is safe (only the retiredsvgDraw.drawRelsreferenced those markers), and thegetIntersectPointsrewrite withPointobjects is cleaner and behaviorally equivalent to the old version. - 🎉 [praise]
UpdateRelStyleoffsets/colours andbirelbidirectional arrowheads are both honoured, and the defensivetoOffsetcoercion (strings vs numbers depending on positional vs named args) shows you've thought about the db's quirks.
Things to address
-
🟡 [important] — the
getIntersectPointscomment describes behaviour the code doesn't have.c4Renderer.ts(the block added abovegetIntersectPoints) says it "Uses the drawn shape's ownnode.intersect(its real outline) when available, so arrows meet the person/cylinder/queue silhouette rather than a bounding box; falls back to the rectangular approximation…". But the function only ever calls the rectangulargetIntersectPointfor both endpoints — there is nonode.intersectanywhere in the C4 folder (grep finds zero references), andinsertEdgeis called with identitynodeStubs +skipIntersect: true, so no real-outline clipping happens either. As written, arrows meet the bounding box, not the silhouette. Could you either wire up thenode.intersectlookup the comment promises, or correct the comment to match reality? Right now it'll actively mislead. -
🟡 [important] —
htmlLabels: falserenders the markup as literal text.buildEdgeLabelemits<b>…</b>,<small><i>…</i></small>,<br/>. WhenhtmlLabelsis disabled globally,createTexttakes the non-HTML branch (nonMarkdownToLines→tspan.text()/ textContent), which doesn't interpret those tags — so a relationship label shows up as the literal string<b>Uses</b>,<small><i>[HTTPS]</i></small>, etc. That's a regression from the legacy renderer, which drew plain readable text. Worth gatingbuildEdgeLabelongetEffectiveHtmlLabels(config)(emitting a plain-text label when HTML labels are off), or switching tolabelType: 'markdown'with**name**so both branches render correctly. A small e2e case withhtmlLabels: falsewould lock this down. -
🟡 [important] — no changeset. This is a user-visible rendering change (dashed relationship lines, HTML-formatted labels, new arrowheads, offset/colour support) under a
feat(c4)title, so it should carry a changeset (pnpm changeset, likelyminor). If the umbrella plan (#7849) intends a single changeset for the whole migration at the end, no problem — just confirm that's the plan so it doesn't fall through the cracks.
Nits (non-blocking)
- 🟢 [nit] — CSS injection hardening on the colour values.
style: ['stroke:${lineColor}']andcssStyles: ['color:${textColor}']interpolate rawUpdateRelStylevalues into a CSS declaration string. It's not script-executable and is blocked by GitHub/GitLab CSP, so no security blocker — but it does widen the surface vs the legacy presentation-attribute approach (a value likered;background:url(https://…)injects extra declarations). Consider validating withCSS.supports('color', value)or a small colour allowlist before interpolating, and dropping anything containing;/url(//*. - 🟢 [nit] — edge-label map retention.
insertEdgeLabelstores each label in the module-leveledgeLabelsMap keyed byedge.id, but C4 never callsedges.jsclear()(norpositionEdgeLabel). Harmless since ids are unique per render, but it retains stale d3 selections across renders. Minor. - 🟢 [nit] —
edgeLabels.node()?.lastElementChildis a bit fragile. It's correct for C4 today (only a mid-label, no start/end terminal labels), and the comment explains the rationale, so this is fine — just flagging that it assumesinsertEdgeLabelappends exactly one direct child.
Tests
The existing C4 e2e specs (C4.1–C4.6) already exercise Rel, UpdateRelStyle, and BiRel, so Argos will capture this migration's visual change against the baselines — good coverage for the happy path. The two gaps worth a dedicated case are the htmlLabels: false behaviour above and a label containing special characters (<, &, quotes) to pin the escaping.
None of this is far off — mostly the comment/behaviour mismatch, the htmlLabels: false path, and a changeset. Happy to re-review as soon as those are addressed. 🙏
4078c97 to
8c8e95e
Compare
|
Thank you - and thank you especially for tracing the label pipeline end to end rather than eyeballing the escaping. That is a lot of work to do on someone else's PR. All three important items and the first nit are addressed. One of them I have to correct, though the mistake behind it was mine. 🟡 The
|
|
Note on the red The step failed while
|
|
Merged current One resolution worth recording, since it is a semantic merge rather than a textual one. Both sides changed
Neither side's version is the answer on its own, so the widening is applied to the rewritten function. 132 C4 unit tests pass; tsc, eslint and Prettier clean. Merged rather than rebased, and the push is a fast-forward. |
|
Merged develop for the Cypress-to-Playwright migration. This branch adds no e2e specs, so the only conflict was the generated One note in case it recurs elsewhere: autofix.ci had pushed a regeneration of that page computed against the pre-merge base, so its line numbers disagreed with develop's post-merge ones. Develop's are current and autofix re-runs on the push. I did not regenerate locally - a local 132 C4 unit tests pass. |
|
Gentle nudge - no rush. The requested changes were addressed on 2026-08-20 and all review threads are resolved; the only red check is |
|
Same ask as #7859: could a maintainer dismiss or refresh the All three "things to address" are in, and each is checkable:
The Branch Also correcting the PR body: it still says "stacked on #7842", which merged on 2026-07-30. This is standalone against |
|
Correcting one line of my comment above, because it undersells what happened and a maintainer reading it would check the wrong thing. I wrote that the The PR description said the same thing all along and is therefore accurate; I have only corrected its stale opening line, which still claimed this was stacked on #7842 (merged 2026-07-30). It is standalone against Sorry for the double post - better to have the record right than tidy. |
01f3e5b to
73b60ba
Compare
The second slice of the in-place migration, after elements moved in mermaid-js#7842. Relationships now go through the shared edge renderer: dashed lines, arrowheads, labels and the offsets and colours `UpdateRelStyle` sets, instead of the legacy renderer's own line drawing. Two things the shared path needs that the legacy one did not: A label is built for the effective `htmlLabels` setting, because `createText`'s non-markdown branch emits tags as ordinary words - with HTML labels off, markup rendered as literal characters. Unit tests cover both forms and a hostile string. A colour from `UpdateRelStyle` is checked with `CSS.supports('color', ...)` before it reaches a style string, so a value carrying a `;` cannot corrupt the declaration it lands in.
73b60ba to
05df8c2
Compare
|
The knsv-bot review from 2026-08-10 still shows changes requested. Its three blocking items are addressed on the current head (
Happy to add the two e2e fixtures if a maintainer wants them in this PR. |
This branch has not been deployed
3 participants
What this is
The edges slice of the in-place C4 renderer migration (plan: #7849). Standalone against
develop. (It was stacked on #7842, the shapes slice, which merged on 2026-07-30; the diff is now this PR's own 5 files, +202/-42.) Its commits are:feat(c4): render C4 relationships through the unified edge renderingstyle(c4): dashed relationship lines and labelsfeat(c4): honour UpdateRelStyle offsets and colours for relationship labelsfix(c4): place relationship endpoints via each shape's intersectionThis slice: migrate relationships
The legacy
svgDrawrelationship drawing is replaced with mermaid's unified edge renderer (insertEdge/insertEdgeLabel): eachRelbecomes a dashed edge with an arrowhead and an HTML label (BiRel-> arrowheads at both ends), matching c4model.com. The legacy grid still supplies the geometry (a straight line between the two shapes' boundary intersection points), fed toinsertEdgewithskipIntersect. Those endpoints now come from each shape's ownnode.intersect(its real outline), so arrows meet the person/cylinder/queue silhouette instead of a bounding box, with the rectangular approximation kept as a fallback. Boundaries and the grid layout are untouched - the layout slice is next.UpdateRelStyleis honoured:$offsetX/$offsetYposition the label (nudging it off nodes and separating parallel relationships) and$textColor/$lineColorrecolour it. (The db stores named offsets as strings, so the renderer coerces them.)Impact / how to see it
No flag - the existing C4 cypress baselines now route relationships through the unified renderer, so Argos shows the relationship change per diagram. Layout/positions are unchanged from the shapes slice.
Note
These
$offsetnudges are a legacy-syntax crutch; real auto-spacing arrives with the layout slice (dagre). Honouring them here keeps parity with the legacy renderer (no regression).Summary
Migrates C4 relationship rendering to Mermaid’s unified edge system as part of an in-place C4 renderer migration, replacing legacy
svgDrawrelationship drawing withinsertEdgeandinsertEdgeLabel. Relationships now render as dashed edges with arrowheads (includingBiRelarrowheads on both ends) and relationship labels are built/rendered through unified edge-label primitives. The legacy grid continues to supply relationship geometry and boundary intersection points, while layout/boundary recursion ordering is preserved viaasyncrendering.Highlights
Unified C4 relationship rendering
svgDrawtoinsertEdge/insertEdgeLabel.BiRelsupports arrowheads at both ends.UpdateRelStylefor label offsets and color, including coercing stored string$offsetX/$offsetYvalues into numeric offsets when needed.drawC4ShapeArray,drawRels, and boundary recursion are converted toasyncto ensure computed bounds/intersections exist before subsequent drawing steps.New unified C4 shape/label adapters
c4ShapeAdapter.tsto adapt legacy C4 node/edge inputs into unified-renderer shapes/labels.shape/sprite→tags→ legacytypeC4Shapemapping (including external variants) → defaultrounded.buildC4Nodewith computed CSS classes/styles and legacy layout fields retained for geometry.c4LabelHelper.tsfor stacked C4-style labels as pure SVG text (name/type/stereotype + multi-line description), including wrapping behavior constrained byc4.width.labelHelperto usec4LabelHelperwhen stereotyping is present.stereotype?: stringinrendering-util/types.ts.C4 styling + edge/label CSS alignment
getStyles()with a fuller C4 stylesheet insrc/diagrams/c4/styles.js.getConfig().c4.Person shape enablement
personshape handler to the unified shapes registry (rendering-util/rendering-elements/shapes.ts).personrenderer (person.ts) using unified node/label helpers, including support for hand-drawn (look === 'handDrawn'/ roughjs) rendering modes.personshape and updates shape documentation snapshots accordingly.Related shape/layout correctness tweaks
cylinder.tsandtiltedCylinder.tsdimension normalization to improve width/height edge cases when only one dimension is provided.Docs/tests/changesets updated
wrap: true+c4.width).C4.7: per-elementpersonFontFamily/personFontSizesnapshot variationsC4.8:c4.widthwrapping vs non-wrapping behaviordocs.spec.tsinline snapshots by regenerating the shapes table after introducing C4c4-*shapes and related shape docs.c4-unified-shapes) and updates the cspell dictionary withStructurizr.Testing
multi-diagram-id-uniquenessanddocs.spec.developshowed updated output across the C4 corpus tiles (including general/directional andRel_Backrelationship cases).