feat(gitgraph): implement neo look#7413
feat(gitgraph): implement neo look#7413omkarht wants to merge 10 commits intofeature/neo-look-implementedfrom
Conversation
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
🦋 Changeset detectedLatest commit: cbb6e10 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/neo-look-implemented #7413 +/- ##
===============================================================
- Coverage 3.59% 3.52% -0.08%
===============================================================
Files 481 502 +21
Lines 49929 51749 +1820
Branches 740 771 +31
===============================================================
+ Hits 1793 1822 +29
- Misses 48136 49927 +1791
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>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for the work on this, @omkarht — bringing neo-look support to gitGraph is a nice addition to the ongoing neo-look implementation effort. The test coverage across multiple themes is particularly solid. A few things to address before this is ready to merge.
What's working well
🎉 [praise] Excellent E2E test coverage — 5 carefully documented test scenarios covering all commit types (NORMAL/REVERSE/HIGHLIGHT), merge/cherry-pick with tags, all 4 directions (LR/TB/BT), branch ordering, hidden branches/labels, parallel commits, and cycling past THEME_COLOR_LIMIT. Each test case has clear comments explaining what it covers. This is really well done.
🎉 [praise] Changeset correctly uses minor bump with feat: prefix — follows project conventions perfectly.
Things to address
🔴 [blocking] Diagram-specific logic in shared styles.ts (packages/mermaid/src/styles.ts)
The PR adds a type === 'gitGraph' conditional and a genGitGraphGradient function to the shared styles.ts file. This breaks the established pattern on the feature/neo-look-implemented base branch, which uses generic [data-look="neo"] CSS attribute selectors — those apply to any diagram type without JS-level type checking.
Adding diagram-specific JS conditionals to shared code violates the isolation rules (CLAUDE.md: "Don't add diagram-specific logic to shared rendering code") and sets a precedent where every diagram's neo-look would add its own type === 'diagramName' block to this file.
The gradient CSS should be generated in packages/mermaid/src/diagrams/git/styles.js instead. The svgId is available in the base branch's styles.ts signature and could be passed through to the diagram-specific style function if needed (worth coordinating with the team on the best approach for this).
🟡 [important] E2E tests don't exercise look: 'neo' code paths (cypress/integration/rendering/gitGraph-neo-look.spec.js:162)
All 20 test cases use { look: 'classic', theme }. However, several code paths in the renderer and styles are gated on look === 'neo':
gitGraphRenderer.ts~line 311: branch label drop-shadow anddata-look="neo"attribute — only runs whenlook === 'neo'styles.ts:genGitGraphGradient— only runs whenlook === 'neo'
It would be great to add test variants with look: 'neo' to ensure those code paths are covered. Otherwise, these features could break silently.
🟡 [important] SVG filter ID collision and invalid SVG structure (gitGraphRenderer.ts ~line 866)
Two issues with the drop-shadow filter creation:
-
Hardcoded
id='drop-shadow'— if a page has multiple gitGraph diagrams, they'll all create filters with the same ID. SVG IDs must be unique per page. Consider incorporating the diagram'ssvgIdor a unique suffix. -
defsappended insidebkg—bkgappears to be arectelement (it hasrx,ry,width,heightattributes). SVGdefselements cannot be children ofrect— they must be children ofsvgorgelements. This may silently produce invalid SVG. Consider appending thedefsto the parentsvgor agelement instead.
🟡 [important] Missing optional chaining on theme in styles.js (packages/mermaid/src/diagrams/git/styles.js)
Two lines use theme.includes(...) without optional chaining:
genColor:const isReduxTheme = theme.includes('redux');(around the top of the function)getStyles:const isReduxTheme = theme.includes('redux') || theme.includes('neo');
If theme is undefined (e.g., config with no theme set), these will throw a TypeError. The renderer code correctly uses theme?.includes(...) — it would be great to apply the same defensive pattern here.
🟡 [important] Inconsistent isReduxTheme definitions (gitGraphRenderer.ts vs styles.js)
The same concept is defined differently in two files:
- Renderer (
gitGraphRenderer.ts):const isReduxTheme = theme?.includes('redux'); - Styles (
styles.js):const isReduxTheme = theme.includes('redux') || theme.includes('neo');
This means a theme named neo (without "redux") would trigger neo-look styling in CSS but not in the renderer's geometry adjustments (bullet sizes, padding, etc.). This could cause visual mismatches. Worth aligning these definitions — perhaps extracting a shared helper or using a consistent check in both files.
🟢 [nit] Typo: 'dar' should be 'dark' (styles.js ~line 456 in the diff)
fill: ${theme?.includes('dar') ? options.mainBkg : borderColorArray[colorIndex]};This works by coincidence (since 'dark' contains 'dar'), but it would also match a hypothetical theme containing just 'dar'. Should be 'dark'.
Tally: 1 🔴 blocking / 4 🟡 important / 1 🟢 nit / 0 💡 suggestion / 2 🎉 praise
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…for neo look on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
…rt for neo theme on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for the continued iteration on this, @omkarht. Good to see a couple of the prior review items addressed — here's an updated pass on the current state.
What's working well
🎉 E2E tests now correctly use look: 'neo'. The prior review flagged that tests were using look: 'classic' and missing the neo code paths. This is now fixed — all test cases pass { look: 'neo', theme }. The test scenarios themselves remain excellent — thorough coverage of commit types, directions, branch ordering, and THEME_COLOR_LIMIT cycling.
🎉 calcColorIndex is a clean, well-documented helper. Extracting the color index logic into a named, exported function with a clear JSDoc comment is a nice improvement over inline modulo operations. The avoidDefaultColor parameter for redux-color themes is well-thought-out.
Outstanding items from prior review (still present)
🔴 [blocking] — Diagram-specific logic in shared styles.ts
packages/mermaid/src/styles.ts:155: The type === 'gitGraph' conditional and genGitGraphGradient function are still in the shared styles.ts. This was flagged as blocking in the prior review.
This file is imported by every diagram type. Adding diagram-specific JS conditionals here violates isolation rules and sets a precedent where each diagram's neo-look would add its own type === 'diagramName' block. The gradient CSS generation should live in packages/mermaid/src/diagrams/git/styles.js instead.
If svgId is needed for the gradient URL reference, it could be threaded through the options parameter that's already passed to diagram-specific style functions.
🟡 [important] — SVG filter ID collision and defs inside rect-like element
gitGraphRenderer.ts:~895-908 (in drawBranches): Two issues persist from the prior review:
-
Hardcoded
id='drop-shadow'— multiple gitGraph diagrams on one page will create duplicate IDs. Use the diagram'sidparameter: e.g.,id + '-drop-shadow'. -
bkg.append('defs')—bkgis used as arect(it getsrx,ry,width,heightattributes). SVG<defs>cannot be children of<rect>. This produces invalid SVG that browsers may silently ignore or handle inconsistently. Append the<defs>to the parentsvgorgelement instead.
🟡 [important] — Missing optional chaining on theme in styles.js
packages/mermaid/src/diagrams/git/styles.js: Two calls still use theme.includes(...) without ?.:
genColorfunction:const isReduxTheme = theme.includes('redux');(line ~4 of the function)getStylesfunction:const isReduxTheme = theme.includes('redux') || theme.includes('neo');
These will throw TypeError: Cannot read properties of undefined if config has no theme set. The renderer correctly uses theme?.includes(...) — please apply the same pattern here.
🟡 [important] — Inconsistent isReduxTheme definitions
Still divergent between files:
- Renderer (
gitGraphRenderer.ts):isReduxTheme = theme?.includes('redux')— true only for redux themes - Styles (
styles.js):isReduxTheme = theme.includes('redux') || theme.includes('neo')— true for redux AND neo themes
This means the neo theme (without "redux" in the name) gets redux-style CSS rules but not redux-style geometry adjustments in the renderer (bullet sizes, padding, border radius). This will produce visual mismatches. Worth aligning — either both should include 'neo' or neither should.
🟢 [nit] — Typo: 'dar' should be 'dark'
styles.js (in the color theme else branch):
fill: ${theme?.includes('dar') ? options.mainBkg : borderColorArray[colorIndex]};Works by coincidence but should be 'dark'.
New findings
🟡 [important] — Hardcoded colors in cherry-pick rendering
gitGraphRenderer.ts:~237-256: The cherry-pick symbol uses hardcoded #fff / #000000:
.attr('fill', isDark ? '#000000' : '#fff')
.attr('stroke', isDark ? '#000000' : '#fff')This is an improvement over the base branch's unconditional #fff, but hardcoded colors bypass the theme system entirely. If a custom theme uses a non-white/non-black background, these will look wrong. Consider using a theme variable (e.g., options.mainBkg or themeVariables.cScaleLabel0) instead.
🟢 [nit] — Hardcoded loop limit 7 in genColor for neo themes
styles.js (neo theme branch in genColor): for (let i = 0; i <= 7; i++) uses a hardcoded 7, while the redux and color branches use options.THEME_COLOR_LIMIT. This creates a silent inconsistency — if THEME_COLOR_LIMIT is changed from 8, the neo branch won't follow. Consider using options.THEME_COLOR_LIMIT consistently.
Security
No XSS or injection issues identified. The changes use D3's .attr() and .text() for DOM manipulation with config-derived values (theme variables, numeric indices). Branch names and commit IDs flow through .text() (safe) or .attr('class', ...) (not a dangerous sink). The CSS template literals interpolate only theme variables, not user-controlled diagram text. The gradient stop-color values come from themeVariables.gradientStart/Stop, which are config values, not user diagram input.
Self-check
- At least one 🎉 [praise] item
- No duplicate comments
- Severity tally: 1 🔴 blocking / 4 🟡 important / 2 🟢 nit / 0 💡 suggestion / 2 🎉 praise
- Verdict matches criteria: REQUEST_CHANGES (🔴 present)
- Not a draft PR
- Tone check: constructive, acknowledges fixes
Summary
| Severity | Count |
|---|---|
| 🔴 blocking | 1 |
| 🟡 important | 4 |
| 🟢 nit | 2 |
| 💡 suggestion | 0 |
| 🎉 praise | 2 |
The test coverage fix is great — that was a key gap. The main blocker remains the diagram-specific code in shared styles.ts. Once that's moved to the git diagram's own styles.js, and the optional chaining / consistency issues are cleaned up, this should be in good shape. Happy to re-review!
…amline options handling on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for the continued work on this, @omkarht — good progress since the last round. The main blocker from prior reviews (diagram-specific logic in shared styles.ts) is resolved, and several other items have been cleaned up. A few things still need attention before this is ready.
What's working well
🎉 [praise] The blocking styles.ts issue is fully resolved. The gradient generation logic has been moved from the shared styles.ts into diagrams/git/styles.js where it belongs. The shared file now just generically threads svgId through options — clean, reusable by any diagram type. This was the right approach.
🎉 [praise] calcColorIndex is a well-designed helper. Extracting the color index logic into a named, exported function with clear JSDoc and the avoidDefaultColor parameter for redux-color themes is a nice improvement over scattered inline modulo operations.
🎉 [praise] Prior review items addressed. Optional chaining added to styles.js, 'dar' typo fixed to 'dark', hardcoded loop limit replaced with options.THEME_COLOR_LIMIT, and E2E tests correctly use look: 'neo'. Appreciate the thorough follow-through.
Items addressed from prior reviews
| Item | Status |
|---|---|
🔴 Diagram-specific logic in shared styles.ts |
✅ Fixed — moved to diagrams/git/styles.js |
🟡 E2E tests using look: 'classic' instead of 'neo' |
✅ Fixed (round 2) |
🟡 Missing optional chaining on theme in styles.js |
✅ Fixed |
🟢 'dar' typo |
✅ Fixed |
🟢 Hardcoded loop limit 7 in neo theme branch |
✅ Fixed — uses THEME_COLOR_LIMIT |
Outstanding items
🟡 [important] SVG filter ID collision and invalid SVG structure
gitGraphRenderer.ts:~900-910 (in drawBranches): Both issues from the prior review persist:
-
Hardcoded
id='drop-shadow'— multiple gitGraph diagrams on the same page will create duplicate SVG filter IDs. SVGidattributes must be unique per document. Consider using the diagram'sidparameter: e.g.,id + '-drop-shadow'. -
bkg.append('defs')—bkgis created viag.insert('rect')(line ~889), so it's a<rect>element. SVG<defs>elements cannot be children of<rect>— they must be direct children of<svg>or<g>. Browsers may silently ignore this or handle it inconsistently. Consider appending the<defs>to the parentsvgelement or theggroup instead.
These two could be fixed together — define the filter once in the diagram's root <defs> (similar to how the gradient is added in draw() at line ~990) with a unique ID like id + '-drop-shadow'.
🟡 [important] Inconsistent isReduxTheme definitions between renderer and styles
Still divergent between files:
- Renderer (
gitGraphRenderer.ts:217):isReduxTheme = theme?.includes('redux')— true only for redux themes - Styles (
styles.js:105):isReduxTheme = theme?.includes('redux') || theme?.includes('neo')— true for redux AND neo themes
This means for the neo and neo-dark themes (without "redux" in the name):
- CSS: Gets neo-look styling (single-color branches, themed fonts, thin arrows) ✅
- Renderer geometry: Gets classic styling (larger bullets r=10, wider padding, rounded corners) ❌
This mismatch will produce visual inconsistencies for neo/neo-dark themes. Worth aligning — if both redux and neo themes should share the same geometry, the renderer should also check for 'neo'.
Similarly, the .arrow rule in styles.js:133 checks theme?.includes('redux') directly instead of using the isReduxTheme variable, so neo/neo-dark themes would get stroke-width: 8 (classic) instead of options.strokeWidth.
🟡 [important] Hardcoded colors in cherry-pick rendering
gitGraphRenderer.ts:253-276: The cherry-pick symbol uses hardcoded #fff / #000000:
.attr('fill', isDark ? '#000000' : '#fff')
.attr('stroke', isDark ? '#000000' : '#fff')This is better than the base branch's unconditional #fff, but hardcoded colors bypass the theme system. If a user configures a custom theme with a non-white/non-black background, these won't match. Consider using a theme variable like themeVariables.mainBkg or themeVariables.cScaleLabel0 — these are already available via getConfig() in the same function.
🟢 [nit] normalTheme still uses hardcoded [0, 1, 2, 3, 4, 5, 6, 7] array
styles.js:91: The classic theme path uses a hardcoded array while all the neo/redux branches now correctly use options.THEME_COLOR_LIMIT. Not critical since THEME_COLOR_LIMIT is currently 8, but it would be nice to align for consistency. A simple Array.from({length: options.THEME_COLOR_LIMIT}, (_, i) => i) would do.
Security
No XSS or injection issues identified. All user-controlled content (branch names, commit IDs) flows through D3's .text() (safe) or .attr('class', ...) (not a dangerous sink). CSS template literals interpolate only theme variables from config, not user diagram input. The gradient stop-color values come from themeVariables, which are config values. DOMPurify sanitization downstream remains intact.
Self-check
- At least one 🎉 [praise] item
- No duplicate comments
- Severity tally: 0 🔴 blocking / 3 🟡 important / 1 🟢 nit / 0 💡 suggestion / 3 🎉 praise
- Verdict matches criteria: REQUEST_CHANGES (3 🟡 items)
- Not a draft PR
- Tone check: acknowledges significant progress, constructive on remaining items
| Severity | Count |
|---|---|
| 🔴 blocking | 0 |
| 🟡 important | 3 |
| 🟢 nit | 1 |
| 💡 suggestion | 0 |
| 🎉 praise | 3 |
Good progress — the major structural issue is resolved and the test coverage is solid. The remaining items are focused fixes around SVG validity, theme consistency, and color hardcoding. Happy to re-review once these are addressed — let's get this across the finish line! 🙌
…dow and adjust styles on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
This PR has been through 3 rounds of automated review with requested changes still outstanding.
Escalating to a human reviewer for further guidance — automated reviews are most useful in the first couple of rounds, and after that a human eye is better placed to navigate the remaining tradeoffs.
Outstanding items from previous reviews:
-
🟡 SVG filter ID collision and invalid SVG structure in
drawBranches— The drop-shadow<filter>elements are created inside<circle>elements in the branch label rendering path. SVG spec does not allow<filter>as a child of<circle>. Additionally, the filter IDs (e.g.,gitgraph-shadow-branch-0) are not scoped to the SVG instance, which can collide on pages with multiple gitGraph diagrams. Suggested fix: hoist<defs>to the top-level SVG<defs>block and scope filter IDs with thesvgId. -
🟡 Inconsistent
isReduxThemedefinitions between renderer and styles —gitGraphRenderer.tscheckstheme?.startsWith('redux')whilestyles.jsuses a separateisRedux(theme)helper with similar but potentially divergent logic. A single shared constant (likeREDUX_THEMESSet used in other neo-look PRs) would eliminate this risk. -
🟡 Hardcoded colors
#fff/#000000in cherry-pick rendering — The cherry-pick circle rendering indrawCommitsuses hardcoded white/black instead of theme variables, which will look wrong on dark/forest themes. These should referencecommitLabelBackground/commitLabelColoror equivalent theme variables. -
🟢
normalThemehardcoded[0,1,2,3,4,5,6,7]array — The fallback branch-color array could be derived from a constant or config rather than a magic literal.
@maintainers — this one needs a human look. 👀
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for the continued work on this, @omkarht — good progress since the last round. The filter/defs structure is now correct, calcColorIndex is clean and well-documented, and the E2E coverage is excellent.
What's working well
🎉 [praise] calcColorIndex is well-documented, exported for testability, and cleanly handles the avoidDefaultColor cycling logic for redux-color themes. Nice abstraction.
🎉 [praise] SVG filter and gradient <defs> are now properly hoisted to the root <svg> with diagram-scoped IDs (id + '-drop-shadow', id + '-gradient'). This addresses the Round 3 feedback about filter placement and multi-diagram collision. Well done.
🎉 [praise] Excellent E2E coverage — 5 scenarios × 6 themes = 30 test cases covering all directions (LR/TB/BT), all commit types (NORMAL/REVERSE/HIGHLIGHT/CHERRY_PICK), branch ordering, parallel commits, custom mainBranchName, and cherry-pick of merge commits. This is thorough.
🎉 [praise] Changeset included with correct minor bump and feat: prefix.
Things to address
🟡 [important] Inconsistent isReduxTheme semantics across files
The variable isReduxTheme means different things in different files:
gitGraphRenderer.ts(lines ~216, ~851):isReduxTheme = theme?.includes('redux')— gates geometry adjustments (smaller bullets, sharper edges)styles.js:getStyles(line ~101):isReduxTheme = theme?.includes('redux') || theme?.includes('neo')— gates which color generation function to use
For neo and neo-dark themes, styles will use the neo/redux color path (genColor) while the renderer uses standard (non-redux) geometry. If this is intentional — neo themes need custom color generation but standard geometry — the variable names should reflect that. Something like useNeoColorGen in styles vs useReduxGeometry in the renderer would make the intent explicit and prevent future contributors from "fixing" what looks like a bug.
Additionally, the .arrow section at the end of getStyles uses a raw theme?.includes('redux') check instead of the isReduxTheme variable, creating a third inconsistency within the same file.
🟡 [important] Fragile includes() theme matching throughout
Both files use theme?.includes('color'), theme?.includes('redux'), theme?.includes('dark') for theme detection. A hypothetical future theme like 'colorful' would incorrectly match includes('color').
Other PRs in the neo-look series (#7384 sequence, #7388 ER) adopted Set-based matching:
const COLOR_THEMES = new Set(['redux-color', 'redux-dark-color']);
const REDUX_THEMES = new Set(['redux', 'redux-dark', 'redux-color', 'redux-dark-color']);
// Usage: COLOR_THEMES.has(theme)It would be great to use the same pattern here for consistency across the feature branch and resilience against future theme additions.
🟢 [nit] normalTheme hardcoded array
styles.js:normalTheme uses [0, 1, 2, 3, 4, 5, 6, 7].map(...). Since THEME_COLOR_LIMIT is 8, this could be Array.from({length: options.THEME_COLOR_LIMIT}, (_, i) => i).map(...) to stay in sync if the limit ever changes.
💡 [suggestion] configApi.getConfig() in styles.js — coordinate with PR #7388
The direct configApi.getConfig() calls in genColor and getStyles are currently necessary because the base branch doesn't provide options.theme/options.look. PR #7388 (ER neo look) adds these to the options via mermaidAPI.ts. Once that PR merges, it would be worth migrating to options.theme/options.look to follow the styles function contract and avoid coupling to global state.
Security
No XSS or injection issues identified. All theme variable interpolation follows pre-existing patterns. CSS output goes through stylis compilation and final SVG output goes through DOMPurify sanitization. No new DOM sinks, no bypasses of the sanitization pipeline.
Verdict: COMMENT — Two 🟡 items (naming inconsistency + fragile matching) that are worth discussing but not blocking on their own. The core functionality is solid and the previous structural issues are resolved. Let's get this across the finish line. 🚀
📑 Summary
This PR implements the neo look for the gitGraph diagram, extending the existing neo look support (already available for other diagram types) to gitgraph-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:.