feat(ER): implement neo look#7388
feat(ER): implement neo look#7388darshanr0107 wants to merge 6 commits intofeature/neo-look-implementedfrom
Conversation
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
🦋 Changeset detectedLatest commit: 8878e1d 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 #7388 +/- ##
===============================================================
- Coverage 3.57% 3.56% -0.01%
===============================================================
Files 481 480 -1
Lines 49886 50258 +372
Branches 740 740
===============================================================
+ Hits 1784 1793 +9
- Misses 48102 48465 +363
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>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
darshanr0107
changed the title
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 working on extending neo look support to ER diagrams, @darshanr0107! The e2e test coverage is thorough and the changeset is well-structured. A few items to address before this is ready to merge.
File Triage
| Tier | Count | Files |
|---|---|---|
| Tier 1 (full read) | 2 | render.ts, markers.js |
| Tier 2 (diff + context) | 3 | erRenderer-unified.ts, erBox.ts, styles.ts |
| Tier 3 (diff only) | 1 | changeset |
What's Working Well
🎉 [praise] Excellent e2e test coverage — 8 test scenarios across all neo theme/look combinations covering relationship types, attributes, keys, comments, aliases, recursive relationships, standalone entities, and various attribute types. This is exactly the kind of thorough visual regression coverage the project needs.
🎉 [praise] The lineToPolygon approach for converting divider lines to polygons in erBox.ts is a smart technique for getting better rendering quality with roughjs.
🎉 [praise] Proper changeset with minor bump and feat: prefix.
Things to Address
🔴 [blocking] — render.ts:80,93: flood-color logic change affects all diagram types
The conditional changed from:
theme?.includes('dark') ? '#FFFFFF' : '#000000'to:
theme === 'redux' || theme === 'redux-color' ? '#000000' : '#FFFFFF'This is in shared rendering code (rendering-util/render.ts) and changes the flood-color for every diagram type, not just ER. The original logic was theme-darkness-aware — light themes got black flood, dark themes got white. The new logic inverts this for any theme that isn't explicitly redux or redux-color (e.g., the neo light theme now gets white flood instead of black).
If this is intentional for the new theme system, it would be great to add a comment explaining the rationale. If the intention is only to fix ER-specific behavior, this change should be scoped to the ER diagram instead of modifying shared code. Either way, please confirm this doesn't regress existing themes by running the full e2e suite.
🟡 [important] — erBox.ts:65,242: Global DOM query for color indexing is fragile
const nodes = document.querySelectorAll('g.node.default');
const nodeIndex = Array.from(nodes).findIndex((n) => n.id === node.id);This queries ALL g.node.default elements in the entire document — not scoped to the current diagram's SVG. On pages with multiple mermaid diagrams, this will produce incorrect color indices. Additionally, the color assignment depends on DOM insertion order, which is an implementation detail that could change.
Consider scoping the query to the diagram's SVG element (e.g., parent.selectAll('g.node.default')) or computing the index from the diagram's data (e.g., the entity list from the DB). This pattern appears in two places — once for the no-attributes path (line ~65) and once for the attributes path (line ~242).
🟡 [important] — styles.ts:18-40: configApi.getConfig() breaks the styles function contract
The ER styles function now calls configApi.getConfig() directly to read look, theme, and themeVariables. Other diagram styles functions (e.g., flowchart, journey) receive all needed data solely through the options parameter. This couples the styles function to global config state and breaks the established pattern.
It would be better to pass look, theme, and the needed theme variables through the options parameter, consistent with how other diagrams handle this. If the options type needs extending, that's fine — but the styles function shouldn't reach into global state.
🟡 [important] — styles.ts:21,37: Fragile theme name matching
if (!theme?.includes('color')) { ... }
// and
theme?.includes('redux')String.includes() on theme names will match any future theme containing those substrings (e.g., a hypothetical "colorful" or "redux-v2" theme). Use exact equality or a set check:
if (theme !== 'redux-color') { ... }
// or
const reduxThemes = new Set(['redux', 'redux-dark', 'redux-color']);
if (reduxThemes.has(theme)) { ... }🟡 [important] — markers.js: Hardcoded fill: 'white' in neo markers
The zero_or_one_neo and zero_or_more_neo marker functions use .attr('fill', 'white') for circles. On dark themes, these white fills will look wrong. I notice the existing non-neo markers have the same issue (pre-existing), but since the neo markers are new code, it would be good to use the theme background color instead:
const { themeVariables } = config;
// Use themeVariables.mainBkg or similar
.attr('fill', themeVariables.mainBkg || 'white')The // Fill white for now? comments in the code suggest this was already flagged as a TODO — would be great to address it now rather than carrying the tech debt forward.
🟢 [nit] — markers.js:261: Leftover debug comment
.attr('refY', 5) // What!!!??This comment should be removed or replaced with something descriptive before merge.
🟢 [nit] — erBox.ts: lineToPolygon diagonal case is dead code
The lineToPolygon helper handles diagonal/angled lines, but ER box dividers are always axis-aligned (horizontal or vertical). The diagonal branch can never be reached. Consider removing it to keep the code minimal, or add a comment noting it's for potential future use.
Security
No XSS or injection issues identified. The changes are limited to SVG marker definitions, CSS class manipulation, and style attribute setting with theme-derived values. No user-controlled input flows to dangerous DOM sinks.
Checklist
- At least one 🎉 [praise] item exists
- No duplicate comments
- Severity tally: 1 🔴 blocking / 4 🟡 important / 2 🟢 nit / 0 💡 suggestion / 3 🎉 praise
- Verdict matches criteria: REQUEST_CHANGES (1 🔴 + 4 🟡)
- Tone check: collaborative and constructive
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…maid-js/mermaid into feature/neo-look-er-diagram
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Great work on the follow-up, @darshanr0107! All 7 items from the previous review have been addressed — including removing the render.ts shared-code change entirely, which was the right call. One remaining consistency issue to flag.
Previous Review Items — Resolution Status
| # | Prev Severity | Issue | Status |
|---|---|---|---|
| 1 | 🔴 | render.ts flood-color change in shared code |
✅ Removed from PR |
| 2 | 🟡 | Global DOM query for color indexing (erBox.ts) |
✅ Fixed with entityNode.colorIndex from DB |
| 3 | 🟡 | styles.ts calling configApi.getConfig() directly |
✅ Fixed — uses options parameter now |
| 4 | 🟡 | Fragile theme?.includes() matching in styles.ts |
✅ Fixed with exact equality |
| 5 | 🟡 | Hardcoded fill: 'white' in neo markers |
✅ Fixed with mainBkg ?? 'white' |
| 6 | 🟢 | Leftover debug comment // What!!!?? |
✅ Removed |
| 7 | 🟢 | lineToPolygon diagonal dead code |
✅ Removed |
What's Working Well
🎉 [praise] Removing the render.ts change was the cleanest resolution to the previous 🔴 — it completely eliminates the cross-diagram regression risk. Good judgment call.
🎉 [praise] The colorIndex approach in erDb.ts:212-216 is much better than the global DOM query it replaces. Computing the index from data during getData() is deterministic and multi-diagram safe. Clean fix.
🎉 [praise] The mermaidAPI.ts:166-169 change to spread look and theme into the style options is the right architectural approach — it lets diagram styles functions access config values through the established options parameter contract rather than reaching into global state. This pattern will benefit other diagram types adopting neo look.
🎉 [praise] The styles.ts cleanup is thorough — configApi import removed, all theme checks use exact equality (theme !== 'redux-color', theme === 'redux-color'), and the DiagramStylesProvider type is now used correctly. This is a good reference implementation for other diagrams.
Things to Address
🟡 [important] — erBox.ts:62,239,318: Fragile theme?.includes() still present
The exact same fragile matching pattern that was fixed in styles.ts still exists in erBox.ts:
// erBox.ts:62 and :239
if (theme?.includes('color')) { ... }
// erBox.ts:318
if (theme?.includes('redux')) { ... }For consistency with the styles.ts fix, these should use exact matching:
// Lines 62 and 239
if (theme === 'redux-color' || theme === 'redux-dark-color') { ... }
// or
const COLOR_THEMES = new Set(['redux-color', 'redux-dark-color']);
if (COLOR_THEMES.has(theme)) { ... }
// Line 318
const REDUX_THEMES = new Set(['redux', 'redux-dark', 'redux-color', 'redux-dark-color']);
if (REDUX_THEMES.has(theme)) { ... }This is the same concern from the previous review (item #4) — it was fixed in styles.ts but the same pattern in erBox.ts was missed.
Security
No XSS or injection issues identified. The changes are limited to:
- SVG marker definitions with hardcoded geometry attributes (no user input)
- Integer
colorIndexderived from entity insertion order (not user-controlled) - CSS template literals interpolating theme variables (validated by the config system)
data-color-idattributes set from integer indices via D3's safesetAttribute()
Checklist
- At least one 🎉 [praise] item exists (4)
- No duplicate comments
- Severity tally: 0 🔴 blocking / 1 🟡 important / 0 🟢 nit / 0 💡 suggestion / 4 🎉 praise
- Verdict matches criteria: COMMENT (1 🟡)
- Tone check: collaborative and appreciative
This is very close to ready — just the one consistency fix in erBox.ts and it should be good to go.
2 participants
📑 Summary
This PR implements the neo look for the ER diagram, extending the existing neo look support (already available for other diagram types) to ER 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:.