feat(sequence): implement neo look and themes#7384
feat(sequence): implement neo look and themes#7384omkarht wants to merge 9 commits intofeature/neo-look-implementedfrom
Conversation
🦋 Changeset detectedLatest commit: a57ce5a 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 #7384 +/- ##
===============================================================
- Coverage 3.59% 3.57% -0.02%
===============================================================
Files 481 480 -1
Lines 49929 50171 +242
Branches 740 740
===============================================================
Hits 1793 1793
- Misses 48136 48378 +242
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 ↗︎
|
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…ling on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…maid-js/mermaid into feature/neo-look-sequence
…maid-js/mermaid into feature/neo-look-sequence
…maid-js/mermaid into feature/neo-look-sequence
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
omkarht
changed the title
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this work on bringing neo look to sequence diagrams, @omkarht! The scope is significant — all 8 actor types plus notes, loops, activations, and messages. Great coverage. A few items need attention before this is ready.
File Triage
| Tier | Count | Files |
|---|---|---|
| Tier 2 (diff + context) | 3 | sequenceRenderer.ts, svgDraw.js, styles.js |
| Tier 2 (new test) | 1 | sequenceDiagram-redux-themes.spec.ts |
| Tier 3 (diff only) | 1 | changeset |
What's Working Well
🎉 [praise] The semantic data-et, data-type, and data-id attributes are a great addition. They enable much better test targeting, debugging, and potentially custom styling without fragile class-based selectors. This is forward-thinking work that benefits the whole project.
🎉 [praise] Thorough e2e test coverage — 5 test scenarios across all 4 redux themes, exercising all participant types (actor, participant, boundary, control, entity, database, collections, queue), plus creation/destruction, notes, loops, and parallel processes.
🎉 [praise] Proper changeset with minor bump and feat: prefix.
Things to Address
🔴 [blocking] — svgDraw.js:16: Module-level getConfig() captures stale config
const config = configApi.getConfig();This is called once at module import time and cached for the lifetime of the module. The drawRect function then uses config.look to decide whether to add data-look="neo". This means:
- If someone renders a sequence diagram with
look: 'neo'first, then renders one without neo, the second render will still getdata-look="neo"attributes. - On pages with multiple diagrams using different looks, all diagrams after the first will use the first one's config.
Fix: Move getConfig() inside drawRect or accept look as a parameter:
export const drawRect = function (elem, rectData) {
const rectElement = svgDrawCommon.drawRect(elem, rectData);
const { look } = configApi.getConfig();
if (look === 'neo') {
rectElement.attr('data-look', 'neo');
}
return rectElement;
};🔴 [blocking] — svgDraw.js: Duplicate drop-shadow filter IDs
The insertDropShadow function (called once from the renderer) creates a <filter id="drop-shadow">. Then drawActorTypeParticipant creates another <filter id="drop-shadow"> inside each participant's <g> (line ~421 in the diff). Multiple SVG elements with the same id is invalid SVG — the browser will use whichever definition it finds first, and the behavior is undefined across browsers.
Fix: Remove the per-participant filter definition in drawActorTypeParticipant. The diagram-level filter from insertDropShadow is sufficient — all elements reference it via url(#drop-shadow) already.
🟡 [important] — svgDraw.js: Removed default fill/stroke from control and database actors
In drawActorTypeControl, the circle previously had:
.attr('fill', '#eaeaf7')
.attr('stroke', '#666')
.attr('stroke-width', 1.2);These inline styles are removed. The new code only applies colors in the theme?.includes('color') branch or sets actorBorder in the else. But there's no fill fallback for non-color themes — the control circle will have no fill (transparent), which is likely a visual regression for the base neo themes (redux, redux-dark).
Similarly for drawActorTypeDatabase — fill: '#eaeaea', stroke: '#000', stroke-width: 1 are removed from the cylinder path. The CSS class is moved to the parent g, so fill/stroke may inherit, but it would be good to verify this renders correctly in all non-color themes.
🟡 [important] — svgDraw.js: Fragile theme?.includes('color') matching
This pattern appears throughout the PR (~10 occurrences):
if (theme?.includes('color')) { ... }This will match any future theme name containing "color" (e.g., "colorful", "bicolor"). Use exact matching:
const colorThemes = new Set(['redux-color', 'redux-dark-color']);
if (colorThemes.has(theme)) { ... }Same issue with theme?.includes('redux') in a few places.
🟡 [important] — svgDraw.js: Repeated O(n) actor lookup in every actor draw function
Every actor type function does:
let actorCount = [...diagObj.db.getActors().values()].findIndex((a) => a.name === actor.name);This spreads the entire actor map into an array and does a linear search for each actor — O(n²) total for n actors. Since diagObj is now passed to all actor functions, consider precomputing the index map once:
const actorIndexMap = new Map(
[...diagObj.db.getActors().keys()].map((key, i) => [key, i])
);Pass this map instead of diagObj to each draw function, or compute it once in drawActor.
🟡 [important] — svgDraw.js:1017-1020: Database actor identical if/else branches
if (!isFooter) {
cylinderGroup.attr('transform', `translate(${w}, ${ry})`);
} else {
cylinderGroup.attr('transform', `translate(${w}, ${ry})`);
}Both branches do the exact same thing — this looks like a copy-paste error. If the footer position should differ, fix the else branch. Otherwise, remove the conditional entirely.
🟡 [important] — styles.js: g rect.rect CSS selector is overly broad
g rect.rect {
filter: ${dropShadow};
stroke: ${options.nodeBorder};
}This targets ALL rect.rect inside any g, which could affect non-sequence elements if styles leak (e.g., in multi-diagram pages). Consider scoping with a sequence-specific class or using the existing .sequenceDiagram wrapper.
🟢 [nit] — svgDraw.js: Entity actor unconditionally applies drop-shadow filter
actElem.attr('filter', 'url(#drop-shadow)');This is applied regardless of look (no if (look === 'neo') guard). Other actor types conditionally apply the filter. If the drop-shadow filter isn't defined for non-neo looks, this could produce a console warning or no-op, but it's inconsistent.
🟢 [nit] — PR body has template placeholder for linked issue
The PR body says "Resolves #" — would be good to link the actual issue if one exists, or remove the template text if there isn't one.
Security
No XSS or injection issues identified in the changes introduced by this PR. Key observations:
- All DOM manipulation uses D3's
.append()/.attr()API (safe). data-*attributes set fromactor.nameuse D3'ssetAttribute()internally, which is injection-safe.dropShadowandnoteFontWeightin styles come from theme variables, which are validated bysanitizeDirective(regex-limited to[\d "#%(),.;A-Za-z]+). CSS injection risk is low.- The
flood-colorattribute ininsertDropShadowuses only hardcoded values based on theme name comparison — no user input.
Checklist
- At least one 🎉 [praise] item exists
- No duplicate comments
- Severity tally: 2 🔴 blocking / 5 🟡 important / 2 🟢 nit / 0 💡 suggestion / 3 🎉 praise
- Verdict matches criteria: REQUEST_CHANGES (2 🔴 + 5 🟡)
- Tone check: collaborative and constructive
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]
Thanks for the thorough follow-up, @omkarht! The new commit addresses the feedback from the previous review cleanly. Here's where things stand.
Previous Review Items — Resolution Status
| # | Prev Severity | Issue | Status |
|---|---|---|---|
| 1 | 🔴 | Module-level getConfig() stale config |
✅ Fixed + unit test |
| 2 | 🔴 | Duplicate drop-shadow filter IDs |
✅ Fixed |
| 3 | 🟡 | Removed default fill/stroke (control/database) | ✅ Fixed |
| 4 | 🟡 | Fragile theme?.includes('color') matching |
✅ Fixed |
| 5 | 🟡 | O(n²) actor lookup | ✅ Fixed |
| 6 | 🟡 | Identical if/else branches (database/boundary) | ✅ Fixed |
| 7 | 🟡 | Overly broad g rect.rect CSS selector |
↓ Downgraded (see below) |
| 8 | 🟢 | Entity unconditional drop-shadow |
✅ Fixed |
| 9 | 🟢 | PR body template placeholder | Not addressed |
What's Working Well
🎉 [praise] The actorIndexMap precomputation in sequenceRenderer.ts:1070-1072 is a clean solution — computed once, threaded through to all actor draw functions, with a sensible fallback in drawActor for any call sites that don't pass it. This eliminates the O(n²) lookups without overcomplicating the API.
🎉 [praise] The COLOR_THEMES Set (svgDraw.js:17) is the right pattern — explicit, closed, and won't false-match future theme names. Good call defining it as a module-level constant since the set of color themes is static.
🎉 [praise] The unit test for per-render config resolution (svgDraw.spec.js:42-68) is well-constructed — it verifies that two consecutive drawRect calls get independent config reads, which directly validates the stale-config fix. This is exactly the kind of test that prevents regressions.
🎉 [praise] The duplicate filter removal in drawActorTypeParticipant is clean, with a comment pointing to the global insertDropShadow() — helpful breadcrumb for future contributors.
Remaining Items (Non-blocking)
🟢 [nit] — styles.js: g rect.rect CSS selector
The previous review flagged this as 🟡. On closer inspection, Mermaid's createUserStyles() scopes all diagram CSS under the SVG element's ID selector (e.g., #mermaid-123 g rect.rect), so cross-diagram leakage on multi-diagram pages isn't a real risk. Within the sequence diagram itself, rect.rect is a reasonable target. Downgrading to a nit — if you wanted to be extra precise, rect.rect[data-look="neo"] would limit it to neo-look elements only, but this is entirely optional.
🟢 [nit] — PR body still has Resolves #<your issue id here> template text
Worth updating if there's a linked issue, or removing the placeholder if there isn't one. Cosmetic only.
Security
No XSS or injection issues identified in the changes. Key observations:
- All DOM manipulation uses D3's
.append()/.attr()API (safe — usessetAttribute()internally). data-*attributes set fromactor.namego through D3's safe attribute setter.dropShadowandnoteFontWeightinstyles.jscome from theme variables validated bysanitizeDirective.flood-colorininsertDropShadowuses only hardcoded values based on exact theme name comparison — no user input.drawImage/drawEmbeddedImagein shared code usesanitizeUrl()for link attributes.
Checklist
- At least one 🎉 [praise] item exists (4)
- No duplicate comments
- Severity tally: 0 🔴 blocking / 0 🟡 important / 2 🟢 nit / 0 💡 suggestion / 4 🎉 praise
- Verdict matches criteria: APPROVE (no 🔴 or 🟡)
- Tone check: constructive and appreciative
All blocking and important items from the previous review have been addressed. Nice work getting these fixes in — this is ready to go. 🚀
2 participants
📑 Summary
This PR implements Neo look support for Sequence Diagrams, building on the foundation established in the base Neo look PR. It enhances sequence diagram rendering with modern styling including drop shadows, rounded corners, configurable stroke widths, and multi-color actor support for Redux color themes.
Resolves #
📏 Design Decisions
Describe the way your implementation works or what design decisions you made if applicable.
📋 Tasks
1. Neo Look Integration for Sequence Diagrams
2. Multi-Color Actor Support
redux,redux-dark)redux-color,redux-dark-color)bkgColorArrayandborderColorArray.3. Enhanced Styling System
styles.jsto accept dynamic drop shadow values from theme variablesdata-look="neo"attributes to elements for targeted CSS stylingnoteFontWeighttheme variable4. Data Attributes for Element Tracking
data-et(element type),data-type, anddata-idattributes to sequence diagram elementsMake 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:.