fix(canvas): move and split actions has no effect(#1406) #1064
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/canvas/DesignCanvas/src/api/useCanvas.js (1)
359-386: Add validation and error handling to the move operation.The move operation could be more robust with:
- Validation of
parentIdbefore performing operations- Error handling for failed delete/insert operations
- Transaction-like behavior to rollback on failure
move: (operation) => { const { id, parentId, position, referTargetNodeId } = operation + if (!parentId) { + console.warn('Invalid parentId provided for move operation') + return + } const targetNode = getNode(id, true) if (!targetNode) { + console.warn(`Node with id ${id} not found`) return } + try { /** * delete node from parent */ operationTypeMap.delete({ id }) /** * add node to new parent */ operationTypeMap.insert({ parentId, position, referTargetNodeId, newNodeData: targetNode.node }) + } catch (error) { + console.error('Failed to move node:', error) + // TODO: Implement rollback mechanism + } },packages/canvas/container/src/container.js (1)
322-348: Add type checking and input validation.The function could benefit from:
- Type checking for parameters
- Validation of addend value
- JSDoc documentation for parameters
+ /** + * Move a child node within its parent's children array + * @param {Object} parent - The parent node containing the children array + * @param {Object} selected - The node to be moved + * @param {number} addend - The offset to move the node (-1 for up, 1 for down) + */ export const moveChildNode = (parent, selected, addend) => { + if (!parent || typeof parent !== 'object') { + console.warn('Invalid parent node provided') + return + } + if (!selected || typeof selected !== 'object') { + console.warn('Invalid selected node provided') + return + } + if (typeof addend !== 'number' || ![-1, 1].includes(addend)) { + console.warn('Invalid addend value, must be -1 or 1') + return + } const { children: list } = parent || {} if (!list || list.length < 2) { return }packages/canvas/container/src/components/CanvasDivider.vue (1)
Line range hint
132-170: Add documentation to clarify the complex logic.The implementation is correct but could benefit from better documentation explaining:
- The logic for handling existing children
- The position calculation based on index
- The different cases being handled
+ // When splitting a non-empty CanvasCol: + // 1. If first child is not a row, move existing children to first row + // 2. Create a new row as the second row + // 3. Position the split based on the node's index in parent const children = [ { ...extend(true, {}, ROW_SNIPPET),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/canvas/DesignCanvas/src/api/useCanvas.js(1 hunks)packages/canvas/container/src/components/CanvasAction.vue(2 hunks)packages/canvas/container/src/components/CanvasDivider.vue(3 hunks)packages/canvas/container/src/container.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (6)
packages/canvas/container/src/container.js (1)
274-274: LGTM! Fixed typo in function name.The correction from
inserAftertoinsertAfterimproves code clarity.packages/canvas/container/src/components/CanvasAction.vue (2)
198-200: LGTM! Clean implementation of moveUp.The change simplifies the code by using the new
moveChildNodefunction with the correct offset (-1) for upward movement.
202-205: LGTM! Clean implementation of moveDown.The change simplifies the code by using the new
moveChildNodefunction with the correct offset (+1) for downward movement.packages/canvas/container/src/components/CanvasDivider.vue (3)
85-92: LGTM! Clean implementation of handleSplit.The change properly uses
insertNodewithPOSITION.RIGHTfor column insertion, and correctly handles data cloning.
99-106: LGTM! Clean implementation of row insertion.The change properly uses
insertNodewithPOSITION.RIGHTfor row insertion when the schema is a CanvasRow.
114-129: LGTM! Clean implementation of empty CanvasCol splitting.The change correctly uses
insertNodewithPOSITION.INtwice to create two rows in an empty CanvasCol.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: #1406
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes