Emit theme colors as CSS vars with fallbacks; add webCompatibility SVG normalize - #8008
Open
AMDphreak wants to merge 4 commits into
Open
Emit theme colors as CSS vars with fallbacks; add webCompatibility SVG normalize#8008AMDphreak wants to merge 4 commits into
AMDphreak wants to merge 4 commits into
Conversation
Emit theme colors as var(--mermaid-<slot>, <resolved>) after concrete render, and normalize SVG for responsive host embedding. Distinct from accepting CSS vars as themeVariables inputs (mermaid-js#6860); implements mermaid-js#8007. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds opt-in CSS-variable theme rewriting and web-compatible SVG normalization. The change defines, validates, defaults, documents, tests, and applies both options during Mermaid SVG serialization. ChangesSVG Web Compatibility Configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MermaidAPI
participant prepareMermaidSvgForWeb
participant rewriteMermaidSvgCssVars
participant normalizeMermaidSvgForWeb
MermaidAPI->>prepareMermaidSvgForWeb: serialized SVG and configuration
prepareMermaidSvgForWeb->>rewriteMermaidSvgCssVars: theme variables and CSS prefix
rewriteMermaidSvgCssVars-->>prepareMermaidSvgForWeb: rewritten SVG
prepareMermaidSvgForWeb->>normalizeMermaidSvgForWeb: web compatibility options
normalizeMermaidSvgForWeb-->>prepareMermaidSvgForWeb: normalized SVG
prepareMermaidSvgForWeb-->>MermaidAPI: transformed SVG before sanitization
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
🦋 Changeset detectedLatest commit: 7071d00 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 @@
## develop #8008 +/- ##
===========================================
+ Coverage 77.54% 77.64% +0.10%
===========================================
Files 564 573 +9
Lines 74906 75600 +694
Branches 12661 15006 +2345
===========================================
+ Hits 58087 58703 +616
- Misses 15821 15893 +72
- Partials 998 1004 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/mermaid/src/mermaidAPI.ts (1)
594-603: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover this branch through
serializeSvg().The new tests exercise
prepareMermaidSvgForWebdirectly, but not configuration propagation or the transformed output after sandbox/DOMPurify processing. Add a focusedmermaidAPIregression test for the new flags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mermaid/src/mermaidAPI.ts` around lines 594 - 603, Add a focused mermaidAPI regression test covering serializeSvg() with cssVariableTheme and webCompatibility enabled, verifying both configuration propagation to prepareMermaidSvgForWeb and the resulting sanitized/transformed SVG output. Reuse the existing test setup and assert the behavior through the public serialization path rather than calling prepareMermaidSvgForWeb directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mermaid/src/utils/sanitizeDirective.ts`:
- Around line 70-74: Replace the blanket early skip for cssVariableTheme and
webCompatibility in sanitizeDirective with explicit nested-option validation.
Preserve the prototype-pollution key checks, validate prefix and
preserveAspectRatio as strings restricted to a safe charset, and validate other
supported nested options as booleans while rejecting unknown keys; reuse the
existing validation patterns or helpers where applicable.
- Around line 70-74: In packages/mermaid/src/utils/sanitizeDirective.ts:70-74,
replace the cssVariableTheme/webCompatibility bypass with targeted nested
validation: require prefix and preserveAspectRatio to be strings using a safe
charset excluding ", <, >, ), and ;, require other nested values to be booleans,
and retain the __/proto/constr guard. In
packages/mermaid/src/utils/svgCssVars.ts:181-191, harden setAttr/removeAttr by
escaping attribute values, including ", <, and &, before interpolation; also
escape prefix at the CSS var construction sites around lines 127 and 158.
In `@packages/mermaid/src/utils/svgCssVars.ts`:
- Around line 17-29: Update rewriteMermaidSvgCssVars to scope color-variable
replacement to SVG styling contexts only: fill/stroke/style attribute values and
generated <style> CSS content. Do not perform global serialized-SVG text
replacement, and ensure element text content such as labels remains unchanged;
adjust LOOKS_LIKE_COLOR handling as needed to support this scoped matching.
- Around line 208-219: Change the stripBackground default in the options
normalization block to false so webCompatibility enabled via the boolean form
does not remove diagram backgrounds implicitly. Preserve explicit
options.stripBackground values while keeping the existing defaults for
responsiveWidth, responsiveHeight, ensureViewBox, and preserveAspectRatio
unchanged.
- Around line 85-97: Update colorOccurrenceRegex so the shorthand alternative is
generated only when each expanded hex color component is a doubled pair;
otherwise match only the full-length hex value. Apply this validation for both
six-digit and eight-digit values, preserving the existing case-insensitive regex
behavior and fallback handling.
---
Nitpick comments:
In `@packages/mermaid/src/mermaidAPI.ts`:
- Around line 594-603: Add a focused mermaidAPI regression test covering
serializeSvg() with cssVariableTheme and webCompatibility enabled, verifying
both configuration propagation to prepareMermaidSvgForWeb and the resulting
sanitized/transformed SVG output. Reuse the existing test setup and assert the
behavior through the public serialization path rather than calling
prepareMermaidSvgForWeb directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af86abec-bf7c-4418-a091-820bd8ecb7b2
📒 Files selected for processing (10)
docs/config/setup/defaultConfig/variables/configKeys.mddocs/config/setup/mermaid/interfaces/MermaidConfig.mddocs/config/setup/mermaid/interfaces/UnknownDiagramError.mdpackages/mermaid/src/config.type.tspackages/mermaid/src/defaultConfig.tspackages/mermaid/src/mermaidAPI.tspackages/mermaid/src/schemas/config.schema.yamlpackages/mermaid/src/utils/sanitizeDirective.tspackages/mermaid/src/utils/svgCssVars.spec.tspackages/mermaid/src/utils/svgCssVars.ts
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Scope color rewrites to paint contexts, default stripBackground to off, validate nested directive options, and add a changeset. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mermaid/src/utils/sanitizeDirective.ts`:
- Around line 105-162: Ensure all CSS custom-property prefixes begin with “--”.
In packages/mermaid/src/utils/sanitizeDirective.ts lines 105-162, reject or
normalize prefix values lacking that marker; in
packages/mermaid/src/utils/svgCssVars.ts lines 122-139, pass the normalized
prefix to buildBindings; and in packages/mermaid/src/utils/svgCssVars.ts lines
307-317, update sanitizeCssIdent to add “--” for direct API calls. Add coverage
in packages/mermaid/src/utils/sanitizeDirective.spec.ts lines 48-73 for a prefix
without “--”, asserting rejection or normalization.
In `@packages/mermaid/src/utils/svgCssVars.ts`:
- Around line 181-197: Update rewritePaintContexts so rewrite is applied only to
values of paint declarations within <style> blocks and style attributes,
including fill, stroke, stop-color, color, flood-color, and lighting-color.
Preserve selectors, custom-property names, URLs, and unrelated declaration
values; ensure `.red { fill: red; }` retains its selector while rewriting only
the fill value, and add a regression test for this case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f547fc21-e4da-47b8-a5e7-fbddbe6314eb
📒 Files selected for processing (9)
.changeset/css-var-theme-web-compat.mddocs/config/setup/mermaid/interfaces/MermaidConfig.mdpackages/mermaid/src/config.type.tspackages/mermaid/src/mermaidAPI.spec.tspackages/mermaid/src/schemas/config.schema.yamlpackages/mermaid/src/utils/sanitizeDirective.spec.tspackages/mermaid/src/utils/sanitizeDirective.tspackages/mermaid/src/utils/svgCssVars.spec.tspackages/mermaid/src/utils/svgCssVars.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/mermaid/src/config.type.ts
- packages/mermaid/src/utils/svgCssVars.spec.ts
- packages/mermaid/src/schemas/config.schema.yaml
- docs/config/setup/mermaid/interfaces/MermaidConfig.md
Comment on lines
+105
to
+162
| /** Reject `"`, `<`, `>`, `)`, `;` so values cannot break out of SVG attrs / CSS `var()`. */ | ||
| const SAFE_OPTION_STRING = /^[^"<>);]+$/; | ||
| const CSS_VAR_PREFIX = /^(--)?[\w-]*$/; | ||
|
|
||
| const CSS_VARIABLE_THEME_NESTED = new Set(['prefix']); | ||
| const WEB_COMPATIBILITY_NESTED = new Set([ | ||
| 'responsiveWidth', | ||
| 'responsiveHeight', | ||
| 'ensureViewBox', | ||
| 'stripBackground', | ||
| 'preserveAspectRatio', | ||
| ]); | ||
|
|
||
| const sanitizeSvgPostProcessOptions = ( | ||
| topKey: 'cssVariableTheme' | 'webCompatibility', | ||
| opts: Record<string, unknown> | ||
| ): void => { | ||
| const allowed = | ||
| topKey === 'cssVariableTheme' ? CSS_VARIABLE_THEME_NESTED : WEB_COMPATIBILITY_NESTED; | ||
| for (const nested of Object.keys(opts)) { | ||
| const value = opts[nested]; | ||
| if ( | ||
| nested.startsWith('__') || | ||
| nested.includes('proto') || | ||
| nested.includes('constr') || | ||
| !allowed.has(nested) | ||
| ) { | ||
| log.debug('sanitize deleting nested key:', topKey, nested); | ||
| delete opts[nested]; | ||
| continue; | ||
| } | ||
| if (nested === 'prefix') { | ||
| if ( | ||
| typeof value !== 'string' || | ||
| !CSS_VAR_PREFIX.test(value) || | ||
| !SAFE_OPTION_STRING.test(value) | ||
| ) { | ||
| log.debug('sanitize deleting invalid prefix:', value); | ||
| delete opts[nested]; | ||
| } | ||
| continue; | ||
| } | ||
| if (nested === 'preserveAspectRatio') { | ||
| if (typeof value === 'boolean') { | ||
| continue; | ||
| } | ||
| if (typeof value !== 'string' || !SAFE_OPTION_STRING.test(value)) { | ||
| log.debug('sanitize deleting invalid preserveAspectRatio:', value); | ||
| delete opts[nested]; | ||
| } | ||
| continue; | ||
| } | ||
| if (typeof value !== 'boolean') { | ||
| log.debug('sanitize deleting non-boolean nested option:', nested, value); | ||
| delete opts[nested]; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure every emitted CSS custom property starts with --.
A valid directive or API option such as { prefix: 'host-' } currently emits var(host-primaryColor, ...). That is not a CSS custom property reference, so the affected paint declaration becomes invalid.
packages/mermaid/src/utils/sanitizeDirective.ts#L105-L162: reject or normalize prefixes that do not start with--.packages/mermaid/src/utils/svgCssVars.ts#L122-L139: ensurebuildBindingsreceives a normalized prefix.packages/mermaid/src/utils/svgCssVars.ts#L307-L317: makesanitizeCssIdentadd--when it is absent, so direct API calls are safe.packages/mermaid/src/utils/sanitizeDirective.spec.ts#L48-L73: add coverage for a prefix without--and assert rejection or normalized output.
📍 Affects 3 files
packages/mermaid/src/utils/sanitizeDirective.ts#L105-L162(this comment)packages/mermaid/src/utils/svgCssVars.ts#L122-L139packages/mermaid/src/utils/svgCssVars.ts#L307-L317packages/mermaid/src/utils/sanitizeDirective.spec.ts#L48-L73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mermaid/src/utils/sanitizeDirective.ts` around lines 105 - 162,
Ensure all CSS custom-property prefixes begin with “--”. In
packages/mermaid/src/utils/sanitizeDirective.ts lines 105-162, reject or
normalize prefix values lacking that marker; in
packages/mermaid/src/utils/svgCssVars.ts lines 122-139, pass the normalized
prefix to buildBindings; and in packages/mermaid/src/utils/svgCssVars.ts lines
307-317, update sanitizeCssIdent to add “--” for direct API calls. Add coverage
in packages/mermaid/src/utils/sanitizeDirective.spec.ts lines 48-73 for a prefix
without “--”, asserting rejection or normalization.
Comment on lines
+181
to
+197
| /** | ||
| * Rewrite colors only in paint contexts: `<style>` CSS and fill/stroke/style attrs. | ||
| * Never touch element text (labels like "Red Team"). | ||
| */ | ||
| function rewritePaintContexts(svg: string, rewrite: (chunk: string) => string): string { | ||
| let out = svg.replace(/<style\b[^>]*>[\s\S]*?<\/style>/gi, (block) => rewrite(block)); | ||
| out = out.replace( | ||
| /\b(fill|stroke|stop-color|color|flood-color|lighting-color)\s*=\s*("[^"]*"|'[^']*')/gi, | ||
| (_full, name: string, quoted: string) => { | ||
| const q = quoted[0]; | ||
| return `${name}=${q}${rewrite(quoted.slice(1, -1))}${q}`; | ||
| } | ||
| ); | ||
| out = out.replace(/\bstyle\s*=\s*("[^"]*"|'[^']*')/gi, (_full, quoted: string) => { | ||
| const q = quoted[0]; | ||
| return `style=${q}${rewrite(quoted.slice(1, -1))}${q}`; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Limit rewriting to paint declaration values.
rewritePaintContexts rewrites the complete <style> block and complete style attribute value. If a fallback is red, it rewrites a selector such as .red { ... } into invalid CSS. It can also rewrite matching text in non-paint declarations.
Rewrite only the values of paint properties such as fill, stroke, and stop-color. Do not rewrite selectors, custom-property names, URLs, or unrelated declaration values. Add a regression test for .red { fill: red; }.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mermaid/src/utils/svgCssVars.ts` around lines 181 - 197, Update
rewritePaintContexts so rewrite is applied only to values of paint declarations
within <style> blocks and style attributes, including fill, stroke, stop-color,
color, flood-color, and lighting-color. Preserve selectors, custom-property
names, URLs, and unrelated declaration values; ensure `.red { fill: red; }`
retains its selector while rewriting only the fill value, and add a regression
test for this case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cssVariableThemeconfig: after concrete theme render, rewrite SVG theme colors in<style>and presentation attrs tovar(--mermaid-<slot>, <resolvedFallback>)so one prerendered SVG works under host light/dark themes.webCompatibilityconfig: ensureviewBox, setwidth="100%"/height="auto", setpreserveAspectRatiowhen missing, optionally strip hardcoded backgrounds.packages/mermaid/src/utils/svgCssVars.ts(thin in-tree mirror of https://github.com/openshellorg/mermaid-svg-css-vars — no new dependency).svgCssVars.spec.ts.This is emit-time CSS vars with fallbacks. It is not #6860 (CSS vars as themeVariables inputs fail because khroma derivation needs concrete colors).
Closes / implements: #8007
Related: #6860
Test plan
vitest run packages/mermaid/src/utils/svgCssVars.spec.ts(5 tests passed)tsx .esbuild/build.tssucceeds with updated config schemamermaid.renderwith{ cssVariableTheme: true, webCompatibility: true }and confirm SVG containsvar(--mermaid-…)and responsive root attrsfalseleaves existing SVG output unchangedSummary
Adds opt-in SVG post-processing configuration:
cssVariableThemerewrites theme colors in SVG paint contexts to CSS variables with resolved fallbacks.webCompatibilitynormalizes SVG sizing,viewBox,preserveAspectRatio, and optional background removal.Defaults remain disabled to preserve existing output. The build succeeds. Manual validation is pending.