fix(venn): synthesize implied pairwise subsets for higher-arity unions#7758
fix(venn): synthesize implied pairwise subsets for higher-arity unions#7758mk24x7 wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 4e51e97 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: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7758 +/- ##
==========================================
- Coverage 3.26% 3.26% -0.01%
==========================================
Files 599 599
Lines 60839 60862 +23
Branches 917 917
==========================================
Hits 1986 1986
- Misses 58853 58876 +23
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 ↗︎
|
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this, @mk24x7 — this is a really well-reasoned fix, and the PR description does an excellent job explaining why the bug happens (venn.js needs pairwise intersection constraints to lay out overlapping circles) rather than just what changed. 🎉
I checked this out locally and verified the behaviour:
expandImpliedSubsetsunit tests: all pass with the fix; all 6 fail without it (function not exported) — they genuinely exercise the new logic.- Cypress visual tests #17/#18 are the right verification for a rendering change and cover the issue's exact 3-way input plus a 4-way case.
- Ran a dedicated XSS/injection pass: no issues. Synthesized entries carry
label: undefinedandsetscopied from already-parsed identifiers; they only reach venn.js geometry and D3.text()/textContentsinks. No DOMPurify/.html()/attribute changes.
What's working well
- 🎉 [praise] Implementing this as a pure, exported
expandImpliedSubsets(subsets)transform keeps the parser/DB untouched and makes the logic trivially testable. Returning the same array reference on the no-op path (and a fresh array only when expanding) is a nice touch. - 🎉 [praise] Thorough unit coverage: no-op identity, all-pairwise-declared, bare 3-way, partial-preservation, bare 4-way
C(N,2), and default size/label. The dedup correctly accounts for pairs shared across multiple higher-arity unions. - 🎉 [praise] Honest, precise PR write-up — the explicit caveat about 4+ way label visibility and the rationale for synthesizing only pairwise (not intermediate arities) show good judgement about venn.js's actual constraints.
- 🎉 [praise] Changeset present with a clear description, correct
patchbump andfix:prefix, links the issue.
Things to address
-
🟡 [important] The render-level jsdom test
renders label for 3-way union without explicit pairwise unions (issue #7656)(vennRenderer.spec.ts:~167) passes even without the fix — I verified by reverting onlyvennRenderer.tsand running just that test (it stays green). venn.js emits the.venn-intersection textelement for any declared labeled union regardless of whether the circles actually overlap, so asserting only on label-text presence doesn't distinguish fixed from broken behaviour. The commit message describes this test as "verifying the label renders for the reporter's exact input," but as written it gives false confidence and won't catch a regression whereexpandImpliedSubsetsstops being applied. It would be great to strengthen the assertion to verify the actual effect — e.g. that the implied pairwise subsets reach the layout / that the three circles genuinely overlap (non-degenerate pairwise intersection geometry), or assert on the.venn-intersectionpath forA∩B∩Chaving real area. TheexpandImpliedSubsetsunit tests and the Cypress visual tests do capture the bug, so this is about closing a misleading-coverage gap, not the correctness of the fix itself. -
🟢 [nit]
defaultPairSize = 10 / 4(vennRenderer.ts:~39) is a magic number. It intentionally mirrorsvennDB.addSubsetData's default for a 2-way subset (10 / Math.pow(identifierList.length, 2)with length 2). A short inline comment, or deriving it from the same expression / a shared constant, would prevent silent drift if that default ever changes. -
🟢 [nit] Docs:
packages/mermaid/src/docs/syntax/venn.mdonly shows 2-setunionexamples and the PR's docs checkbox is unchecked. Nothing in the docs is now wrong (there's no obsolete "declare the pairwise unions manually" workaround note to remove), but since this fix makes a bare 3-way union actually render correctly, adding a 3-set example (the issue's "Innovation" diagram is perfect) would round it off nicely. Not blocking — this is a bug fix, not a new feature. -
💡 [suggestion] Out of scope / pre-existing: the
sets.join('|')keying (shared withstableSetsKey/buildStyleByKey) is ambiguous if a set identifier contains a literal|. This PR neither introduces nor worsens it (synthesized entries only add geometry withlabel: undefined, and the security pass confirmed no injection risk), but an escaped/non-printable delimiter across the venn renderer would be a reasonable future hardening.
The fix itself is correct and well-targeted, and it's verified at both the unit and visual level — the only real ask is tightening that one render-level test so it actually guards the behaviour it claims to. Let's get this across the finish line. 🙏
Address review feedback on PR mermaid-js#7758. - vennRenderer.spec.ts: the existing jsdom render test for issue mermaid-js#7656 only asserted that the "Innovation" label appeared in the DOM, which venn.js emits for any declared labeled union regardless of whether the circles actually overlap. The test passed even with expandImpliedSubsets bypassed. Replace it with an assertion on the rendered geometry: the three implied pairwise intersections (A-B, A-C, B-C) must be present in addition to the user-declared A-B-C intersection, and the 3-way intersection path must not be the degenerate "M 0 0" placeholder venn.js emits when an area has no visible geometry. Verified to fail when the fix is bypassed. - vennRenderer.ts: extract the synthesized-pair default size into a defaultSubsetSize(arity) helper using the same expression as vennDB.addSubsetData (10 / arity^2), and add a comment so the two defaults stay in sync. - docs/syntax/venn.md: add a "Higher-arity unions" section with a 3-set Desirable/Feasible/Viable Innovation example, since bare 3-way unions now render correctly.
|
Thanks for the careful review. Addressed all three points in [important] Misleading jsdom render test. You're right that the old assertion just checked for the Replaced the assertion to verify the layout actually overlaps, via two complementary DOM-level checks:
Confirmed the new test fails with [nit] Magic number [nit] Docs. Added a "Higher-arity unions" section to [suggestion] Let me know if anything else needs attention. |
|
Thanks @mk24x7 for addressing the issues in commit 2d6527f! Some additional feedback, with just a minor thing : What's working well 🎉 The second commit addresses every item from the prior review cleanly and completely. 🎉 expandImpliedSubsets is a well-scoped pure function — easy to reason about, easy to test, zero DOM involvement. 🎉 The render-level test now properly guards the fix: it asserts that all three implied pairwise paths (A|B, A|C, B|C) are present and that the 3-way intersection path is 🎉 defaultSubsetSize(arity: number) => 10 / Math.pow(arity, 2) — extracting this as a named helper and aligning it with the formula in vennDB.addSubsetData means the two 🎉 The second commit addresses every item from the prior review cleanly and completely. 🎉 expandImpliedSubsets is a well-scoped pure function — easy to reason about, easy to test, zero DOM involvement. 🎉 The render-level test now properly guards the fix: it asserts that all three implied pairwise paths (A|B, A|C, B|C) are present and that the 3-way intersection path is not the degenerate 'M 0 0' 🎉 defaultSubsetSize(arity: number) => 10 / Math.pow(arity, 2) — extracting this as a named helper and aligning it with the formula in vennDB.addSubsetData means the two defaults now stay in sync. 🎉 The "Higher-arity unions" docs section is clear with a concrete Desirable/Feasible/Viable["Innovation"] example — exactly the pattern reported in the issue. Minor observation (non-blocking) 🟢 [nit] docs/syntax/venn.md (the top-level generated folder) is committed alongside the source at packages/mermaid/src/docs/syntax/venn.md. The top-level /docs folder is auto-generated — in CI
|
2d6527f to
2952083
Compare
Address review feedback on PR mermaid-js#7758. - vennRenderer.spec.ts: the existing jsdom render test for issue mermaid-js#7656 only asserted that the "Innovation" label appeared in the DOM, which venn.js emits for any declared labeled union regardless of whether the circles actually overlap. The test passed even with expandImpliedSubsets bypassed. Replace it with an assertion on the rendered geometry: the three implied pairwise intersections (A-B, A-C, B-C) must be present in addition to the user-declared A-B-C intersection, and the 3-way intersection path must not be the degenerate "M 0 0" placeholder venn.js emits when an area has no visible geometry. Verified to fail when the fix is bypassed. - vennRenderer.ts: extract the synthesized-pair default size into a defaultSubsetSize(arity) helper using the same expression as vennDB.addSubsetData (10 / arity^2), and add a comment so the two defaults stay in sync. - docs/syntax/venn.md: add a "Higher-arity unions" section with a 3-set Desirable/Feasible/Viable Innovation example, since bare 3-way unions now render correctly.
|
Thanks @pbrolin47 for taking a look! On the docs nit: I checked locally and the generated I should have surfaced that the docs were regenerated rather than hand-edited in the original PR description though, since "manual edit to the generated folder" was a reasonable read from the outside. Will be more explicit about that next time. Also rebased on |
The underlying venn.js layout requires explicit pairwise intersection sizes to make circles overlap. When a user declares only a higher-arity union (for example, `union A,B,C[label]`) without the underlying pairwise unions, the layout has no overlap constraints, the circles render disjointly, and there is no intersection region for the label. Before passing subsets to venn.js, synthesize any missing pairwise subsets implied by higher-arity unions. User-declared subsets keep their sizes, labels, and styles; only the missing implied pairs are filled in, with a default size. Adds: - expandImpliedSubsets helper in vennRenderer.ts (exported for tests) - unit tests covering no-op, 3-way, 4-way, and partial-overlap cases - a render-level jsdom test verifying the label renders for the reporter's exact input - Cypress visual tests for bare 3-way and 4-way unions Fixes mermaid-js#7656
Address review feedback on PR mermaid-js#7758. - vennRenderer.spec.ts: the existing jsdom render test for issue mermaid-js#7656 only asserted that the "Innovation" label appeared in the DOM, which venn.js emits for any declared labeled union regardless of whether the circles actually overlap. The test passed even with expandImpliedSubsets bypassed. Replace it with an assertion on the rendered geometry: the three implied pairwise intersections (A-B, A-C, B-C) must be present in addition to the user-declared A-B-C intersection, and the 3-way intersection path must not be the degenerate "M 0 0" placeholder venn.js emits when an area has no visible geometry. Verified to fail when the fix is bypassed. - vennRenderer.ts: extract the synthesized-pair default size into a defaultSubsetSize(arity) helper using the same expression as vennDB.addSubsetData (10 / arity^2), and add a comment so the two defaults stay in sync. - docs/syntax/venn.md: add a "Higher-arity unions" section with a 3-set Desirable/Feasible/Viable Innovation example, since bare 3-way unions now render correctly.
Head branch was pushed to by a user without write access
e87bdee to
4e51e97
Compare
|
Rebased onto latest |
|
Heads up - looks like the rebase force-push automatically disabled the auto-merge @ashishjain0512 had enabled (GitHub safety policy on non-collaborator pushes). Sorry about that. Could you re-enable auto-merge or merge manually when convenient? No code change since approval - just a rebase onto latest |
📑 Summary
venn.jsneeds pairwise intersection sizes to make circles overlap. When the user declares only a higher-arity union (union A,B,C[label]) without the underlying pairwise unions, the circles render disjointly and there is no intersection region for the label.Before passing subsets to venn.js in
vennRenderer.ts, synthesize any missing pairwise subsets implied by higher-arity unions. User-declared subsets are untouched; only the missing pairs get filled in with a default size.Resolves #7656.
📏 Design Decisions
vennRenderer.ts(expandImpliedSubsets), keeping the parser and DB untouched. Happy to move it intovennDB.tsif you prefer that location.10/4, matching the existing 2-way default) andundefinedlabel. The existing unstyled-intersection rendering path makes them transparent.@upsetjs/venn.jsupstream, but that repo has been inactive since November 2024, and the fix is achievable cleanly on the Mermaid side.Caveat: 4+ set unions now render as overlapping circles, but venn.js still does not produce a clean centermost text region for them, so the top-level label on a bare 4+ way union may not be visible. Strictly better than the previous disjoint layout, but a full fix would need work in venn.js itself.
📋 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:.