fix: refactor multi-node click and fix outline tree click problem#1166
fix: refactor multi-node click and fix outline tree click problem#1166hexqi merged 7 commits intoopentiny:release/v2.3.0from
Conversation
WalkthroughThis pull request refactors how multi-selection state is managed across several Vue components and composables. A new composable ( Changes
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (8)
🪧 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 (5)
packages/canvas/container/src/composables/useMultiSelect.js (4)
7-7: Consider initializing with an empty array instead of body.The initial state includes
{ id: 'body', componentName: 'body' }which might not always be the desired default selection. Consider initializing with an empty array to avoid unintended selection effects on page load.-const multiSelectedStates = ref([{ id: 'body', componentName: 'body' }]) +const multiSelectedStates = ref([])
29-47: Add validation for node IDs in setMultiSelection.When appending nodes, you're checking if the node ID already exists, but when replacing the selection, there's no validation to prevent duplicate IDs. This could lead to the same node appearing multiple times in the selection array.
else { if (Array.isArray(node)) { - multiSelectedStates.value = node + // Filter out duplicates by ID + const uniqueNodes = []; + const nodeIds = new Set(); + for (const item of node) { + if (item.id && !nodeIds.has(item.id)) { + nodeIds.add(item.id); + uniqueNodes.push(item); + } + } + multiSelectedStates.value = uniqueNodes; } else { multiSelectedStates.value = [node] } }
51-60: Improve toggleMultiSelection with more concise code.The current implementation has unnecessary assignment when filtering the array. This can be simplified for better readability and performance.
const toggleMultiSelection = (selectState) => { const nodeId = selectState?.id const isExistNode = multiSelectedStates.value.map((state) => state.id).includes(nodeId) if (nodeId && isExistNode) { - const exList = multiSelectedStates.value.filter((state) => state.id !== nodeId) - setMultiSelection(exList) + multiSelectedStates.value = multiSelectedStates.value.filter((state) => state.id !== nodeId) } else { setMultiSelection(selectState, true) } }
62-87: Add error handling for invalid element in getMultiSelectionState.The function properly checks if
elementis falsy, but doesn't handle cases where the element might not have expected attributes or structure.const getMultiSelectionState = (element) => { if (!element) { return null; } + try { // Using cached position information const { top, left, width, height } = getCachedRect(element) const nodeTag = element?.getAttribute(NODE_TAG) const nodeId = element?.getAttribute(NODE_UID) || 'body' // Get node information const { node } = useCanvas().getNodeWithParentById(nodeId) || {} lastSelectedNode.value = nodeId return { id: nodeId, componentName: nodeTag, doc: getDocument(element), top, left, width, height, schema: toRaw(node) } + } catch (error) { + console.error('Error retrieving multi-selection state:', error); + return null; + } }packages/canvas/container/src/container.js (1)
258-274: Good refactoring: exporting getRect function improves code reusabilityMaking the
getRectfunction exportable allows it to be used by other components, which is a good practice for code reusability. This change is likely related to the multi-selection refactoring, where this utility function is now needed by the newuseMultiSelectcomposable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/container/src/CanvasContainer.vue(5 hunks)packages/canvas/container/src/composables/useMultiSelect.js(1 hunks)packages/canvas/container/src/container.js(2 hunks)packages/canvas/container/src/keyboard.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (12)
packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
163-165: Good addition of fallback for node not found.This fallback logic ensures that when a schemaItem is not found by ID, the pageSchema ID defaults to 'body'. This addresses the issue where clicking on an outline tree node wasn't correctly displaying the selection effect on canvas elements.
packages/canvas/container/src/composables/useMultiSelect.js (1)
1-103: Well-structured composable for centralizing multi-selection logic.This new composable nicely encapsulates all multi-selection related functionality that was previously spread across different files. The implementation offers several benefits:
- Centralized state management for selected nodes
- Efficient node position caching with WeakMap
- Clear methods for setting, toggling, and clearing selections
- Proper error handling for invalid node objects
The approach of extracting this logic into a composable improves code maintainability and reusability.
packages/canvas/container/src/CanvasContainer.vue (4)
59-59: Import successfully incorporates new composable.Good integration of the new useMultiSelect composable, which centralizes multi-selection logic.
110-111: Effective destructuring of composable methods.Clear and concise destructuring of all required methods from the useMultiSelect composable, making them available in this component.
113-147: Function signature simplified correctly.The
setCurrentNodefunction has been successfully simplified by removing the unnecessarydocparameter, and the logic has been updated to use the new multi-selection composable methods. This improves the function's clarity and maintainability.
215-221: Event handling updated for multi-selection.The event handling code for multi-selection has been updated to use the new composable methods. This addresses the issue where selection effect wasn't being properly managed during multi-select operations.
packages/canvas/container/src/keyboard.js (5)
17-17: Properly imported useMultiSelect composable.Good integration of the new composable in the keyboard handling module.
44-44: Effective destructuring of required methods.Clear and focused destructuring that extracts only the methods needed in this file.
51-51: Key fix: proper clearing of selection after deletion.This change addresses one part of the PR objective by ensuring that selection effects are properly cleared after operations on multiple selected nodes.
122-123: Key fix: clear selection after copy operation.This change directly addresses the second issue in the PR objectives: "After copying multiple selected nodes, the selection effect does not disappear as expected." By calling clearMultiSelection() after a copy operation, the selection effect will now properly disappear.
170-170: Improved exports by removing direct state export.Removing the direct export of multiSelectedStates enhances encapsulation and ensures that interaction with the selection state happens only through the provided methods of the composable.
packages/canvas/container/src/container.js (1)
258-274:Details
✅ Verification successful
Verify that all callers of getRect are updated correctly
Now that
getRectis exported and likely used by the new multi-selection composable, ensure all call sites are updated correctly across the codebase.
🏁 Script executed:
#!/bin/bash # Search for all usages of getRect to ensure they're compatible with the new exported function # Find usages of getRect rg -A 1 "getRect\(" --glob "*.{js,vue}" # Find imports of getRect from this file rg -A 1 "getRect.+from.+container" --glob "*.{js,vue}"Length of output: 1180
All
getRectcall sites appear updated correctly.
After reviewing the search results, the usage inpackages/canvas/container/src/container.jsand the import and call inpackages/canvas/container/src/composables/useMultiSelect.jscorrectly pass a single element togetRect. There were no outdated or incompatible call patterns found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/canvas/container/src/composables/useMultiSelect.js (2)
6-9: **Set an initial multi-selection state **Defining
initMultiStateas{ id: 'body' }and initializingmultiSelectedStateswith an empty array is clear. Consider adding a comment or JSDoc explaining that'body'acts as a fallback or “root” selection node for clarity.
96-100: **Clear selection & cached areas carefully **
clearMultiSelectionreinitializesnodeRectCache = new WeakMap(). If you want partial cache invalidation or advanced selection states, consider extracting a dedicated cache manager. For now, this works well for a full reset.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/container/src/composables/useMultiSelect.js(1 hunks)packages/canvas/container/src/container.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (10)
packages/canvas/container/src/composables/useMultiSelect.js (6)
1-5: **Use standard Vue imports effectively **The imports from Vue and related packages are well-structured and support the composable’s functionality without unnecessary dependencies.
14-22: **Ensure cache invalidation for complex dynamic layouts **
getCachedRectstores each element's bounding rect innodeRectCache. If elements move frequently, consider verifying that stale data won't accumulate. In highly dynamic UIs, we might need an invalidation strategy or forced recalculation.Would you like a follow-up script for automated checks of bounding-rect usage across the codebase?
30-54: **Robust multi-selections with optional appending **The
initMultiSelectandsetMultiSelectionfunctions handle single or multiple node updates accurately, resetting or appending node references. The logic is concise and easily extensible for future multi-selection features.
56-67: **Toggle logic is straightforward **
toggleMultiSelectioncleanly removes an existing node or appends a new one tomultiSelectedStatesbased on presence. This approach is well-suited for quickly modifying the selection state.
69-94: **Handle missing node data gracefully **
getMultiSelectionStatemay return undefinednodeif the lookup fails (e.g., node doesn’t exist). The function already tolerates that by defaulting the ID to'body'. Confirm that downstream consumers handle aschemaofundefinedgracefully.
102-111: **Composable pattern is well-structured **Returning the reactive state and functions in a single object aligns with usual Vue composable patterns, enhancing modularity and code clarity.
packages/canvas/container/src/container.js (4)
27-27: **New dependency injection for multi-selection composable **Importing
useMultiSelectat line 27 cleanly integrates the new multi-selection logic within the container module.
259-275: **Exporting getRect for broader reusability **Changing
getRecttoexport const getRectis a good move, allowing it to be shared across the codebase. The logic handlesBODYas a special case well, returning the viewport dimensions effectively.
713-713: **Retrieve multi-select composable functions locally **Destructuring
clearMultiSelectionandinitMultiSelecthere keeps the usage concise and modular. This is an effective approach for reusing composable logic in the container scope.
722-725: **Reset multi-selection on outline tree click **Clearing and reinitializing multi-selection when
type === 'clickTree'ensures a fresh selection state. Confirm that users won’t unexpectedly lose prior selections if they navigate via the outline tree. If this is intended behavior, then it’s correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/canvas/container/src/composables/useMultiSelect.js (4)
6-7: Consider documenting the default state.The
initMultiStatevariable would benefit from a brief comment explaining why 'body' is the default ID.
35-44: Enhance type handling in setMultiSelection.Consider adding stricter type checking or documenting expected input types. The function checks if the input is an array or an object, but there's no validation for empty arrays or handling of primitive values beyond string/number conversion.
const setMultiSelection = (nodes) => { if (Array.isArray(nodes)) { + // Filter out any invalid nodes + const validNodes = nodes.filter(node => node && typeof node === 'object') - multiSelectedStates.value = nodes + multiSelectedStates.value = validNodes } else if (nodes && typeof nodes === 'object') { multiSelectedStates.value = [nodes] } else { multiSelectedStates.value = [] } }
56-67: Consider refactoring for more consistent behavior.The
toggleMultiSelectionfunction has similar logic to theaddMultiSelectionfunction but with the additional check to remove the node if it exists. Consider usingaddMultiSelectionwithin this function to reduce duplication.const toggleMultiSelection = (selectState) => { const nodeId = selectState?.id const isExistNode = multiSelectedStates.value.map((state) => state.id).includes(nodeId) if (nodeId && isExistNode) { const exList = multiSelectedStates.value.filter((state) => state.id !== nodeId) setMultiSelection(exList) } else { - addMultiSelection(selectState) + if (selectState && typeof selectState === 'object') { + addMultiSelection(selectState) + } } }
69-94: Potential unhandled null reference in getMultiSelectionState.Line 81 uses object destructuring on the result of
useCanvas().getNodeWithParentById(nodeId), but does not verify that the node exists before accessing it. While there's a fallback fornodeId, an explicit error handling for missing nodes would improve robustness.// 获取节点信息 - const { node } = useCanvas().getNodeWithParentById(nodeId) || {} + const result = useCanvas().getNodeWithParentById(nodeId) || {} + const { node } = result lastSelectedNode.value = nodeIdpackages/canvas/container/src/container.js (1)
397-419: Enhanced setSelectRect with multi-selection support.The function has been extended to handle multi-selection states, updating individual selection states when a multiNodeId is provided.
However, the map operation doesn't return anything, which might be confusing:
if (multiNodeId) { - multiSelectedStates.value.map((state) => { + multiSelectedStates.value = multiSelectedStates.value.map((state) => { if (state.id === multiNodeId) { - return Object.assign(state, selectState) + return { ...state, ...selectState } } + return state }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/canvas/container/src/CanvasContainer.vue(6 hunks)packages/canvas/container/src/composables/useMultiSelect.js(1 hunks)packages/canvas/container/src/container.js(8 hunks)packages/canvas/container/src/keyboard.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/canvas/container/src/keyboard.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (18)
packages/canvas/container/src/composables/useMultiSelect.js (5)
1-5: Clean import statements and module structure.The imports are well-organized and the file uses appropriate dependencies for its functionality.
14-22: Good use of cache for performance optimization.The caching mechanism for node rectangles is well-implemented. Using a WeakMap is an excellent choice as it allows garbage collection when elements are no longer referenced elsewhere.
46-54: Improved check for node existence.Using
nodeIds.includes(node.id)is more efficient than creating a Set, aligning with the past review comment.
96-100: Good state cleanup in clearMultiSelection.The implementation properly resets all related state including the WeakMap cache, which prevents memory leaks.
102-111: Complete API exposure.The returned interface from the composable exposes all necessary methods and state for external usage, facilitating good integration with Vue components.
packages/canvas/container/src/CanvasContainer.vue (7)
51-52: Clean imports with good modularization.The imports are well-organized, and the addition of the useMultiSelect composable shows good separation of concerns.
Also applies to: 59-59
70-71: Updated imports to match implementation changes.The imports have been correctly updated to include the new
syncNodeScrollfunction and avoid importing removed functions.Also applies to: 78-78
110-111: Improved state management with composable.Extracting multi-selection logic into a separate composable promotes better code organization and reusability.
113-127: Simplified setCurrentNode signature and implementation.The function now has a cleaner signature without the
docparameter and uses the new composable functions for state management. This makes the code more maintainable.However, there's still room for improvement in error handling:
const setCurrentNode = async (event) => { const { clientX, clientY } = event const element = getElement(event.target) closeMenu() let node = getCurrent().schema if (element) { const currentElement = querySelectById(getCurrent().schema?.id) if (!currentElement?.contains(element) || event.button === 0) { const selectedState = getMultiSelectionState(element) if (selectedState) { setMultiSelection(selectedState) } + // Consider adding an else case to handle when selectedState is null }
215-222: Improved multi-selection handling with modifier keys.The implementation correctly uses the new composable function
toggleMultiSelectionfor Ctrl/Cmd + click behavior to add/remove nodes from selection.
227-228: Simplified function call by removing unnecessary parameter.The
setCurrentNodefunction call has been updated to match its new signature, removing the unnecessarydocparameter.
271-271: Updated scroll event listener.The scroll event listener now uses
syncNodeScrollfor better performance and consistency with the new multi-selection approach.packages/canvas/container/src/container.js (6)
27-27: Added import for the new composable.The import statement for the new useMultiSelect composable has been correctly added.
259-275: Exported getRect function for reuse.Good practice to export the getRect function since it's now reused in the useMultiSelect composable, promoting code reuse.
395-395: Efficient destructuring of useMultiSelect.Using destructuring for frequently accessed properties and methods is a good practice for readability.
436-446: Added syncNodeScroll for better handling of multi-selection during scrolling.The new function ensures that selection states are correctly updated when scrolling, differentiating between single and multi-selection scenarios.
743-746: Clear multi-selection when clicking tree nodes.The code now correctly clears and initializes multi-selection when clicking on tree nodes, which addresses the first issue mentioned in the PR description about outline tree click selection effect.
906-906: Updated canvasApi exports to include syncNodeScroll.The API export has been updated to include the new syncNodeScroll function, ensuring external components can use it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/canvas/container/src/composables/useMultiSelect.js (3)
36-44: Avoid duplicate entries when setting multiple nodes at once.
Currently, if an incoming array contains duplicate node objects, they may be added unresolved. Consider de-duplicating the array to maintain a clean selection state.You could, for example, apply a
.filter()or use aSetbased on node IDs:- multiSelectedStates.value = nodes + const uniqueNodes = [] + const seenIds = new Set() + for (const n of nodes) { + if (!seenIds.has(n.id)) { + seenIds.add(n.id) + uniqueNodes.push(n) + } + } + multiSelectedStates.value = uniqueNodes
82-86: Consider more granular cache invalidation.
Clearing the entire cache on every selection reset might cause unnecessary re-computation if only certain elements change. If possible, remove only updated or unselected Node entries from the cache to retain performance benefits for the rest.
88-114: Add SHIFT-click handling for range selection.
If you intend to support selecting multiple consecutive nodes, consider adding SHIFT-click logic, similar to many file explorers. This would spare users from having to CTRL-click each node individually.Would you like me to propose a reference implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/container/src/CanvasContainer.vue(8 hunks)packages/canvas/container/src/composables/useMultiSelect.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/canvas/container/src/CanvasContainer.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/canvas/container/src/composables/useMultiSelect.js (2)
14-22: Ensure the cached geometry remains valid after layout changes.
If the DOM layout or element size changes, the cached rectangle might become outdated. Consider adding a strategy to invalidate or refresh the cache selectively to avoid returning stale measurements.
50-51: Good use ofArray.prototype.some().
This approach is clear and avoids the previously mentioned pitfalls of looping.
4 participants
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?
1、初始化点击 大纲树 节点时，画布元素框选效果不显示
2、多选节点复制后，节点框选中效果滚动异常
Issue Number: N/A
What is the new behavior?
1、点击 大纲树 节点时，画布元素正常显示
2、多选节点复制后，节点框选中效果正常展示
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor