feat: accept more nullish parameter in various methods #892
Conversation
This prepares the migration of the "Wires" story to TypeScript. Add a new factory method in `CellMarker` to override the `CellHighlight` instance Export missing `ConnectionHandlerCellMarker` class The signature of some methods of the following classes now accepts null and/or undefined because their implementation already supported it: - CellMixin - ConnectionHandler - EdgeMixin - Geometry.setTerminalPoint - GraphView.updateFixedTerminalPoint - VertexMixin Improve JSDoc in various locations.
WalkthroughThis change updates method signatures in various core graph classes and interfaces to make parameters optional, enhances export declarations for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AbstractGraph
participant Cell
participant Geometry
User->>AbstractGraph: addCell(cell, [parent?], [index?], [source?], [target?])
AbstractGraph->>Cell: (cell is created/modified)
AbstractGraph->>Geometry: setTerminalPoint(point | null, isSource)
Note right of AbstractGraph: All relevant parameters are now optional where applicable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/core/src/view/plugins/ConnectionHandler.ts (1)
1296-1313: Guard against undefinededgeState.stylebefore mutation
updateEdgeStatewrites tothis.edgeState.style.*without confirming the object exists.
A defensive guard avoids a rare but possibleTypeErrorwhenstylehasn’t been initialised:- if (this.sourceConstraint?.point) { + if (this.edgeState.style && this.sourceConstraint?.point) { this.edgeState.style.exitX = this.sourceConstraint.point.x; this.edgeState.style.exitY = this.sourceConstraint.point.y; } - if (constraint?.point) { + if (this.edgeState.style && constraint?.point) { this.edgeState.style.entryX = constraint.point.x; this.edgeState.style.entryY = constraint.point.y; } else if (this.edgeState.style) { this.edgeState.style.entryX = 0; this.edgeState.style.entryY = 0; }This keeps the tight loop allocation-free and avoids undefined access.
♻️ Duplicate comments (1)
packages/core/src/view/mixins/VertexMixin.ts (1)
118-118: Same non-null assertion concern as in EdgeMixin.The non-null assertion
id!poses the same potential runtime risk as identified inEdgeMixin.tsline 259, where an optionalidparameter is asserted as non-null.
🧹 Nitpick comments (2)
packages/core/src/view/GraphView.ts (1)
1129-1136: JSDoc still implies the argument is always aConnectionConstraintSince
constraintcan now benull, the doc-comment should reflect this to avoid confusion.- * @param constraint {@link ConnectionConstraint} that specifies the connection. + * @param constraint {@link ConnectionConstraint} | null – optional connection constraint.packages/core/src/view/plugins/ConnectionHandler.ts (1)
715-741: Remove TODO or add JSDoc forgetIconPositionThere’s still a “TODO: Document me!” banner above
getIconPosition. Either finish the JSDoc (preferred) or drop the TODO to avoid noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/core/src/index.ts(1 hunks)packages/core/src/view/GraphView.ts(1 hunks)packages/core/src/view/cell/CellHighlight.ts(1 hunks)packages/core/src/view/cell/CellMarker.ts(1 hunks)packages/core/src/view/geometry/Geometry.ts(1 hunks)packages/core/src/view/mixins/CellsMixin.type.ts(1 hunks)packages/core/src/view/mixins/EdgeMixin.ts(1 hunks)packages/core/src/view/mixins/EdgeMixin.type.ts(2 hunks)packages/core/src/view/mixins/VertexMixin.ts(2 hunks)packages/core/src/view/mixins/VertexMixin.type.ts(2 hunks)packages/core/src/view/plugins/ConnectionHandler.ts(20 hunks)packages/core/src/view/plugins/PanningHandler.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#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
PR: maxGraph/maxGraph#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
PR: maxGraph/maxGraph#835
File: packages/html/stories/Orthogonal.stories.ts:114-117
Timestamp: 2025-05-26T12:34:54.318Z
Learning: The `null!` assertion pattern exists across multiple stories in the TypeScript migration and should be addressed systematically rather than in individual PRs.
Learnt from: tbouffard
PR: maxGraph/maxGraph#770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.
Learnt from: tbouffard
PR: maxGraph/maxGraph#715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.
Learnt from: tbouffard
PR: maxGraph/maxGraph#593
File: .github/workflows/create-github-release.yml:42-42
Timestamp: 2025-01-28T16:22:25.804Z
Learning: In the GitHub release notes template for maxGraph, TODO comments are intentionally included to mark sections that must be reviewed by maintainers before publishing the release. These TODOs should not be removed or converted to validation steps as they are part of the release review process.
Learnt from: tbouffard
PR: maxGraph/maxGraph#791
File: packages/core/src/view/style/edge/Segment.ts:26-35
Timestamp: 2025-04-29T13:29:45.122Z
Learning: In the maxGraph project, JSDoc `{@link}` references to symbols don't need to be imported in the current file to work correctly. TypeDoc can resolve symbols across the entire project namespace, not just within the current file, which is common in modern documentation systems and IDEs.
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.
packages/core/src/view/cell/CellMarker.ts (1)
Learnt from: tbouffard
PR: #776
File: packages/core/src/view/Graph.ts:85-92
Timestamp: 2025-04-24T12:34:10.366Z
Learning: The Graph class intentionally doesn't use collaborators (cellRenderer, selectionModel, view) from options in initializeCollaborators, as it's designed to replicate the previous implementation using only its factory methods for backward compatibility.
packages/core/src/view/GraphView.ts (2)
Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.
Learnt from: tbouffard
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.
packages/core/src/view/mixins/CellsMixin.type.ts (1)
Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.
packages/core/src/view/mixins/VertexMixin.ts (1)
Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.
packages/core/src/view/mixins/EdgeMixin.type.ts (2)
Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.
Learnt from: tbouffard
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.
packages/core/src/view/mixins/VertexMixin.type.ts (3)
Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.
Learnt from: tbouffard
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
PR: #776
File: packages/core/src/view/Graph.ts:85-92
Timestamp: 2025-04-24T12:34:10.366Z
Learning: The Graph class intentionally doesn't use collaborators (cellRenderer, selectionModel, view) from options in initializeCollaborators, as it's designed to replicate the previous implementation using only its factory methods for backward compatibility.
packages/core/src/view/mixins/EdgeMixin.ts (3)
Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.
Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:30:18.686Z
Learning: In the EdgeStyles documentation, tbouffard prefers the existing text "A perimeter is a function matching the EdgeStyleFunction type" over suggestions that might create redundancy, even if terminology might be technically misaligned.
Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:32:14.572Z
Learning: In the EdgeStyles documentation, the sentence "A perimeter is a function matching the EdgeStyleFunction type" contains an incorrect term - "perimeter" should be replaced with "EdgeStyle" or "custom EdgeStyle" as the document is specifically about EdgeStyles.
packages/core/src/view/plugins/ConnectionHandler.ts (3)
Learnt from: tbouffard
PR: #791
File: packages/core/src/view/style/edge/Segment.ts:26-35
Timestamp: 2025-04-29T13:29:45.122Z
Learning: In the maxGraph project, JSDoc {@link} references to symbols don't need to be imported in the current file to work correctly. TypeDoc can resolve symbols across the entire project namespace, not just within the current file, which is common in modern documentation systems and IDEs.
Learnt from: tbouffard
PR: #776
File: packages/core/src/view/Graph.ts:85-92
Timestamp: 2025-04-24T12:34:10.366Z
Learning: The Graph class intentionally doesn't use collaborators (cellRenderer, selectionModel, view) from options in initializeCollaborators, as it's designed to replicate the previous implementation using only its factory methods for backward compatibility.
Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:30:18.686Z
Learning: In the EdgeStyles documentation, tbouffard prefers the existing text "A perimeter is a function matching the EdgeStyleFunction type" over suggestions that might create redundancy, even if terminology might be technically misaligned.
🧬 Code Graph Analysis (6)
packages/core/src/index.ts (3)
packages/html/stories/Anchors.stories.js (2)
MyCustomConnectionHandler(37-43)createConnectionHandler(60-62)packages/html/stories/FixedPoints.stories.js (2)
isConnectableCell(105-107)MyCustomConnectionHandler(42-101)packages/html/stories/Events.stories.js (1)
MyCustomConnectionHandler(40-43)
packages/core/src/view/plugins/PanningHandler.ts (1)
packages/core/src/view/event/InternalMouseEvent.ts (1)
isPopupTrigger(159-161)
packages/core/src/view/mixins/CellsMixin.type.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)
packages/core/src/view/mixins/VertexMixin.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)
packages/core/src/view/mixins/EdgeMixin.type.ts (2)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)packages/core/src/types.ts (1)
EdgeParametersValue(1066-1066)
packages/core/src/view/plugins/ConnectionHandler.ts (6)
packages/core/src/types.ts (2)
GraphPlugin(1179-1181)MouseListenerSet(1211-1215)packages/core/src/util/Constants.ts (3)
TOOLTIP_VERTICAL_OFFSET(75-75)VALID_COLOR(150-150)INVALID_COLOR(156-156)packages/core/src/index.ts (1)
ConnectionHandlerCellMarker(52-52)packages/html/stories/Anchors.stories.js (3)
MyCustomConnectionHandler(37-43)createConnectionHandler(60-62)MyCustomGraph(45-63)packages/html/stories/FixedPoints.stories.js (2)
MyCustomConnectionHandler(42-101)isConnectableCell(105-107)packages/html/stories/Events.stories.js (2)
MyCustomConnectionHandler(40-43)createConnectionHandler(56-58)
⏰ 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 (windows-2022)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build
🔇 Additional comments (16)
packages/core/src/view/cell/CellHighlight.ts (1)
231-231: LGTM: Documentation fix improves accuracy.The JSDoc comment correction properly references
CellStateandInternalEvent.MARKinstead of the previous typos/incomplete references.packages/core/src/view/plugins/PanningHandler.ts (1)
267-269: LGTM: Documentation formatting enhancement.The JSDoc formatting improvements with inline code backticks and proper
@linkreferences enhance readability and IDE integration.packages/core/src/index.ts (1)
52-52: Good addition: Improves API surface for connection handling.Making
ConnectionHandlerCellMarkeravailable as a named export enables external customization of connection handling, which aligns with the usage patterns seen in the story files.packages/core/src/view/cell/CellMarker.ts (1)
132-141: Excellent extensibility improvement using factory method pattern.The introduction of the
createCellHighlightfactory method enables subclasses to customize theCellHighlightinstance creation without overriding the entire constructor. This follows established patterns in the codebase for enabling extensibility.packages/core/src/view/geometry/Geometry.ts (1)
195-195: Good type accuracy improvement.Making the
pointparameter nullable (Point | null) accurately reflects the method's implementation and the nullable nature of thesourcePointandtargetPointproperties it sets. This enhances type safety and API flexibility.packages/core/src/view/GraphView.ts (1)
1138-1146: Signature widening and cast removal look goodThe parameter now explicitly allows
nulland the superfluous<Point>cast has been dropped. This aligns with the return typePoint | nullfromgetFixedTerminalPointand with the PR’s intent to accept nullish values. No further action needed.packages/core/src/view/mixins/CellsMixin.type.ts (1)
302-302: LGTM! Parameter made appropriately optional.Making the
parentparameter optional in theaddCellmethod signature improves API flexibility while maintaining type safety. This change aligns well with the PR's objective of accepting more nullish parameters.packages/core/src/view/mixins/EdgeMixin.ts (2)
250-255: LGTM! Unused parameters appropriately marked.Prefixing unused parameters with underscores (
_parent,_source,_target) follows TypeScript best practices and aligns with existing maxGraph patterns.
259-259: Verifyid!assertion safety inEdgeMixin(andVertexMixin)The non-null assertion (
id!) may pass an actualundefinedintosetIdat runtime. Automated searches didn’t locate thesetIdsignature or its handling ofundefined, so please manually confirm:• Whether
setId’s parameter is truly optional (e.g.id?: string) and
• HowsetId(undefined)behaves (auto-generates a new ID vs. assignsundefined)If
idcan be undefined here, either guard the call or supply a default/generated ID before invoking:packages/core/src/view/mixins/EdgeMixin.ts:259
- edge.setId(id!); // the auto-generated id is done when adding the cell to the model + if (id != null) { + edge.setId(id); + } + // else rely on model to generate an ID when adding the cellRepeat the same check in
VertexMixin.ts.packages/core/src/view/mixins/VertexMixin.ts (1)
101-103: LGTM! Parameters made appropriately optional.Making the
_parent,id, andvalueparameters optional aligns with the updated type signatures and improves API flexibility.packages/core/src/view/mixins/EdgeMixin.type.ts (2)
186-192: LGTM! Method signature appropriately relaxed.Making the
parent,id, andvalueparameters optional ininsertEdgeimproves API flexibility while maintaining type safety. The changes are well-coordinated with the implementation.
216-222: LGTM! Consistent parameter flexibility for createEdge.The optional parameters in
createEdgealign well with the implementation changes and provide consistent API flexibility across edge creation methods.packages/core/src/view/mixins/VertexMixin.type.ts (2)
92-102: LGTM! Consistent API flexibility for insertVertex.Making the
parent,id, andvalueparameters optional maintains consistency with the edge methods and provides the same flexible API experience across vertex operations.
165-175: LGTM! Well-coordinated createVertex signature.The optional parameters in
createVertexcomplete the consistent pattern across all cell creation methods, maintaining type safety while improving API flexibility.packages/core/src/view/plugins/ConnectionHandler.ts (2)
205-208: Verify export strategy – default vs namedThe class is now declared with
export default, whilepackages/core/src/index.tsappears to re-exportConnectionHandleras a named export.
Having bothexport defaultand a re-exported name is fine, but please double-check that:
- The index file does not add a second
export default(rollup/tsc will complain).- External consumers keep using a single, canonical import style to avoid duplication in bundle output.
No change requested if the build remains clean; this is a heads-up.
2014-2029: 👍 Good call on exportingConnectionHandlerCellMarkerPromoting the marker to a public export solves long-standing extension friction and aligns with storybook examples that subclass it.
|
This prepares the migration of the "Wires" story to TypeScript.
Add a new factory method in
CellMarkerto override theCellHighlightinstanceExport missing
ConnectionHandlerCellMarkerclassThe signature of some methods of the following classes now accepts null and/or undefined because their implementation
already supported it:
Improve JSDoc in various locations.
Summary by CodeRabbit
New Features
ConnectionHandlerCellMarkerand madeConnectionHandleravailable as a default export.Documentation
Refactor
Style