feat: add Cynefin framework diagram type#7535
Conversation
Add a Cynefin framework diagram type for categorizing problems by complexity domain. Five fixed domains (Complex, Complicated, Clear, Chaotic, Confusion) with wavy organic boundaries, items placed within domains, and transition arrows between them. Features: - Langium grammar with domain blocks, items, and transitions - Wavy SVG bezier boundaries (deterministic via seeded PRNG) - The "cliff" between Clear and Chaotic - Domain labels with decision model and practice type subtitles - Item badges stacked within each domain - Curved transition arrows with optional labels - Full theme integration (5 themes with domain-specific colors) - Schema-driven config (width, height, padding, boundaryAmplitude) - Accessibility (ARIA roles, labels) Ref: mermaid-js#7534
🦋 Changeset detectedLatest commit: 48d65d0 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 #7535 +/- ##
==========================================
+ Coverage 3.31% 3.32% +0.01%
==========================================
Files 543 555 +12
Lines 57217 57919 +702
Branches 840 867 +27
==========================================
+ Hits 1898 1928 +30
- Misses 55319 55991 +672
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- 19 unit tests: DB operations, boundary path generators, PRNG determinism - 13 integration tests: full parse-to-db pipeline for domains, items, transitions, accessibility, complex diagrams, re-parse after clear
There was a problem hiding this comment.
[sisyphus-bot]
Review: feat: add Cynefin framework diagram type
Hey @ragelink — first off, thanks for taking this on. A text-based DSL for Cynefin is a genuinely novel contribution, and the amount of thought that went into the domain modeling (wavy boundaries, the cliff, confusion overlay, deterministic PRNG) is impressive. Sorry this sat without review for almost two weeks — let's get this across the finish line.
This is a substantial, well-structured PR. There are a few blocking items that need addressing before merge, but the architecture is sound and the path forward is clear. They are mostly admin pieces but for us they are important to keep the flow going.
What's working well
🎉 [praise] Langium grammar — correct choice for new diagrams, and the grammar is clean and expressive. The DOMAIN_NAME terminal constraining valid domain names at the grammar level is a smart design choice.
🎉 [praise] Deterministic boundary rendering — The seeded PRNG (cynefinBoundaries.ts) ensures reproducible wavy boundaries while still looking organic. Well-tested with proper determinism assertions.
🎉 [praise] Comprehensive test coverage for a first pass — DB unit tests, boundary path tests, full integration parsing tests (basic, transitions, accessibility, complex diagrams, clear-and-reparse), and multi-diagram ID uniqueness coverage. Strong foundation.
🎉 [praise] Clean diagram architecture — follows the radar/wardley pattern nicely: parser → db → renderer pipeline, proper clear() reset, Langium module registration, lazy-loaded detector. No cross-diagram imports.
🎉 [praise] Security — All user text inserted via D3's .text() (safe text nodes), no .html()/innerHTML, no string interpolation into SVG markup, Langium grammar constrains input. DOMPurify pipeline is intact. Clean bill of health.
Blocking items
🔴 [blocking] Missing Langium generated files
The Cynefin grammar is added to langium-config.json and module.ts imports CynefinGrammarGeneratedModule from ../generated/module.js, but the generated files in packages/parser/src/language/generated/ were not regenerated. The index.ts exports (Cynefin, isCynefin, isDomainBlock, isDomainItem, isTransition, CynefinGrammarGeneratedModule) don't exist in the current generated code. Please run:
pnpm --filter parser langium:generateand commit the regenerated files.
🔴 [blocking] Missing Cypress e2e visual regression tests
The parser/DB coverage is solid. But new diagram types require visual regression tests in cypress/integration/rendering/ using the imgSnapshotTest() helper. This is one of the project's most important quality safeguards — it catches rendering regressions across themes and configurations. The test page (test-cynefin.html) already has 4 great test scenarios that could be adapted directly. Reference: any existing e2e test like cypress/integration/rendering/radar.spec.js.
🔴 [blocking] Missing changeset
No changeset file is included. For a new feature:
pnpm changeset
# Select: packages/mermaid, minor bump
# Message: feat: add Cynefin framework diagram typeImportant items
🟡 [important] Renderer ignores config values
The config schema defines width, height, padding, showDomainDescriptions, and boundaryAmplitude — but the renderer hardcodes width = 800, height = 600, padding = 40 at cynefinRenderer.ts:72-74 and never reads from db.getConfig(). The showDomainDescriptions and boundaryAmplitude settings have no effect. It would be great to wire these up so users can actually configure the diagram.
🟡 [important] Renderer uses hardcoded colors instead of theme variables
cynefinRenderer.ts:55-61 defines DOMAIN_COLORS with hardcoded hex values, and item badges at lines 207-215 use hardcoded #ffffff, #cccccc, #333333. Meanwhile, all 5 theme files define cynefin-specific variables (complexBg, complicatedBg, chaoticBg, clearBg, confusionBg, textColor, labelColor) — but these are never consumed by the renderer or styles.ts. This means switching themes won't actually change domain colors. Worth wiring the theme variables through so the diagram respects theme selection.
🟡 [important] Missing documentation
New diagram types need docs in packages/mermaid/src/docs/. Use MERMAID_RELEASE_VERSION as the version placeholder in any "Available since" references. The syntax examples in the PR body and test-cynefin.html are a great starting point.
🟡 [important] test-cynefin.html committed to package root
packages/mermaid/test-cynefin.html looks like a development/debugging file. It uses securityLevel: 'loose' and imports from ./dist/. This probably shouldn't be committed — consider removing it or moving the test scenarios into the Cypress e2e tests instead.
Nits and suggestions
🟢 [nit] cynefinDb.ts:31,46 — setDomains(blocks: any[]) and setTransitions(transitions: any[]) use any[]. Consider using the Langium-generated AST types for stronger type safety.
💡 [suggestion] Handdrawn/rough mode — Not a blocker, but it would be good to explicitly note whether handdrawn mode is supported or not. If it can't be supported yet, a brief mention in the docs is sufficient so users know.
💡 [suggestion] config.type.ts regeneration — The changes to config.type.ts look correct and properly generated from the schema. Just double-check you ran pnpm run --filter mermaid types:build-config rather than editing manually — CI will verify this.
Self-check
- At least one 🎉 praise item (5 total)
- No duplicate comments
- Severity tally: 3 🔴 / 4 🟡 / 1 🟢 / 2 💡 / 5 🎉
- Verdict matches criteria: REQUEST_CHANGES (3 🔴 blocking)
- Not a draft PR
- Tone check: constructive and appreciative ✅
|
Thank you for the thoughtful reply and consideration. I've been a longtime fan of mermaid and have been eager to contribute for a while. I even tried another PR for Wardley Maps (another diagramming construct I find very useful, but I was a bit late on that one: ragelink#1). I'm looking forward to helping out and eager for feedback, so please let me know if there's anything else I can do to make this more valuable to the community. |
|
Quick update: I'm working on the review items now. Should have an updated push soon with the changeset, e2e tests, config and theme wiring, docs, and the rest. Thanks for your patience! |
Review items addressed: - Add changeset for the cynefin feature (minor bump) - Add Cypress e2e visual regression tests covering basic, transitions, empty domains, many items, config overrides, theme overrides, zero amplitude, and accessibility - Wire all config values into the renderer (width, height, padding, showDomainDescriptions, boundaryAmplitude); these now flow from the schema through db.getConfig() at render time - Wire theme variables into the renderer for all colors (domain backgrounds, boundaries, cliff, arrows, text); item badges now use per-domain colors instead of hardcoded values - Pass boundaryAmplitude to the boundary path generators so users can control or disable waviness - Replace any[] with proper Langium AST types (DomainBlock, Transition) in cynefinDb.ts and the unit test helpers - Type getConfig() return as Required<CynefinDiagramConfig> - Add comprehensive syntax documentation at src/docs/syntax/cynefin.md with examples, config table, theme variable table, and accessibility notes - Add cynefin to the vitepress sidebar - Remove the dev-only test-cynefin.html file from the package root
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
Pushed an update addressing the rest of the feedback. One note on the generated files item: Happy to commit them if you'd prefer that for this PR, just let me know. Everything else is in:
|
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for tackling this, @ragelink — and especially for the substantial follow-up work in commit ea1c48f that addressed prior review items (changeset added, Cypress e2e tests written, schema config wired through to the renderer, theme variables threaded across all five themes, dev test file removed, any[] replaced with proper Langium AST types, comprehensive syntax docs added). That kind of responsiveness on a large feature is exactly what makes new diagram types land. This PR is close to ready — most of what's below is final polish and a few correctness edges to tighten before flipping it on.
I'm doing a full New Diagram Type review per the project's contributor checklist, so this is structured rather than narrative. Good news first.
What's working well
🎉 Langium-first. cynefin.langium is the right choice for a new diagram (no JISON), and the grammar is concise and readable. The fixed DOMAIN_NAME terminal is a smart constraint — it prevents typos at parse time and gives the renderer a closed enum to switch on.
🎉 Plugin contract is complete. cynefinDetector.ts (id + detector + loader), cynefinDiagram.ts (parser + db + renderer + styles), cynefinDb.ts, cynefinParser.ts, cynefinRenderer.ts, styles.ts, types.ts — every piece is present and registered in diagram-orchestration.ts:32-115. Examples wired in (packages/examples/src/index.ts), cspell entry added, langium-config entry added, parser exports + parse-dispatcher added in packages/parser/src/parse.ts. Nothing missed in the wiring.
🎉 Excellent test coverage. 19 unit tests for DB/PRNG/path generators in cynefin.spec.ts, 13 integration tests covering parse-to-DB pipeline (basic, transitions, accessibility, complex, re-parse-after-clear) in cynefin.integration.spec.ts, and 8 Cypress visual regression scenarios in cypress/integration/rendering/cynefin.spec.js (basic, transitions, empty, many items, config override, theme override, zero amplitude, accessibility). The Cypress coverage is exactly what the project asks for on new diagrams.
🎉 Theme integration across all five themes. theme-base.js, theme-dark.js, theme-default.js, theme-forest.js, theme-neutral.js each define a complete cynefin block with domain backgrounds, cliff color, boundary color, and font sizes. Color choices visibly reflect each theme's palette (forest has earth tones, dark has saturated brights). This is the part new diagrams most often skimp on, and you didn't.
🎉 Schema-driven config done right. New optional CynefinDiagramConfig in config.schema.yaml with width, height, padding, showDomainDescriptions, boundaryAmplitude. All optional with sensible defaults — non-breaking. Properly threaded through db.getConfig() → renderer at cynefinRenderer.ts:84-93. The Cypress test at cynefin.spec.js:78-87 actually exercises the config override path. Beautiful.
🎉 Determinism via seeded PRNG. seededRandom (mulberry32) + hashString(id) means the wavy boundaries are stable per diagram id. Critical for visual snapshot testing — and the cynefin.spec.ts tests assert the determinism explicitly. Smart.
🎉 Comprehensive docs. packages/mermaid/src/docs/syntax/cynefin.md is 213 lines including conceptual intro, syntax reference, transition examples, configuration table, theme-variable table, accessibility section, and MERMAID_RELEASE_VERSION placeholder usage. Linked sidebar entry added.
Things to address
🟡 [important] — Self-loop transitions produce NaN coordinates
packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:326-356
The grammar allows complex --> complex (the Transition rule has no constraint that from and to differ). When that happens, the renderer computes:
const dx = x2 - x1; // 0
const dy = y2 - y1; // 0
const len = Math.sqrt(dx * dx + dy * dy); // 0
const nx = -dy / len; // NaN
const ny = dx / len; // NaN
const cpx = mx + nx * offsetAmount - padding; // NaN
const cpy = my + ny * offsetAmount - padding; // NaNThe resulting path string M{cx},{cy} Q{NaN},{NaN} {cx},{cy} will silently fail or render an invisible path in most browsers, and the optional label gets placed at NaN. Since users could plausibly draw "self-reflection" transitions on the canonical Cynefin diagram, this is worth a defensive guard. Either:
- Reject self-loops at the grammar/DB level with a helpful error, or
- In the renderer, special-case
len === 0to either skip the transition or draw a small loopback arc above the domain center.
Whichever fits the conceptual model of the diagram better. Test case for either approach should join the integration suite.
🟡 [important] — Item badge width estimated heuristically; will misalign with real labels
packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:276
const textLen = item.label.length * 7 + itemPaddingX * 2;The 7-pixels-per-character estimate is a rough average — fine for short ASCII strings, but it will produce visibly wrong badge widths for:
- Wide characters (W, M, asian glyphs)
- Narrow characters (i, l)
- Different fonts / font sizes (the badge font is configurable via theme)
- Strings shorter than ~3 chars (badges become awkwardly small)
The standard Mermaid pattern is to render the text first, call getBBox() to measure, then size the wrapping rect. See journeyRenderer.js or pieRenderer.ts for examples — the helper renders text invisibly, measures it, then positions the background rect around the measured bounds. Worth investing in the getBBox approach now since item badges are the most prominent UI element of the diagram and contributors will copy this pattern.
🟡 [important] — Confusion domain item placement spills outside the visible ellipse
packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:171-181, 264-303
The confusion ellipse is sized to roughly 10% of width × 10% of height (confusionRx = width * 0.1, confusionRy = height * 0.1 — at the default 800×600, that's an 80×60 ellipse). But items placed in the confusion domain start at layout.cy + (showDomainDescriptions ? 25 : 15) and stack downward at 30px per badge — meaning even one item will extend below the ellipse, and any more than that will appear floating in dead space at the diagram's center.
A few directions to consider:
- Use a larger confusion region (the canonical Cynefin diagram has confusion as a real central area, not a small overlay)
- Constrain the confusion-domain item count visually (collapse to "+N more" or similar)
- Render confusion items differently (e.g., as a stacked list outside the ellipse with a connector)
Whichever direction fits the design intent — the current behavior will look broken with realistic content (and the empty-domain Cypress test passes precisely because no confusion items are placed).
🟡 [important] — Inline .attr() styling duplicates the styles.ts CSS
packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:135-181, 220-242, 281-301, 351-374 and styles.ts:1-104
The renderer applies presentation via D3 attribute calls (.attr('fill', ...), .attr('stroke', ...), .attr('font-size', ...), etc.) and the diagram ships a styles.ts that emits CSS for selectors like .cynefinDomainLabel, .cynefinSubtitle, .cynefinItem text, .cynefinArrowLine, etc. The CSS is largely shadowed by the inline attributes — for example, .cynefinDomainLabel { fill: ... } in styles.ts is overridden by the .attr('fill', theme.labelColor) inline at cynefinRenderer.ts:196.
This double-styling makes future theme changes confusing (which one wins? where do I edit?). Pick one model:
- CSS classes only (preferred — matches the rest of the codebase): keep
styles.ts, drop the inline.attr('fill', ...)/.attr('font-size', ...)calls, and let the CSS handle presentation. Reference:journey/styles.js,pie/styles.ts. - Inline attrs only: drop
styles.tsentirely. Less consistent with the project but simpler. Not recommended.
Going with CSS classes also makes the diagram play more nicely with user CSS overrides (which is a documented Mermaid extension point).
🟡 [important] — Handdrawn / rough mode not addressed
The New Diagram Type checklist asks that handdrawn mode either be implemented or explicitly documented as unavailable. The cynefin docs (packages/mermaid/src/docs/syntax/cynefin.md) don't mention rough mode. Given the wavy organic boundaries are already a "handdrawn-feeling" aesthetic, this might be a case where the diagram is implicitly handdrawn-only — but that should be made explicit in the docs (one-liner stating handdrawn mode is not separately supported, with a brief reason).
Per the checklist this is "not a hard blocker for merging" but does need explicit acknowledgement.
🟢 [nit] — aria-label on root SVG vs <title> element
packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:111-113
Other Mermaid diagrams typically attach accessibility text via <title> and <desc> SVG child elements (the W3C recommendation for SVG accessibility). The cynefin renderer uses aria-label for the title and <desc> for the description — half and half. Consider switching the title to a <title> child element for consistency with diagrams like pieRenderer.ts. Not a security or functional issue (the XSS sub-agent confirmed accTitle is pre-sanitized via sanitizeText in commonDb.ts), just consistency.
🟢 [nit] — Grammar entry rule has three alternatives for the type marker
packages/parser/src/language/cynefin/cynefin.langium:5
('cynefin-beta' | 'cynefin-beta:' | 'cynefin-beta' ':')
The third alternative ('cynefin-beta' ':') accepts cynefin-beta : with a space before the colon. Three alternatives for a type marker is unusual — most diagrams accept only the bare token. If both cynefin-beta and cynefin-beta: need support, two alternatives should suffice. Worth a short comment in the grammar explaining the intent, or simplifying to one form (other Langium-based diagrams like radar and treemap use just the bare keyword).
🟢 [nit] — Detector regex has no end boundary
packages/mermaid/src/diagrams/cynefin/cynefinDetector.ts:10
return /^\s*cynefin-beta/.test(txt);Would also match cynefin-betaXYZ or cynefin-beta-extension. Other diagram detectors have the same quirk so this is consistent, but a \b word boundary or (?:\s|$) lookahead would be more precise. Take it or leave it.
🟢 [nit] — getCynefinTheme casts via any
packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts:53-75
return (themeVariables as any).cynefin as { /* ... */ };The shape is locally typed (good), but the any cast loses the connection to the theme system. If you can extend the MermaidThemeVariables interface with an optional cynefin?: CynefinThemeVariables field, you'd avoid the cast. Existing diagrams (e.g., radar, venn) augment the theme typings — worth following that pattern.
💡 [suggestion] — Use Resolves #7534 instead of Ref: #7534 in the PR description
GitHub auto-closes the linked issue when the PR merges if you use Resolves/Closes/Fixes. With Ref: someone has to close it manually. Tiny thing.
💡 [suggestion] — Item overflow handling
Long item labels or many items per domain will overflow the domain bounds. Consider word-wrapping, text truncation with ellipsis, or scaling. Not blocking — first beta is allowed to be opinionated about content fitting — but worth a TODO comment or follow-up issue so it doesn't get lost.
💡 [suggestion] — Constraint: transitions to/from confusion
Architecturally, "confusion" represents not knowing which domain an item belongs to, so transitions like complex --> confusion are conceptually questionable (the canonical model has items move OUT of confusion, not into it). Worth a sentence in the docs about which transitions are conceptually meaningful, or constraining the grammar to allow only confusion --> X. Doesn't have to be done now.
Security
✅ No XSS or injection issues identified. The XSS sub-agent reviewed all 12 changed files in the cynefin diagram + parser. All user-controlled text (item.label, transition.label, title, accDescription) flows through D3's .text() method (text-node creation, not HTML parsing). accTitle is set via .attr('aria-label', ...) after pre-sanitization in commonDb. No innerHTML, .html(), eval, Function(), foreignObject, event handlers, <script>, or javascript: URIs introduced. The styles.ts CSS interpolation follows the same pattern as existing diagrams — pre-existing accepted approach, not a new attack surface. The diagram does not bypass the standard mermaidAPI render pipeline, so DOMPurify sanitization still applies to final SVG output.
Verdict
Requesting changes for the 4 🟡 items in aggregate (each individually borderline, but together they shape the visual quality and correctness of the diagram's first beta). The handdrawn-mode acknowledgement is the cheapest fix; the confusion-overflow and self-loop NaN are the most user-visible. Once those land, this should be in great shape for the first text-based Cynefin DSL anywhere.
Genuinely impressive scope for a single PR — Langium grammar + renderer + 5 themes + schema + 40 tests + comprehensive docs. Looking forward to the next iteration.
Self-check
- 🔴 blocking: 0
- 🟡 important: 4
- 🟢 nit: 4
- 💡 suggestion: 3
- 🎉 praise: 6
|
[sisyphus-bot] Hey @ragelink — thanks again for the follow-up work in Before going further: I want to flag a discrepancy so we can untangle it. The comment above from @smd-lm describes a set of fixes (self-loop rejection in Could be a branch that hasn't been pushed, a sync glitch, or just crossed wires — could you confirm what's intended to be in this PR? Happy to re-review once the branch reflects the fixes. For clarity, here are the Round 2 items from the prior review that still look outstanding against the current HEAD: 🟡 Self-loop transitions produce NaN — 🟡 Badge width heuristic — 🟡 Confusion domain overflow — 🟡 Inline attrs duplicating CSS classes — 🟢 🟢 🟢 Detector regex — No hurry — just let us know what's going on with the branch state and we'll take it from there. Really do want to land this one. |
|
Addressed all review feedback — here's a rundown of what changed: 🟡 Self-loop NaN (cynefinDb.ts + cynefinRenderer.ts)Self-loops are now rejected at the DB level in 🟡 Badge width via getBBox() (cynefinRenderer.ts)Replaced 🟡 Confusion domain overflow (cynefinRenderer.ts)
🟡 Inline attrs vs. CSS duplication (cynefinRenderer.ts + styles.ts)Went with the CSS-classes-only model. The renderer now only sets structural/positional attrs and per-domain fill colors (which must be inline since they vary per domain). All font-size, font-weight, fill, stroke, and stroke-width attrs were removed from the renderer.
🟢 Nits
Note: this comment was originally posted under a secondary account (@smd-lm) by mistake — reposted here under the correct account. |
- Self-loop transitions now rejected in setTransitions() with log.warn; renderer has defensive guard - Badge width uses getBBox() for accurate measurement, falls back to char-count heuristic for SSR - Confusion domain radii increased to 0.15; items capped at MAX_CONFUSION_ITEMS=3 with +N more overflow badge - Renderer switched to CSS-classes-only model; styles.ts reads from cynefin theme block directly - Detector regex anchored with (?:[\s:]|$) to avoid false matches - Grammar third alternative removed, comment added - aria-label replaced with <title> child element per W3C SVG recommendation - as any cast removed from getCynefinTheme
…ature/cynefin-diagram
|
Sorry about the confusion on this one — the round 2 items were actually all addressed, I just forgot to push the commit. They've been sitting there locally this whole time. Also sorry for the earlier comment showing up from a secondary account (@smd-lm), that was me with the wrong GH credentials loaded. All cleaned up now and the branch is current. |
No worries, I know exactly how it is. I have multiple as well: knsv, knsv-boty, and others. :) We'll do another review then with the commit pushed ! |
There was a problem hiding this comment.
A word from @knsv. Great progress, just a few small things left. The diagrams look great!
[sisyphus-bot]
Thanks so much for this contribution, @ragelink — a first-of-its-kind text DSL for Cynefin is a lovely thing to land in Mermaid, and the quality of the work shows throughout. I went through the parser, renderer, boundaries, themes, schema, tests, and docs, plus a focused security pass. Great shape overall; a few small things below to iron out before merge.
🎉 What's working well
- 🎉 Type-safe grammar.
DOMAIN_NAMEis restricted to the five canonical literals inpackages/parser/src/language/cynefin/cynefin.langium, which means domain lookups in the renderer (domainBg[domainName],layouts[…]) are guaranteed safe by the parser. Elegant. - 🎉 Separation of concerns. Boundary path math lives in its own module (
cynefinBoundaries.ts) with a deterministic seeded PRNG — reproducible output across renders and easy to unit-test. - 🎉 Defense in depth for self-loops. Filtered in
cynefinDb.ts:setTransitionsand also guarded incynefinRenderer.tsinside the transition loop. - 🎉 Theme integration is thorough. All five themes (default/dark/forest/neutral/base) define a
cynefinblock with overridable domain colors, boundary/cliff widths, font sizes, etc. The dark and forest themes supply genuinely different palettes. - 🎉 Tests are solid. 162 lines of DB unit tests + 275 lines of parser integration tests + 10 Cypress visual cases (including config overrides, theme overrides, accessibility, straight vs wavy boundaries). This is well above the bar for a new diagram type.
- 🎉 Multi-diagram uniqueness test updated. The entry in
multi-diagram-id-uniqueness.spec.tsis exactly what we want for scoped marker IDs likecynefin-arrow-${id}. - 🎉 Changeset +
MERMAID_RELEASE_VERSIONplaceholder in docs — followed the conventions precisely. - 🎉 Thoughtful UX touches. The overflow badge (
+N more) when the confusion ellipse has more thanMAX_CONFUSION_ITEMS, and the accessibility<title>/<desc>elements.
Security
I ran a focused XSS/injection review over the renderer, DB, parser, boundaries, and styles. No XSS or injection issues identified. All user-controlled text (item labels, transition labels, title, accTitle, accDescription, overflow badge) flows to D3's .text() (sets textContent, which HTML-escapes). No .html()/innerHTML, no foreignObject, no xlink:href/javascript: sinks, no DOMPurify config changes. Title/accTitle/accDescription additionally pass through sanitizeText upstream in commonDb.ts. The styles.ts CSS generator interpolates theme variables only, not diagram input.
Things to address
🟡 [important] useMaxWidth config is ignored
packages/mermaid/src/diagrams/cynefin/cynefinRenderer.ts passes true literally:
configureSvgSize(svg, totalHeight, totalWidth, true);CynefinDiagramConfig extends BaseDiagramConfig, which supplies useMaxWidth, so users currently can't disable it. The radar renderer (which the PR notes as the reference pattern) reads from config — see packages/mermaid/src/diagrams/radar/renderer.ts:57:
configureSvgSize(svg, totalHeight, totalWidth, config.useMaxWidth ?? true);Worth matching that so the config option actually works.
🟡 [important] Config key doesn't match the -beta keyword
The diagram is introduced via cynefin-beta at the syntax level, but the detector id and config key are cynefin (no -beta):
packages/mermaid/src/diagrams/cynefin/cynefinDetector.ts:7—const id = 'cynefin';packages/mermaid/src/schemas/config.schema.yaml—cynefin: $ref: '#/$defs/CynefinDiagramConfig'cynefinDb.tsreadscommonGetConfig().cynefin
Compare to wardley-beta (the other -beta diagram in the codebase) — packages/mermaid/src/diagrams/wardley/wardleyDetector.ts:7 uses const id = 'wardley-beta'; and the schema key is wardley-beta:. The convention is: keyword == detector id == config/schema key.
Two concrete effects:
- A user writing
%%{init: {"cynefin-beta": {...}}}%%(by analogy withwardley-beta) will silently get no config applied. - When the
-betasuffix is dropped in a later release, the schema key won't need to change but the detector id and grammar keyword will — the current asymmetry makes that migration more confusing, not less.
Easiest fix: rename detector id, schema key, and config property to cynefin-beta to match the keyword. (If the intent was specifically to avoid renaming the config block when going GA, please add a short comment to that effect on the schema entry so the inconsistency is discoverable.)
💡 [suggestion] Transition control-point coordinate math is fiddly
In cynefinRenderer.ts (transitions section), x1/y1/x2/y2 and mx/my are computed in padded SVG coords, then cpx/cpy subtract padding to land in root-local coords to match the M/Q path which uses unpadded fromLayout.cx,cy. It works, but a future reader has to hold two coordinate spaces in their head. Computing everything in root-local (drop the + padding on x1…y2, drop the - padding on cpx/cpy) would be algebraically equivalent and easier to follow.
💡 [suggestion] Overflow badge only for the confusion ellipse
MAX_CONFUSION_ITEMS = 3 caps confusion with a +N more badge, but the four quadrant domains can visually overflow their boxes with a long list. Not a blocker for v1 — the wavy boundaries are forgiving — but worth either applying the same pattern or documenting the expectation that quadrant item counts should be kept small.
💡 [suggestion] PR description test-plan checkboxes are stale
The PR body lists Cypress e2e and Cynefin unit/integration tests as unchecked ("happy to add if requested"), but cypress/integration/rendering/cynefin.spec.js, cynefin.spec.ts, and cynefin.integration.spec.ts are all present. A quick edit of the description will avoid a reviewer chasing those down.
Really excited about this one — let's get it across the finish line. 🙌
- Honor `useMaxWidth` config in renderer (match radar reference pattern) - Rename config/detector/schema key to `cynefin-beta` to match keyword and align with the `wardley-beta` convention - Simplify transition control-point math into a single coordinate space - Document the expectation that quadrant item counts stay small
|
Thanks so much for the thorough pass, @knsv — really appreciate the care on this one. Pushed the fixes in 🟡
|
|
Hey @ragelink Thanks for keeping up with this one. I see some unit test checks on the CI are failing. Can you check? |
…feature/cynefin-diagram
- Move cypress/integration/rendering/cynefin.spec.js into a cynefin/ subfolder so e2e-diagram-scope.spec.ts (every diagram folder must have a matching spec subfolder) passes. - Update packages/examples/src/examples/cynefin.ts id from 'cynefin' to 'cynefin-beta' to match the registered detector id, satisfying example.spec.ts (every registered diagram must have an example).
Aligns Cynefin with the radar diagram convention so that the diagram's internal identity matches the rest of the codebase. Previously cynefin diverged: the registered detector id and config key were 'cynefin-beta', which forced bracket-access (DEFAULT_CONFIG['cynefin-beta']) — the only such case in defaultConfig.ts. Radar by contrast uses id 'radar' / config key 'radar' with the '-beta' marker living only in the syntax keyword. Changes: - Detector id: 'cynefin-beta' -> 'cynefin' - Config schema key (cynefin-beta -> cynefin) in config.schema.yaml, defaultConfig.ts, .build/jsonSchema.ts, and the regenerated config.type.ts - Example id back to 'cynefin' (matches the registered diagram id) - Test/spec config-object keys updated (multi-diagram uniqueness spec, cypress cynefin spec) - Docs source updates for the config-key references; generated docs regenerated via pnpm --filter mermaid docs:build Unchanged: the syntax keyword 'cynefin-beta' remains the trigger token in the Langium grammar, parser tokenBuilder, detector regex, and all diagram code examples — exactly mirroring radar-beta.
…ature/cynefin-diagram
|
Hey @ashishjain0512 — thanks for the nudge! Pushed two commits to fix the failing unit checks:
Full vitest suite passes locally (4,131 / 4,131). Let me know if anything else needs tweaking. |
Moving cynefin.spec.js into cypress/integration/rendering/cynefin/ broke the relative import for ../../helpers/util — needs an extra level (../../../helpers/util) to match the radar and wardley specs.
|
Alright @ragelink! Thanks for the contribution and stamina. I think is ready now! |
|
Hello @knsv sorry to bug, one small follow-up as I'm a bit unfamiliar with the process, what happens next? does this go into a queue for merging? is there any additional work needed on my end? Thanks! |
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Hey @ragelink — final pass to formally close this out. No new findings; the prior rounds covered everything substantive, and nothing has shifted under the Cynefin code since @knsv approved on April 29 (only the merges from develop and a couple of CI-scoping fix-ups).
Worth restating why this lands well, because four review rounds in it's easy to lose sight of the win:
- 🎉 Genuinely novel — first text-based DSL for Cynefin anywhere.
- 🎉 Clean architecture: Langium grammar with a closed
DOMAIN_NAMEenum, deterministic seeded PRNG behind the wavy boundaries in their own module, and full theme integration across all five themes. - 🎉 Solid test coverage end-to-end — DB unit tests, parser integration tests, boundary determinism, multi-diagram ID uniqueness, and Cypress visual cases including the cliff and confusion overlay.
- 🎉 The persistence across multiple review rounds — including chasing the Argos and CI fix-ups at the end — is exactly the kind of follow-through that makes new diagram types ship. Genuinely appreciated.
To answer your "what happens next" question: nothing further needed from you. The PR is approved and a maintainer will merge it when the timing fits — likely bundled with the next mermaid minor bump given this is a feat: change. You can ignore the queue from here; if develop shifts under you and a conflict shows up, someone will ping you. Otherwise sit back and watch it land.
Thanks again for the stamina on this one. 🎉
|
@ragelink, Thank you for the contribution! |
4 participants
Summary
Adds a Cynefin framework diagram type to mermaid. The Cynefin framework (created by Dave Snowden) is a decision-making framework that categorizes problems into five complexity domains, widely used in agile, incident management, strategy, and organizational design.
There is currently no standard text-based DSL for Cynefin diagrams anywhere. This is a first.
Ref: #7534
Syntax
Features
Architecture
cynefinBoundaries.ts)Test plan
pnpm run buildpasses with 0 errorscypress/integration/rendering/cynefin.spec.js)cynefin.spec.ts) and integration tests (cynefin.integration.spec.ts)Closes #7534