refactor: migrate the "AutoLayout" story to TypeScript by tbouffard · Pull Request #638 · maxGraph/maxGraph
Rationale:
- Ease the maintenance
- Detect the errors earlier
Summary by CodeRabbit
-
Documentation
- Updated documentation comments for the
EdgeHandlerclass, making them more concise. - Improved code readability by renaming method parameters.
- Updated documentation comments for the
-
Refactor
- Enhanced type safety in
AutoLayout.stories.tsby adding TypeScript type annotations. - Updated method signatures with more precise type declarations.
- Improved variable naming and type specifications.
- Enhanced type safety in
-
Style
- Consolidated documentation comments.
- Cleaned up code formatting.
Warning
Rate limit exceeded
@tbouffard has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
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 (1)
packages/html/stories/AutoLayout.stories.ts(4 hunks)
Walkthrough
The pull request introduces modifications to enhance type safety and documentation clarity in two files: EdgeHandler.ts and AutoLayout.stories.ts. Key changes include the renaming of method parameters, the addition of TypeScript type annotations, and the consolidation of documentation comments. These adjustments aim to improve the overall clarity and precision of the code without affecting the existing functionality of the methods.
Changes
| File | Change Summary |
|---|---|
packages/core/src/view/handler/EdgeHandler.ts |
- Renamed method parameters isClone to _isClone and me to _me - Consolidated documentation comments |
packages/html/stories/AutoLayout.stories.ts |
- Added explicit TypeScript type annotations to method signatures - Updated parameter and return types for various methods - Renamed variable v1 to vertex1 for clarity - Added type casting for plugin retrieval |
Possibly related PRs
- refactor(VertexHandler): simplify management of
style.rotation#634: Theconnectmethod in theMyCustomEdgeHandlerclass has a similar signature update to theconnectmethod in theEdgeHandlerclass, indicating a related change in parameter naming conventions.
Suggested labels
javascript
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
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)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai generate docstringsto generate docstrings for this PR. (Beta)@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/html/stories/AutoLayout.stories.ts (3)
71-98: Improve event type safety and reduce code duplication.
- The event type for pointerdown/mousedown should be more specific
- The touchstart event handler duplicates the pointerdown logic
- (evt: MouseEvent | KeyboardEvent) => { + (evt: MouseEvent) => { overlay.fireEvent(new EventObject('pointerdown', { event: evt, state })); } ); if (!Client.IS_POINTER && Client.IS_TOUCH) { - InternalEvent.addListener( - shape.node, - 'touchstart', - (evt: MouseEvent | KeyboardEvent) => { - overlay.fireEvent(new EventObject('pointerdown', { event: evt, state })); - } - ); + InternalEvent.addListener(shape.node, 'touchstart', handlePointerDown); }
130-134: Consider using a regular method instead of an arrow function.Converting
resizeCellto an arrow function changes its binding behavior and prevents proper method overriding in derived classes.- resizeCell = (cell: Cell, bounds: Rectangle, recurse?: boolean): Cell => { - const resizedCell = super.resizeCell(cell, bounds, recurse); - executeLayout(); - return resizedCell; - }; + resizeCell(cell: Cell, bounds: Rectangle, recurse?: boolean): Cell { + const resizedCell = super.resizeCell(cell, bounds, recurse); + executeLayout(); + return resizedCell; + }
249-253: Remove commented-out code and TODO comment.The commented-out code appears to be obsolete since the
resizeCellmethod has been reimplemented above. This commented section should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/view/handler/EdgeHandler.ts(1 hunks)packages/html/stories/AutoLayout.stories.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/html/stories/AutoLayout.stories.ts
[error] 184-184: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (3)
packages/html/stories/AutoLayout.stories.ts (2)
Line range hint
1-70: LGTM! TypeScript migration for imports and story configuration is well done.The migration includes proper type imports and story configuration setup.
100-112: LGTM! Well-typed EdgeHandler implementation.The MyCustomEdgeHandler class is properly typed with TypeScript annotations.
packages/core/src/view/handler/EdgeHandler.ts (1)
1687-1700: LGTM! Good use of TypeScript parameter naming convention.The renaming of parameters to
_isCloneand_mefollows the TypeScript convention for indicating unused parameters.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/html/stories/AutoLayout.stories.ts (1)
89-95: Consider consolidating duplicate event handling logic.The touch event handling duplicates the event firing logic. Consider extracting the common logic into a shared function.
class MyCustomCellRenderer extends CellRenderer { installCellOverlayListeners(state: CellState, overlay: CellOverlay, shape: Shape) { super.installCellOverlayListeners(state, overlay, shape); + + const firePointerdownEvent = (evt: MouseEvent | KeyboardEvent) => { + overlay.fireEvent(new EventObject('pointerdown', { event: evt, state })); + }; InternalEvent.addListener( shape.node, Client.IS_POINTER ? 'pointerdown' : 'mousedown', - (evt: MouseEvent | KeyboardEvent) => { - overlay.fireEvent(new EventObject('pointerdown', { event: evt, state })); - } + firePointerdownEvent ); if (!Client.IS_POINTER && Client.IS_TOUCH) { InternalEvent.addListener( shape.node, 'touchstart', - (evt: MouseEvent | KeyboardEvent) => { - overlay.fireEvent(new EventObject('pointerdown', { event: evt, state })); - } + firePointerdownEvent ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/html/stories/AutoLayout.stories.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/html/stories/AutoLayout.stories.ts
[error] 184-184: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (windows-2022)
- GitHub Check: build
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (2)
packages/html/stories/AutoLayout.stories.ts (2)
27-42: LGTM! Well-organized type imports.The TypeScript types are properly imported and organized, which enhances code clarity and type safety.
108-110: Verify executeLayout function scope.The
executeLayoutfunction is called before it's defined. Consider moving the function definition before its usage or declaring its type upfront to ensure proper scoping and type safety.
|
|
||
| // Installs a handler for clicks on the overlay | ||
| overlay.addListener(InternalEvent.CLICK, (sender, evt2) => { | ||
| overlay.addListener(InternalEvent.CLICK, (_name: string, _funct: Function) => { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Replace 'Function' type with a specific function signature.
The static analysis tool correctly flags the use of the Function type. This type is too broad and can lead to type-safety issues.
- overlay.addListener(InternalEvent.CLICK, (_name: string, _funct: Function) => { + overlay.addListener(InternalEvent.CLICK, (_name: string, _funct: (sender: EventTarget, evt: EventObject) => void) => {
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Comment on lines +190 to +202
| const geo = cell.getGeometry(); | ||
| vertex = graph.insertVertex({ | ||
| parent, | ||
| value: 'World!', | ||
| position: [geo.x, geo.y], | ||
| position: [geo!.x, geo!.y], | ||
| size: [80, 30], | ||
| }); | ||
| addOverlay(v2); | ||
| graph.view.refresh(v2); | ||
| const e1 = graph.insertEdge({ | ||
| addOverlay(vertex); | ||
| graph.view.refresh(); | ||
| graph.insertEdge({ | ||
| parent, | ||
| source: cell, | ||
| target: v2, | ||
| target: vertex, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add proper null checks instead of non-null assertions.
Replace the non-null assertion (!) with proper null checks to handle the case where geometry might be undefined.
- const geo = cell.getGeometry(); - vertex = graph.insertVertex({ - parent, - value: 'World!', - position: [geo!.x, geo!.y], - size: [80, 30], - }); + const geo = cell.getGeometry(); + if (!geo) { + throw new Error('Cell geometry is required'); + } + vertex = graph.insertVertex({ + parent, + value: 'World!', + position: [geo.x, geo.y], + size: [80, 30], + });
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const geo = cell.getGeometry(); | |
| vertex = graph.insertVertex({ | |
| parent, | |
| value: 'World!', | |
| position: [geo.x, geo.y], | |
| position: [geo!.x, geo!.y], | |
| size: [80, 30], | |
| }); | |
| addOverlay(v2); | |
| graph.view.refresh(v2); | |
| const e1 = graph.insertEdge({ | |
| addOverlay(vertex); | |
| graph.view.refresh(); | |
| graph.insertEdge({ | |
| parent, | |
| source: cell, | |
| target: v2, | |
| target: vertex, | |
| const geo = cell.getGeometry(); | |
| if (!geo) { | |
| throw new Error('Cell geometry is required'); | |
| } | |
| vertex = graph.insertVertex({ | |
| parent, | |
| value: 'World!', | |
| position: [geo.x, geo.y], | |
| size: [80, 30], | |
| }); | |
| addOverlay(vertex); | |
| graph.view.refresh(); | |
| graph.insertEdge({ | |
| parent, | |
| source: cell, | |
| target: vertex, |
tbouffard
deleted the
refactor/migrate_story_to_ts_AutoLayout
branch
This was referenced
Mar 18, 2025This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters