Repository navigation
fix(c4): keep every relationship that repeats a from/to pair - #8143
filipsajdak wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: fa8c1a2 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: |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughC4 relationship storage now preserves repeated source-target interactions. Relationship styling updates all matching relationships. Parser and end-to-end tests verify rendering, directionality, styling, and relationship metadata. ChangesC4 relationship handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant C4Parser
participant c4Db
participant C4Renderer
C4Parser->>c4Db: Add repeated Rel entries
c4Db-->>C4Parser: Return all relationships
C4Parser->>c4Db: Apply UpdateRelStyle
c4Db->>c4Db: Style every matching relationship
c4Db-->>C4Renderer: Provide retained interactions
C4Renderer-->>C4Renderer: Render labels and sequence numbers
Merge Risk: ⚪ Minimal · up to Repeated C4 relationships are now preserved and styled consistently without changing external access, privileges, services, or deployment behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary, references issue Full details: Linked Issues checkExplanation The PR satisfies the requirement to retain and render all repeated relationships and preserves directional matching. It does not satisfy the linked issue's separate requirement that repeated relationships must not collide visually; the description explicitly defers that requirement to Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8143 +/- ##
===========================================
- Coverage 81.15% 79.50% -1.66%
===========================================
Files 622 600 -22
Lines 86016 77210 -8806
Branches 16421 15710 -711
===========================================
- Hits 69803 61382 -8421
- Misses 15207 15828 +621
+ Partials 1006 0 -1006
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The
#8103 fixes it, by emitting The Playwright case added here passes locally against this branch, and fails against |
|
Regression check, since this changes I rendered all 25 C4 fixtures under That is the expected result: Note there is no Argos build on this PR to compare against - the |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎ Awaiting the start of a new Argos build… |
`addRel` looked for an existing relationship with the same `from` and `to` and wrote into it when one was found, so a second `Rel(a, b, ...)` replaced the first rather than adding to it. A C4Dynamic diagram, where each interaction is numbered and repeated pairs are the normal case, therefore showed only the last of them (mermaid-js#7183). Each statement now creates its own relationship. `UpdateRelStyle` names a from/to pair rather than one line, so it applies to every relationship between that pair; previously it could only ever reach the single merged entry. Note this fixes the data, not the drawing: the legacy renderer places both lines by the same grid rule, so two relationships between one pair still land on top of each other. They separate once the layout slice (mermaid-js#8042) routes C4 edges through the unified pipeline, which spaces parallel edges. 127 existing C4 tests pass; 4 new ones cover the repeated pair, the two directions of a pair, and UpdateRelStyle reaching both.
The unit tests cover the db, but the report in mermaid-js#7183 is about what reaches the SVG, so assert that there too. On develop the same diagram renders a single line reading `1: Interaction 2` - the first statement is gone and the numbering has closed over the gap - so this fails without the fix. Both lines share a grid position, so the assertion is that each is drawn and numbered rather than where it sits.
4dcc911 to
6fc4d6c
Compare
|
@pbrolin47 this is ready for review: CI is green apart from Argos, and it is mergeable. |
# Conflicts: # e2e/rendering/c4/c4-characterization.spec.js
…sed e2e layout The case asserting that two Rels between one pair both render lived in the c4-characterization spec that the e2e restructuring deleted. The render becomes a pr-check fixture (a bug fix gets a reproducing fixture in the smoke tier). A fixture cannot assert on SVG text, so a small pr-check spec renders the same source without a screenshot and keeps the check that both "1: Interaction 1" and "2: Interaction 2" are drawn. The parse side stays covered by c4Rel.spec.ts.
Fixes #7183.
C4Dynamicnumbers its interactions, so several relationships between the same pair are the normal case rather than an edge case. Only the last of them survived parsing.The defect
c4Db.addRellooked for an existing relationship with the samefromandtoand wrote into it when it found one:So the second
Rel(a, b, "Interaction 2")overwrote the first. Rendering @aristotelos's diagram ondevelopproduces a single line reading1: Interaction 2- the first statement's label is gone, and because the number is a positional index overrels, the numbering silently closes over the gap. The relationship never reaches the renderer, which is why no amount of styling brings it back.The change
Each statement creates its own relationship. That is the whole fix; nothing downstream needed touching, because the
C4Dynamiccounter inc4Renderer.tsis a plain index overrelsand starts numbering1:,2:correctly once the entries exist.UpdateRelStyle(a, b, ...)finds its target the same way, by from/to, so once a pair can name several relationships that lookup is ambiguous. It now applies to every relationship between the named pair. @pbrolin47 chose those semantics over first-match-only on the issue:Direction still matters:
UpdateRelStyle(a, b, ...)does not reach aRel(b, a, ...). That is unchanged, and covered by a test.Evidence
Four unit tests in the new
parser/c4Rel.spec.tscover the repeated pair, the two directions of a pair staying separate, andUpdateRelStylereaching both relationships.The report is about what reaches the SVG, so there is a browser-level assertion too -
CHAR.dynamic-repeated-pairine2e/rendering/c4/c4-characterization.spec.jsasserts both1: Interaction 1and2: Interaction 2are present. It fails ondevelopwith exactly the symptom above:131 C4 unit tests pass.
What this does not fix
The issue asks for two things, and this delivers one of them. All the relationships are now rendered, but they do not yet avoid each other: the legacy grid places both lines by the same rule, so they land on top of one another - correctly numbered
1:and2:, at identical coordinates.They separate under #8042, which routes C4 edges through the unified pipeline. Measured, rather than assumed: applying this change on that branch and rendering the same diagram gives two distinct paths,
about 13px apart at the endpoints, with both labels drawn. So the complete answer to the report is this change plus that one, and this half is independent of it and worth having on
developnow.What's working well
C4Dynamicrelationships remain distinct.UpdateRelStyleapplies to all matching directed relationships.Security
No XSS or injection issues identified. The changes do not alter SVG sanitization or introduce new output sinks.
Things to address
No blocking or important issues found. Visual path separation remains out of scope and is tracked by
#8042.Verdict
APPROVE
Severity tally: 0 blocking / 0 important / 0 nit / 0 suggestion / 4 praise