chore(typescript): always use explicit override #921
Conversation
Easily detect when super methods are removed in the "core" package. Already in place in other packages.
WalkthroughAdds explicit TypeScript override modifiers across many classes (codecs, canvas, handlers, layouts, shapes, plugins, etc.) and removes Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/core/src/view/canvas/XmlCanvas2D.ts (2)
357-373: setDashed ignores fixDash changes when compressed=true.When dashed stays the same but fixDash changes, early return prevents updating state/XML. Include fixDash in the equality check.
Apply:
override setDashed(value: boolean, fixDash: boolean): void { if (this.compressed) { - if (this.state.dashed === value) { + if (this.state.dashed === value && this.state.fixDash === fixDash) { return; } super.setDashed(value, fixDash); }
349-356: Fix JSDoc param name for setDashed.Second @param should be fixDash, not value.
/** * Enables or disables dashed lines. * * @param value Boolean that specifies if dashed lines should be enabled. - * @param value Boolean that specifies if the stroke width should be ignored + * @param fixDash Boolean that specifies if the stroke width should be ignored * for the dash pattern. * @default false */packages/core/src/serialization/codecs/StylesheetCodec.ts (2)
145-147: Bug: Map accessed with bracket notation; use get()
stylesis aMap, soobj.styles[extend]is alwaysundefined. This breaks style inheritance in decode.Fix:
- let style = extend ? clone(obj.styles[extend]) : null; + let style = extend ? clone(obj.styles.get(extend)) : null;
177-179: Don’t drop falsy values (e.g., 0) when decoding
if (value)skips valid values like0. Use a null/undefined check instead.Fix:
- if (value) { + if (value != null) { style[key] = value; }packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
233-243: Fix incorrect callback reference in selectMode handler (TypeError at runtime)The change handler calls funct(), which is undefined here. It should call create().
Apply this diff:
- into.toolbar!.selectMode(img, (evt: MouseEvent) => { + into.toolbar!.selectMode(img, (evt: MouseEvent) => { const pt = convertPoint( editor.graph.container, getClientX(evt), getClientY(evt) ); - - return editor.addVertex(null, funct(), pt.x, pt.y); + return editor.addVertex(null, create(), pt.x, pt.y); });
🧹 Nitpick comments (55)
packages/core/src/view/shape/node/RhombusShape.ts (1)
49-55: Optional: align JSDoc with actual typesJSDoc still references mxAbstractCanvas2D while the TS type is AbstractCanvas2D. Consider updating or dropping the JSDoc type (TS already provides types).
Apply this minimal doc tweak:
- * @param {mxAbstractCanvas2D} c + * @param {AbstractCanvas2D} cpackages/core/src/view/shape/node/RectangleShape.ts (4)
44-57: Good use of explicit override; add an explicit return type for clarityConsider annotating the return type to make the API surface clearer and consistent with other methods.
Apply:
- override isHtmlAllowed() { + override isHtmlAllowed(): boolean {
62-68: Add an explicit ': void' return type to paintBackgroundKeeps signatures explicit and consistent with paintForeground.
Apply:
- override paintBackground( + override paintBackground( c: AbstractCanvas2D, x: number, y: number, w: number, h: number - ) { + ): void {
94-96: Add an explicit ': boolean' return type to isRoundableAligns with explicit typing elsewhere.
Apply:
- override isRoundable(c: AbstractCanvas2D, x: number, y: number, w: number, h: number) { + override isRoundable( + c: AbstractCanvas2D, + x: number, + y: number, + w: number, + h: number + ): boolean {
101-107: Doc nit: comment says “background” for paintForegroundThe JSDoc above this method refers to “background” while this is the foreground pass. Suggest updating the comment for accuracy.
Update the nearby JSDoc to:
/** * Generic foreground painting implementation. */packages/core/src/view/shape/edge/LineShape.ts (1)
55-61: Optional: add explicit return type for clarityAdding an explicit void return type can improve readability and consistency.
Apply this diff:
- ) { + ): void {packages/core/src/view/shape/node/HexagonShape.ts (1)
38-38: Optional: add an explicit return type.Add
: voidfor clarity and to guard against inference drift.Apply this diff:
- override redrawPath(c: AbstractCanvas2D, x: number, y: number, w: number, h: number) { + override redrawPath(c: AbstractCanvas2D, x: number, y: number, w: number, h: number): void {packages/core/src/view/shape/edge/PolylineShape.ts (4)
51-53: Add explicit return type for clarity (number).Keeps signature explicit and aligned with the base type.
Apply this diff:
- override getRotation() { + override getRotation(): number {
58-60: Add explicit return type for clarity (number).Keeps signature explicit and aligned with the base type.
Apply this diff:
- override getShapeRotation() { + override getShapeRotation(): number {
65-67: Add explicit return type for clarity (boolean).Keeps signature explicit and aligned with the base type.
Apply this diff:
- override isPaintBoundsInverted() { + override isPaintBoundsInverted(): boolean {
72-82: Guarantee canvas state restoration; use try/finally and add void return type.Prevents leaving pointerEventsValue in a bad state if painting throws; also makes return type explicit.
Apply this diff:
- override paintEdgeShape(c: AbstractCanvas2D, pts: Point[]) { - const prev = c.pointerEventsValue; - c.pointerEventsValue = 'stroke'; - - if (!this.style?.curved) { - this.paintLine(c, pts, this.isRounded); - } else { - this.paintCurvedLine(c, pts); - } - c.pointerEventsValue = prev; - } + override paintEdgeShape(c: AbstractCanvas2D, pts: Point[]): void { + const prev = c.pointerEventsValue; + c.pointerEventsValue = 'stroke'; + try { + if (!this.style?.curved) { + this.paintLine(c, pts, this.isRounded); + } else { + this.paintCurvedLine(c, pts); + } + } finally { + c.pointerEventsValue = prev; + } + }packages/core/src/view/layout/EdgeLabelLayout.ts (4)
42-44: Unused constructor parameter 'radius'If not used, prefix to avoid unused warnings without breaking the public API.
Apply this diff:
- constructor(graph: AbstractGraph, radius: number) { + constructor(graph: AbstractGraph, _radius: number) { super(graph); }
51-51: Remove unused localmodelin executeAvoid dead code.
Apply this diff:
- const model = this.graph.getDataModel();
54-56: Type local arrays to avoidany[]Improves type safety when passing to
placeLabels.Apply this diff:
- const edges = []; - const vertices = []; + const edges: CellState[] = []; + const vertices: CellState[] = [];
100-103: Docstring mismatch foravoidClarify purpose of the method.
Apply this diff:
- /** - * Places the labels of the given edges. - */ + /** + * Adjusts the edge label offset to avoid overlap with the given vertex. + */packages/core/src/view/shape/node/CylinderShape.ts (2)
55-61: Add explicit return type for clarityNit: declare the return type to mirror the base signature and prevent accidental returns.
Apply this diff:
- ) { + ): void {
85-92: Add missing override on redrawPathThis likely overrides Shape.redrawPath; add the modifier to benefit from TS checks and align with the PR objective.
Apply this diff:
- redrawPath( + override redrawPath(packages/core/src/view/shape/node/ImageShape.ts (4)
54-56: Add explicit return type for clarity.Declare the return type to keep method signatures consistent and ease refactoring.
- override getSvgScreenOffset() { + override getSvgScreenOffset(): number {
68-78: Add explicit return type.This method returns nothing; make it explicit.
- override apply(state: CellState) { + override apply(state: CellState): void {
91-93: Add explicit return type.Keep boolean return explicit.
- override isRoundable(c: AbstractCanvas2D, x: number, y: number, w: number, h: number) { + override isRoundable(c: AbstractCanvas2D, x: number, y: number, w: number, h: number): boolean {
98-104: Add explicit return type.Paint methods are side-effect only; declare void.
- override paintVertexShape( + override paintVertexShape( c: AbstractCanvas2D, x: number, y: number, w: number, - h: number - ) { + h: number + ): void {packages/core/src/view/layout/CircleLayout.ts (1)
87-87: Add explicit: voidreturn type for consistency with otherexecuteoverrides.CompactTreeLayout’s
executealready uses: void. Aligning improves readability and consistency.Apply this diff:
- override execute(parent: Cell) { + override execute(parent: Cell): void {packages/core/src/view/layout/StackLayout.ts (1)
283-291: Nit: add an explicit type for fillValue (clarity, stricter typing).This keeps types precise in strict setups.
Apply:
- let fillValue = null; + let fillValue: number | null = null;packages/core/src/view/handler/ElbowEdgeHandler.ts (1)
176-178: Avoid non-null assertion on geometry (optional)Safer to handle potential null geometry without a non-null assertion.
Apply:
- } else if (g!.points != null && g!.points.length > 0) { + } else if (g?.points != null && g.points.length > 0) {packages/core/src/view/layout/PartitionLayout.ts (1)
80-112: Explicit override on moveCell: LGTMMatches GraphLayout contract; no behavior change. Noting the in-method TODO about orientation for a future follow-up.
If useful, I can draft a small PR to handle orientation in the index computation and open a tracking issue.
packages/core/src/view/shape/edge/ArrowShape.ts (2)
60-65: Overrides look correct; consider adding explicit return typeFor consistency with other shapes (e.g., ArrowConnectorShape), add an explicit void return type.
- override augmentBoundingBox(bbox: Rectangle) { + override augmentBoundingBox(bbox: Rectangle): void {
70-116: Overrides look correct; add return type and a small guard against degenerate geometry
- Add explicit void return type.
- Optional robustness: early-return if pts are degenerate to avoid dividing by zero (ArrowConnectorShape already guards for this case).
- override paintEdgeShape(c: AbstractCanvas2D, pts: Point[]) { + override paintEdgeShape(c: AbstractCanvas2D, pts: Point[]): void { @@ - const dx = pe.x - p0.x; - const dy = pe.y - p0.y; - const dist = Math.sqrt(dx * dx + dy * dy); + const dx = pe.x - p0.x; + const dy = pe.y - p0.y; + const dist = Math.sqrt(dx * dx + dy * dy); + if (!Number.isFinite(dist) || dist === 0) { + return; + }packages/core/src/view/shape/edge/ArrowConnectorShape.ts (2)
61-66: Property override LGTM; tweak JSDoc to avoid default-value confusionBase default is false; this shape intentionally overrides to true. Suggest updating the doc to reflect that.
/** * Allows to use the SVG bounding box in SVG. - * @defaultValue `false` for performance reasons. + * Overrides the base default (`false`) to `true` for this shape. */ - override useSvgBoundingBox = true; + override useSvgBoundingBox = true;
70-72: Method override LGTM; add explicit return type for consistencyAdd : boolean to match common style across the codebase.
- override isRoundable() { + override isRoundable(): boolean {packages/core/src/view/shape/node/CloudShape.ts (1)
42-42: Add explicit return type for clarity.Annotate the method as returning void to match base and improve readability.
Apply this diff:
- override redrawPath(c: AbstractCanvas2D, x: number, y: number, w: number, h: number) { + override redrawPath(c: AbstractCanvas2D, x: number, y: number, w: number, h: number): void {packages/core/src/view/shape/node/SwimlaneShape.ts (3)
77-116: Prefer strict equality and add explicit return type for clarity.Use === for boolean comparison and annotate the return type.
Apply:
- override getLabelBounds(rect: Rectangle) { + override getLabelBounds(rect: Rectangle): Rectangle { @@ - const realHorizontal = horizontal == !shapeVertical; + const realHorizontal = horizontal === !shapeVertical;
121-127: Add explicit return type to getGradientBounds for consistency.This improves API clarity without changing behavior.
- override getGradientBounds( + override getGradientBounds( c: AbstractCanvas2D, x: number, y: number, w: number, h: number - ) { + ): Rectangle {
159-165: Add explicit return type to paintVertexShape.Minor readability improvement; no behavior change.
- override paintVertexShape( + override paintVertexShape( c: AbstractCanvas2D, x: number, y: number, w: number, h: number - ) { + ): void {packages/core/src/view/shape/node/ActorShape.ts (1)
63-69: Optional: add explicit: voidreturn type to lock the public API.Prevents accidental return value changes and aligns with stricter API surfaces.
- ) { + ): void {packages/core/src/view/layout/RadialTreeLayout.ts (2)
126-130: Use vertex.getConnections() here for consistencyBoth graph.getConnections(cell, parent?) (ConnectionsMixin) and Cell#getConnections() exist; CompactTreeLayout already uses vertex.getConnections(). Since no parent is passed here, prefer vertex.getConnections().
- return ( - super.isVertexIgnored(vertex) || this.graph.getConnections(vertex).length === 0 - ); + return ( + super.isVertexIgnored(vertex) || vertex.getConnections().length === 0 + );
142-151: Prefer an optionalroot?: Cellparam and pass it through tosuperChange the signature to
root?: Celland callsuper.execute(parent, root)to avoid thenull→undefinedconversion (matches CompactTreeLayout).- override execute(parent: Cell, root: Cell | null = null): void { + override execute(parent: Cell, root?: Cell): void { @@ - super.execute(parent, root || undefined); + super.execute(parent, root);packages/core/src/view/shape/node/EllipseShape.ts (1)
42-48: Optional: declare the explicit return type.Improves readability and guards against accidental return changes.
- override paintVertexShape( + override paintVertexShape( c: AbstractCanvas2D, x: number, y: number, w: number, h: number - ) { + ): void {packages/core/src/view/geometry/Geometry.ts (1)
345-361: Prefer not narrowing the parameter type in overrides
equalsnarrows the parameter toGeometry | nullwhile the base method (inRectangle) accepts a broader type. To keep LSP and avoid subtle unsoundness at call sites, widen the parameter toRectangle | nulland guard forGeometry.- override equals(geom: Geometry | null) { + override equals(geom: Rectangle | null) { + if (!(geom instanceof Geometry)) return false; if (!geom) return false; return ( super.equals(geom) && this.relative === geom.relative && ((this.sourcePoint === null && geom.sourcePoint === null) || !!this.sourcePoint?.equals(geom.sourcePoint)) && ((this.targetPoint === null && geom.targetPoint === null) || !!this.targetPoint?.equals(geom.targetPoint)) && equalPoints(this.points, geom.points) && ((this.alternateBounds === null && geom.alternateBounds === null) || !!this.alternateBounds?.equals(geom.alternateBounds)) && ((this.offset === null && geom.offset === null) || !!this.offset?.equals(geom.offset)) ); }packages/core/src/view/geometry/Rectangle.ts (1)
166-175: Align Rectangle.equals signature with Point.equals and add a runtime guardPoint.equals is declared
equals(p: Point | null); change Rectangle.equals to acceptPoint | nulland guard for Rectangle before comparing.File: packages/core/src/view/geometry/Rectangle.ts
- override equals(rect: Rectangle | null) { + override equals(rect: Point | null) { + if (!(rect instanceof Rectangle)) return false; if (!rect) return false; return ( rect.x === this.x && rect.y === this.y && rect.width === this.width && rect.height === this.height ); }packages/core/src/view/layout/ParallelEdgeLayout.ts (1)
111-116: Optional: treat empty cells array distinctly from nullif (cells) treats [] as falsy and falls back to scanning all children. Consider checking nullish explicitly.
Apply:
- if (cells) { + if (cells != null) { for (const cell of cells) { addCell(cell); } } else {packages/core/src/view/handler/EdgeHandler.ts (1)
2323-2340: Explicit override on isValidState: OK. Add explicit boolean return type.Apply:
- override isValidState = (state: CellState) => { + override isValidState = (state: CellState): boolean => {packages/core/src/view/shape/node/LabelShape.ts (1)
113-115: Doc/logic mismatch in isHtmlAllowed (likely inverted check for indicator shape)Comment says “no indicator shape,” but code allows HTML only when an indicator shape exists. Consider flipping the condition. Follow‑up outside this chore PR.
Apply:
- return super.isHtmlAllowed() && this.indicatorColor === NONE && !!this.indicatorShape; + return super.isHtmlAllowed() && this.indicatorColor === NONE && !this.indicatorShape;packages/core/src/view/canvas/XmlCanvas2D.ts (2)
164-173: Avoid creating unused element.Only create the element when actually needed.
- const elem = this.createElement('rotate'); - if (theta !== 0 || flipH || flipV) { + if (theta !== 0 || flipH || flipV) { + const elem = this.createElement('rotate'); elem.setAttribute('theta', String(this.format(theta))); elem.setAttribute('flipH', flipH ? '1' : '0'); elem.setAttribute('flipV', flipV ? '1' : '0'); elem.setAttribute('cx', String(this.format(cx))); elem.setAttribute('cy', String(this.format(cy))); this.root.appendChild(elem); }
88-94: Addoverrideto the remaining XmlCanvas2D canvas methodsAbstractCanvas2D declares/implements rect, roundrect, ellipse, image, text, stroke, fill, fillAndStroke, end and updateText — mark the corresponding implementations in packages/core/src/view/canvas/XmlCanvas2D.ts with
overridefor consistency and stronger compiler checks.packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
130-196: Optional: consider marking getStringValue as override (if it exists on ObjectCodec)If ObjectCodec defines
getStringValue, addoverridefor consistency with this PR’s goal. If not, ignore.packages/core/src/view/layout/datatypes/GraphHierarchyNode.ts (1)
151-180: Clean up redundant null-check and harden against non-array hashCodeRemove the unreachable duplicate null-check and guard for array shape to avoid potential runtime errors.
- if ( - otherNode != null && - this.hashCode != null && - otherNode.hashCode != null && - this.hashCode.length < otherNode.hashCode.length - ) { + if ( + otherNode != null && + Array.isArray(this.hashCode) && + Array.isArray(otherNode.hashCode) && + this.hashCode.length < otherNode.hashCode.length + ) { if (this.hashCode === otherNode.hashCode) { return true; } - - if (this.hashCode == null || this.hashCode == null) { - return false; - }packages/core/src/view/layout/datatypes/GraphHierarchyEdge.ts (1)
105-122: Add explicit return type for clarityAnnotate the return type to mirror the base shape and improve readability.
- override getPreviousLayerConnectedCells(layer: number) { + override getPreviousLayerConnectedCells(layer: number): GraphAbstractHierarchyCell[] {packages/core/src/view/layout/CompositeLayout.ts (2)
69-75: override on moveCell is appropriate; consider minor call simplification.You can call directly without apply:
- this.master.moveCell(cell, x, y)
- this.layouts[0].moveCell(cell, x, y)
Also ensure layouts has at least one element before accessing [0].
80-86: execute override LGTM; loop can be simplified.Use for‑of and direct calls for readability:
for (const layout of this.layouts) layout.execute(parent);packages/core/src/serialization/codecs/editor/EditorKeyHandlerCodec.ts (1)
70-87: Parse XML attributes to correct types before binding actionsBindAction expects number/boolean; attributes are strings. Convert safely to avoid subtle runtime issues.
Apply this diff within the decode loop:
- const as = node.getAttribute('as'); - const action = node.getAttribute('action'); - const control = node.getAttribute('control'); - - into.bindAction(as, action, control); + const asAttr = node.getAttribute('as'); + const action = node.getAttribute('action') || ''; + const controlAttr = node.getAttribute('control'); + const code = asAttr != null ? parseInt(asAttr, 10) : NaN; + const ctrl = controlAttr === '1' || controlAttr === 'true'; + if (!Number.isNaN(code)) { + into.bindAction(code, action, ctrl); + }packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
231-231: Avoid usingnull!to satisfy non-nullable parametersPassing
null!to addPrototype is brittle. Prefer loosening addPrototype’s signature to acceptpressed?: string | nullandinsert?: Function | null, or passundefinedwhere supported.packages/core/src/serialization/codecs/editor/EditorPopupMenuCodec.ts (1)
40-42: Type_objasEditorPopupMenufor clarityNarrowing the parameter improves readability and intent without affecting behavior.
Apply this diff:
- override encode(_enc: Codec, _obj: Element): Element | null { + override encode(_enc: Codec, _obj: EditorPopupMenu): Element | null {packages/core/src/serialization/codecs/ModelCodec.ts (2)
39-43: Parameter type should be GraphDataModel, not CellThis codec encodes GraphDataModel; using Cell here is misleading.
Apply this diff:
- override encodeObject(enc: Codec, obj: Cell, node: Element) { + override encodeObject(enc: Codec, obj: GraphDataModel, node: Element) {
48-54: Narrowobjto GraphDataModel indecodeChildUnion with
Cellis unnecessary; simplifies typing and intent.Apply this diff:
- override decodeChild(dec: Codec, child: Element, obj: Cell | GraphDataModel) { + override decodeChild(dec: Codec, child: Element, obj: GraphDataModel) { @@ - this.decodeRoot(dec, child, <GraphDataModel>obj); + this.decodeRoot(dec, child, obj);packages/core/src/view/shape/node/TextShape.ts (1)
444-534: updateBoundingBox override: LGTM.Optional: If zero-size foreignObject text should still set a 0x0 bounding box, prefer explicit null checks:
- let ow = null; - let oh = null; + let ow: number | null = null; + let oh: number | null = null; ... - if (ow && oh) { + if (ow != null && oh != null) { this.boundingBox = new Rectangle(this.bounds.x, this.bounds.y, ow, oh); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
packages/core/src/serialization/codecs/CellCodec.ts(3 hunks)packages/core/src/serialization/codecs/ChildChangeCodec.ts(5 hunks)packages/core/src/serialization/codecs/GenericChangeCodec.ts(1 hunks)packages/core/src/serialization/codecs/ModelCodec.ts(2 hunks)packages/core/src/serialization/codecs/RootChangeCodec.ts(2 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(2 hunks)packages/core/src/serialization/codecs/TerminalChangeCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorCodec.ts(2 hunks)packages/core/src/serialization/codecs/editor/EditorKeyHandlerCodec.ts(2 hunks)packages/core/src/serialization/codecs/editor/EditorPopupMenuCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts(2 hunks)packages/core/src/serialization/codecs/mxGraph/mxCellCodec.ts(1 hunks)packages/core/src/serialization/codecs/mxGraph/mxGeometryCodec.ts(1 hunks)packages/core/src/view/animate/Morphing.ts(1 hunks)packages/core/src/view/canvas/SvgCanvas2D.ts(6 hunks)packages/core/src/view/canvas/XmlCanvas2D.ts(34 hunks)packages/core/src/view/cell/CellOverlay.ts(1 hunks)packages/core/src/view/cell/CellState.ts(1 hunks)packages/core/src/view/cell/CellTracker.ts(1 hunks)packages/core/src/view/geometry/Geometry.ts(2 hunks)packages/core/src/view/geometry/Rectangle.ts(2 hunks)packages/core/src/view/handler/EdgeHandler.ts(2 hunks)packages/core/src/view/handler/EdgeSegmentHandler.ts(7 hunks)packages/core/src/view/handler/ElbowEdgeHandler.ts(3 hunks)packages/core/src/view/layout/CircleLayout.ts(1 hunks)packages/core/src/view/layout/CompactTreeLayout.ts(2 hunks)packages/core/src/view/layout/CompositeLayout.ts(2 hunks)packages/core/src/view/layout/EdgeLabelLayout.ts(1 hunks)packages/core/src/view/layout/FastOrganicLayout.ts(2 hunks)packages/core/src/view/layout/HierarchicalLayout.ts(2 hunks)packages/core/src/view/layout/ParallelEdgeLayout.ts(1 hunks)packages/core/src/view/layout/PartitionLayout.ts(2 hunks)packages/core/src/view/layout/RadialTreeLayout.ts(4 hunks)packages/core/src/view/layout/StackLayout.ts(2 hunks)packages/core/src/view/layout/SwimlaneLayout.ts(2 hunks)packages/core/src/view/layout/datatypes/GraphAbstractHierarchyCell.ts(1 hunks)packages/core/src/view/layout/datatypes/GraphHierarchyEdge.ts(5 hunks)packages/core/src/view/layout/datatypes/GraphHierarchyNode.ts(3 hunks)packages/core/src/view/plugins/ConnectionHandler.ts(4 hunks)packages/core/src/view/shape/edge/ArrowConnectorShape.ts(3 hunks)packages/core/src/view/shape/edge/ArrowShape.ts(2 hunks)packages/core/src/view/shape/edge/ConnectorShape.ts(2 hunks)packages/core/src/view/shape/edge/LineShape.ts(1 hunks)packages/core/src/view/shape/edge/PolylineShape.ts(1 hunks)packages/core/src/view/shape/node/ActorShape.ts(1 hunks)packages/core/src/view/shape/node/CloudShape.ts(1 hunks)packages/core/src/view/shape/node/CylinderShape.ts(1 hunks)packages/core/src/view/shape/node/DoubleEllipseShape.ts(2 hunks)packages/core/src/view/shape/node/EllipseShape.ts(1 hunks)packages/core/src/view/shape/node/HexagonShape.ts(1 hunks)packages/core/src/view/shape/node/ImageShape.ts(3 hunks)packages/core/src/view/shape/node/LabelShape.ts(6 hunks)packages/core/src/view/shape/node/RectangleShape.ts(3 hunks)packages/core/src/view/shape/node/RhombusShape.ts(2 hunks)packages/core/src/view/shape/node/SwimlaneShape.ts(4 hunks)packages/core/src/view/shape/node/TextShape.ts(11 hunks)packages/core/src/view/shape/node/TriangleShape.ts(1 hunks)packages/core/src/view/style/edge/EdgeStyleRegistry.ts(2 hunks)packages/core/tsconfig.json(0 hunks)
💤 Files with no reviewable changes (1)
- packages/core/tsconfig.json
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
🧬 Code graph analysis (16)
packages/core/src/serialization/codecs/editor/EditorKeyHandlerCodec.ts (1)
packages/core/src/editor/EditorKeyHandler.ts (1)
EditorKeyHandler(48-105)
packages/core/src/view/layout/ParallelEdgeLayout.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)
packages/core/src/view/layout/FastOrganicLayout.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)
packages/core/src/view/layout/CompactTreeLayout.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
packages/core/src/editor/EditorToolbar.ts (1)
EditorToolbar(59-478)
packages/core/src/view/layout/RadialTreeLayout.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)
packages/core/src/view/layout/datatypes/GraphHierarchyEdge.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)
packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
packages/core/src/types.ts (2)
EdgeStyleFunction(1300-1306)EdgeStyleMetaData(1554-1565)
packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
packages/core/src/view/style/Stylesheet.ts (1)
Stylesheet(49-200)
packages/core/src/serialization/codecs/editor/EditorCodec.ts (1)
packages/core/src/editor/Editor.ts (1)
Editor(390-2596)
packages/core/src/view/shape/edge/ArrowConnectorShape.ts (1)
packages/core/src/util/Constants.ts (1)
ARROW_SPACING(342-342)
packages/core/src/serialization/codecs/CellCodec.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)
packages/core/src/serialization/codecs/ModelCodec.ts (2)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)packages/core/src/view/GraphDataModel.ts (1)
GraphDataModel(218-1180)
packages/core/src/view/plugins/ConnectionHandler.ts (1)
packages/core/src/util/Constants.ts (1)
NONE(383-383)
packages/core/src/view/canvas/XmlCanvas2D.ts (1)
packages/html/stashed/grapheditor/www/js/Shapes.js (3)
flipH(166-166)flipV(167-167)fixDash(3431-3431)
packages/core/src/serialization/codecs/editor/EditorPopupMenuCodec.ts (1)
packages/core/src/editor/EditorPopupMenu.ts (1)
EditorPopupMenu(41-329)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: build (windows-2022)
🔇 Additional comments (130)
packages/core/src/view/shape/node/RhombusShape.ts (2)
44-46: LGTM: explicit override for isRoundableMatches the base Shape contract; no behavior change.
56-62: LGTM: explicit override for paintVertexShapeSignature is consistent with the base; override improves safety against upstream changes.
packages/core/src/view/shape/edge/LineShape.ts (1)
55-61: Explicit override correctly appliedMatches the PR objective; no behavior change. This will surface breakages if the base definition changes.
packages/core/src/view/shape/node/HexagonShape.ts (1)
38-38: LGTM: explicit override correctly applied.This aligns with the PR goal; signature and behavior remain unchanged.
packages/core/src/view/layout/EdgeLabelLayout.ts (1)
49-49: Explicit override on execute: LGTMMatches the PR goal; no behavior change.
packages/core/src/view/cell/CellTracker.ts (1)
114-121: Explicit override on destroy is correct and consistent.Good alignment with the PR goal; idempotent teardown and listener cleanup remain intact.
packages/core/src/view/shape/node/CylinderShape.ts (1)
50-50: Explicit override on svgStrokeTolerance: LGTMConsistent with the PR goal; no behavior change.
packages/core/src/view/style/edge/EdgeStyleRegistry.ts (2)
36-40: Explicit override on add: LGTMSignature remains compatible with BaseRegistry.add while extending with optional metaData; behavior unchanged and clearer intent.
55-55: Explicit override on clear: LGTMEnsures side metadata maps are cleared in sync with the base registry.
packages/core/src/view/shape/edge/ConnectorShape.ts (3)
54-80: override on paintEdgeShape — LGTMSignature unchanged; explicit override clarifies intent.
144-161: override on augmentBoundingBox — LGTMNo behavior change; override makes inheritance explicit.
46-49: Explicit override on updateBoundingBox — confirm noImplicitOverride for packages/coreGood use of
override; automated search found no tsconfig entries fornoImplicitOverrideunder packages/core — ensurecompilerOptions.noImplicitOverride = trueis set in packages/core/tsconfig.json or inherited from the repo root/extended tsconfig.packages/core/src/view/layout/CompactTreeLayout.ts (2)
237-239: LGTM: explicit override onisVertexIgnored.Matches the base contract; no behavior change.
258-258: LGTM: explicit override onexecute.Signature and explicit
: voidare consistent with the codebase.packages/core/src/view/layout/StackLayout.ts (2)
155-199: Explicit override on moveCell: verified — signature matches GraphLayoutGraphLayout.ts declares moveCell(cell: Cell, x: number, y: number): void, so the override is compatible.
278-439: LGTM — explicit override of execute matches base signature.
GraphLayout declares execute(parent: Cell): void at packages/core/src/view/layout/GraphLayout.ts:91.packages/core/src/view/handler/ElbowEdgeHandler.ts (3)
145-159: convertPoint override — looks goodSignature and behavior preserved; override adds compile-time safety.
167-217: redrawInnerBends override — looks goodNo behavior changes; override adds clarity and safety.
63-91: Explicit override on createBends — good improvementClear signal of intentional overriding; helps catch base signature drift.
Run to confirm base declarations exist and config enforces explicit overrides:
Expected: matches in EdgeHandler; tsconfig shows noImplicitOverride: true in core (and/or root) to enforce usage.
packages/core/src/view/layout/PartitionLayout.ts (1)
118-208: Explicit override on execute: LGTMConsistent with base GraphLayout; purely declarative change.
packages/core/src/view/cell/CellOverlay.ts (1)
218-220: Explicit override added on toString: LGTMMatches PR goal; no behavioral change introduced.
packages/core/src/view/layout/HierarchicalLayout.ts (2)
180-252: Override signature is compatible — no action requiredGraphLayout declares
execute(parent: Cell): void; multiple subclasses already override it with extra optional parameters (e.g., CircleLayout, SwimlaneLayout, ParallelEdgeLayout, RadialTreeLayout). HierarchicalLayout's signature follows the same pattern and should not cause override errors.
595-694: Override signature OK — base method existsGraphLayout.traverse is declared (packages/core/src/view/layout/GraphLayout.ts) and packages/core/src/view/layout/types.ts shows HierarchicalGraphLayoutTraverseArgs extends GraphLayoutTraverseArgs — signature is compatible.
packages/core/src/view/shape/edge/ArrowConnectorShape.ts (4)
77-80: resetStyles override LGTMMatches base behavior and preserves spacing reset.
85-92: apply override LGTMSignature and behavior unchanged; explicit override improves safety.
97-111: augmentBoundingBox override LGTMConsistent with base contract; no functional change.
116-116: paintEdgeShape override LGTMSignature annotated correctly; no behavior changes introduced.
packages/core/src/view/shape/node/CloudShape.ts (2)
42-51: LGTM: explicit override added correctly.Signature matches expected shape override pattern; no functional changes.
42-42: Verified: noImplicitOverride is enabled for corepackages/core/tsconfig.json extends ../../tsconfig.json and the root tsconfig.json contains "noImplicitOverride": true, so explicit override usage is enforced.
packages/core/src/view/layout/FastOrganicLayout.ts (3)
160-164: LGTM: explicit override on isVertexIgnoredSignature aligns with the base API; no behavior change.
170-341: LGTM: explicit override on executeConsistent with base class contract; no runtime changes introduced.
170-170: Confirmed: root tsconfig.json enables "noImplicitOverride"; ensure core inherits ittsconfig.json (root) contains "noImplicitOverride": true (line 11). Verify packages/core/tsconfig.json extends the root config or does not override that option.
packages/core/src/view/shape/node/SwimlaneShape.ts (2)
63-65: LGTM: explicit override on isRoundable.Signature and intent are clear.
58-58: Override on imageSrc is compatible with base Shape.
Shape.imageSrc is declared as string | null in packages/core/src/view/shape/Shape.ts, so the override is type-compatible.packages/core/src/view/shape/node/ActorShape.ts (1)
63-69: LGTM —overrideonpaintVertexShapeis correct.Base method declared at packages/core/src/view/shape/Shape.ts:729; packages/core/tsconfig.json does not set noImplicitOverride — using
overrideis valid and consistent with other shapes.packages/core/src/view/layout/RadialTreeLayout.ts (3)
69-69: LGTM: explicit override for levelDistanceMatches base contract; no behavior change.
75-75: LGTM: explicit override for nodeDistanceConsistent with base; safe change.
88-88: LGTM: explicit override for sortEdgesAnnotation only; behavior unchanged.
packages/core/src/view/shape/node/EllipseShape.ts (1)
42-48: Approve — no access modifier change needed. Shape.paintVertexShape is declared without an access modifier (public by default) in packages/core/src/view/shape/Shape.ts (around line 729), so the override in packages/core/src/view/shape/node/EllipseShape.ts is consistent and requires no change.packages/core/src/view/geometry/Geometry.ts (1)
363-365: LGTM: explicit override for cloneMatches intent; covariant return type is appropriate.
packages/core/src/view/geometry/Rectangle.ts (1)
177-179: LGTM: explicit override for cloneNo behavior change; consistent with the base class.
packages/core/src/view/shape/node/DoubleEllipseShape.ts (3)
66-72: LGTM: override annotation addedExplicit
overrideimproves safety when base API changes.
80-86: LGTM: override annotation addedConsistent with project-wide explicit overrides.
108-122: LGTM: override annotation addedNo functional change; clearer intent.
packages/core/src/view/layout/SwimlaneLayout.ts (2)
722-731: Explicit override on traverse: LGTM; ensure a matching base method exists.If GraphLayout doesn’t declare
traverse(even abstract), this will fail underoverride. Please confirm with the script above.
192-192: Approve — override is compatible with baseexecuteGraphLayout declares
execute(parent: Cell): void, so SwimlaneLayout'soverride execute(parent: Cell, swimlanes: Cell[] | null = null): voidis type‑compatible; LGTM.packages/core/src/view/cell/CellState.ts (1)
341-373: Verified: Rectangle.clone exists — check TypeScript versions
- Rectangle declares override clone() (packages/core/src/view/geometry/Rectangle.ts) and CellState uses override clone() (packages/core/src/view/cell/CellState.ts).
- Root package.json uses TypeScript ~5.9.2 (supports the override modifier), but packages/ts-support/package.json pins TypeScript 3.8.2 (pre-override). Upgrade ts-support's TypeScript to >=4.3 or ensure that package will not tsc-compile these sources.
packages/core/src/view/layout/ParallelEdgeLayout.ts (2)
83-83: Good: explicit override on executeMatches PR goal; no behavior change.
81-81: JSDoc link style OK — repo search inconclusivepackages/core/src/view/layout/ParallelEdgeLayout.ts contains "Implements {@link GraphLayout.execute}"; ripgrep run returned "No files were searched" and no matches. Run an unrestricted repo search for GraphLayout#execute / GraphLayout.execute (e.g. rg -nP '\bGraphLayout[#.]execute\b|{@link\s+GraphLayout[#.]execute\s*}' -uu) to confirm no '#' links remain.
packages/core/src/view/handler/EdgeHandler.ts (1)
2273-2319: Add explicit return type to getCellArrow-field override is fine on TS ≥ 4.9 — add a return type to lock the API:
- override getCell = (me: InternalMouseEvent) => { + override getCell = (me: InternalMouseEvent): Cell | null => {Verify locally that the repo uses TypeScript ≥4.9 (or that noImplicitOverride isn't enforced) and that Cell is in scope/imported.
packages/core/src/view/shape/node/LabelShape.ts (7)
56-56: Explicit override on imageSrc: OKMatches the PR’s goal and keeps intent clear.
63-63: Explicit override on spacing: OKConsistent with base Shape APIs.
79-79: Explicit override on indicatorImageSrc: OKNo behavior change; annotation looks correct.
98-107: redraw override: OKOrder and assignments preserved; no behavior changes.
125-131: paintForeground override: OKSignature and call order look correct.
262-293: redrawHtmlShape override: OKLogic unchanged; explicit override aligns with PR goals.
84-92: Verify base 'init' signature matches overrideConfirm the base class's init signature (parameter types and return) is assignable to the override
init(container: SVGElement)in packages/core/src/view/shape/node/LabelShape.ts:84–92.packages/core/src/view/handler/EdgeSegmentHandler.ts (9)
66-116: LGTM: getPreviewPoints override is correctSignature and behavior look consistent with the base handler.
121-219: Potential lost return value from convertPoint (verify intent)
convertPointis used without capturing its return in two places; if it doesn’t mutate in place,y0/yemay be wrong. If it mutates, this is fine. Please confirm.If
convertPointreturns a new point, apply:- if (sc != null) { - const pt = this.graph.getConnectionPoint(source, sc); - - if (pt != null) { - this.convertPoint(pt, false); - y0 = pt.y; - } - } + if (sc != null) { + const pt0 = this.graph.getConnectionPoint(source, sc); + if (pt0 != null) { + const cpt0 = this.convertPoint(pt0, false); + y0 = cpt0.y; + } + } @@ - if (tc) { - const pt = this.graph.getConnectionPoint(target, tc); - - if (pt != null) { - this.convertPoint(pt, false); - ye = pt.y; - } - } + if (tc) { + const pt1 = this.graph.getConnectionPoint(target, tc); + if (pt1 != null) { + const cpt1 = this.convertPoint(pt1, false); + ye = cpt1.y; + } + }
281-283: LGTM: tooltip overrideReturning
nullto disable tooltips matches the stated intent. Ensure base return type allowsnull.
288-299: LGTM: start overrideNo behavior change; explicit override improves maintainability.
304-343: LGTM: createBends overrideConsistent with existing behavior; explicit override only.
348-351: LGTM: redraw overrideExplicit override with pre-refresh keeps current semantics.
356-412: LGTM: redrawInnerBends overrideMatches existing logic; override marker is appropriate.
224-276: No change required —abspointsis defined on EdgeHandlerEdgeHandler declares
abspoints: (Point | null)[] = [](packages/core/src/view/handler/EdgeHandler.ts), so EdgeSegmentHandler inherits it andthis.abspointsis valid.
34-34: No change required — overridden property type matches baseEdgeHandler declares
points: Point[] = [];and EdgeSegmentHandler usesoverride points: Point[] = [];— types match exactly.packages/core/src/view/canvas/XmlCanvas2D.ts (1)
88-94: Good addition of explicit override on core canvas methods.Signatures read consistent with AbstractCanvas2D usage; no runtime behavior changes observed.
Also applies to: 106-111, 116-121, 128-132, 140-145, 157-163, 181-192, 200-211, 219-230, 237-252, 270-307, 314-329, 336-347, 357-373, 384-395, 404-415, 424-435, 443-454, 462-479, 486-503, 510-527, 535-548, 557-570, 578-595, 602-613, 615-627, 629-640, 642-654, 755-759, 771-778, 786-793, 803-812, 824-831, 847-849
packages/core/src/serialization/codecs/StylesheetCodec.ts (2)
52-80: encode override: LGTMSignature and usage align with ObjectCodec patterns; no logic change introduced.
47-47: Acceptstatic override allowEvalas-is.
ObjectCodec declaresstatic allowEval = falseand the repo uses TypeScript ~5.9.2 (override on static members is supported since TS 4.3) — no change needed.packages/core/src/view/layout/datatypes/GraphAbstractHierarchyCell.ts (2)
103-105: LGTM: explicit override on isEdgeConsistent with Cell.isEdge; no behavior change.
110-112: LGTM: explicit override on isVertexMatches inheritance intent; no behavior change.
packages/core/src/view/layout/datatypes/GraphHierarchyNode.ts (2)
133-135: LGTM: override isVertex always trueCorrect for hierarchy nodes; aligns with base contract.
56-56: Approve: narrowingidtostringis safeCell.id is declared as
string | nullin packages/core/src/view/cell/Cell.ts, so overriding it withstringis assignable and safe.packages/core/src/view/layout/datatypes/GraphHierarchyEdge.ts (5)
34-35: LGTM: override edges propertyConsistent with Cell.edges; no behavior change.
85-100: LGTM: explicit override getNextLayerConnectedCellsSignature aligns with abstract base; return type narrowed to non-null array is acceptable and matches implementation.
127-129: LGTM: explicit override on isEdgeConsistent with hierarchy edge semantics.
49-50: Narrowed target type is safe — writes use GraphHierarchyNodeConfirmed: assignments to target occur in packages/core/src/view/layout/hierarchical/GraphHierarchyModel.ts and SwimlaneModel.ts as
internalEdge.target = internalTargetCell(internalTargetCell comes from the vertexMapper and is null-checked and therefore a GraphHierarchyNode), and invert() swaps source/target (both typed GraphHierarchyNode|null).
44-45: Narrowed source type — write sites validatedAll assignments to GraphHierarchyEdge.source set GraphHierarchyNode|null (internalEdge.source = internalVertices[i] in GraphHierarchyModel and SwimlaneModel; invert() swaps source/target). No other write sites assign a plain Cell to this property.
packages/core/src/serialization/codecs/GenericChangeCodec.ts (1)
55-64: afterDecode correctly marked as override.Signature and behavior remain consistent with ObjectCodec.
packages/core/src/serialization/codecs/mxGraph/mxCellCodec.ts (2)
27-29: getName correctly annotated with override.No changes in behavior.
31-38: decodeAttribute override is correct.Conditional style conversion remains intact; super delegation preserved.
packages/core/src/view/shape/node/TriangleShape.ts (2)
39-41: isRoundable override is appropriate.No issues.
46-56: redrawPath override looks good.Signature matches base expectations; logic unchanged.
packages/core/src/serialization/codecs/TerminalChangeCodec.ts (1)
51-54: afterDecode override is correct.Behavior unchanged; previous assigned from terminal.
packages/core/src/serialization/codecs/editor/EditorCodec.ts (2)
106-124: afterDecode override LGTM.Template resolution for defaultEdge/defaultGroup unchanged; explicit override adds clarity.
129-142: decodeChild override LGTM.Custom handling for Array/ui nodes retained; falls back to super appropriately.
packages/core/src/serialization/codecs/mxGraph/mxGeometryCodec.ts (2)
28-30: getName correctly marked as override.All good.
36-58: afterDecode override is correct; conversion logic unchanged.Point array normalization remains intact; no functional changes.
packages/core/src/view/animate/Morphing.ts (1)
92-112: Explicit override is correct; enforced via root tsconfigpackages/core/tsconfig.json extends ../../tsconfig.json and the repo root tsconfig.json sets "noImplicitOverride": true (tsconfig.json:11), so explicit override is enforced project‑wide; no separate ESLint rule enforcing overrides was found.
packages/core/src/serialization/codecs/editor/EditorKeyHandlerCodec.ts (1)
38-40: LGTM: explicit override for encodeNo behavior change; matches ObjectCodec contract.
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (2)
48-50: LGTM: explicit override for encodeNo functional changes; consistent with other codecs.
133-285: LGTM: override for decode; behavior unchanged aside from above fixOverride modifier is appropriate; rest of logic mirrors existing behavior.
packages/core/src/serialization/codecs/editor/EditorPopupMenuCodec.ts (1)
47-57: LGTM: explicit overrides for encode/decodeSemantics unchanged; codec still forwards config correctly.
packages/core/src/view/canvas/SvgCanvas2D.ts (6)
238-238: LGTM: property marked as overrideMatches AbstractCanvas2D contract.
349-351: LGTM: override formatNo change in behavior; explicit override is correct.
373-376: LGTM: override resetResets gradients after super.reset(); good.
882-901: LGTM: override setLinkMaintains previous behavior; explicit override adds clarity.
906-948: LGTM: override rotateLogic unchanged; override modifier is appropriate.
951-956: LGTM: override beginComment cleanup and explicit override; correct.
packages/core/src/serialization/codecs/RootChangeCodec.ts (3)
46-49: LGTM: override afterEncodeNo behavior change; preserves existing encoding of root.
54-74: LGTM: override beforeDecodeClone-then-decode pattern retained; correct.
79-83: LGTM: override afterDecodeState restoration unchanged.
packages/core/src/serialization/codecs/ChildChangeCodec.ts (5)
54-59: LGTM: override isReferenceMatches intent; no behavior change.
64-72: LGTM: override isExcludedKeeps existing exclusion rules.
77-90: LGTM: override afterEncodeHandles reference vs. full encode as before.
95-133: LGTM: override beforeDecodeRetains logic to avoid re-inserting existing cells.
138-157: LGTM: override afterDecodeMaintains state adjustments; OK.
packages/core/src/view/plugins/ConnectionHandler.ts (5)
2023-2023: LGTM: property marked as overridehotspotEnabled override is appropriate.
2038-2106: LGTM: override getCellBehavior unchanged; override annotation adds clarity.
2109-2114: LGTM: override isValidStatePreserves existing highlighting logic.
2119-2122: LGTM: override getMarkerColorConsistent with NONE when not highlighting.
2126-2131: LGTM: override intersectsSemantics unchanged; explicit override is correct.
packages/core/src/serialization/codecs/CellCodec.ts (4)
82-87: Override on isExcluded: LGTM.Signature and logic unchanged; just adds explicit override.
92-109: Override on afterEncode: LGTM.Behavior preserved; node wrapping logic remains intact.
114-173: Override on beforeDecode: LGTM.No functional changes; matches base hook semantics.
75-77: Approve — explicit override is correct; enforcement & TS support verified../tsconfig.json sets noImplicitOverride: true (packages/core/tsconfig.json extends it). Root devDependency typescript is ~5.9.2 (TS ≥4.3 supports override on class properties).
packages/core/src/view/shape/node/TextShape.ts (14)
141-143: flipH/flipV property overrides: LGTM.
175-176: verticalTextRotation override: LGTM.
205-207: getSvgScreenOffset override: LGTM.
212-223: checkBounds override: LGTM.
228-302: paint override: LGTM.Signature matches existing usage; behavior unchanged.
307-339: redraw override: LGTM.
344-363: resetStyles override: LGTM.
370-400: apply override: LGTM.
539-541: getShapeRotation override: LGTM.
546-548: getTextRotation override: LGTM.
554-556: isPaintBoundsInverted override: LGTM.
561-567: configureCanvas override: LGTM.
625-700: redrawHtmlShape override: LGTM.
119-127: Property overrides added (bounds, spacing): LGTM — verify TypeScript used to build coreRoot package.json -> devDependencies: ~5.9.2 (supports override since 4.3). packages/ts-support/package.json -> devDependencies: 3.8.2 (pre-4.3). Ensure packages/core is compiled with TS >=4.3 (or upgrade the older package) so the
overridemodifier on fields is supported.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/__tests__/view/mixins/ConnectionsMixin.test.ts (2)
50-52: LGTM; optionally add explicit return type for clarityAdding
overrideis good. Consider making the return type explicit.- override parseDescription() { + override parseDescription(): void { // do nothing }
53-55: LGTM; optionally add explicit return type for claritySame here; explicit
voidimproves readability and guards signatures.- override parseConstraints() { + override parseConstraints(): void { // do nothing, constraints passed in constructor }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/__tests__/utils.ts(0 hunks)packages/core/__tests__/view/mixins/ConnectionsMixin.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/tests/utils.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
⏰ 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 (windows-2022)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/view/canvas/XmlCanvas2D.ts (3)
124-132: Fix JSDoc param name to match signature in scale()Doc still references “scale” while the parameter is “value”.
/** * Scales the output. * - * @param scale Number that represents the scale where 1 is equal to 100%. + * @param value Number that represents the scale where 1 is equal to 100%. */ - override scale(value: number): void { + override scale(value: number): void {
352-373: setDashed: correct second @param name and simplify attribute write
- JSDoc duplicates “value”; should document “fixDash”.
- Since fixDash is a boolean, the null check is unnecessary.
/** * Enables or disables dashed lines. * * @param value Boolean that specifies if dashed lines should be enabled. - * @param value Boolean that specifies if the stroke width should be ignored + * @param fixDash Boolean that specifies if the stroke width should be ignored * for the dash pattern. * @default false */ override setDashed(value: boolean, fixDash: boolean): void { @@ const elem = this.createElement('dashed'); elem.setAttribute('dashed', value ? '1' : '0'); - if (fixDash != null) { - elem.setAttribute('fixDash', fixDash ? '1' : '0'); - } + elem.setAttribute('fixDash', fixDash ? '1' : '0'); this.root.appendChild(elem); }
663-670: Add explicitoverrideto XmlCanvas2D implementationsAbstractCanvas2D declares these methods as abstract — add
overrideto the corresponding XmlCanvas2D implementations to make the overrides explicit and catch accidental mismatches.File: packages/core/src/view/canvas/XmlCanvas2D.ts — add
overrideto: rect, roundrect, ellipse, image, updateText, end, text, stroke, fill, fillAndStroke.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/__tests__/utils.ts(0 hunks)packages/core/__tests__/view/mixins/ConnectionsMixin.test.ts(1 hunks)packages/core/src/view/canvas/XmlCanvas2D.ts(34 hunks)packages/core/src/view/cell/CellState.ts(1 hunks)packages/core/src/view/shape/node/ImageShape.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/core/tests/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/view/cell/CellState.ts
- packages/core/src/view/shape/node/ImageShape.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
⏰ 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). (3)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2022)
🔇 Additional comments (4)
packages/core/__tests__/view/mixins/ConnectionsMixin.test.ts (2)
53-55: Explicit override on parseConstraints: LGTMConsistent with the explicit override policy; no behavioral change.
50-52: Explicit override on parseDescription — OKStencilShape declares parseDescription() (public by default) in packages/core/src/view/shape/stencil/StencilShape.ts; the test's override matches and no visibility change is required.
packages/core/src/view/canvas/XmlCanvas2D.ts (2)
88-94: Overrides added — LGTMSignatures and control flow unchanged; explicit overrides improve safety.
Also applies to: 106-111, 116-121, 140-145, 157-163, 181-192, 200-211, 219-230, 237-252, 270-307, 314-329, 336-347, 384-395, 404-415, 424-435, 443-454, 462-479, 486-503, 510-527, 535-548, 557-570, 601-612, 614-626, 628-639, 641-653, 754-758, 770-777, 785-792, 802-811, 823-841, 846-848
572-578: JSDoc: fix reference for font style flagsLinking to FONT_STYLE_MASK is likely broken. Prefer referencing the constants container.
/** * Sets the current font style. * - * @param value Numeric representation of the font style. This is the sum of the font styles from {@link FONT_STYLE_MASK}. + * @param value Numeric representation of the font style. This is the sum of the + * font styles from {@link mxConstants}. */ override setFontStyle(value: number | null = 0): void {If mxConstants isn’t exported in this package, point to the appropriate symbol (e.g., Constants) or drop the link and describe it as “sum of the font style flags (bold, italic, underline, shadow, strike-through)”.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/view/canvas/XmlCanvas2D.ts (2)
353-368: MakefixDashoptional to preserve API and base signature compatibility.The JSDoc says default is false, and the body already treats it as optional, but the signature requires it. This is a breaking change for call sites that pass a single argument and may conflict with the base method if it’s declared as optional there.
Apply this diff:
- override setDashed(value: boolean, fixDash: boolean): void { + override setDashed(value: boolean, fixDash?: boolean): void {Optionally, reflect optionality in the JSDoc param description.
612-624: Restore NONE-to-null normalization insetShadowColor(behavior regression).Other color setters normalize
NONEtonull. Omitting it here changes state comparisons and emitted XML for callers passingNONE.Apply this diff:
override setShadowColor(value: string | null = null): void { + if (value === NONE) { + value = null; + } if (this.compressed) { if (this.state.shadowColor === value) { return; }
🧹 Nitpick comments (2)
packages/core/src/view/canvas/XmlCanvas2D.ts (2)
626-636: Optional: roundshadowAlphalike other numeric attributes for consistency.Other numeric attributes use
this.format(...). Consider doing the same here.- elem.setAttribute('alpha', String(value)); + elem.setAttribute('alpha', String(this.format(value)));
752-756: Addoverrideto XmlCanvas2D methods that implement AbstractCanvas2DAbstractCanvas2D declares these methods as abstract — mark the implementations with
overrideto get compile-time protection against signature/behavior drift.packages/core/src/view/canvas/XmlCanvas2D.ts — rect (661), roundrect (680), ellipse (699), image (720), end (758), text (869), stroke (935), fill (942), fillAndStroke (949)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/view/canvas/XmlCanvas2D.ts(34 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
🧬 Code graph analysis (1)
packages/core/src/view/canvas/XmlCanvas2D.ts (1)
packages/html/stashed/grapheditor/www/js/Shapes.js (3)
flipH(166-166)flipV(167-167)fixDash(3431-3431)
🔇 Additional comments (2)
packages/core/src/view/canvas/XmlCanvas2D.ts (2)
88-95: Good move: explicitoverrideadoption.Consistent use of
overrideacross the class improves safety and future refactor signals. Looks solid here and in similar methods below.
573-575: JSDoc typo fixed; verify link target.“font style” wording is correct now—thanks. Please confirm
{@link FONT_STYLE_MASK}resolves in the docs; adjust to the exported symbol (e.g., constants module or enum) if not.#!/bin/bash # Check for a resolvable symbol for the JSDoc link rg -nP --type=ts '\bFONT_STYLE_MASK\b|enum\s+FontStyle\b|FONT_STYLE_' -C2
Easily detect when super methods are removed in the "core" package.
Already in place in other packages.
Summary by CodeRabbit