refactor: introduce AbstractPathShape base class for shape hierarchy #953
Conversation
Replace ActorShape as the base class for CloudShape, HexagonShape, and TriangleShape. These classes had no functional relation with ActorShape - the inheritance was only for technical convenience. Introduce AbstractPathShape as a dedicated abstract base class to make the hierarchy more explicit and semantically clear. ActorShape now extends AbstractPathShape alongside the other shapes. Changes: - Add new AbstractPathShape abstract class - Refactor ActorShape to extend AbstractPathShape - Update CloudShape, HexagonShape, and TriangleShape to extend AbstractPathShape
|
Warning Rate limit exceeded@tbouffard has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughIntroduces AbstractPathShape, a new abstract base class consolidating common painting and styling logic for path-based vertex shapes. Refactors ActorShape, CloudShape, HexagonShape, and TriangleShape to extend AbstractPathShape instead of previous base classes. Updates Shape.ts documentation to use { Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/view/shape/node/CloudShape.ts (1)
19-38: Update CloudShape JSDoc to match new base classThe move to
AbstractPathShapeand the optional constructor are fine, but the JSDoc still claims this “Extends {@link ActorShape} to implement a cloud shape.”To avoid confusion for readers, consider updating that line, for example:
-/** - * Extends {@link ActorShape} to implement a cloud shape. +/** + * Path-based cloud vertex shape built on {@link AbstractPathShape}.The rest of the implementation can stay as-is.
🧹 Nitpick comments (1)
packages/core/src/view/shape/node/ActorShape.ts (1)
20-39: ActorShape refactor is sound; JSDoc can mention AbstractPathShapeThe change to derive
ActorShapefromAbstractPathShapewith an optional constructor that forwards all four arguments tosuperlooks correct and consistent with the other path-based shapes.redrawPathcontinues to operate in the normalized 0..w/0..h space, which matchesAbstractPathShape.paintVertexShape’stranslate(x, y)behavior.The class-level JSDoc still reads:
Extends {@link Shape} to implement an actor shape.
Given the new hierarchy, you might want to reference the path-specific base to make the design clearer, for example:
-/** - * Extends {@link Shape} to implement an actor shape. +/** + * Path-based actor vertex shape built on {@link AbstractPathShape}.Functionality-wise, no changes are needed.
Also applies to: 44-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/src/view/shape/Shape.ts(1 hunks)packages/core/src/view/shape/node/AbstractPathShape.ts(1 hunks)packages/core/src/view/shape/node/ActorShape.ts(1 hunks)packages/core/src/view/shape/node/CloudShape.ts(2 hunks)packages/core/src/view/shape/node/HexagonShape.ts(2 hunks)packages/core/src/view/shape/node/TriangleShape.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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.
📚 Learning: 2025-04-24T12:34:56.726Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.
Applied to files:
packages/core/src/view/shape/node/AbstractPathShape.ts
🧬 Code graph analysis (5)
packages/core/src/view/shape/node/TriangleShape.ts (1)
packages/core/src/types.ts (1)
ColorValue(909-909)
packages/core/src/view/shape/node/HexagonShape.ts (1)
packages/core/src/types.ts (1)
ColorValue(909-909)
packages/core/src/view/shape/node/CloudShape.ts (1)
packages/core/src/types.ts (1)
ColorValue(909-909)
packages/core/src/view/shape/node/AbstractPathShape.ts (2)
packages/core/src/types.ts (1)
ColorValue(909-909)packages/core/src/util/Constants.ts (1)
NONE(378-378)
packages/core/src/view/shape/node/ActorShape.ts (3)
packages/core/src/view/canvas/XmlCanvas2D.ts (2)
fill(942-944)stroke(935-937)packages/core/src/view/canvas/SvgCanvas2D.ts (2)
fill(1807-1809)stroke(1800-1802)packages/core/src/types.ts (1)
ColorValue(909-909)
⏰ 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 (ubuntu-24.04)
- GitHub Check: build (macos-14)
🔇 Additional comments (3)
packages/core/src/view/shape/Shape.ts (1)
1019-1020: JSDoc link style change looks goodUsing
{@link ...}forboundingBox,createBoundingBox, andaugmentBoundingBoxis consistent with the rest of the file and doesn’t affect behavior.packages/core/src/view/shape/node/HexagonShape.ts (1)
19-40: HexagonShape migration to AbstractPathShape is correctImports, inheritance, and the optional constructor forwarding to
super(bounds, fill, stroke, strokeWidth)align with the new path-based base while preserving the expected instantiation contract.packages/core/src/view/shape/node/TriangleShape.ts (1)
20-40: TriangleShape refactor to AbstractPathShape is consistentThe new inheritance and constructor delegation to
AbstractPathShapeare consistent with Hexagon/Cloud/Actor and keep the public instantiation pattern intact.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/view/shape/node/ActorShape.ts (1)
32-39: Constructor is redundant and can be safely omittedThe constructor just forwards parameters to
super()without additional logic. Since TypeScript automatically inherits the base class constructor signature when a derived class doesn't declare its own, you can remove this constructor entirely and rely on the inherited constructor fromAbstractPathShape. This reduces duplication while maintaining identical behavior and type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/view/shape/node/ActorShape.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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 (ubuntu-24.04)
- GitHub Check: build (windows-2022)
- GitHub Check: build (macos-14)
🔇 Additional comments (3)
packages/core/src/view/shape/node/ActorShape.ts (3)
19-23: Imports now align with the new AbstractPathShape baseSwitching to
AbstractPathShapeand using a type-only import forColorValueis consistent with the new hierarchy and keeps types explicit without impacting runtime.
25-31: Doc and base class correctly reflect the refactored shape hierarchyThe updated description and
extends AbstractPathShapecleanly express thatActorShapeis a path-based vertex shape and decouple it from unrelated shapes, matching the PR’s architectural goal.
41-52:redrawPathoverride remains correct and benefits from the override keywordMarking
redrawPathwithoverridekeeps the existing path logic while adding compile-time checks against the abstract base, which is exactly what we want for this refactor.
…asses not redefining constructors
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/view/shape/node/CloudShape.ts (1)
19-42: CloudShape correctly migrated toAbstractPathShape, but JSDoc still mentionsActorShape.The switch to
AbstractPathShapeand the type-onlyAbstractCanvas2Dimport look consistent with the new path-based hierarchy;CloudShapeonly customizesredrawPath, so behavior should remain unchanged.The JSDoc still says “Extends {@link ActorShape}”, which is now inaccurate and could confuse readers. Consider updating it to reference
AbstractPathShapeor rephrasing without naming the base class.Based on learnings, treating this as a non-blocking doc tweak aligned with the refactor’s scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/view/shape/node/ActorShape.ts(1 hunks)packages/core/src/view/shape/node/CloudShape.ts(2 hunks)packages/core/src/view/shape/node/HexagonShape.ts(2 hunks)packages/core/src/view/shape/node/TriangleShape.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/view/shape/node/ActorShape.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 857
File: packages/core/src/view/plugins/SelectionHandler.ts:1081-1092
Timestamp: 2025-06-26T13:27:19.012Z
Learning: User tbouffard prefers to keep PRs focused on their main objective and defer implementation improvements or logic fixes to future dedicated PRs. For maxGraph, refactoring PRs like Dictionary→Map migration should not include unrelated logic improvements even when valid issues are identified in the migrated code.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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.
🔇 Additional comments (2)
packages/core/src/view/shape/node/TriangleShape.ts (1)
20-52: TriangleShape migration toAbstractPathShapelooks consistent and behavior-preserving.Imports, the new
extends AbstractPathShape, and theredrawPathoverride align with a path-based base class, and the use ofgetBaseArcSize/addPointssuggests shared rounding logic now lives in the base. No functional regressions are apparent in this class.packages/core/src/view/shape/node/HexagonShape.ts (1)
19-51: HexagonShape is cleanly aligned withAbstractPathShapewithout changing behavior.The new base class, imports, and
redrawPathimplementation are consistent with the other refactored shapes. Relying on the inherited constructor and usinggetBaseArcSize/addPointscentralizes path logic as intended, with no visible functional changes.
|
refactor: introduce AbstractPathShape base class for shape hierarchy
Replace ActorShape as the base class for CloudShape, HexagonShape, and
TriangleShape. These classes had no functional relation with ActorShape -
the inheritance was only for technical convenience.
Introduce AbstractPathShape as a dedicated abstract base class to make
the hierarchy more explicit and semantically clear. ActorShape now extends
AbstractPathShape alongside the other shapes.
Changes:
Summary by CodeRabbit
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.