-
Notifications
You must be signed in to change notification settings - Fork 0
feat(core): use built-in react-flow resizer #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
802a561 to
a964261
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates from the custom re-resizable library to React Flow's built-in NodeResizeControl component for handling node resizing. The changes centralize size configuration by moving it from node data config to the plugin manifest level using a new minSize property.
Key Changes:
- Removed
NodeDefaultConfigtype andsizefrom node config, replacing withminSizeinPluginComponentinterface - Added
updateNodefunction to the nodes store andupdateNodeSizeto the useNode hook for programmatic size updates - Migrated all resizable nodes to use React Flow's
NodeResizeControlinstead ofre-resizable
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/types.ts | Removed NodeDefaultConfig type and moved size config to plugin-level minSize property |
| packages/core/src/store/nodesStore.ts | Added updateNode function for updating arbitrary node properties |
| packages/core/src/store/index.ts | Updated to use minSize instead of defaultConfig.size for control panel height |
| packages/core/src/hooks/useNode.ts | Added updateNodeSize function and fixed trailing commas |
| packages/core/src/constants.ts | Extracted portColors constant for reuse across components |
| packages/core/src/components/Node/index.tsx | Replaced re-resizable with NodeResizeControl, moved config modal to use Modal component |
| packages/core/src/components/Modal.tsx | Added outerBackground prop to allow customization of modal backdrop |
| packages/core/src/components/AddNode/index.tsx | Updated to spread minSize into new nodes |
| packages/base-nodes/src/webNoise/*/index.ts | Added minSize configuration to all resizable nodes |
| packages/base-nodes/src/webNoise/*/types.ts | Removed size property from config interfaces |
| packages/base-nodes/src/webNoise/*/defaultConfig.ts | Removed size from default configurations |
| packages/base-nodes/src/webNoise/Sticker/Node.tsx | Replaced re-resizable with NodeResizeControl, moved config to modal |
| packages/base-nodes/src/webNoise/Gauge/* | Updated to pass size dynamically to worker for canvas sizing |
| packages/base-nodes/src/scriptNodes/ScriptNode.tsx | Replaced re-resizable with NodeResizeControl |
| packages/base-nodes/package.json | Removed re-resizable dependency and reordered peerDependencies alphabetically |
Comments suppressed due to low confidence (2)
packages/base-nodes/src/webNoise/Gauge/Gauge.tsx:33
- Unused variable width.
const { data, width = 300, height = 150 } = props;
packages/base-nodes/src/webNoise/Gauge/Gauge.tsx:33
- Unused variable height.
const { data, width = 300, height = 150 } = props;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {ConfigNode && configMode && ( | ||
| <ConfigModal | ||
| onClose={() => setShowConfigMode(false)} | ||
| outerBackground="#333333cc" |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The hard-coded color value #333333cc should be replaced with a theme color to maintain consistency with the application's theming system and allow for dynamic theme changes.
| }, | ||
| targetPosition: Position.Left, | ||
| sourcePosition: Position.Right, | ||
| ...(minSize ? minSize : {}), |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The conditional spread ...(minSize ? minSize : {}) can be simplified to ...(minSize || {}) or just ...minSize since spreading undefined/null is safe in JavaScript and has no effect.
| flex-direction: column; | ||
| `; | ||
|
|
||
| export const ConfigModal = styled(Modal)<any>` |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using <any> as a type parameter defeats the purpose of TypeScript's type checking. Consider using the proper ModalProps interface or extending it with additional props if needed.
| height: 100%; | ||
| width: 100%; | ||
| min-height: 5rem; | ||
| min-width: 30rem; |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The min-width: 30rem (480px) is larger than the minWidth={500} set in ScriptNode's NodeResizeControl. While close, these values should match to avoid potential layout inconsistencies. Consider making them consistent, e.g., both 500px or both 30rem (depending on the desired unit).
| min-width: 30rem; | |
| min-width: 500px; |
|
|
||
| const Gauge = ({ node: props, audioNode }: GaugeProps) => { | ||
| const { data } = props; | ||
| const { data, width = 300, height = 150 } = props; |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width and height are destructured from props but are never used in the component. They should be passed to the Scope component so that the canvas can be properly sized based on the node's dimensions. Consider passing them as size={{ width, height }} to the Scope component on line 50.
| }; | ||
| worker.postMessage(message); | ||
| }, [config, worker]); | ||
| }, [config, size, canvasRef, worker]); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size prop is passed to the useEffect dependency array but is never used in the component since Scope receives it but the size is actually calculated from canvasRef.current.getBoundingClientRect(). The dependency on size is unnecessary and could cause extra re-renders. Consider removing it from the dependency array or using it directly if it's meant to override the calculated size.
| }, [config, size, canvasRef, worker]); | |
| }, [config, canvasRef, worker]); |
| minWidth={100} | ||
| minHeight={100} |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minWidth and minHeight values (100x100) don't match the minSize defined in the plugin configuration (240x140). This inconsistency could lead to confusion and unexpected behavior. The values should match to ensure consistent sizing constraints across the application.
| minWidth={500} | ||
| minHeight={300} |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minWidth and minHeight values (500x300) don't match the minSize defined in the plugin configuration (200x100). This inconsistency could lead to confusion and unexpected behavior. The values should match to ensure consistent sizing constraints across the application.
| minWidth={500} | |
| minHeight={300} | |
| minWidth={200} | |
| minHeight={100} |
| NodeResizeControl, | ||
| Position, | ||
| } from "@xyflow/react"; | ||
| import { DRAG_HANDLE_CLASS, portColors, PortType } from "../../constants"; |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import PortType.
| <TitleBarWrapper> | ||
| <StickerWrapper | ||
| onDoubleClick={() => setEditMode(true)} | ||
| style={editMode ? { display: "none" } : {}} |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use of variable 'editMode' always evaluates to false.
No description provided.