refactor: fix most problems in the Wires story #893
Conversation
WalkthroughThis change removes the old JavaScript-based Wires story and introduces a new, TypeScript-based Storybook story for demonstrating wire-style connections in a graph visualization. It also updates the Wires documentation to correct a variable name, aligning it with code conventions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Storybook
participant GraphContainer
participant MyCustomGraph
participant Plugins
participant UIControls
User->>Storybook: Select "Connections/Wires" story
Storybook->>GraphContainer: Render container with grid background
GraphContainer->>MyCustomGraph: Initialize custom graph instance
MyCustomGraph->>Plugins: Register plugins (editor, tooltip, selection, connection, etc.)
MyCustomGraph->>MyCustomGraph: Register custom edge style and resistor shape
MyCustomGraph->>MyCustomGraph: Configure styles for vertices and edges
MyCustomGraph->>MyCustomGraph: Insert sample vertices and edges
MyCustomGraph->>UIControls: Add zoom, undo/redo, delete, wire mode, grid toggle
User->>UIControls: Interact (e.g., zoom, toggle wire mode)
UIControls->>MyCustomGraph: Update graph behavior or appearance accordingly
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. ✨ 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 (
|
The labels are now at the right position. Restore the original background specific to this story (dot instead of grid) The terminal points of the R1 shape displays when hovering a edge in construction. Let switch to a dark theme by using the Storybook controls. Add a documentation page taking the technical explanations from the mxGraph example and that was previously hidden. Also display a description at the top of the Graph to help understand what the story is doing. Also migrate it to TypeScript. This facilitates maintenance and enables errors to be detected earlier. Some refactoring - no longer override PanningHandler.isPopupTrigger as it does not exist (same in mxGraph). This is a method of InternalEvent - add nullish checks and simplify conditions
eca3704 to
c86409f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/html/stories/Wires.stories.ts (3)
256-286: Custom highlighting implementation needs improvement.While the highlighting logic works, directly modifying the cell state's style could cause issues if multiple highlights are active simultaneously. Consider using a separate highlight shape overlay instead of modifying the original style.
612-714: Complex edge routing logic - consider adding more inline documentation.The WireConnector function implements sophisticated routing logic with waypoint handling and direction changes. Consider adding more inline comments to explain the routing algorithm, especially around lines 678-691 where waypoints are processed.
863-864: Document the special use of portConstraint.The comment indicates
portConstraintis being used for a different purpose than its original intent. This should be properly documented or consider using a dedicated property to avoid confusion.- // @ts-expect-error - portConstraint not used as usual, we should use a dedicated routing property - portConstraint: 'horizontal', + // Use a dedicated routing property instead of repurposing portConstraint + routingDirection: 'horizontal',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/html/stories/Wires.mdx(1 hunks)packages/html/stories/Wires.stories.js(0 hunks)packages/html/stories/Wires.stories.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/html/stories/Wires.stories.js
🧰 Additional context used
🧠 Learnings (3)
📓 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#740
File: packages/core/src/util/requestUtils.ts:79-106
Timestamp: 2025-03-31T08:32:17.055Z
Learning: PR #740 is focused on reorganizing code into namespaces (guiUtils and requestUtils) without changing any implementations. Implementation improvements should be suggested for future PRs.
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#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#720
File: packages/html/stories/Wrapping.stories.ts:37-37
Timestamp: 2025-03-19T06:56:13.709Z
Learning: In Storybook stories for maxGraph, arguments are typed as strings (Record<string, string>) even when they represent boolean values in the argTypes definition. Boolean conversions like Boolean(args.someFlag) are used to convert string representations to actual boolean values.
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#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/html/stories/Wires.mdx (1)
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/html/stories/Wires.stories.ts (6)
Learnt from: tbouffard
PR: #720
File: packages/html/stories/Wrapping.stories.ts:37-37
Timestamp: 2025-03-19T06:56:13.709Z
Learning: In Storybook stories for maxGraph, arguments are typed as strings (Record<string, string>) even when they represent boolean values in the argTypes definition. Boolean conversions like Boolean(args.someFlag) are used to convert string representations to actual boolean values.
Learnt from: tbouffard
PR: #791
File: packages/ts-example/vite.config.js:30-30
Timestamp: 2025-04-29T13:25:31.494Z
Learning: In the maxGraph project, each example package (ts-example, ts-example-selected-features, ts-example-without-defaults) implements different use cases with varying features, resulting in different application sizes. Therefore, each package has its own specific chunkSizeWarningLimit value in its vite.config.js file, calibrated to its expected bundle size.
Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.
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: #598
File: packages/website/docs/manual/getting-started.md:70-70
Timestamp: 2024-12-15T18:19:56.236Z
Learning: In code examples within the documentation, such as in packages/website/docs/manual/getting-started.md, we assume that the graph-container element exists and is an HTMLElement, and we avoid adding error handling for its initialization to keep the code simple.
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.
🔇 Additional comments (6)
packages/html/stories/Wires.mdx (1)
20-20: LGTM! Documentation correction aligns with code conventions.The capitalization of
Statecorrectly reflects that this is referencing a class/type rather than an instance variable.packages/html/stories/Wires.stories.ts (5)
163-185: Well-structured tooltip generation with proper null handling.The recursive tooltip construction provides comprehensive information about connected cells and their relationships. Good defensive programming with the null checks.
309-310: Good integration with checkbox control.The wire mode implementation correctly checks the checkbox state to determine connection behavior. This provides a clean separation between UI control and graph behavior.
412-420: Good defensive programming with null check.The added null check for
terminalprevents the TypeError mentioned in the comment. This is an improvement over the original mxGraph implementation.
470-477: Consider uncommenting the grid snapping code.The commented-out grid snapping code might be useful for aligning connection points. Was this intentionally disabled?
988-1015: UI controls implementation looks good.The checkbox controls for wire mode and grid toggle are properly implemented with appropriate event handlers. The grid toggle correctly switches between dot pattern and plain background based on dark mode.
Also use insert cells method with a single parameter
|
ℹ️ All fixable issues detected by SonarQube have been managed. |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/html/stories/Wires.stories.ts (7)
110-110: Remove unusedlabelparameterThe
labelparameter is destructured but never used in the function.-const Template = ({ label, ...args }: Record<string, string>) => { +const Template = (args: Record<string, string>) => {
133-134: Consider scoping style changes to avoid side effectsModifying
StyleDefaultsConfig.shadowColorglobally could affect other stories if they run concurrently. Consider applying this style directly to the graph instance instead.
236-253: Simplify conditional class assignmentThe conditional assignment of
MyCustomCellEditorHandlerbased on darkMode can be simplified.- let MyCustomCellEditorHandler; - - if (darkMode) { - container.style.backgroundColor = 'black'; - - // White in-place editor text color - MyCustomCellEditorHandler = class extends CellEditorHandler { - override startEditing(cell: Cell, trigger: MouseEvent | null) { - super.startEditing(cell, trigger); - - if (this.textarea != null) { - this.textarea.style.color = '#FFFFFF'; - } - } - }; - } else { - MyCustomCellEditorHandler = CellEditorHandler; - } + if (darkMode) { + container.style.backgroundColor = 'black'; + } + + const MyCustomCellEditorHandler = darkMode + ? class extends CellEditorHandler { + override startEditing(cell: Cell, trigger: MouseEvent | null) { + super.startEditing(cell, trigger); + if (this.textarea != null) { + this.textarea.style.color = '#FFFFFF'; + } + } + } + : CellEditorHandler;
516-519: Address TODO: Implement edge terminal checkThe TODO comment indicates missing logic to check if the terminal is an edge before setting the terminal point.
Would you like me to implement the edge terminal check or create an issue to track this task?
670-684: Remove commented debug codeThis block of commented code appears to be debug/experimental code from the original mxGraph example and should be removed.
319-319: Consider hoisting checkbox declaration to avoid forward reference
checkboxWireModeis referenced in line 319 but declared much later in line 1031. While this works due to JavaScript hoisting in the compiled code, it's better practice to declare it before use.Move the checkbox declarations closer to the class definitions or pass them as parameters to avoid forward references.
Also applies to: 1031-1031
766-766: Document the purpose of deleting endArrowThe
delete style.endArrowoperation is unusual. Consider adding a comment explaining why the default end arrow is removed.+ // Remove default arrow to use wire-style connections delete style.endArrow;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/html/stories/Wires.stories.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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#740
File: packages/core/src/util/requestUtils.ts:79-106
Timestamp: 2025-03-31T08:32:17.055Z
Learning: PR #740 is focused on reorganizing code into namespaces (guiUtils and requestUtils) without changing any implementations. Implementation improvements should be suggested for future PRs.
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#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#720
File: packages/html/stories/Wrapping.stories.ts:37-37
Timestamp: 2025-03-19T06:56:13.709Z
Learning: In Storybook stories for maxGraph, arguments are typed as strings (Record<string, string>) even when they represent boolean values in the argTypes definition. Boolean conversions like Boolean(args.someFlag) are used to convert string representations to actual boolean values.
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#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/html/stories/Wires.stories.ts (6)
Learnt from: tbouffard
PR: #720
File: packages/html/stories/Wrapping.stories.ts:37-37
Timestamp: 2025-03-19T06:56:13.709Z
Learning: In Storybook stories for maxGraph, arguments are typed as strings (Record<string, string>) even when they represent boolean values in the argTypes definition. Boolean conversions like Boolean(args.someFlag) are used to convert string representations to actual boolean values.
Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.
Learnt from: tbouffard
PR: #791
File: packages/ts-example/vite.config.js:30-30
Timestamp: 2025-04-29T13:25:31.494Z
Learning: In the maxGraph project, each example package (ts-example, ts-example-selected-features, ts-example-without-defaults) implements different use cases with varying features, resulting in different application sizes. Therefore, each package has its own specific chunkSizeWarningLimit value in its vite.config.js file, calibrated to its expected bundle size.
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: #598
File: packages/website/docs/manual/getting-started.md:70-70
Timestamp: 2024-12-15T18:19:56.236Z
Learning: In code examples within the documentation, such as in packages/website/docs/manual/getting-started.md, we assume that the graph-container element exists and is an HTMLElement, and we avoid adding error handling for its initialization to keep the code simple.
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.
| if (pt!.y !== hint!.y) { | ||
| pt!.y = hint!.y; |
There was a problem hiding this comment.
Add null checks to prevent potential runtime errors
Multiple non-null assertions could cause runtime errors if pt or hint are null.
- if (pt!.y !== hint!.y) {
- pt!.y = hint!.y;
- result.push(pt!.clone());
+ if (pt && hint && pt.y !== hint.y) {
+ pt.y = hint.y;
+ result.push(pt.clone());
}Apply similar null checks to other locations using non-null assertions.
Also applies to: 696-697, 716-720
🤖 Prompt for AI Agents
In packages/html/stories/Wires.stories.ts around lines 691-692, 696-697, and
716-720, the code uses non-null assertions on variables pt and hint without
checking if they are null, which can cause runtime errors. Add explicit null
checks before accessing properties of pt and hint to ensure they are not null or
undefined. Apply similar null checks consistently in all mentioned locations to
prevent potential runtime exceptions.
The labels are now at the right position.
Restore the original background specific to this story (dot instead of grid)
The terminal points of the R1 shape displays when hovering a edge in construction.
Let switch to a dark theme by using the Storybook controls.
Add a documentation page taking the technical explanations from the mxGraph example and that was previously hidden.
Also display a description at the top of the Graph to help understand what the story is doing.
Also migrate it to TypeScript. This facilitates maintenance and enables errors to be detected earlier.
Some refactoring
Screenshots
dark mode
dot point displayed to connect to R1
maxgraph_pr-893_dot_on_R1_terminals.mp4
Limitations
Note
This problem will be managed in #894
Edge preview from/to edge has incorrect terminal points 👇🏿
maxgraph_pr-893_edge_wrong_points_while_creating.mp4
This was working in mxGraph 4.2.2. See https://jgraph.github.io/mxgraph/javascript/examples/wires.html
mxGraph_wires_example_working_edge_preview.mp4
Summary by CodeRabbit
New Features
Documentation
Refactor