chore: add some unicorn eslint plugin rules #925
Conversation
Start using the plugin but only activate a few rules to limit the number of changes. This fixes some errors detected by Sonar.
WalkthroughAdds eslint-plugin-unicorn and enables two rules in ESLint config. Replaces global parseInt/parseFloat/isNaN/isFinite with Number.* across many files. Refactors numerous if/else ladders to switch statements. Expands editor decoding (UI/resources/stylesheet), toolbar decoding (actions/modes/templates/combos), and stencil drawing (SVG-like commands, rounded paths). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant XML as Editor XML
participant Codec as EditorCodec
participant Doc as Document
participant UI as Editor UI
participant I18n as GlobalConfig.i18n
XML->>Codec: decode(node)
alt nodeName == "add"
Codec->>Doc: getElementById(element)
alt element found
Codec->>UI: apply style (if any)
UI->>UI: attach existing element
else create new window
Codec->>Doc: createElement(div)
Codec->>UI: new MaxWindow(div, x,y[,w,h])
UI->>UI: apply style, show
end
else nodeName == "resource"
Codec->>I18n: addResource(basename)
else nodeName == "stylesheet"
Codec->>Doc: inject <link rel="stylesheet" href=basename>
end
sequenceDiagram
autonumber
participant XML as Toolbar XML
participant TCodec as EditorToolbarCodec
participant TB as Toolbar
participant Doc as Document
XML->>TCodec: decode(node)
alt nodeName == "separator"/"br"/"hr"
TCodec->>TB: addSeparator/Break/Line
else nodeName == "add"
alt has action
TCodec->>TB: addAction(item)
else has mode
TCodec->>TB: addMode(item[, evaluated text])
else has template
TCodec->>TB: create prototype from template
TB->>TB: optional insertFunction
else has children
alt has icon
TCodec->>TB: build action combo
else
TCodec->>TB: build standard combo
end
TCodec->>TB: populate options (+separators/adds/styles)
TB->>TB: onChange -> insert selected
end
TCodec->>Doc: set id if present
end
sequenceDiagram
autonumber
participant Node as Stencil XML Node
participant Stencil as StencilShape.drawNode
participant C as Canvas2D
Note over Stencil: paint branch
Node->>Stencil: dispatch by nodeName (switch)
alt path (rounded && only move/line)
Stencil->>Stencil: collect segs (M/L)
Stencil->>C: addPoints(segs, rounded)
else path (regular or mixed)
Stencil->>C: traverse children -> moveTo/lineTo/curveTo/arcTo/close
end
alt rect/roundrect/ellipse
Stencil->>C: draw with computed dims/radius
end
alt include-shape
Stencil->>Stencil: resolve referenced stencil and draw
end
alt image
Stencil->>C: drawImage(x,y,w,h,src[,flip])
end
alt text
Stencil->>C: drawText(alignment, rotation[, flipH/V])
end
alt styling nodes
Stencil->>C: set stroke/fill/width/dash/colors/alpha/font
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
packages/core/src/view/layout/hierarchical/CoordinateAssignment.ts (1)
328-328: LGTM! Consider simplifying the unnecessary conversion.The change from
parseInttoNumber.parseIntcorrectly aligns with the PR objectives and ESLint unicorn plugin requirements.However, since
cellWrapper.rankIndexis already a number (assigned from loop indexjat line 305), theString()andNumber.parseInt()conversions are redundant.Consider simplifying to:
-const rankIndex = Number.parseInt(String(cellWrapper.rankIndex)); +const rankIndex = cellWrapper.rankIndex;packages/core/src/view/style/perimeter/TrianglePerimeter.ts (1)
51-73: LGTM! Clean refactor to switch statement.The refactoring from if/else to switch is correct and aligns with ESLint best practices. Each case properly handles the direction-specific geometry transformations with explicit breaks.
Optionally, consider making the
// No defaultcomment more descriptive to improve maintainability:- // No default + // No default: 'east' direction uses the initial values set abovepackages/core/src/view/style/edge/Loop.ts (1)
90-110: Consider adding braces and a break for consistency.The switch refactor is correct and aligns with the PR objectives. Two optional style improvements:
- Add braces after
else(line 90) for better readability- Add
break;to the default case (line 107) for defensive programmingApply this diff:
- } else - switch (dir) { + } else { + switch (dir) { case 'north': { y = source.y - 2 * dx; break; } case 'south': { y = source.y + source.height + 2 * dx; break; } case 'east': { x = source.x - 2 * dy; break; } default: { x = source.x + source.width + 2 * dy; + + break; } } + }packages/core/src/view/style/perimeter/EllipsePerimeter.ts (1)
46-47: Simplify numeric conversion.The pattern
Number.parseInt(String(px - cx))converts a number to a string and back to a number, which is inefficient. Sincepx - cxandpy - cyare already numeric values, useMath.round()orMath.trunc()directly for integer conversion.Apply this diff:
- const dx = Number.parseInt(String(px - cx)); - const dy = Number.parseInt(String(py - cy)); + const dx = Math.round(px - cx); + const dy = Math.round(py - cy);Note: Use
Math.trunc()instead ofMath.round()if truncation (rather than rounding) is the desired behavior.packages/core/src/serialization/codecs/editor/EditorCodec.ts (3)
206-210: Avoid throwing on unimplemented 'map'; log and continueThrowing breaks decoding for XMLs that include map; prefer warning + ignore to keep backward compatibility.
- case 'map': { - throw new Error('Unimplemented'); - } + case 'map': { + GlobalConfig.logger.warn('EditorCodec: <add as="map"> is not implemented; ignoring.'); + break; + }
214-217: Guard missing basename inbasename may be null; calling addResource with null can fail. Guard and log.
- case 'resource': { - GlobalConfig.i18n.addResource(tmp.getAttribute('basename')); - break; - } + case 'resource': { + const basename = tmp.getAttribute('basename'); + if (basename) { + GlobalConfig.i18n.addResource(basename); + } else { + GlobalConfig.logger.warn('EditorCodec: <resource> missing "basename" attribute; ignoring.'); + } + break; + }
218-221: Validate stylesheet name instead of using non-null assertionNon-null assertion doesn’t prevent runtime null. Guard and log to avoid invalid link element.
- case 'stylesheet': { - addLinkToHead('stylesheet', tmp.getAttribute('name')!); - break; - } + case 'stylesheet': { + const name = tmp.getAttribute('name'); + if (name) { + addLinkToHead('stylesheet', name); + } else { + GlobalConfig.logger.warn('EditorCodec: <stylesheet> missing "name" attribute; ignoring.'); + } + break; + }packages/core/src/view/shape/stencil/StencilShape.ts (2)
368-443: Rounded path parsing: verify arcSize units and consider supporting inside path
- Rounded mode only accepts move/line; any forces fallback. Consider supporting to allow closed rounded subpaths without duplicating the first point.
- arcSize is passed raw to addPoints; confirm expected units/range (pixels vs percentage vs [0,1]). If it expects a normalized factor, normalize here for consistency.
642-660: Make dashpattern parsing robust (trim, commas, invalid numbers)Support comma or multi-space separators and filter non-numeric tokens to avoid NaNs.
- if (value != null) { - const tmp = value.split(' '); - const pat = []; - - for (let i = 0; i < tmp.length; i += 1) { - if (tmp[i].length > 0) { - pat.push(Number(tmp[i]) * minScale); - } - } - - value = pat.join(' '); - canvas.setDashPattern(value); - } + if (value != null) { + const tokens = value.trim().split(/[,\s]+/); + const pat = tokens + .map((t) => Number(t) * minScale) + .filter((n) => Number.isFinite(n)); + canvas.setDashPattern(pat.join(' ')); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
eslint.config.mjs(2 hunks)package.json(1 hunks)packages/core/src/editor/Editor.ts(3 hunks)packages/core/src/gui/MaxWindow.ts(3 hunks)packages/core/src/i18n/Translations.ts(1 hunks)packages/core/src/serialization/ObjectCodec.ts(1 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts(1 hunks)packages/core/src/serialization/codecs/mxGraph/utils.ts(1 hunks)packages/core/src/util/gestureUtils.ts(1 hunks)packages/core/src/util/mathUtils.ts(3 hunks)packages/core/src/util/styleUtils.ts(1 hunks)packages/core/src/util/xmlUtils.ts(1 hunks)packages/core/src/view/GraphDataModel.ts(1 hunks)packages/core/src/view/GraphView.ts(1 hunks)packages/core/src/view/canvas/SvgCanvas2D.ts(3 hunks)packages/core/src/view/canvas/XmlCanvas2D.ts(1 hunks)packages/core/src/view/cell/CellOverlay.ts(1 hunks)packages/core/src/view/cell/CellPath.ts(2 hunks)packages/core/src/view/cell/CellRenderer.ts(1 hunks)packages/core/src/view/layout/hierarchical/CoordinateAssignment.ts(1 hunks)packages/core/src/view/mixins/CellsMixin.ts(2 hunks)packages/core/src/view/mixins/ConnectionsMixin.ts(2 hunks)packages/core/src/view/mixins/EventsMixin.ts(1 hunks)packages/core/src/view/mixins/SwimlaneMixin.ts(2 hunks)packages/core/src/view/other/DragSource.ts(1 hunks)packages/core/src/view/shape/Shape.ts(1 hunks)packages/core/src/view/shape/node/TextShape.ts(1 hunks)packages/core/src/view/shape/stencil/StencilShape.ts(1 hunks)packages/core/src/view/style/edge/Loop.ts(1 hunks)packages/core/src/view/style/edge/Manhattan.ts(1 hunks)packages/core/src/view/style/perimeter/EllipsePerimeter.ts(1 hunks)packages/core/src/view/style/perimeter/TrianglePerimeter.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (6)
packages/core/src/internal/i18n-utils.ts (1)
translate(29-35)packages/core/src/util/domUtils.ts (2)
getTextContent(88-90)getChildNodes(271-286)packages/core/src/internal/utils.ts (1)
doEval(24-27)packages/core/src/util/config.ts (1)
GlobalConfig(35-73)packages/core/src/util/styleUtils.ts (1)
convertPoint(309-317)packages/core/src/util/EventUtils.ts (2)
getClientX(37-39)getClientY(44-46)
packages/core/src/view/mixins/CellsMixin.ts (4)
packages/html/stories/Boundary.stories.js (1)
state(162-162)packages/html/stories/Clipboard.stories.js (2)
state(145-145)geo(148-148)packages/html/stories/Labels.stories.js (1)
geo(74-74)packages/html/stories/Scrollbars.stories.js (1)
s(130-130)
packages/core/src/serialization/codecs/editor/EditorCodec.ts (4)
packages/core/src/gui/MaxWindow.ts (1)
MaxWindow(178-957)packages/core/src/internal/i18n-utils.ts (1)
translate(29-35)packages/core/src/util/config.ts (1)
GlobalConfig(35-73)packages/core/src/util/domUtils.ts (1)
addLinkToHead(341-363)
packages/core/src/view/shape/stencil/StencilShape.ts (6)
packages/core/src/view/canvas/XmlCanvas2D.ts (1)
close(844-846)packages/core/src/view/shape/Shape.ts (1)
paint(569-660)packages/core/src/view/shape/node/TextShape.ts (1)
paint(228-302)packages/core/src/util/Constants.ts (1)
RECTANGLE_ROUNDING_FACTOR(331-331)packages/core/src/types.ts (3)
AlignValue(934-934)VAlignValue(936-936)ColorValue(909-909)packages/core/src/view/shape/stencil/StencilShapeRegistry.ts (1)
StencilShapeRegistry(46-46)
packages/core/src/editor/Editor.ts (4)
packages/core/src/view/canvas/XmlCanvas2D.ts (1)
scale(128-132)packages/html/stories/OrgChart.stories.js (2)
scale(256-256)panningHandler(84-84)packages/html/stories/SecondLabel.stories.js (1)
scale(142-142)packages/core/src/view/canvas/AbstractCanvas2D.ts (1)
scale(256-260)
packages/core/src/util/xmlUtils.ts (3)
packages/core/src/util/Constants.ts (1)
NODE_TYPE(413-426)packages/core/src/util/domUtils.ts (1)
getTextContent(88-90)packages/core/src/util/StringUtils.ts (2)
trim(62-63)htmlEntities(138-151)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-2022)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (29)
packages/core/src/view/style/perimeter/TrianglePerimeter.ts (1)
102-122: LGTM! Well-structured switch with proper default case.The switch statement correctly handles all four directions with the default case covering 'east'. The explicit breaks and default case demonstrate good defensive programming.
packages/core/src/view/mixins/SwimlaneMixin.ts (2)
148-165: LGTM! Switch statement correctly refactors direction mapping.The switch statement appropriately handles the directional cases. The "No default" comment clarifies that 'east' (and any other value) intentionally leaves
nunchanged, preserving the original if-else chain behavior.
188-207: LGTM! Switch statement with appropriate default case.The switch statement correctly maps each direction to its corresponding dimension property. The default case appropriately handles the 'east' direction and provides a sensible fallback.
packages/core/src/util/xmlUtils.ts (1)
150-234: LGTM: Switch statement refactoring improves readability.The refactoring from if-else chain to switch statement is well-executed. Each case has an explicit
breakstatement preventing fall-through, and the logic flow is preserved for all node types (DOCUMENT, DOCUMENT_FRAGMENT, COMMENT, TEXT, CDATA, and elements).packages/core/src/util/gestureUtils.ts (1)
140-141: LGTM! Numeric parsing updated to use Number namespace.The replacement of
parseIntwithNumber.parseIntfor parsing width and height is correct and aligns with modern JavaScript best practices.packages/core/src/view/style/edge/Manhattan.ts (1)
115-115: LGTM! Numeric parsing updated to use Number namespace.The replacement of
parseIntwithNumber.parseIntfor coordinate parsing is correct. The explicit radix parameter (10) is maintained, which is good practice.packages/core/src/view/mixins/EventsMixin.ts (1)
711-728: LGTM! Control flow refactored to switch statement.The refactor from if/else ladder to a switch statement improves readability while maintaining the same behavior. Each case has an explicit break, and the comment indicates that no default case is intentional.
packages/core/src/view/GraphDataModel.ts (1)
489-489: LGTM! Numeric parsing updated to use Number namespace.The replacement of
parseIntwithNumber.parseIntfor updatingnextIdis correct and maintains the existing logic for preventing ID reuse.packages/core/src/serialization/ObjectCodec.ts (1)
585-589: LGTM! Numeric parsing and validation updated to use Number namespace.The replacements of
parseFloat,isNaN, andisFinitewith theirNumber.*equivalents are correct. The error handling logic (defaulting to 0 for invalid values) is preserved.packages/core/src/view/shape/node/TextShape.ts (1)
214-222: LGTM! Numeric validation updated to use Number namespace.The replacements of
isNaNandisFinitewithNumber.isNaNandNumber.isFinitefor bounds validation are correct and maintain the existing validation logic.packages/core/src/i18n/Translations.ts (1)
354-354: LGTM! Numeric parsing updated to use Number namespace.The replacement of
parseIntwithNumber.parseIntfor parsing placeholder indices is correct and maintains the existing logic for converting 1-based placeholder indices to 0-based array indices.packages/core/src/serialization/codecs/mxGraph/utils.ts (1)
51-55: LGTM! Numeric parsing and validation updated to use Number namespace.The replacements of
parseFloat,isNaN, andisFinitewith theirNumber.*equivalents are correct. The error handling logic (defaulting to 0 for invalid values) is preserved.package.json (1)
29-29: LGTM!The addition of
eslint-plugin-unicornaligns with the PR objective to enable additional linting rules for code quality improvements.packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
173-173: LGTM!The replacement of
parseFloatwithNumber.parseFloatis correct and aligns with the ESLint unicorn plugin'sprefer-number-propertiesrule.packages/core/src/view/other/DragSource.ts (1)
600-601: LGTM!The replacement of
parseIntwithNumber.parseIntfor parsing width and height from element styles is correct.packages/core/src/view/cell/CellPath.ts (2)
93-93: LGTM!The replacement of
parseIntwithNumber.parseIntfor parsing cell path tokens is correct.
112-113: LGTM!The replacement of
parseIntwithNumber.parseIntfor comparing path components is correct.packages/core/src/view/cell/CellOverlay.ts (1)
184-224: LGTM!The conversion from if-else chains to switch statements for alignment handling improves readability. The logic is preserved correctly with appropriate break statements and error handling for invalid values.
packages/core/src/view/canvas/XmlCanvas2D.ts (1)
88-94: LGTM!The replacement of
parseFloatwithNumber.parseFloatin theformatmethod is correct. The double parsing for strings (parse → toFixed → parse) ensures consistent 2-decimal precision.packages/core/src/view/cell/CellRenderer.ts (1)
888-890: LGTM!The replacement of
parseFloatwithNumber.parseFloatfor comparing directional spacing values is correct.packages/core/src/view/GraphView.ts (1)
1020-1064: LGTM! Switch statement improves clarity.The refactor from if/else to switch preserves the original logic while making the control flow more explicit. Each case correctly handles the horizontal label positioning with appropriate breaks.
packages/core/src/view/mixins/CellsMixin.ts (2)
377-414: LGTM! Switch statements improve readability.The refactor from nested if/else to switch statements for alignment calculations is well-structured. The first switch determines the initial alignment coordinate, and the second switch updates it based on the alignment direction. Both correctly preserve the original logic with explicit breaks.
435-464: LGTM! Alignment offset application is correct.The switch statement cleanly handles the different alignment cases (center, right, top, middle, bottom) with appropriate geometry offset calculations. The default case correctly handles the left alignment by adjusting geo.x.
eslint.config.mjs (1)
21-21: LGTM! Incremental plugin adoption is prudent.The addition of eslint-plugin-unicorn with two targeted rules (
prefer-number-propertiesandprefer-switch) is a good incremental approach. This limits the scope of changes while still improving code consistency and modern JavaScript practices.Also applies to: 97-108
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
141-295: LGTM! Switch statement improves structure.The refactor from if/else to switch for nodeName checking is well-executed. The switch cleanly handles the four node types (separator, br, hr, add) with explicit breaks. The complex nested logic within the 'add' case correctly preserves the original behavior for handling actions, modes, templates, and combo boxes.
packages/core/src/editor/Editor.ts (3)
1231-1231: LGTM! Number.parseFloat and Number.isNaN are more precise.Replacing the global
parseFloatwithNumber.parseFloatandisNaNwithNumber.isNaNimproves code clarity and precision.Number.isNaNspecifically avoids type coercion, making the NaN check more accurate.Also applies to: 1233-1233
1127-1130: LGTM! Consistent use of Number.parseFloat.The geometry coordinate parsing correctly uses
Number.parseFloatfor x, y, width, and height values, maintaining consistency with the modern JavaScript number parsing approach.
2386-2403: LGTM! Switch statement improves mode handling.The refactor from if/else to switch for setMode logic is clean and maintainable. Each mode ('select', 'connect', 'pan') correctly configures the panning handler and graph connectability with explicit breaks.
packages/core/src/view/shape/stencil/StencilShape.ts (1)
632-634: Confirm strokewidth scaling semanticswidth × (fixed ? 1 : minScale) implies the value in is unscaled when fixed=1; scaled otherwise. Please confirm this matches stencil expectations and the global strokewidth logic in drawShape (inherit vs numeric), to avoid double- or under-scaling.
| const as = <string>tmp.getAttribute('as'); | ||
| const elt = tmp.getAttribute('element'); | ||
| const style = tmp.getAttribute('style'); | ||
| let element = null; | ||
|
|
||
| if (elt != null) { | ||
| element = document.getElementById(elt); | ||
| if (elt != null) { | ||
| element = document.getElementById(elt); | ||
|
|
||
| if (element != null && style != null) { | ||
| element.style.cssText += `;${style}`; | ||
| if (element != null && style != null) { | ||
| element.style.cssText += `;${style}`; | ||
| } | ||
| } else { | ||
| const x = Number.parseInt(<string>tmp.getAttribute('x')); | ||
| const y = Number.parseInt(<string>tmp.getAttribute('y')); | ||
| const width = tmp.getAttribute('width') || null; | ||
| const height = tmp.getAttribute('height') || null; | ||
|
|
||
| // Creates a new window around the element | ||
| element = document.createElement('div'); | ||
| if (style != null) { | ||
| element.style.cssText = style; | ||
| } | ||
|
|
||
| const wnd = new MaxWindow( | ||
| translate(as) || as, | ||
| element, | ||
| x, | ||
| y, | ||
| width ? Number.parseInt(width) : null, | ||
| height ? Number.parseInt(height) : null, | ||
| false, | ||
| true | ||
| ); | ||
| wnd.setVisible(true); | ||
| } |
There was a problem hiding this comment.
Fix NaN coordinates and null container handling; add radix and typing
- Number.parseInt on possibly-null x/y yields NaN; passed into MaxWindow.
- If element id is provided but not found, element stays null and is later used in setGraph/Toolbar/... calls.
- Add radix 10 and type the element.
Apply:
- const as = <string>tmp.getAttribute('as');
- const elt = tmp.getAttribute('element');
- const style = tmp.getAttribute('style');
- let element = null;
+ const as = <string>tmp.getAttribute('as');
+ const eltId = tmp.getAttribute('element');
+ const style = tmp.getAttribute('style');
+ let element: HTMLElement | null = null;
- if (elt != null) {
- element = document.getElementById(elt);
-
- if (element != null && style != null) {
- element.style.cssText += `;${style}`;
- }
- } else {
- const x = Number.parseInt(<string>tmp.getAttribute('x'));
- const y = Number.parseInt(<string>tmp.getAttribute('y'));
- const width = tmp.getAttribute('width') || null;
- const height = tmp.getAttribute('height') || null;
-
- // Creates a new window around the element
- element = document.createElement('div');
- if (style != null) {
- element.style.cssText = style;
- }
-
- const wnd = new MaxWindow(
- translate(as) || as,
- element,
- x,
- y,
- width ? Number.parseInt(width) : null,
- height ? Number.parseInt(height) : null,
- false,
- true
- );
- wnd.setVisible(true);
- }
+ if (eltId != null) {
+ element = document.getElementById(eltId);
+ if (element != null && style != null) {
+ element.style.cssText += (element.style.cssText ? ';' : '') + style;
+ }
+ }
+ // If no element provided or not found, create a window and container
+ if (element == null) {
+ const xAttr = tmp.getAttribute('x');
+ const yAttr = tmp.getAttribute('y');
+ let x = xAttr != null ? Number.parseInt(xAttr, 10) : 0;
+ let y = yAttr != null ? Number.parseInt(yAttr, 10) : 0;
+ if (Number.isNaN(x)) x = 0;
+ if (Number.isNaN(y)) y = 0;
+ const widthAttr = tmp.getAttribute('width');
+ const heightAttr = tmp.getAttribute('height');
+ const width = widthAttr != null ? Number.parseInt(widthAttr, 10) : null;
+ const height = heightAttr != null ? Number.parseInt(heightAttr, 10) : null;
+
+ // Creates a new window around the element
+ element = document.createElement('div');
+ if (style != null) {
+ element.style.cssText = style;
+ }
+
+ const wnd = new MaxWindow(
+ translate(as) || as,
+ element,
+ x,
+ y,
+ width,
+ height,
+ false,
+ true
+ );
+ wnd.setVisible(true);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const as = <string>tmp.getAttribute('as'); | |
| const elt = tmp.getAttribute('element'); | |
| const style = tmp.getAttribute('style'); | |
| let element = null; | |
| if (elt != null) { | |
| element = document.getElementById(elt); | |
| if (elt != null) { | |
| element = document.getElementById(elt); | |
| if (element != null && style != null) { | |
| element.style.cssText += `;${style}`; | |
| if (element != null && style != null) { | |
| element.style.cssText += `;${style}`; | |
| } | |
| } else { | |
| const x = Number.parseInt(<string>tmp.getAttribute('x')); | |
| const y = Number.parseInt(<string>tmp.getAttribute('y')); | |
| const width = tmp.getAttribute('width') || null; | |
| const height = tmp.getAttribute('height') || null; | |
| // Creates a new window around the element | |
| element = document.createElement('div'); | |
| if (style != null) { | |
| element.style.cssText = style; | |
| } | |
| const wnd = new MaxWindow( | |
| translate(as) || as, | |
| element, | |
| x, | |
| y, | |
| width ? Number.parseInt(width) : null, | |
| height ? Number.parseInt(height) : null, | |
| false, | |
| true | |
| ); | |
| wnd.setVisible(true); | |
| } | |
| const as = <string>tmp.getAttribute('as'); | |
| const eltId = tmp.getAttribute('element'); | |
| const style = tmp.getAttribute('style'); | |
| let element: HTMLElement | null = null; | |
| if (eltId != null) { | |
| element = document.getElementById(eltId); | |
| if (element != null && style != null) { | |
| element.style.cssText += (element.style.cssText ? ';' : '') + style; | |
| } | |
| } | |
| // If no element provided or not found, create a window and container | |
| if (element == null) { | |
| const xAttr = tmp.getAttribute('x'); | |
| const yAttr = tmp.getAttribute('y'); | |
| let x = xAttr != null ? Number.parseInt(xAttr, 10) : 0; | |
| let y = yAttr != null ? Number.parseInt(yAttr, 10) : 0; | |
| if (Number.isNaN(x)) x = 0; | |
| if (Number.isNaN(y)) y = 0; | |
| const widthAttr = tmp.getAttribute('width'); | |
| const heightAttr = tmp.getAttribute('height'); | |
| const width = widthAttr != null ? Number.parseInt(widthAttr, 10) : null; | |
| const height = heightAttr != null ? Number.parseInt(heightAttr, 10) : null; | |
| // Creates a new window around the element | |
| element = document.createElement('div'); | |
| if (style != null) { | |
| element.style.cssText = style; | |
| } | |
| const wnd = new MaxWindow( | |
| translate(as) || as, | |
| element, | |
| x, | |
| y, | |
| width, | |
| height, | |
| false, | |
| true | |
| ); | |
| wnd.setVisible(true); | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/serialization/codecs/editor/EditorCodec.ts around lines 152
to 186, Number.parseInt is called without a radix on possibly-null x/y leading
to NaN, element is untyped and can remain null if an id is provided but not
found, and that null is later used; change parseInt calls to include radix 10
and parse into numbers with a safe default (e.g., 0 or null-consistent values)
when parseInt yields NaN, type the element variable as HTMLElement | null, and
when an elt id is provided but document.getElementById returns null, create a
fallback container element (or throw/handle explicitly) before applying styles
or passing into MaxWindow so downstream code never receives a null container.
Ensure width/height parsing also uses radix 10 and preserves expected null
semantics.
| case NODE_TYPE.CDATA: { | ||
| const value = getTextContent(<Text>(<unknown>node)); | ||
|
|
||
| if (value.length > 0) { | ||
| result.push(`${indent}<![CDATA[${value}]]${newline}`); | ||
| } | ||
|
|
||
| break; | ||
| } |
There was a problem hiding this comment.
Fix CDATA section syntax.
Line 198 is missing the closing > bracket for the CDATA section. The correct XML syntax for CDATA is <![CDATA[...]]>, but the current code produces <![CDATA[...]].
Apply this diff to fix the CDATA syntax:
case NODE_TYPE.CDATA: {
const value = getTextContent(<Text>(<unknown>node));
if (value.length > 0) {
- result.push(`${indent}<![CDATA[${value}]]${newline}`);
+ result.push(`${indent}<![CDATA[${value}]]>${newline}`);
}
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case NODE_TYPE.CDATA: { | |
| const value = getTextContent(<Text>(<unknown>node)); | |
| if (value.length > 0) { | |
| result.push(`${indent}<![CDATA[${value}]]${newline}`); | |
| } | |
| break; | |
| } | |
| case NODE_TYPE.CDATA: { | |
| const value = getTextContent(<Text>(<unknown>node)); | |
| if (value.length > 0) { | |
| result.push(`${indent}<![CDATA[${value}]]>${newline}`); | |
| } | |
| break; | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/util/xmlUtils.ts around lines 194 to 202, the CDATA emitter
is missing the closing '>' in the generated section (`<![CDATA[...]]`); update
the pushed string to append the final '>' so it emits `<![CDATA[${value}]]>`
(preserve indent and newline handling).
| case 'arc': { | ||
| canvas.arcTo( | ||
| Number(node.getAttribute('rx')) * sx, | ||
| Number(node.getAttribute('ry')) * sy, | ||
| Number(node.getAttribute('x-axis-rotation')), | ||
| toBoolean(node.getAttribute('large-arc-flag')), | ||
| toBoolean(node.getAttribute('sweep-flag')), | ||
| x0 + Number(node.getAttribute('x')) * sx, | ||
| y0 + Number(node.getAttribute('y')) * sy | ||
| ); | ||
|
|
||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Arc flags parsing may default to true on missing attributes
toBoolean(value !== '0') returns true for null/undefined, making large-arc-flag/sweep-flag true when attrs are absent. That changes path semantics.
- Parse explicitly: only '1' (or 'true') should be true; everything else false.
Suggested helper (outside this hunk):
// Replace existing helper
const toBoolean = (value: string | null) => value === '1' || value === 'true';🏁 Script executed:
#!/bin/bash
rg -nP "function toBoolean" -g '*.ts' -C2
rg -nP "const toBoolean" -g '*.ts' -C2Length of output: 487
Strict boolean parsing for arc flags
Current toBoolean (StencilShape.ts:59) treats null/missing as true (value !== '0'). Update it to only return true for explicit “1” (or “true”) to preserve SVG path semantics:
// packages/core/src/view/shape/stencil/StencilShape.ts:59
const toBoolean = (value: string | null) => value === '1' || value === 'true';🤖 Prompt for AI Agents
In packages/core/src/view/shape/stencil/StencilShape.ts around lines 486 to 498,
the arc flag boolean conversion currently treats null/missing values as true
which breaks SVG semantics; change the toBoolean helper (defined near line 59)
to return true only for explicit '1' or 'true' and false otherwise, then use
that updated helper when passing large-arc-flag and sweep-flag into canvas.arcTo
so missing attributes are treated as false.
| case 'alpha': { | ||
| canvas.setAlpha(Number(node.getAttribute('alpha'))); | ||
|
|
||
| break; | ||
| } | ||
| case 'fillalpha': { | ||
| canvas.setAlpha(Number(node.getAttribute('alpha'))); | ||
|
|
||
| break; | ||
| } | ||
| case 'strokealpha': { | ||
| canvas.setAlpha(Number(node.getAttribute('alpha'))); | ||
|
|
||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
fillalpha/strokealpha call setAlpha; likely should be channel-specific
Using setAlpha for fillalpha and strokealpha applies the same alpha globally. Prefer setFillAlpha and setStrokeAlpha if available.
- case 'fillalpha': {
- canvas.setAlpha(Number(node.getAttribute('alpha')));
+ case 'fillalpha': {
+ canvas.setFillAlpha(Number(node.getAttribute('alpha')));
break;
}
- case 'strokealpha': {
- canvas.setAlpha(Number(node.getAttribute('alpha')));
+ case 'strokealpha': {
+ canvas.setStrokeAlpha(Number(node.getAttribute('alpha')));
break;
}If the canvas API lacks these, consider adding them to AbstractCanvas2D and implementations (XmlCanvas2D, etc.).
🏁 Script executed:
#!/bin/bash
rg -n 'class AbstractCanvas2D' -C3 packages/core
rg -n 'interface AbstractCanvas2D' -C3 packages/core
rg -n 'setAlpha' -C3 packages/core
rg -n 'setFillAlpha' -C3 packages/core
rg -n 'setStrokeAlpha' -C3 packages/core
rg -n 'class XmlCanvas2D' -C3 packages/coreLength of output: 10719
Use channel-specific alpha setters for fillalpha and strokealpha
Replace the two cases in StencilShape.ts:
case 'fillalpha': {
- canvas.setAlpha(Number(node.getAttribute('alpha')));
+ canvas.setFillAlpha(Number(node.getAttribute('alpha')));
break;
}
case 'strokealpha': {
- canvas.setAlpha(Number(node.getAttribute('alpha')));
+ canvas.setStrokeAlpha(Number(node.getAttribute('alpha')));
break;
}🤖 Prompt for AI Agents
In packages/core/src/view/shape/stencil/StencilShape.ts around lines 686 to 699,
the cases for 'fillalpha' and 'strokealpha' incorrectly call
canvas.setAlpha(Number(node.getAttribute('alpha'))); instead of using
channel-specific setters; change the 'fillalpha' case to call
canvas.setFillAlpha(Number(node.getAttribute('fillalpha') ??
node.getAttribute('alpha'))); and change the 'strokealpha' case to call
canvas.setStrokeAlpha(Number(node.getAttribute('strokealpha') ??
node.getAttribute('alpha'))); ensuring you convert to Number and fall back to
the generic 'alpha' attribute if needed.
|
@coderabbitai thanks for your feedback, the problem you mention are not related to this PR, they already existed. |
|
Tip For best results, initiate chat on the files or code changes. @tbouffard, thank you for clarifying! I completely understand wanting to keep this PR focused on the linting changes. That's a good approach to maintain a clean, reviewable scope. If you'd like, I can help create a GitHub issue to track the deferred feedback for the follow-up PR. Just let me know, and I'll be happy to open one with the relevant details. |
Start using the plugin but only activate a few rules on the core package to limit the number of changes.
This fixes some errors detected by Sonar.
Summary by CodeRabbit