fix: insert logic and multi node insert issue #1148
Conversation
WalkthroughThis pull request updates the node insertion logic and clipboard handling. In the canvas API, the insertion index now defaults to 0 when a reference node is not found, and the behavior of 'before' and 'default' cases has been adjusted. In the container module, a blank line was removed for formatting, and the clipboard functionality has been enhanced to support multi-selected nodes with new copy and paste logic while removing redundant functions. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/canvas/container/src/keyboard.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/canvas".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/canvas/.eslintrc.cjs » @vue/eslint-config-typescript". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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/container/src/container.js (1)
331-350: Improved insertion logic with flexible targetThe enhanced
insertAfterfunction now accepts atargetparameter, providing more control over insertion. The addition of conditional logic to includereferTargetNodeIdwhen no target is specified improves how nodes are referenced during insertion operations.Consider adding a comment explaining the difference between using the
targetvs. not using it, to help future developers understand the insertion behavior:const insertAfter = ({ parent, node, data, target }) => { if (!data.id) { data.id = utils.guid() } + // When target is provided, insert after that specific target + // When target is not provided, insert after the node specified by node.id const targetParentId = target ? target.id : parent.idpackages/canvas/container/src/keyboard.js (2)
106-116: Added support for multi-node clipboard operationsThe paste handler now checks for multiple selected nodes in localStorage, enhancing the clipboard functionality to handle copying and pasting multiple nodes at once.
Consider adding error handling for the localStorage operations:
- const multiSelectedNodes = JSON.parse(localStorage.getItem('multiSelectedNodes') || '[]') + let multiSelectedNodes = []; + try { + multiSelectedNodes = JSON.parse(localStorage.getItem('multiSelectedNodes') || '[]'); + } catch (error) { + console.error('Failed to parse multiSelectedNodes from localStorage:', error); + }
124-139: Implemented robust copy logic for multiple node selectionThe new
handleCopyEventfunction intelligently handles both single and multiple selection scenarios, storing selected nodes in localStorage when multiple nodes are selected.Consider adding error handling for localStorage operations and a clear variable name for better readability:
- localStorage.setItem('multiSelectedNodes', JSON.stringify([])) + try { + // Clear previous multi-selection state + localStorage.setItem('multiSelectedNodes', JSON.stringify([])) + } catch (error) { + console.error('Failed to clear multiSelectedNodes in localStorage:', error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/canvas/DesignCanvas/src/api/useCanvas.js(2 hunks)packages/canvas/container/src/container.js(1 hunks)packages/canvas/container/src/keyboard.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (5)
packages/canvas/DesignCanvas/src/api/useCanvas.js (3)
320-322: Fixed insertion behavior when reference node is not foundChanged the fallback behavior to insert at the beginning (index 0) when a reference node is not found but a reference ID is provided. This is a better default than appending at the end, as it ensures visibility of newly inserted elements.
327-329: Improved insertion logic for 'before' positionThe use of a ternary operator provides clearer conditional logic: insert at the specified index if reference target exists, otherwise insert at the beginning of the children array.
339-343: Enhanced default insertion case with conditional logicThe updated default case now explicitly handles both scenarios - with or without a reference target ID. This makes the insertion behavior more predictable and consistent across the application.
packages/canvas/container/src/keyboard.js (2)
146-147: Updated clipboard event handling for copy operationsThe function now uses the new
handleCopyEventhandler, which provides better support for multi-selection scenarios.
149-151: Simplified paste event handlingThe paste operation now directly calls
handleClipboardPastewithout additional logic, making the code more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/canvas/container/src/utils.js (1)
32-45: Enhanced parsing logic handles both single and multiple nodes.The improved validation and parsing logic now correctly handles both array and non-array data structures, making the clipboard functionality more robust for multi-node operations.
Consider using optional chaining for cleaner code in the condition check:
- if (Array.isArray(parsedData) && parsedData.every((item) => item && item.componentName)) { + if (Array.isArray(parsedData) && parsedData.every((item) => item?.componentName)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 42-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/canvas/DesignCanvas/src/api/useCanvas.js(2 hunks)packages/canvas/container/src/keyboard.js(2 hunks)packages/canvas/container/src/utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/canvas/DesignCanvas/src/api/useCanvas.js
- packages/canvas/container/src/keyboard.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/canvas/container/src/utils.js
[error] 42-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/canvas/container/src/utils.js (2)
16-29: Function parameter and implementation improved to handle node list strings.The refactoring from accepting a node object to accepting a string representation (
nodeStr) aligns with the PR objective to fix multi-node operations. The validation logic ensures robust input handling.
47-62: Graceful error handling with fallback text component.Instead of returning null on parsing failure, providing a default text component is a more robust approach that prevents potential null reference errors downstream in the application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/canvas/container/src/keyboard.js (1)
108-114: Consider using optional chaining for safer property access.To make your code more robust, consider using optional chaining when accessing nested properties:
- if (node?.componentName && schema?.componentName && allowInsert(getConfigure(schema.componentName), node)) { + if (node?.componentName && schema?.componentName && allowInsert(getConfigure(schema?.componentName), node)) {packages/canvas/container/src/utils.js (2)
40-45: Use optional chaining for safer property access.According to the static analysis tool, you should use optional chaining when accessing properties that might be undefined:
-if (Array.isArray(parsedData) && parsedData.every((item) => item && item.componentName)) { +if (Array.isArray(parsedData) && parsedData.every((item) => item && item?.componentName)) { return parsedData -} else if (!Array.isArray(parsedData) && parsedData.componentName) { +} else if (!Array.isArray(parsedData) && parsedData?.componentName) { // 如果解析结果不是数组，将其转为数组 return [parsedData] }🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
36-49: Validate parsed data more thoroughly for security.Since you're parsing data from the clipboard which can come from external sources, consider adding more thorough validation of the parsed data structure. This could prevent potential security issues if malicious data is pasted.
try { const parsedData = JSON.parse(clipText) // 如果是数组且每个项都有 componentName if (Array.isArray(parsedData) && parsedData.every((item) => item && item.componentName)) { + // Additional validation to ensure the structure is safe + if (parsedData.some(item => !isValidComponentStructure(item))) { + console.warn('Invalid component structure detected in clipboard data') + return getDefaultTextComponent(clipText) + } return parsedData } else if (!Array.isArray(parsedData) && parsedData.componentName) { // 如果解析结果不是数组，将其转为数组 + if (!isValidComponentStructure(parsedData)) { + console.warn('Invalid component structure detected in clipboard data') + return getDefaultTextComponent(clipText) + } return [parsedData] } } catch (error) { // eslint-disable-next-line no-console console.warn('剪贴板数据解析失败，转换为文本组件:', error) } // Helper function to validate component structure +function isValidComponentStructure(component) { + return ( + typeof component === 'object' && + typeof component.componentName === 'string' && + (!component.props || typeof component.props === 'object') && + (!component.children || Array.isArray(component.children)) + ) +} + +function getDefaultTextComponent(text) { + return [{ + componentName: 'Text', + props: { + style: 'display: inline-block;', + text: text, + className: 'component-base-style' + }, + children: [] + }] +}🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/canvas/container/src/container.js(0 hunks)packages/canvas/container/src/keyboard.js(2 hunks)packages/canvas/container/src/utils.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/canvas/container/src/container.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/canvas/container/src/utils.js
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (7)
packages/canvas/container/src/keyboard.js (5)
13-17: Consider organizing imports according to best practices.Third-party libraries should be imported before local files. Consider reordering your imports to follow this convention:
-import { ref } from 'vue' -import { useHistory, useCanvas, getMetaApi, META_APP } from '@opentiny/tiny-engine-meta-register' -import { getCurrent, insertNode, selectNode, POSITION, removeNodeById, allowInsert, getConfigure } from './container' -import { copyObject } from '../../common' -import { getClipboardSchema, setClipboardSchema } from './utils' +import { ref } from 'vue' +import { useHistory, useCanvas, getMetaApi, META_APP } from '@opentiny/tiny-engine-meta-register' +import { copyObject } from '../../common' +import { getCurrent, insertNode, selectNode, POSITION, removeNodeById, allowInsert, getConfigure } from './container' +import { getClipboardSchema, setClipboardSchema } from './utils'
105-115: Improved multi-node paste handling.The refactored logic to handle multiple nodes during paste operations is a good improvement. It now properly checks each node against insertion compatibility rules before adding it to the canvas.
117-122: Copy functionality enhanced for multiple nodes.The new copy handler properly collects all selected nodes and serializes them for clipboard operations. This fixes the multi-node copy issue mentioned in the PR.
130-131: Verify if the schema parameter is needed.In the previous review, it was noted that there might be a parameter inconsistency. The
handleCopyEventfunction doesn't use aschemaparameter, but it's called with one on line 130. Please verify if this is intentional or if the parameter needs to be handled.
126-127:❓ Verification inconclusive
Verify clipboard data handling for multiple nodes.
The clipboard data flow has changed significantly. Ensure that
getClipboardSchemanow properly returns an array that can be handled byhandleClipboardPaste.
🏁 Script executed:
#!/bin/bash # Description: Check the return type of getClipboardSchema implementation rg -A 10 "getClipboardSchema" --type js packages/canvas/container/src/utils.jsLength of output: 190
Action: Verify Multiple-Node Clipboard Handling
The current implementation of
getClipboardSchemasimply returns the result oftranslateStringToSchema, as shown below:export const getClipboardSchema = (event) => translateStringToSchema(event.clipboardData.getData('text/plain'))Please confirm that
translateStringToSchemacorrectly returns an array when the clipboard data corresponds to multiple nodes, ensuring that the output is fully compatible withhandleClipboardPaste. If it only returns a single node object, consider updating it to support multiple nodes.packages/canvas/container/src/utils.js (2)
19-29: Improved clipboard schema handling for multi-node operations.The function now properly handles a string representation of node list instead of a single node object. The validation has been enhanced to check if the input is a non-empty string.
31-63: Enhanced error handling for clipboard data parsing.The improvements to
translateStringToSchemaare excellent:
- Better input validation
- Proper handling of both array and single-object inputs
- Fallback to a default text component when parsing fails
These changes make the clipboard functionality more robust.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/canvas/container/src/keyboard.js (1)
105-115: Good improvement to the clipboard paste logic.The updated
handleClipboardPastefunction now properly handles multiple nodes and usesallowInsertto determine the correct insertion position, which fixes the multi-node paste issue. The empty array check is also improved.One observation: There's no error handling if insertion fails or if the operation encounters an exception. Consider adding try/catch blocks for robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/container/src/keyboard.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/canvas/container/src/keyboard.js (4)
13-15: Reorder imports to follow convention.Third-party imports (like Vue) should come before internal imports following the convention mentioned in previous code reviews.
-import { ref } from 'vue' import { useHistory, useCanvas, getMetaApi, META_APP } from '@opentiny/tiny-engine-meta-register' +import { ref } from 'vue' import { getCurrent, insertNode, selectNode, POSITION, removeNodeById, allowInsert, getConfigure } from './container'
126-134: Verify clipboard schema handling.The changes to use
getClipboardSchemaand the new helper functions look good, but there's a concern from previous reviews about using localStorage for clipboard operations.#!/bin/bash # Check if localStorage is used for clipboard operations and how getClipboardSchema works echo "Checking getClipboardSchema implementation:" rg "function getClipboardSchema|const getClipboardSchema" -A 10 echo "\nChecking for localStorage usage in clipboard operations:" rg "localStorage" -A 3 ./packages/canvas/container/src/This approach might cause issues in the following scenarios:
- When copying something outside the canvas and then pasting in the canvas
- When copying in the canvas, then copying something else outside, and then pasting in the canvas
130-130: Schema parameter usage needs verification.Previous reviews mentioned that line 130 passes a
schemaparameter, but the receiving function doesn't accept it. Check if this has been addressed in the updated implementation.For verification:
#!/bin/bash # Check the definition of handleCopyEvent and if it uses the schema parameter grep -n "handleCopyEvent" ./packages/canvas/container/src/keyboard.js --include="*.js" -r -A 5
117-122:✅ Verification successful
Well-implemented copy functionality for multiple nodes.
The new
handleCopyEventfunction effectively handles copying multiple selected nodes by extracting them frommultiSelectedStates, creating a JSON representation, and setting it as clipboard content.Note that the previous implementation of
setClipboardSchemaaccepted an object, but now it's receiving a string. Ensure that the receiving function has been updated to handle this change.
🏁 Script executed:
#!/bin/bash # Check how setClipboardSchema is defined and used elsewhere echo "Checking setClipboardSchema implementation:" rg "function setClipboardSchema|const setClipboardSchema" -A 3 echo "\nChecking setClipboardSchema usages:" rg "setClipboardSchema\(" -A 1Length of output: 866
Copy Functionality Update Verified – Approved
- The updated
handleCopyEventnow converts the node schemas into a JSON string (viaJSON.stringify) and passes that string tosetClipboardSchema.- Verification shows that in
packages/canvas/container/src/utils.js,setClipboardSchemanow correctly checks that the second argument is a non-empty string.- As a note, there remains one instance where
setClipboardSchemais called with the result ofcopyObject(schema). Please double-check that this invocation is intentional and, if needed, update it to maintain consistency with the new string-based interface.
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: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Refactor