refactor: migrate the Orthogonal story to TypeScript#835
Conversation
Ease the maintenance and detect errors earlier. Add a description at the container to explain to the user what the story does.
WalkthroughThe changes refactor the orthogonal projection story to improve modularity by introducing helper functions, explicit plugin handling, and safer type usage. The story's setup logic is restructured, and method overrides are made more robust. Documentation is updated to reference the TypeScript version of the source code for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Storybook
participant Template
participant Graph
participant Plugins
User->>Storybook: Select Orthogonal story
Storybook->>Template: Render with args
Template->>Graph: Initialize graph with helper functions
Template->>Plugins: Retrieve and override plugin methods
Plugins->>Graph: Modify behavior (guides, edge handling, snapping)
Template->>Graph: Add vertices and edges with updated logic
Graph-->>Template: Rendered graph with orthogonal projection
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
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: 2
🧹 Nitpick comments (2)
packages/html/stories/Orthogonal.stories.ts (2)
50-56: Good TypeScript migration with improved user context.The function signature provides type safety and the description helps users understand the story's purpose, aligning with the PR objectives.
Consider defining a more specific interface for the args parameter in the future to improve type safety:
interface OrthogonalStoryArgs { label?: string; rubberBand?: boolean; // ... other specific properties }
140-190: Consider safer geometry access patterns.While geometry is typically set after vertex creation, using non-null assertions throughout could hide potential issues.
Consider adding runtime checks or using optional chaining for safer access:
if (v11.geometry) { v11.geometry.offset = new Point(-5, -5); } // or v11.geometry?.offset = new Point(-5, -5);This pattern would make the code more robust against unexpected null/undefined values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/html/stories/Orthogonal.stories.ts(6 hunks)packages/website/docs/usage/perimeters.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
🔇 Additional comments (3)
packages/html/stories/Orthogonal.stories.ts (3)
19-35: Well-organized imports for TypeScript migration.The imports are properly structured with all necessary types and utilities for the TypeScript implementation.
59-63: Excellent plugin management refactoring.The explicit plugin handling with
getDefaultPlugins()and conditional addition improves modularity and maintainability.
180-190: Good use of framework utilities.Using
cellArrayUtils.cloneCellinstead of direct cloning follows framework patterns and improves maintainability.
| connectionHandler.createEdgeState = function (_me) { | ||
| const edge = graph.createEdge(null, null!, null, null, null); | ||
| return new CellState(this.graph.view, edge, this.graph.getCellStyle(edge)); | ||
| }; |
There was a problem hiding this comment.
Avoid using non-null assertion operator with null.
Using null! bypasses TypeScript's null safety checks and could lead to runtime errors if the method expects a non-null value.
Consider using undefined or providing a default value:
- const edge = graph.createEdge(null, null!, null, null, null);
+ const edge = graph.createEdge(null, null, null, null, null);If the second parameter truly cannot be null, investigate what value should be provided instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connectionHandler.createEdgeState = function (_me) { | |
| const edge = graph.createEdge(null, null!, null, null, null); | |
| return new CellState(this.graph.view, edge, this.graph.getCellStyle(edge)); | |
| }; | |
| connectionHandler.createEdgeState = function (_me) { | |
| const edge = graph.createEdge(null, null, null, null, null); | |
| return new CellState(this.graph.view, edge, this.graph.getCellStyle(edge)); | |
| }; |
🤖 Prompt for AI Agents
In packages/html/stories/Orthogonal.stories.ts between lines 114 and 117, the
code uses the non-null assertion operator `null!` which bypasses TypeScript's
null safety checks and may cause runtime errors. Remove the `!` and replace
`null!` with an appropriate value such as `undefined` or a valid default that
aligns with the method's expected parameter type. If the parameter cannot be
null or undefined, determine the correct value to pass instead of `null!`.
There was a problem hiding this comment.
Thx, this will be managed later. Another story needs it too.
There was a problem hiding this comment.
@tbouffard Thanks for the clarification! Since this pattern affects multiple stories, would you like me to create a follow-up issue to track removing the null! assertions across all the affected stories? This would help ensure it gets addressed systematically in a future PR.
✏️ Learnings added
Learnt from: tbouffard
PR: maxGraph/maxGraph#835
File: packages/html/stories/Orthogonal.stories.ts:114-117
Timestamp: 2025-05-26T12:34:54.306Z
Learning: The `null!` assertion pattern exists across multiple stories in the TypeScript migration and should be addressed systematically rather than in individual PRs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Ease the maintenance and detect errors earlier.
Add a description at the container to explain to the user what the story does.
Summary by CodeRabbit