feat(requirement):implement neo look#7446
feat(requirement):implement neo look#7446darshanr0107 wants to merge 7 commits intofeature/neo-look-implementedfrom
Conversation
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
🦋 Changeset detectedLatest commit: a9e730a 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 |
github-actions
bot
added
Type: Enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/neo-look-implemented #7446 +/- ##
===============================================================
- Coverage 3.59% 3.58% -0.01%
===============================================================
Files 481 481
Lines 49929 50032 +103
Branches 740 740
===============================================================
Hits 1793 1793
- Misses 48136 48239 +103
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
left a comment
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for working on the neo look for requirement diagrams! This is a nice addition to the diagram ecosystem. Here's some early feedback while the PR is still in draft — take your time with these.
What's working well
🎉 Good separation of neo vs classic markers. Creating requirement_arrow_neo and requirement_contains_neo as separate functions rather than adding conditionals inside the existing markers is a clean approach that avoids regressing the classic look.
🎉 Clean renderer change. The requirementRenderer.ts change is minimal and well-scoped — just selecting the right marker set based on look.
Things to address
🟡 [important] — DOM query in shared rendering code creates O(n²) performance risk
requirementBox.ts:121-125:
const nodes = document.querySelectorAll('g.node.default');
const nodeIndex = Array.from(nodes).findIndex((n) => n.id === node.id);This runs querySelectorAll + findIndex inside the shape rendering function, which is called once per node. For a diagram with N requirement nodes, this is O(n²). On large requirement diagrams (50+ nodes), this could cause noticeable rendering lag.
Consider passing the color index through the node data (e.g., via LayoutData) instead of querying the DOM at render time. Other diagram types that support color themes (like journey) handle this through the data pipeline rather than DOM queries.
🟡 [important] — Shared rendering-util/ code modified — regression risk
Both markers.js and requirementBox.ts live in rendering-util/ and are shared across diagram types. Two specific concerns:
-
markers.js— The existingrequirement_arrowandrequirement_containsfunctions now havemarkerUnits: 'userSpaceOnUse'added. This changes behavior for the classic look too, not just neo. Was this intentional? If so, it would be good to verify classic requirement diagrams still render correctly. -
requirementBox.ts— The divider line changed fromrc.line()torc.polygon()with thickness0.001. This affects all looks, not just neo. The polygon approach may render differently in rough/handdrawn mode.
🟡 [important] — Hardcoded theme name string checks are fragile
Several places use string matching on theme names:
styles.js:theme?.includes('color'),theme === 'redux-color'requirementBox.ts:theme?.includes('color'),theme?.includes('redux')
These will silently break if theme names are renamed or new themes are added. Consider using theme properties/flags rather than string matching on theme names. Check how other diagram types handle theme-conditional styling — most use theme variables directly rather than checking theme names.
🟡 [important] — No tests added
Codecov reports 0% patch coverage with 107 uncovered lines. For a renderer/style change, the project requires:
- E2E visual regression tests in
cypress/integration/rendering/usingimgSnapshotTest()— at minimum one test showing the neo look rendering correctly - Unit tests for the
genColorfunction if it stays as a separate utility
This is especially important since shared rendering-util/ code is modified.
🟡 [important] — Missing changeset
The changeset bot flagged this. Since this is a user-facing feature, it needs a changeset:
pnpm changesetSelect minor bump with feat: prefix.
🟢 [nit] — Removed .edgeLabel styles
The .edgeLabel, .edgeLabel .label rect, and .edgeLabel .label text CSS rules were removed from styles.js. Worth verifying that edge labels in classic requirement diagrams still display correctly — these styles may have been providing necessary styling.
🟢 [nit] — PR description template not filled in
The PR body still has the default template. When you're ready to move out of draft, it would be helpful to fill in:
- What issue this resolves (if any)
- Design decisions (e.g., why separate neo markers vs conditional logic)
- Screenshots showing the neo look
Security
No XSS or injection issues identified. The changes are styling/marker additions that don't introduce new user-controlled text into DOM sinks.
Summary
| Severity | Count |
|---|---|
| 🔴 blocking | 0 |
| 🟡 important | 5 |
| 🟢 nit | 2 |
| 💡 suggestion | 0 |
| 🎉 praise | 2 |
The main areas to focus on: (1) the O(n²) DOM query pattern, (2) ensuring shared code changes don't regress classic look, and (3) adding tests. Since this is a draft, no rush — happy to re-review when you're ready!
…ering on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…arkers on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…maid-js/mermaid into feature/neo-look-requirement
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for the continued work on the neo look for requirement diagrams, @darshanr0107! Nice to see progress since the last round. Here's an updated review based on the current state.
What's working well
🎉 O(n²) DOM query issue resolved. The previous review flagged querySelectorAll + findIndex inside the shape function. You've cleanly replaced this by passing colorIndex through the data pipeline via requirementDb.ts — this is the right approach and matches how other diagram types handle it.
🎉 Theme name string matching removed. The prior review noted fragile theme?.includes('color') checks. Those are gone now — the code uses borderColorArray and config.look directly, which is much more robust.
🎉 Clean marker separation. Creating requirement_arrow_neo and requirement_contains_neo as separate functions continues to be a good pattern — no risk of regressing the classic look through conditionals.
Things to address
🟡 [important] — Divider line change from rc.line() to rc.polygon() affects all looks
requirementBox.ts:156-167: The divider line was changed from rc.line() to rc.polygon() with a thickness of 0.001. This runs for all looks (classic, neo, handDrawn), not just neo.
- In handDrawn mode, roughjs treats polygons and lines very differently — a polygon will get a fill and different stroke rendering, which could change the visual appearance of the divider.
- The
0.001thickness is a creative workaround, but it may render differently across browsers/zoom levels.
It would be great to understand the motivation here. If this was needed specifically for neo styling, consider wrapping it in a node.look === 'neo' check and keeping rc.line() for other looks.
🟡 [important] — shapeSvg.selectAll('path').attr('style', nodeStyles) is overly broad
requirementBox.ts:175-182: This applies nodeStyles to all <path> elements inside the shape group, including paths created by roughjs internals. This could override roughjs stroke/fill styles in unexpected ways. The condition guards against handDrawn, but it still affects the classic look when borderColorArray is present.
Consider targeting more specifically — e.g., .selectAll('.outer-path path') — since you already added the outer-path class to the rect.
🟡 [important] — Removed .edgeLabel styles may affect classic look
styles.js: The .edgeLabel, .edgeLabel .label rect, and .edgeLabel .label text CSS rules were removed entirely. These styles were providing background color and text color for edge labels in the classic requirement diagram look. Worth verifying that classic requirement edge labels still display correctly — if they rely on these rules, removing them is a regression.
🟡 [important] — No tests added
This is a renderer/style change that modifies shared rendering-util/ code. The project requires:
- E2E visual regression tests in
cypress/integration/rendering/usingimgSnapshotTest()— at minimum covering neo look rendering and confirming classic look is not regressed - Unit test coverage for
genColorif it stays as a separate function
🟡 [important] — Missing changeset
Still needed for this user-facing feature:
pnpm changesetSelect minor bump with feat: prefix.
🟢 [nit] — genColor accesses global config inside a styles function
styles.js:3-6: The genColor function calls configApi.getConfig() directly rather than receiving config/theme data through the options parameter like the rest of the styles function. This breaks the pattern used by other diagram styles modules. Consider threading themeVariables through the options parameter instead for consistency.
🟢 [nit] — Unused strokeWidth variable in requirement_arrow_neo
markers.js:422: strokeWidth is destructured from themeVariables but only set on the marker element with .attr('stroke-width', ...). The existing classic requirement_arrow marker doesn't set stroke-width at all, and the marker path itself uses stroke-linejoin: 'miter' but doesn't reference strokeWidth on the path. Double-check this is producing the intended visual.
🟢 [nit] — PR description template not filled in
When ready to move out of draft, filling in the summary, linked issue, and screenshots would help reviewers.
Security
No XSS or injection issues identified. The changes are marker definitions and CSS styling that use config-derived values (theme variables, numeric indices) rather than user-controlled text. The colorIndex is an integer derived from array position. The data-color-id attribute is constructed from this integer. No user input flows to dangerous DOM sinks.
Summary
| Severity | Count |
|---|---|
| 🔴 blocking | 0 |
| 🟡 important | 5 |
| 🟢 nit | 3 |
| 💡 suggestion | 0 |
| 🎉 praise | 3 |
Good progress from the last round — the DOM query and theme-name-matching issues are resolved. The main areas still to address: (1) ensuring the divider and path style changes don't regress classic/handDrawn looks, (2) verifying the .edgeLabel style removal is safe, and (3) adding tests. Happy to re-review when these are addressed!
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…ling on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Follow-up Review (Round 3): feat(requirement): implement neo look
Great work addressing the feedback, @darshanr0107 — this is in much better shape. All 10 🟡 items from the previous two rounds are resolved. The O(n²) DOM query fix, the node.look guards on the divider, and the scoped .outer-path path selector all show careful attention to the review feedback. Let's get this across the finish line.
What's working well
🎉 [praise] All prior blocking feedback addressed cleanly. The colorIndex data pipeline approach, the node.look === 'neo' guard on the divider polygon, and the scoped selectAll('.outer-path path') are all exactly the right fixes.
🎉 [praise] Excellent E2E test coverage — 14 test cases covering simple diagrams, all requirement types, all relationship types, all directions (TB/BT/LR/RL), titles, style/class/classDef shorthand, empty bodies, quoted names, and reverse arrows. This is thorough and covers realistic usage patterns.
🎉 [praise] Clean type extension — adding colorIndex?: number to BaseNode in types.ts is non-breaking (optional field) and makes the color indexing available to any diagram type that wants it.
Previous review items — status
| Round | Severity | Item | Status |
|---|---|---|---|
| 1 | 🟡 | O(n²) DOM query | ✅ Replaced with colorIndex data pipeline |
| 1 | 🟡 | Shared code regression risk (markers) | ✅ Neo markers are separate functions, classic untouched |
| 1 | 🟡 | Theme name string matching | ✅ Uses borderColorArray/config.look directly |
| 1 | 🟡 | No tests | ✅ 389 lines of comprehensive E2E tests |
| 1 | 🟡 | Missing changeset | ✅ Added |
| 2 | 🟡 | Divider rc.polygon() affects all looks |
✅ Guarded by node.look === 'neo' |
| 2 | 🟡 | selectAll('path') overly broad |
✅ Now .selectAll('.outer-path path') |
| 2 | 🟡 | Removed .edgeLabel styles | ✅ Styles still present (restructured) |
| 2 | 🟡 | No tests | ✅ Added |
| 2 | 🟡 | Missing changeset | ✅ Added |
Remaining items
🟡 [important] E2E tests only cover neo look — add at least one classic regression test
requirementDiagram-neo-themes.spec.ts:3: const looks = ['neo'] as const;
Since this PR modifies shared rendering-util/ code (requirementBox.ts divider logic, markers.js with new exports), it would be valuable to include at least one classic look test case to verify no regression. Even a single imgSnapshotTest with { look: 'classic', theme: 'default' } covering a basic requirement diagram with a relationship would provide that safety net.
The existing cypress/integration/rendering/requirement.spec.js may already cover classic, but having explicit coverage in this PR's test file for cross-look comparison would be ideal.
🟢 [nit] getConfig() called in shared markers.js and diagram styles.js
markers.js:422,470: The new requirement_arrow_neo and requirement_contains_neo functions call configApi.getConfig() to read strokeWidth. The classic markers use hardcoded values. Similarly, styles.js calls getConfig() inside both genColor() and getStyles().
This is likely consistent with how other neo markers/styles work in the base branch, so not blocking. Just noting it for awareness — the established pattern for style functions is to receive config through the options parameter.
🟢 [nit] PR description not filled in
The body still has the default template. When ready, filling in the design decisions, linked issue (if any), and screenshots showing the neo look would help reviewers and serve as documentation.
🟢 [nit] Auto-generated docs/LayoutData.md committed
docs/config/setup/mermaid/interfaces/LayoutData.md is in the top-level docs/ folder which is auto-generated. The changes are just line number shifts matching the types.ts addition — this is consistent with regeneration, not manual editing. Just flagging for awareness that generated output is in the PR.
Security
No XSS or injection issues identified. The changes use:
- Numeric
colorIndexvalues derived from array position →data-color-idattribute - Theme variables (colors, strokeWidth) from config → CSS/SVG attributes via D3
.attr() - No user-controlled text flows to dangerous DOM sinks (
.html(),innerHTML) - Text rendering continues to use
sanitizeText(decodeEntities(...))throughcreateText()
Self-check
- At least one 🎉 [praise] item exists (3)
- No duplicate comments
- Severity tally: 0 🔴 / 1 🟡 / 3 🟢 / 0 💡 / 3 🎉
- Verdict: COMMENT (1 🟡 — test coverage gap for classic look regression)
- Tone check: collaborative and appreciative
This is looking great — all the architectural concerns from previous rounds are resolved. The one remaining item is adding a classic look regression test to complement the thorough neo coverage. Once that's in, this should be ready to go!
3 participants
📑 Summary
This PR implements the neo look for the requirement diagram, extending the existing neo look support (already available for other diagram types) to requirement diagram-specific rendering.
📏 Design Decisions
Describe the way your implementation works or what design decisions you made if applicable.
📋 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:.