fix: polyfill CSSStyleDeclaration for sequence diagrams#14
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA runtime patch is added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
pilat
force-pushed
the
fix/sequence-diagram-style-polyfill
branch
from
b2a2417 to
e02c1c1
Compare
pilat
force-pushed
the
fix/sequence-diagram-style-polyfill
branch
from
e02c1c1 to
84123a8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mermaid/render.ts (1)
60-82: Regex patterns may incorrectly match property names that appear as substrings.The current regex patterns search anywhere in the style string, which could match a property name that appears within another property name's text. For example,
getPropertyValue('color')onbackground-color: red;would incorrectly returnredbecausecolor:appears at position 11 in the string.For SVG properties (
fill/fill-opacity,stroke/stroke-width), this is safe since the base property is followed by-rather than:. However, for a more robust shim, anchoring the match to the start of string or after a semicolon would prevent edge cases.Also,
setPropertylacks the'g'flag whileremovePropertyhas it—this inconsistency could cause duplicate properties ifsetPropertyis called when a property already appears multiple times.Suggested fix with anchored regex patterns
function createStyleShim(element: any) { return { getPropertyValue(name: string): string { const style = element.getAttribute('style') || '' - const match = style.match(new RegExp(name + '\\s*:\\s*([^;]+)')) + const match = style.match(new RegExp('(?:^|;)\\s*' + name + '\\s*:\\s*([^;]*)', 'i')) return match ? match[1].trim() : '' }, setProperty(name: string, value: string): void { const style = element.getAttribute('style') || '' - const regex = new RegExp(name + '\\s*:[^;]+;?\\s*') + const regex = new RegExp('(?:^|;\\s*)' + name + '\\s*:[^;]*;?\\s*', 'gi') const cleaned = style.replace(regex, '') const newStyle = cleaned + (cleaned && !cleaned.endsWith(';') ? '; ' : '') + name + ': ' + value + ';' element.setAttribute('style', newStyle.trim()) }, removeProperty(name: string): string { const style = element.getAttribute('style') || '' - const regex = new RegExp(name + '\\s*:[^;]+;?\\s*', 'g') + const regex = new RegExp('(?:^|;\\s*)' + name + '\\s*:[^;]*;?\\s*', 'gi') const oldValue = this.getPropertyValue(name) element.setAttribute('style', style.replace(regex, '').trim()) return oldValue }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mermaid/render.ts` around lines 60 - 82, The style-shim regexes in createStyleShim can falsely match substrings and are inconsistent with flags; update getPropertyValue, setProperty and removeProperty to anchor property matches to the start of the style string or immediately after a semicolon (use a pattern like (?:^|;\s*)<name>\s*:\s*([^;]+) ) so you only match whole property occurrences, add the 'g' flag to the regex used in setProperty (to remove duplicates before inserting), and keep existing trimming/semicolon logic in setProperty and removeProperty so you remove all prior matches and insert a single normalized "name: value;" string; reference functions: createStyleShim, getPropertyValue, setProperty, removeProperty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/mermaid/render.ts`:
- Around line 60-82: The style-shim regexes in createStyleShim can falsely match
substrings and are inconsistent with flags; update getPropertyValue, setProperty
and removeProperty to anchor property matches to the start of the style string
or immediately after a semicolon (use a pattern like
(?:^|;\s*)<name>\s*:\s*([^;]+) ) so you only match whole property occurrences,
add the 'g' flag to the regex used in setProperty (to remove duplicates before
inserting), and keep existing trimming/semicolon logic in setProperty and
removeProperty so you remove all prior matches and insert a single normalized
"name: value;" string; reference functions: createStyleShim, getPropertyValue,
setProperty, removeProperty.
1 participant
Mermaid sequence diagrams were crashing with
this.style.removeProperty is not a function.The root cause is svgdom's broken
.styleimplementation — it returns the element itself instead of a CSSStyleDeclaration object. D3 (bundled in mermaid) callselement.style.removeProperty()during sequence diagram rendering, which fails because there's no such method on the element.Flowcharts work because
htmlLabels: falsebypasses most CSS manipulation, but sequence diagrams have a different code path that always hits these D3 style methods.The fix adds a minimal CSSStyleDeclaration shim that parses and manipulates the
styleattribute string directly. It slots in with the other svgdom/elkjs compatibility patches inloadMermaid().Summary by CodeRabbit