feat: enhance canvas foundation capabilities #1055
Conversation
WalkthroughThis pull request introduces comprehensive changes to the canvas and materials management system, focusing on multi-selection functionality, node insertion, and component management. The modifications span multiple files across the canvas and materials packages, enhancing node interaction, keyboard handling, and component filtering. Key improvements include robust multi-node selection, expanded insertion logic for nodes, and more flexible component management through new utility 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/CanvasContainer.vueOops! 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. 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 (
|
github-actions
bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
packages/canvas/container/src/components/CanvasAction.vue (3)
184-187: Add validation for multiStateLength prop.While the prop is correctly typed, adding validation would prevent potential issues with negative values.
multiStateLength: { type: Number, default: () => 0, + validator: (value) => value >= 0 },
243-256: Consider renaming showQuickAction for clarity.The computed property
showQuickActioncould be more descriptive, such asshowQuickActionForSingleNode, to better reflect its purpose.
Line range hint
1-735: Consider splitting component responsibilities.The component currently handles multiple concerns (selection, resize, quick actions, style calculations). Consider:
- Extracting the style calculation logic into a composable for better reusability
- Splitting the component into smaller, focused components (e.g.,
CanvasResize,CanvasQuickActions)This would improve maintainability and make the code more testable.
packages/plugins/materials/src/composable/useMaterial.js (1)
486-495: Consider enhancing error handling and type validation.While the function implementation is clean and efficient, consider adding:
- Type validation for
groupName- Explicit error handling for invalid group names
const getComponents = (components, groupName) => { if (!Array.isArray(components)) return [] + if (typeof groupName !== 'string') return [] + if (!groupName.trim()) return components return components.filter((item) => item.group === groupName) }packages/plugins/materials/src/meta/component/src/Main.vue (1)
91-98: Consider memoizing the initialization result.The
initComponentsfunction is well-implemented but could benefit from memoization to prevent unnecessary recalculations.+import { computed } from 'vue' -const initComponents = () => { +const initComponents = computed(() => { const groupName = panelState.materialGroup if (groupName) { return getComponents(components, groupName) } return components -} +})Then update the state initialization:
-components: initComponents(), +components: initComponents.value,packages/canvas/DesignCanvas/src/api/useCanvas.js (3)
264-271: Improve error handling for missing parent node.Instead of silently returning an empty object when the parent node is missing, consider throwing an error to help with debugging.
- if (!parentNode) { - return {} - } + if (!parentNode) { + throw new Error('Parent node not found') + }
291-308: Consider using a Map for position handling.The switch statement could be replaced with a Map for better maintainability and performance.
- switch (position) { - case 'before': - parentNode.children.unshift(newNodeData) - break - case 'out': - if (childrenNode) { - newNodeData.children = Array.isArray(childrenNode) ? [...childrenNode] : [childrenNode] - parentNode.children.splice(index, 1, newNodeData) - } - break - case 'bottom': - parentNode.children.splice(index + 1, 0, newNodeData) - break - default: - parentNode.children.push(newNodeData) - break - } + const positionHandlers = new Map([ + ['before', () => parentNode.children.unshift(newNodeData)], + ['out', () => { + if (childrenNode) { + newNodeData.children = Array.isArray(childrenNode) ? [...childrenNode] : [childrenNode] + parentNode.children.splice(index, 1, newNodeData) + } + }], + ['bottom', () => parentNode.children.splice(index + 1, 0, newNodeData)], + ['default', () => parentNode.children.push(newNodeData)] + ]) + const handler = positionHandlers.get(position) || positionHandlers.get('default') + handler()
351-352: Simplify array check using optional chaining.The array check can be simplified while maintaining safety.
- if (Array.isArray(nodeItem?.children) && nodeItem?.children.length) { + if (nodeItem?.children?.length) {packages/canvas/container/src/CanvasContainer.vue (3)
35-42: Ensure proper cleanup of theinsertContainerpanelThe insertion panel for adding a parent container is conditionally rendered based on
insertContainer. Ensure that this state is correctly managed to prevent the panel from remaining open unintentionally.Consider adding a watcher or method to handle the closure of the panel under specific conditions, such as when the user clicks outside the panel or completes the insertion.
213-214: Avoid unnecessary reassignment ofinsertContainerSetting
insertContainer.value = falseon everymousedownevent may interfere with the intended functionality.Review whether this reassignment is necessary here. If the goal is to close the insert container panel when clicking elsewhere, consider adding condition checks or handling this logic within a dedicated method.
229-229: Consistent handling ofinsertContainerin themouseupeventSimilar to the
mousedownevent, ensure that settinginsertContainer.value = falsealigns with the intended user experience.Confirm that this does not prematurely close the insert container panel during user interactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/canvas/DesignCanvas/src/api/useCanvas.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(11 hunks)packages/canvas/container/src/components/CanvasAction.vue(6 hunks)packages/canvas/container/src/components/CanvasMenu.vue(1 hunks)packages/canvas/container/src/container.js(6 hunks)packages/canvas/container/src/keyboard.js(6 hunks)packages/plugins/materials/src/composable/useMaterial.js(2 hunks)packages/plugins/materials/src/meta/component/src/Main.vue(2 hunks)packages/plugins/materials/src/meta/layout/src/Main.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (34)
packages/canvas/container/src/components/CanvasAction.vue (3)
12-12: LGTM! Improved condition for quick action visibility.The change from
resizetoshowQuickActionprovides better semantics and correctly handles multi-selection scenarios.
31-66: LGTM! Enhanced resize handle visibility logic.The resize handles are now correctly shown only for single node selection through the
showActioncomputed property.
420-423: LGTM! Added defensive null check.The null check for
docprevents potential runtime errors and follows defensive programming practices.packages/plugins/materials/src/meta/layout/src/Main.vue (2)
44-46: LGTM! Clean prop definition.The
groupNameprop is well-defined with appropriate type and default value.
55-55: LGTM! Proper state initialization.The
materialGroupis correctly initialized with the prop value in the panel state.packages/plugins/materials/src/composable/useMaterial.js (1)
525-526: LGTM! Clean export addition.The
getComponentsfunction is properly exported alongside other utility functions.packages/plugins/materials/src/meta/component/src/Main.vue (2)
57-57: LGTM! Clean function import.The
getComponentsfunction is properly destructured fromuseMaterial.
101-101: LGTM! Proper state initialization.The state is correctly initialized using the
initComponentsfunction.packages/canvas/DesignCanvas/src/api/useCanvas.js (1)
276-283: LGTM! Good error handling for reference node.The error handling for missing reference nodes is well implemented with a descriptive error message.
packages/canvas/container/src/container.js (6)
33-34: LGTM! Position constants updated correctly.The addition of the
OUTposition constant aligns with the new parent container functionality.
126-147: LGTM! Well-implemented state extraction.The
getMultiStatefunction properly extracts and validates node state with appropriate null checks.
170-181: LGTM! Efficient multi-selection handling.The
handleMultiStatefunction efficiently manages selection toggling using array methods.
380-393: LGTM! Clean implementation of container insertion.The
insertContainerfunction follows the same pattern as other insertion functions and properly handles ID generation.
816-818: LGTM! Proper integration of container insertion.The
insertNodefunction correctly handles the new 'out' position case.
149-168: Consider adding type validation for multiSelectedStates.The function assumes multiSelectedStates is a ref object with a value property.
packages/canvas/container/src/components/CanvasMenu.vue (1)
109-110: LGTM! Menu item properly integrated.The new container menu item is correctly added with appropriate code and value properties, maintaining consistency with existing menu items.
packages/canvas/container/src/CanvasContainer.vue (8)
99-101: InitializecontainerPanelandinsertContainercorrectlyThe references
containerPanelandinsertContainerare added to manage the parent container insertion panel. Verify that these are properly initialized and thatinsertContaineris reactive.The initialization looks correct. Ensure that any components or methods relying on these references handle them appropriately.
102-103: Confirm computed propertymultiStateLengthThe computed property
multiStateLengthcalculates the length ofmultiSelectedStates.This is a good approach to reactively track the number of selected states.
Line range hint
104-116: Verify the modifiedsetCurrentNodefunction for multi-selectionThe
setCurrentNodefunction now accepts adocparameter and includes logic for handling multi-selection withgetMultiStateandsetMultiState.Ensure that
getMultiStateandsetMultiStatefunctions are correctly implemented and that they effectively manage the multi-selection state.If these functions are defined in another file or context, confirm their integration here.
256-256: Register hotkey events consistentlyThe
registerHotkeyEvent(doc)call registers keyboard events within the iframe's document.Good implementation for capturing keyboard events in the iframe context.
279-279: IncludeinsertContainerin outside click handlingIn the
mousedownevent listener on thewindow, the check now includescontainerPanel.This ensures that clicks outside the
insertContainerpanel are handled correctly.
292-295: Implement the insertion logic for the 'out' positionWhen
position === 'out', theinsertContainerstate is updated accordingly.This enables the right-click functionality to add a parent container, aligning with the PR objectives.
303-311: WatchmultiStateLengthto manage the properties panelThe watcher on
multiStateLengthclears the properties panel when more than one state is selected.This is a suitable approach to prevent conflicting property displays during multi-selection.
2-14: Review the usage ofselectStatewithin thev-forloopThe
selectStateprop in<canvas-action>is conditionally set usingmultiStateLength > 1 ? multiState : selectState. This logic could lead to confusion or unintended behavior whenmultiStateLengthis exactly 1.Consider revising the condition to accurately reflect when multiple selections are involved. For instance:
- :selectState="multiStateLength > 1 ? multiState : selectState" + :selectState="multiStateLength >= 1 ? multiState : selectState"Alternatively, confirm that when
multiStateLengthis 1,selectStateshould indeed bemultiState.packages/canvas/container/src/keyboard.js (10)
13-17: Import and define new reactive states for multi-selectionThe imports and reactive references for
multiSelectedStatesandisCtrlPressedare added.This sets up the necessary state management for multi-selection and control key detection.
19-28: Define key codes forKEY_SandKEY_CTRLThe constants
KEY_SandKEY_CTRLare introduced for handling save shortcuts and control key presses.Defining key codes as constants improves code readability and maintainability.
48-53: RefactorhandlerDeleteto support multi-node deletionThe
handlerDeletefunction now iterates overmultiSelectedStatesto delete multiple nodes.This enhancement aligns with the multi-selection functionality, allowing users to delete multiple selected nodes simultaneously.
78-83: Implement save functionality withhandleSaveEventThe
handleSaveEventfunction is added to trigger the save action whenCtrl+Sis pressed.This provides a convenient shortcut for users to save their work, enhancing the user experience.
84-96: UpdatehandlerCtrlto includeKEY_Sfor savingThe control key handler now includes a case for
KEY_Sto invoke the save functionality.This integration ensures that pressing
Ctrl+Swill trigger the save action as expected.
116-130: Handle pasting multiple nodes from the clipboardThe new
handleMultiNodesPastefunction manages the pasting of multiple nodes when multiple nodes are selected.This addition enhances the clipboard functionality to work seamlessly with multi-selection.
157-162: Manage control key state withhandleKeyupEventThe
handleKeyupEventfunction updates theisCtrlPressedstate upon key release.This ensures accurate detection of the control key's state, which is important for multi-selection handling.
163-165: Remove hotkey events appropriatelyThe
removeHotkeyEventfunction now also removes thekeyupevent listener.Cleaning up event listeners helps prevent memory leaks and unintended behavior.
171-175: Register hotkey events includingkeyuplistenerThe
registerHotkeyEventfunction adds thekeyupevent listener to manage control key state.This ensures all necessary events are captured for the enhanced keyboard functionality.
181-181: Export new state variablesThe export statement now includes
multiSelectedStatesandisCtrlPressed.Exporting these variables allows other components to access and reactively use the multi-selection state and control key status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/canvas/container/src/keyboard.js (5)
28-30: Initialize the multiSelectedStates ref with an empty array.For better code clarity and to prevent potential undefined errors, initialize the ref:
-const multiSelectedStates = ref([]) +const multiSelectedStates = ref([]) // Initialize with empty array for multi-node selection
46-51: Add error handling for node removal operations.The current implementation might fail silently if node removal encounters an error. Consider adding try-catch blocks:
function handlerDelete() { - multiSelectedStates.value.forEach(({ id: schemaId }) => { - removeNodeById(schemaId) - }) + multiSelectedStates.value.forEach(({ id: schemaId }) => { + try { + removeNodeById(schemaId) + } catch (error) { + console.error(`Failed to remove node ${schemaId}:`, error) + } + }) multiSelectedStates.value.length = 0 }
76-93: Add user feedback for save operations.Consider adding visual feedback to inform users when the save operation succeeds or fails:
const handleSaveEvent = (event) => { const { openCommon } = getMetaApi(META_APP.Save) event.preventDefault() - openCommon() + try { + openCommon() + // Add toast/notification for successful save + } catch (error) { + console.error('Save failed:', error) + // Add toast/notification for failed save + } }
113-125: Optimize memory usage for large multi-node operations.The current implementation creates deep copies of all selected nodes, which could be memory-intensive for large selections. Consider:
- Adding a size limit for multi-selection
- Using a more memory-efficient copying mechanism
const handleMultiNodesPaste = (node, schema, parent) => { + const MAX_SELECTION = 50 // Prevent excessive memory usage + if (multiSelectedStates.value.length > MAX_SELECTION) { + console.warn(`Selection limit (${MAX_SELECTION}) exceeded. Some nodes will be skipped.`) + return + } if (multiSelectedStates.value.length === 1) { handleClipboardPaste(node, schema, parent) return } const selectedStates = multiSelectedStates.value.map(({ schema, parent }) => { return { node: copyObject(schema), schema: toRaw(schema), parent: toRaw(parent) } }) selectedStates.forEach(({ node, schema, parent }) => { handleClipboardPaste(node, schema, parent) }) }
154-168: Add JSDoc comments for the event registration functions.The rename from 'hostkey' to 'hotkey' is good, but the functions would benefit from clear documentation:
+/** + * Removes keyboard and clipboard event listeners from the specified DOM element. + * @param {HTMLElement} dom - The DOM element to remove events from + */ const removeHotkeyEvent = (dom) => { dom.removeEventListener('keydown', keyboardHandler) dom.removeEventListener('copy', handlerClipboardEvent) dom.removeEventListener('cut', handlerClipboardEvent) dom.removeEventListener('paste', handlerClipboardEvent) } +/** + * Registers keyboard and clipboard event listeners on the specified DOM element. + * @param {HTMLElement} dom - The DOM element to register events on + */ const registerHotkeyEvent = (dom) => { removeHotkeyEvent(dom) // ... rest of the function }packages/canvas/container/src/CanvasContainer.vue (3)
2-14: Optimize rendering performance and key uniqueness.The current implementation might have performance issues with large selections and potential key conflicts:
- Consider adding virtual scrolling for large selections
- Use a more unique key combining id with parent id:
- <div v-for="multiState in multiSelectedStates" :key="multiState.id"> + <div v-for="multiState in multiSelectedStates" :key="`${multiState.id}-${multiState.parent?.id}`">
303-311: Consider combining related watchers for better maintainability.The current implementation adds a separate watcher for multi-selection. Consider combining related watchers:
-watch( - () => multiStateLength.value, - (newVal) => { - if (newVal > 1) { - // 清空属性面板 - selectNode(null) - } - } -) +watch( + [() => multiStateLength.value, () => selectState.value], + ([length, state]) => { + if (length > 1) { + // Clear property panel for multi-selection + selectNode(null) + } else if (state) { + // Handle single selection + // Add your single selection logic here + } + } +)
292-295: Add type safety for position values.The current implementation uses string literals without type safety. Consider using an enum or constants:
+const INSERT_POSITIONS = { + OUT: 'out', + IN: 'in', + // Add other valid positions +} as const + const insertComponent = (position) => { - if (position === 'out') { + if (position === INSERT_POSITIONS.OUT) { insertContainer.value = position return } insertPosition.value = position }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/container/src/CanvasContainer.vue(11 hunks)packages/canvas/container/src/keyboard.js(7 hunks)
🔇 Additional comments (2)
packages/canvas/container/src/keyboard.js (1)
13-17: LGTM! Import statements are well-organized.The imports are appropriate for the multi-selection and keyboard shortcut functionality being added.
packages/canvas/container/src/CanvasContainer.vue (1)
203-211: LGTM! Multi-selection trigger implementation is correct.The implementation properly uses
event.ctrlKeyfor multi-selection, addressing the concerns from the previous review.
SonyLeo
changed the title
|
|
SonyLeo
force-pushed
the
feat/canvasAbility
branch
from
e267d5e to
b96bb99
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/canvas/container/src/keyboard.js (1)
76-93:⚠️ Potential issueAdd platform-specific keyboard shortcut support.
The save shortcut needs to support both Windows (Ctrl+S) and macOS (Command+S).
Apply this diff to add cross-platform support:
const handleSaveEvent = (event) => { const { openCommon } = getMetaApi(META_APP.Save) event.preventDefault() openCommon() } const handlerCtrl = (event) => { + const isMac = /Mac|iPod|iPhone|iPad/.test(navigator.platform) + if (!(isMac ? event.metaKey : event.ctrlKey)) { + return + } const keyCode = event.keyCode switch (keyCode) { case KEY_Y: useHistory().forward() break case KEY_Z: useHistory().back() break case KEY_S: handleSaveEvent(event) break default: break } }
🧹 Nitpick comments (12)
packages/canvas/container/src/keyboard.js (2)
46-52: Add error handling for node deletion.While the implementation works, it could benefit from error handling to gracefully handle cases where node deletion fails.
Consider this improved implementation:
function handlerDelete() { + try { multiSelectedStates.value.forEach(({ id: schemaId }) => { + if (!schemaId) { + console.warn('Skipping deletion of node with undefined id') + return + } removeNodeById(schemaId) }) multiSelectedStates.value = [] + } catch (error) { + console.error('Error deleting nodes:', error) + } }
113-126: Enhance multi-node paste handler with validation and error handling.The implementation works but could be more robust with additional validation and error handling.
Consider this improved implementation:
const handleMultiNodesPaste = (node, schema, parent) => { + if (!node || !schema || !parent) { + console.warn('Invalid arguments for paste operation') + return + } + if (multiSelectedStates.value.length === 1) { handleClipboardPaste(node, schema, parent) return } + try { const selectedStates = multiSelectedStates.value.map(({ schema, parent }) => { + if (!schema || !parent) { + console.warn('Skipping invalid state in multi-paste') + return null + } return { node: copyObject(schema), schema: toRaw(schema), parent: toRaw(parent) } - }) + }).filter(Boolean) selectedStates.forEach(({ node, schema, parent }) => { handleClipboardPaste(node, schema, parent) }) + } catch (error) { + console.error('Error during multi-node paste:', error) + } }packages/canvas/container/src/CanvasContainer.vue (3)
214-225: Enhance multi-selection with platform-specific support.While the implementation works, it could be more robust with platform-specific checks and better error handling.
Consider this improved implementation:
if (element) { + const isMac = /Mac|iPod|iPhone|iPad/.test(navigator.platform) const selectedState = getMultiState(element, doc) - if (event.ctrlKey && event.button === 0) { + if ((isMac ? event.metaKey : event.ctrlKey) && event.button === 0) { + try { handleMultiState(multiSelectedStates, selectedState) + } catch (error) { + console.error('Error handling multi-selection:', error) + } return } }
313-321: Add debouncing to multi-selection watcher.The watcher could benefit from debouncing to prevent rapid successive updates when selecting multiple nodes quickly.
Consider this improved implementation:
+import { debounce } from 'lodash-es' + +const debouncedSelectNode = debounce((value) => { + if (value === null) return + selectNode(value) +}, 150) + watch( () => multiStateLength.value, (newVal) => { if (newVal > 1) { - selectNode(null) + debouncedSelectNode(null) } } )
35-43: Enhance accessibility for the container panel.The container panel should include ARIA attributes and keyboard navigation support.
Consider these accessibility improvements:
<div v-if="insertContainer" ref="containerPanel" - class="insert-panel"> + class="insert-panel" + role="dialog" + aria-label="Add Parent Container" + @keydown.esc="insertContainer = false"> <component :is="materialsPanel" :shortcut="insertContainer" groupName="layout" + tabindex="0" + aria-expanded="true" @close="insertContainer = false" ></component> </div>packages/canvas/DesignCanvas/src/api/useCanvas.js (1)
294-357: Enhance node insertion with additional validation and documentation.While the implementation is well-structured, it could benefit from more thorough validation and documentation.
Consider these improvements:
insert: (operation) => { const { parentId, newNodeData, position, referTargetNodeId } = operation + // Validate operation parameters + if (!newNodeData || typeof newNodeData !== 'object') { + throw new Error('Invalid newNodeData: must be a valid object') + } + const parentNode = getNode(parentId) || pageState.pageSchema // 1. 确认是否存在 ParentNode if (!parentNode) { - return {} + throw new Error(`Parent node with ID ${parentId} not found`) } parentNode.children = parentNode.children || [] // 2. 确保 newNodeData 有唯一 ID, 如果没有，则生成新 ID if (!newNodeData.id) { newNodeData.id = utils.guid() } // 3. 查找参考节点 let referenceNode = null if (referTargetNodeId) { referenceNode = getNode(referTargetNodeId) if (!referenceNode) { throw new Error(`Reference node with ID ${referTargetNodeId} not found`) } } // 4. 根据position参数选择插入位置 let index = parentNode.children.indexOf(referenceNode) if (index === -1 && referTargetNodeId) { index = parentNode.children.length } // 5. 插入节点的逻辑 const childrenNode = toRaw(referenceNode) + // Validate position parameter + const validPositions = ['before', 'out', 'bottom'] + if (position && !validPositions.includes(position)) { + throw new Error(`Invalid position: ${position}. Must be one of: ${validPositions.join(', ')}`) + } + switch (position) { case 'before': + // Insert at the beginning of parent's children parentNode.children.unshift(newNodeData) break case 'out': + // Replace reference node with new node and make reference node a child if (childrenNode) { newNodeData.children = Array.isArray(childrenNode) ? [...childrenNode] : [childrenNode] parentNode.children.splice(index, 1, newNodeData) } break case 'bottom': + // Insert after the reference node parentNode.children.splice(index + 1, 0, newNodeData) break default: + // Append to the end of parent's children parentNode.children.push(newNodeData) break } setNode(newNodeData, parentNode) // 6. 如果新节点有子节点，递归构建 nodeMap if (Array.isArray(newNodeData?.children) && newNodeData.children.length > 0) { const newNode = getNode(newNodeData.id) + // Validate newNode before recursion + if (!newNode) { + throw new Error(`Failed to retrieve newly created node with ID ${newNodeData.id}`) + } generateNodesMap(newNodeData.children, newNode) } // 7. 返回插入结果 return { current: newNodeData, previous: undefined } }packages/canvas/container/src/components/CanvasAction.vue (3)
184-187: Add prop validation for multiStateLength.While the prop is correctly typed, it could benefit from additional validation.
Consider adding validation:
multiStateLength: { type: Number, - default: () => 0 + default: 0, + validator: (value) => { + return value >= 0 && Number.isInteger(value) + } }
243-257: Optimize computed properties with memoization.The computed properties could benefit from memoization to prevent unnecessary recalculations.
Consider this optimization:
+import { computed, unref } from 'vue' const isSingleNode = computed(() => { - return props.multiStateLength < 2 + return unref(props.multiStateLength) < 2 }) const showAction = computed(() => { const { schema, parent } = getCurrent() - if (schema?.props?.['data-id'] === 'root-container') { + // Memoize the result of getCurrent() + const currentSchema = unref(schema) + if (currentSchema?.props?.['data-id'] === 'root-container') { return false } - return !props.resize && parent && parent?.type !== 'JSSlot' && isSingleNode.value + return !unref(props.resize) && parent && parent?.type !== 'JSSlot' && unref(isSingleNode) }) const showQuickAction = computed(() => { - return !props.resize && isSingleNode.value + return !unref(props.resize) && unref(isSingleNode) })
420-422: Enhance error handling for missing document.The null check is good, but could be more explicit about the reason for returning an empty object.
Consider this improvement:
-if (!doc) { +// Ensure document is available before calculating styles +if (!doc || !(doc instanceof Document)) { + console.warn('Document is not available or invalid for style calculation') return {} }packages/canvas/container/src/container.js (3)
126-147: Add error handling and JSDoc documentation.The
getMultiStatefunction would benefit from:
- Error handling for invalid elements or missing attributes
- JSDoc documentation describing parameters and return type
+/** + * Retrieves the state of a selected node including its dimensions and attributes + * @param {HTMLElement} element - The DOM element to get state from + * @param {Document} doc - The document context + * @returns {Object|undefined} The node state or undefined if invalid + */ export const getMultiState = (element, doc) => { + if (!element?.nodeType || !doc?.nodeType) { + return undefined + } const { top, left, width, height } = element.getBoundingClientRect() const nodeTag = element?.getAttribute(NODE_TAG) const nodeId = element?.getAttribute(NODE_UID)
149-168: Add validation for multiSelectedStates parameter.The
setMultiStatefunction should validate themultiSelectedStatesparameter to ensure it's a valid reactive reference.+/** + * Sets the state of multiple selected nodes + * @param {Ref<Array>} multiSelectedStates - Reactive reference to selected states + * @param {Object|Array} node - Node or array of nodes to set + * @param {boolean} [append=false] - Whether to append or replace states + */ export function setMultiState(multiSelectedStates, node, append = false) { + if (!multiSelectedStates?.value) { + console.warn('Invalid multiSelectedStates parameter') + return + } if (!node || typeof node !== 'object') { multiSelectedStates.value = [] return }
380-393: Add error handling and documentation for container insertion.The
insertContainerfunction should include:
- Documentation explaining the OUT position usage
- Validation for required parameters
+/** + * Inserts a container node as a parent of the target node + * @param {Object} params - Insertion parameters + * @param {Object} params.parent - Parent node + * @param {Object} params.node - Target node + * @param {Object} params.data - Container node data + * @throws {Error} If required parameters are missing + */ const insertContainer = ({ parent, node, data }) => { + if (!parent?.id || !node?.id || !data) { + throw new Error('Missing required parameters for container insertion') + } if (!data.id) { data.id = utils.guid() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/canvas/DesignCanvas/src/api/useCanvas.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(11 hunks)packages/canvas/container/src/components/CanvasAction.vue(6 hunks)packages/canvas/container/src/components/CanvasMenu.vue(1 hunks)packages/canvas/container/src/container.js(6 hunks)packages/canvas/container/src/keyboard.js(7 hunks)packages/plugins/materials/src/composable/useMaterial.js(2 hunks)packages/plugins/materials/src/meta/component/src/Main.vue(2 hunks)packages/plugins/materials/src/meta/layout/src/Main.vue(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/canvas/container/src/components/CanvasMenu.vue
- packages/plugins/materials/src/composable/useMaterial.js
- packages/plugins/materials/src/meta/layout/src/Main.vue
- packages/plugins/materials/src/meta/component/src/Main.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (5)
packages/canvas/container/src/keyboard.js (2)
13-17: LGTM! Imports are well organized.The imports are logically grouped and include all necessary dependencies for the new multi-selection functionality.
28-30: LGTM! Good use of Vue's reactivity system.The
multiSelectedStatesref is well-named and appropriately uses Vue's reactivity system for managing multiple selected nodes.packages/canvas/container/src/CanvasContainer.vue (1)
1-14: LGTM! Well-structured template for multi-selection support.The template effectively handles multiple selections by iterating over
multiSelectedStatesand properly passing state information to child components.packages/canvas/container/src/container.js (2)
33-34: LGTM! Position enum updated for external container support.The addition of
OUTposition enhances the canvas's container management capabilities.
800-819: LGTM! Node insertion logic updated for external container support.The switch case addition for
POSITION.OUTmaintains consistency with existing position handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/canvas/container/src/keyboard.js (4)
28-30: Add JSDoc documentation for the multiSelectedStates ref.While the implementation is good, adding documentation would help other developers understand the purpose and structure of the multiSelectedStates ref.
Add documentation above the ref:
+ /** + * Reactive reference for tracking multiple selected nodes in the canvas + * @type {import('vue').Ref<Array<{id: string, schema: Object, parent: Object}>>} + */ const multiSelectedStates = ref([])
39-44: Add null checks for safer navigation.The current implementation could throw errors if parent or children are undefined.
Consider adding null checks:
- const id = (parent?.children[index - 1] || parent)?.id + const id = parent?.children?.[index - 1]?.id || parent?.id - const id = parent?.children[index + 1]?.id + const id = parent?.children?.[index + 1]?.id
113-126: Add error handling for clipboard operations.The multi-node paste operation could benefit from additional error handling and user feedback.
Consider adding try-catch and user feedback:
const handleMultiNodesPaste = (node, schema, parent) => { + try { if (multiSelectedStates.value.length === 1) { handleClipboardPaste(node, schema, parent) return } const selectedStates = multiSelectedStates.value.map(({ schema, parent }) => { return { node: copyObject(schema), schema: toRaw(schema), parent: toRaw(parent) } }) selectedStates.forEach(({ node, schema, parent }) => { handleClipboardPaste(node, schema, parent) }) + } catch (error) { + console.error('Failed to paste multiple nodes:', error) + // Consider adding user notification here + } }
162-169: Consider using AbortController for cleaner event handling.The current implementation could benefit from using AbortController for more robust cleanup.
Consider this pattern:
+const controller = new AbortController() +const { signal } = controller const registerHotkeyEvent = (dom) => { removeHotkeyEvent(dom) - dom.addEventListener('keydown', keyboardHandler) - dom.addEventListener('copy', handlerClipboardEvent) - dom.addEventListener('cut', handlerClipboardEvent) - dom.addEventListener('paste', handlerClipboardEvent) + dom.addEventListener('keydown', keyboardHandler, { signal }) + dom.addEventListener('copy', handlerClipboardEvent, { signal }) + dom.addEventListener('cut', handlerClipboardEvent, { signal }) + dom.addEventListener('paste', handlerClipboardEvent, { signal }) } const removeHotkeyEvent = (dom) => { - dom.removeEventListener('keydown', keyboardHandler) - dom.removeEventListener('copy', handlerClipboardEvent) - dom.removeEventListener('cut', handlerClipboardEvent) - dom.removeEventListener('paste', handlerClipboardEvent) + controller.abort() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/container/src/keyboard.js(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/canvas/container/src/keyboard.js (3)
13-20: LGTM! Well-organized imports and constants.The new imports and constants are properly structured and necessary for the added functionality.
46-52: Use conventional array clearing syntax.As previously suggested, use the conventional way to clear the array.
- multiSelectedStates.value = [] + multiSelectedStates.value.length = 0
76-80: LGTM! Proper cross-platform keyboard shortcut handling.The implementation correctly handles both Ctrl (Windows/Linux) and Command (Mac) keys for shortcuts.
Also applies to: 147-150
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/canvas/container/src/CanvasContainer.vue (2)
214-220: Enhance multi-selection key handling for better cross-platform support.While the code now handles both Ctrl and Command keys, the implementation could be more robust.
Consider using a more explicit check for modifier keys:
-if ((event.ctrlKey && event.button === 0) || event.metaKey) { +if (event.button === 0 && (event.ctrlKey || event.metaKey)) {This change:
- Prioritizes the primary button check
- Groups modifier keys more clearly
1-384: Consider enhancing keyboard accessibility.The component could benefit from improved keyboard navigation support:
- Add visible focus indicators for interactive elements
- Implement proper tab order
- Add ARIA attributes for better screen reader support
Would you like me to provide specific examples of ARIA attributes and focus management implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/container/src/CanvasContainer.vue(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/canvas/container/src/CanvasContainer.vue (4)
2-14: LGTM! Multi-selection implementation looks solid.The v-for loop correctly handles multiple selections with proper key binding and state management.
51-51: LGTM! Import changes align with new features.The imports and naming updates correctly support the multi-selection and container management features.
Also applies to: 77-80
109-112: LGTM! State management is well-structured.The reactive state setup efficiently handles both container management and multi-selection tracking.
313-321: LGTM! Clean handling of multi-selection state changes.The watcher appropriately manages the property panel state when multiple items are selected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/canvas/container/src/CanvasContainer.vue (2)
35-43: Consider adding keyboard navigation handling for the container panel.The panel should handle keyboard navigation appropriately to ensure good accessibility.
Add keyboard event handling to prevent Tab key from moving focus outside the panel:
<div v-if="insertContainer" ref="containerPanel" class="insert-panel"> <component :is="materialsPanel" :shortcut="insertContainer" groupName="layout" + @keydown.tab.prevent @close="insertContainer = false" ></component> </div>
313-321: Consider optimizing the watcher cleanup.The watcher clears the property panel when multiple nodes are selected, but it might be better to preserve the previous selection when returning to single selection.
watch( () => multiStateLength.value, (newVal) => { if (newVal > 1) { - // 清空属性面板 - selectNode(null) + const previousSelection = selectState.value + selectNode(null) + // Store previous selection to restore when returning to single selection + if (previousSelection) { + multiSelectedStates.value.previousSelection = previousSelection + } } else if (newVal === 1 && multiSelectedStates.value.previousSelection) { + // Restore previous selection when returning to single selection + selectNode(multiSelectedStates.value.previousSelection) + delete multiSelectedStates.value.previousSelection } } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/container/src/CanvasContainer.vue(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/canvas/container/src/CanvasContainer.vue (3)
2-14: LGTM! Multi-selection implementation looks good.The v-for loop correctly handles multiple selections with proper key binding and conditional state management.
214-220: LGTM! Multi-selection keyboard handling is now complete.The implementation now correctly handles both Control (Windows/Linux) and Command (Mac) keys for multi-selection.
51-51:❓ Verification inconclusive
Verify the impact of renaming hotkeyEvent registration.
The change from
registerHostkeyEventtoregisterHotkeyEventmight be a breaking change. Please ensure all usages are updated.
🏁 Script executed:
#!/bin/bash # Search for any remaining usage of the old name rg "registerHostkeyEvent|removeHostkeyEvent" -lLength of output: 49
Confirm consistent hotkey event naming across the repository.
I verified that no instances of the old names (
registerHostkeyEventorremoveHostkeyEvent) appear in the codebase via anrgsearch. However, since the initial search produced no output—which might be due to how the command was executed—please manually verify that all usages (including in tests and integrations) have been updated to useregisterHotkeyEventandremoveHotkeyEvent.
- File of concern:
packages/canvas/container/src/CanvasContainer.vue(line 51) now correctly imports:import { registerHotkeyEvent, removeHotkeyEvent, multiSelectedStates } from './keyboard'- Action: Double-check any dependent modules or integration tests that might rely on the old naming.
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?
右键添加父级容器
在画布中右键页面元素 -> 添加父级 -> 容器：
选中指定 容器 点击 鼠标左键，
点击 画布元素 或点击 大纲树，即可查看新增的父容器
节点多选能力
点击 Ctrl + 鼠标左键 进行 多选节点 ，支持快捷键操作复制、粘贴 和 删除
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes