-
Notifications
You must be signed in to change notification settings - Fork 376
refactor: optimize Vue canvas interaction logic #1297
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: develop
Are you sure you want to change the base?
Conversation
WalkthroughThis update introduces a comprehensive refactor of the canvas container's node selection and hover logic, centralizing state management through new composable hooks ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CanvasContainer
participant useHoverNode
participant useSelectNode
participant CanvasHover
participant CanvasInsertLine
User->>CanvasContainer: Mouse event (mousedown/mouseover/contextmenu)
CanvasContainer->>useSelectNode: updateSelectedNode (for selection)
CanvasContainer->>useHoverNode: updateHoverNode (for hover)
CanvasContainer->>CanvasHover: Render hover rectangle (with curHoverState)
CanvasContainer->>CanvasInsertLine: Render insert line (with lineState)
useSelectNode-->>CanvasContainer: Update selectState
useHoverNode-->>CanvasContainer: Update curHoverState
CanvasContainer-->>User: Visual feedback (selection, hover, insert line)
sequenceDiagram
participant Keyboard
participant useSelectNode
participant useHoverNode
participant CanvasContainer
Keyboard->>useSelectNode: selectNodeById / clearSelect (arrow/delete)
Keyboard->>useHoverNode: clearHover (on delete)
useSelectNode-->>CanvasContainer: Update selectState
useHoverNode-->>CanvasContainer: Update curHoverState
CanvasContainer-->>Keyboard: Update UI
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (27)
designer-demo/public/mock/bundle.json (1)
322-324
: Typo in Context Menu Actions
Within the"configure"
object, the"contextMenu"
has an action"bindEevent"
. This appears to be a typographical error and likely should be"bindEvent"
.
Apply the following diff suggestion:- "actions": ["copy", "remove", "insert", "updateAttr", "bindEevent", "createBlock"], + "actions": ["copy", "remove", "insert", "updateAttr", "bindEvent", "createBlock"],packages/canvas/render/src/runner.ts (1)
48-59
: Well-structured component instance referencing system.The
GetComponentByDomNode
plugin provides a way to access Vue component instances from DOM nodes, which is vital for the refactored selection mechanism. The mixin approach ensures all Vue components will have their instance accessible via the DOM element's__vueComponent
property.Consider adding JSDoc comments to document this plugin's purpose. Also, since
__vueComponent
is a non-standard property, you might want to add a comment explaining this convention:+/** + * Plugin that allows retrieving the Vue component instance from a DOM node. + * This is used by the canvas interaction system to access component data when selecting elements. + */ const GetComponentByDomNode = { install: (Vue) => { Vue.mixin({ mounted() { + // Store a reference to the component instance on its root DOM element this.$el.__vueComponent = this?._ }, updated() { this.$el.__vueComponent = this?._ } }) } }packages/canvas/container/src/components/CanvasHover.vue (2)
21-54
: Clean implementation of hover node selection.The script section properly uses the new
useSelectNode
hooks and implements a clear selection function that either updates the selected node with the provided element or selects by ID.Consider adding prop validation to ensure the required properties of the hoverState object are present:
props: { hoverState: { type: Object, - default: () => ({}) + default: () => ({}), + validator: (value) => { + return value.rect !== undefined && value.componentName !== undefined; + } } },
56-101
: Well-structured hover styling with dynamic binding.The CSS effectively uses dynamic v-bind values to position the hover rectangle based on the state. The separate styling for active and inactive hovers provides good visual feedback to users.
Add appropriate keyboard accessibility to the clickable corner mark:
.corner-mark-left { // 为了支持点击选中 pointer-events: auto; + cursor: pointer; + &:focus { + outline: 2px solid var(--te-canvas-container-border-color-checked); + } }Also, consider adding tabindex and role attributes in the template:
-<div class="corner-mark-left" @click="handleSelectHoverNode"> +<div class="corner-mark-left" @click="handleSelectHoverNode" @keydown.enter="handleSelectHoverNode" tabindex="0" role="button">packages/canvas/container/src/interactions/index.ts (2)
12-37
: Well-designed interaction hook selection system.The implementation cleanly maps different interaction hook sets based on DSL mode, providing a flexible, extensible architecture for switching between Vue-specific and default interactions.
Add JSDoc comments to improve code documentation:
+/** + * Map of interaction hooks for different DSL modes + * Each mode provides hooks for hover and selection functionality + */ const interactionHooksMap = { vue: { useHoverNode: useVueHoverNode, useSelectNode: useVueSelectNode }, default: { useHoverNode: useDefaultHoverNode, useSelectNode: useDefaultSelectNode } } +/** + * Returns the appropriate interaction hooks based on the configured DSL mode + * Defaults to standard hooks if no valid mode is found + * @returns Object containing useHoverNode and useSelectNode hooks + */ const getInteractionFn = () => { const dslMode = getMergeMeta('engine.config')?.dslMode?.toLowerCase?.() as keyof typeof interactionHooksMap if (interactionHooksMap[dslMode]) { return interactionHooksMap[dslMode] } return interactionHooksMap.default }
38-54
: Effective implementation of interaction hook providers.The exported functions
useHoverNode
anduseSelectNode
efficiently manage the initialization of interaction functions and memoize the result to avoid redundant calculations.Consider adding type safety for return values:
+type InteractionHooks = typeof interactionHooksMap[keyof typeof interactionHooksMap]; -const interactionsFn = ref<typeof interactionHooksMap[keyof typeof interactionHooksMap] | null>(null) +const interactionsFn = ref<InteractionHooks | null>(null) +/** + * Hook to access hover node functionality + * Initializes interaction functions if not already done + * @returns Object with hover node state and methods + */ export const useHoverNode = () => { if (!interactionsFn.value) { interactionsFn.value = getInteractionFn() } return interactionsFn.value.useHoverNode() } +/** + * Hook to access node selection functionality + * Initializes interaction functions if not already done + * @returns Object with selection state and methods + */ export const useSelectNode = () => { if (!interactionsFn.value) { interactionsFn.value = getInteractionFn() } return interactionsFn.value.useSelectNode() }packages/canvas/container/src/components/CanvasInsertLine.vue (2)
9-23
: Consider removing empty setup function and optimizing propsThe
setup()
function is empty and could be removed entirely as it doesn't add any value.- setup() {}
94-97
: Use CSS variables for consistent stylingThere are hardcoded color values for the hover state of slot selectors. Consider using CSS variables for better consistency with the theme system.
- background: #40a9ff; - color: #fff; + background: var(--te-canvas-container-text-color-checked); + color: var(--te-common-text-dark-inverse);packages/canvas/container/src/components/CanvasAction.vue (1)
382-399
: Consider simplifying the document null checkAfter calling
getDocument()
, there's still a check ifdoc
exists, which seems redundant since you just obtained it. This might be leftover from previous code structure.const { left, top, width, height } = selectState const doc = getDocument() const { width: canvasWidth, height: canvasHeight } = canvasSize // 标签宽度和工具操作条宽度之和加上间距 const fullRectWidth = labelWidth + optionWidth + OPTION_SPACE // 是否 将label 标签放置到底部,判断 top 距离 const isLabelAtBottom = top < LABEL_HEIGHT const labelAlign = new Align({ alignLeft: true, horizontalValue: 0, alignTop: !isLabelAtBottom, verticalValue: -LABEL_HEIGHT }) - if (!doc) { - return {} - }packages/canvas/container/src/interactions/default-interactions.ts (2)
102-109
: Replace event-based communication with proper event systemThere's a TODO comment about changing to event notification, but the implementation is currently using a side-effect approach.
Consider implementing a proper event system instead of temporarily using the 'remove' event to trigger schema updates. This would make the code more maintainable and easier to understand.
- // TODO: 改成事件通知 - // 临时借用 remove 事触发 currentSchema 更新 - canvasState?.emit?.('remove') + // Use a dedicated event for schema updates + canvasState?.emit?.('schemaUpdated')
223-258
: Consider refactoring the timeout pattern in updateSelectedRectThe function uses
setTimeout
with a 0ms delay, which is typically used to defer execution to the next event loop cycle.Consider using
nextTick
from Vue instead ofsetTimeout(fn, 0)
for consistency with other async operations in the codebase.const updateSelectedRect = () => { - setTimeout(() => { + nextTick(() => { if (!selectState.value.length) { return } selectState.value = selectState.value.map((stateItem) => { // ... existing code }) - }, 0) + }) }packages/canvas/container/src/keyboard.ts (1)
184-193
: Commented code should be removedLine 185 has a commented reference to a function that's not being used.
Remove the commented line to keep the code clean:
const selectNodeById = async (id: string, type: string) => { - // commonSelectNodeById(updateSelectedNode, id, type) const element = querySelectById(id) const { node, parent } = useCanvas().getNodeWithParentById(id) || {}
packages/canvas/container/src/interactions/vue-rect.ts (1)
79-80
: ESLint disable comment could be avoided with code restructuringThere's an ESLint disable comment for the no-use-before-define rule.
Consider restructuring the code to define functions in order of dependency, avoiding the need for the ESLint disable comment:
-export const getFragmentRect = (instance: VueInstanceInternal): Rect => { +export const getElementRectByInstance = (instance: VueInstanceInternal): Rect | undefined => { + if (instance?.type?.description === 'v-fgt') { + return getFragmentRect(instance) + } + + if (instance.el?.nodeType === 1) { + return instance.el.getBoundingClientRect() + } + + if (instance.component) { + return getElementRectByInstance(instance.component) + } + + if (instance.subTree) { + return getElementRectByInstance(instance.subTree) + } + + return undefined +} + +export const getFragmentRect = (instance: VueInstanceInternal): Rect => { // ...function body... } - -export const getElementRectByInstance = (instance: VueInstanceInternal): Rect | undefined => { - // ...function body... -}packages/canvas/container/src/interactions/common.ts (2)
71-94
: Clarify purpose of nodeType checkThere's a question comment about the nodeType check that could be clarified.
Add a more descriptive comment explaining why the nodeType check is necessary:
- // QUESTION: 为什么要判断 node Type? + // Only process Element nodes (nodeType === 1), skip text nodes (nodeType === 3) and other node types if (!element || element.nodeType !== 1) { return undefined }
107-125
: Consider improving the MouseEvent creation approachBoth
hoverNodeById
andselectNodeById
functions create fake MouseEvents by type casting, which works but is not ideal.Consider creating a more explicit method for simulating interactions rather than using type casting to create fake events:
+// Create a simulated mouse event targeting the specified element +const createSimulatedMouseEvent = (target: Element): MouseEvent => { + return { + target, + preventDefault: () => {}, + stopPropagation: () => {} + } as unknown as MouseEvent +} + export const hoverNodeById = (id: string, updateHoverNode: (e: MouseEvent) => void) => { const element = querySelectById(id) if (element) { - updateHoverNode({ target: element } as unknown as MouseEvent) + updateHoverNode(createSimulatedMouseEvent(element)) } } export const selectNodeById = async ( updateSelectedNode: (e: MouseEvent, type: string) => void, id: string, type: string ) => { const element = querySelectById(id) if (element) { - updateSelectedNode({ target: element } as unknown as MouseEvent, type) + updateSelectedNode(createSimulatedMouseEvent(element), type) } }packages/canvas/container/src/CanvasContainer.vue (5)
110-112
: Destructuring new composables
ExtractingselectState
,updateSelectedNode
,defaultSelectState
, and similarly for hover is a well-structured approach. Double-check concurrency with each reactive call to ensure no flickering in complex scenarios.
202-203
: Broadcasting 'canvas-mousedown' event
Emitting an event externally can be powerful for cross-component communication. Confirm no duplication or conflict with other mousedown handlers in the environment.
217-226
: Managing scroll events with time-based checks
Using ascrollTimeout
to debounce scroll logic is typically effective. Just ensure it doesn't cause performance issues on large canvases. Consider arequestAnimationFrame
approach for smoother UI updates if needed.
309-311
: Commented-out hover slot logic
The_slotName
function is present but references commented-out hover code. If truly obsolete, consider removing it entirely or reintroducing it if planned for future multi-slot features.
344-346
: Exposing reactive references
srcAttrName
,selectState
, andcurHoverState
are published fromsetup()
. Verify that only the needed references are exposed to the template or parent, to avoid unintentional override or misuse.packages/canvas/container/src/interactions/vue-interactions.ts (5)
24-37
: Imports and references
The relevant references from'vue'
,'@opentiny/tiny-engine-meta-register'
,'../../../common'
, etc. are properly consolidated. Keep an eye on ensuring unused imports don’t accumulate as the codebase evolves.
67-134
: Fetching rect and node data
getRectAndNode
does a thorough job retrieving the ID from nested Vue instances. The while loop ensures we find relevant attributes or a fallback. Verify performance on large component trees, and consider short-circuiting if no match is found within reasonable depth.
137-142
: Hover logic and optional chaining
The condition checksres?.node?.id
and whether it appears inselectState
. The static analysis hint suggests strengthening usage of optional chaining. You might rewrite for clarity, e.g.:-if (!res || (res?.node?.id && selectState.value.some((state) => state?.node?.id === res.node.id))) { +if (!res || (res?.node?.id && selectState.value.some((state) => state?.node?.id === res?.node?.id))) {Nonetheless, functionality appears correct. Confirm that it gracefully handles missing properties.
🧰 Tools
🪛 Biome (1.9.4)
[error] 137-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
224-226
: Selecting node by ID
Bridging to a common function (commonSelectNodeById
) is consistent. Confirm that raising an error or fallback occurs if the node is absent or the ID is invalid, preventing silent failures.
237-248
: Delayed rect update
updateSelectedRect
uses asetTimeout
to refresh dimensions, presumably allowing layout to settle. This is often effective but can be fragile if layout changes are significant. Consider usingnextTick
for more Vue-centric reactivity.packages/canvas/container/src/container.ts (2)
165-167
: Clearing hover on dragStart
dragStart
destructuresclearHover
fromuseHoverNode
and immediately calls it. This helps give a clean drag experience but double-check abrupt hover clearing doesn’t hamper user feedback in certain edge cases.
658-669
: Deprecated selectors
FunctionsselectNode
andhoverNode
now delegate touseSelectNode
/useHoverNode
. This helps backward compatibility. Consider logging a warning to encourage migration for anyone still calling these.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
designer-demo/engine.config.js
(1 hunks)designer-demo/public/mock/bundle.json
(2 hunks)packages/canvas/container/index.ts
(1 hunks)packages/canvas/container/src/CanvasContainer.vue
(9 hunks)packages/canvas/container/src/components/CanvasAction.vue
(6 hunks)packages/canvas/container/src/components/CanvasDivider.vue
(2 hunks)packages/canvas/container/src/components/CanvasHover.vue
(1 hunks)packages/canvas/container/src/components/CanvasInsertLine.vue
(1 hunks)packages/canvas/container/src/components/CanvasResizeBorder.vue
(2 hunks)packages/canvas/container/src/components/CanvasRouterJumper.vue
(1 hunks)packages/canvas/container/src/components/CanvasViewerSwitcher.vue
(1 hunks)packages/canvas/container/src/composables/useMultiSelect.ts
(0 hunks)packages/canvas/container/src/container.ts
(15 hunks)packages/canvas/container/src/interactions/common.ts
(1 hunks)packages/canvas/container/src/interactions/default-interactions.ts
(1 hunks)packages/canvas/container/src/interactions/index.ts
(1 hunks)packages/canvas/container/src/interactions/vue-interactions.ts
(1 hunks)packages/canvas/container/src/interactions/vue-rect.ts
(1 hunks)packages/canvas/container/src/keyboard.ts
(5 hunks)packages/canvas/render/src/render.ts
(0 hunks)packages/canvas/render/src/runner.ts
(2 hunks)packages/plugins/tree/src/Main.vue
(3 hunks)packages/toolbars/save/src/js/index.ts
(2 hunks)
💤 Files with no reviewable changes (2)
- packages/canvas/render/src/render.ts
- packages/canvas/container/src/composables/useMultiSelect.ts
🧰 Additional context used
🧠 Learnings (2)
packages/canvas/container/src/components/CanvasDivider.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-engine#1233
File: packages/canvas/container/src/components/CanvasDivider.vue:184-185
Timestamp: 2025-04-10T06:10:03.602Z
Learning: In CanvasDivider.vue, even though state.verLeft and state.horizontalTop already include 'px' suffix, the CSS properties in state.dividerStyle still need to append 'px' again according to gene9831, suggesting that these state variables might be processed differently than expected when used in style binding.
packages/canvas/container/src/components/CanvasViewerSwitcher.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-engine#1117
File: packages/canvas/container/src/components/CanvasViewerSwitcher.vue:96-117
Timestamp: 2025-04-10T06:10:03.602Z
Learning: In CanvasViewerSwitcher.vue, `state.usedHoverState.element` is guaranteed to have a value when `handleClick` is called, making additional error handling unnecessary.
🧬 Code Graph Analysis (8)
packages/toolbars/save/src/js/index.ts (1)
packages/canvas/container/src/container.ts (1)
canvasApi
(824-870)
packages/canvas/container/src/interactions/vue-rect.ts (2)
packages/canvas/types.ts (1)
Node
(1-6)packages/canvas/container/src/interactions/common.ts (1)
VueInstanceInternal
(21-43)
packages/canvas/container/src/keyboard.ts (5)
packages/canvas/container/src/interactions/default-interactions.ts (2)
useSelectNode
(260-269)useHoverNode
(115-122)packages/canvas/container/src/interactions/index.ts (2)
useSelectNode
(48-54)useHoverNode
(40-46)packages/canvas/container/src/interactions/vue-interactions.ts (2)
useSelectNode
(250-259)useHoverNode
(228-235)packages/canvas/container/src/interactions/common.ts (2)
selectNodeById
(115-125)clearHover
(64-69)packages/canvas/container/src/container.ts (1)
removeNodeById
(309-319)
packages/canvas/container/src/interactions/default-interactions.ts (1)
packages/canvas/container/src/interactions/common.ts (7)
HoverOrSelectState
(6-18)initialHoverState
(51-62)clearHover
(64-69)getClosedElementHasUid
(71-94)getWindowRect
(96-105)hoverNodeById
(107-113)selectNodeById
(115-125)
packages/canvas/container/src/interactions/index.ts (3)
packages/register/src/common.ts (1)
getMergeMeta
(179-181)packages/canvas/container/src/interactions/default-interactions.ts (2)
useHoverNode
(115-122)useSelectNode
(260-269)packages/canvas/container/src/interactions/vue-interactions.ts (2)
useHoverNode
(228-235)useSelectNode
(250-259)
packages/canvas/container/src/interactions/vue-interactions.ts (1)
packages/canvas/container/src/interactions/common.ts (8)
HTMLElementWithVue
(46-49)VueInstanceInternal
(21-43)HoverOrSelectState
(6-18)initialHoverState
(51-62)clearHover
(64-69)getWindowRect
(96-105)hoverNodeById
(107-113)selectNodeById
(115-125)
packages/canvas/container/src/interactions/common.ts (2)
packages/canvas/container/src/container.ts (2)
getWindow
(72-72)querySelectById
(334-352)packages/canvas/container/src/interactions/vue-interactions.ts (1)
updateHoverNode
(136-144)
packages/canvas/container/src/container.ts (3)
packages/canvas/container/src/interactions/default-interactions.ts (2)
useHoverNode
(115-122)useSelectNode
(260-269)packages/canvas/container/src/interactions/index.ts (2)
useHoverNode
(40-46)useSelectNode
(48-54)packages/canvas/container/src/interactions/vue-interactions.ts (2)
useHoverNode
(228-235)useSelectNode
(250-259)
🪛 Biome (1.9.4)
packages/canvas/container/src/interactions/vue-interactions.ts
[error] 137-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (55)
designer-demo/engine.config.js (1)
6-7
: Addition of Vue DSL mode configuration looks goodThe new
dslMode: 'vue'
property has been added to the configuration object, which aligns with the PR objectives to optimize Vue canvas interaction logic. This configuration will support the new selection and hover functionality implemented across the application.Note that according to the PR objectives, this introduces a breaking change that may impact existing applications, particularly in how nodes are selected and interacted with in the Vue canvas environment.
designer-demo/public/mock/bundle.json (2)
282-330
: New Component Addition: 日期选择器 Configuration
A new JSON object defining the "日期选择器" (DatePicker) component has been added with a detailed configuration including metadata (id, version, name, icon, description), npm package info, and an extensive "configure" and "schema" structure.
• Suggestion: Verify that the"id": 1
value does not conflict with other component IDs within the mock data.
14391-14399
: New Child Entry in Element Plus组件
A new child entry for "日期选择器" has been inserted into the"children"
array under the "Element Plus组件" category. The child configuration is minimal, offering properties such as the component name, icon, and snippetName.
• Suggestion: Confirm that the minimal"schema": {}
provided here is sufficient for downstream usage or if additional configuration details are needed in the long term.packages/canvas/container/src/components/CanvasDivider.vue (2)
153-154
: Improved structure for selectState accessThe code now correctly destructures position-related properties (
width
,height
,left
,top
) fromselectState.rect
and assignscomponentName
separately fromselectState.componentName
. This change aligns with the restructured state management approach in the PR, making the component more maintainable.
180-180
: Consistent rect property access patternThe change to access position properties via
props.selectState.rect
is now consistent with the pattern used in other parts of the file and across the codebase, contributing to improved code consistency.packages/canvas/container/index.ts (1)
2-2
:❓ Verification inconclusive
API refactoring from multi-selection to node selection
The module now exports
useSelectNode
instead ofuseMultiSelect
, which is a significant change in the selection paradigm. This aligns with the PR objectives to centralize selection logic and distinguish between Vue canvas and general canvas behaviors.Also applies to: 9-9
🏁 Script executed:
#!/bin/bash # Check where useSelectNode is imported and used to understand the impact rg --type ts --type vue "useSelectNode" -A 3 -B 3Length of output: 77
Manual Verification Needed: Confirm
useSelectNode
Integration Across File Types
- The import in
packages/canvas/container/index.ts
has been updated to useuseSelectNode
instead ofuseMultiSelect
, matching the PR's objectives for centralizing node selection.- The original automated check using
rg --type ts --type vue "useSelectNode" -A 3 -B 3
failed due to an unrecognized file type "vue". This may hide relevant occurrence checks in Vue files.- Action Items:
- Re-run the search without the
--type vue
flag for TypeScript files:rg --type ts "useSelectNode" -A 3 -B 3
- For Vue files, add the file type explicitly:
rg --type-add 'vue:*.vue' "useSelectNode" -A 3 -B 3- Manually verify that all references to the old
useMultiSelect
have been updated and that the new node selection logic is consistently applied.packages/toolbars/save/src/js/index.ts (1)
73-73
: Updated canvas selection clearing mechanismThe code now properly destructures
clearSelect
fromcanvasApi.value
and uses the optional chaining operator when calling it. This change aligns with the new selection API whereclearSelect()
is preferred over the previousselectNode(null)
approach.This implementation is more robust as it follows the new selection management pattern introduced in this PR.
Also applies to: 96-96
packages/canvas/container/src/components/CanvasResizeBorder.vue (2)
124-124
: Structured access to position propertiesUpdated to access rectangle properties from
props.selectState.rect
instead of directly fromprops.selectState
, which aligns with the new state structure. This change is part of the broader refactoring effort to centralize selection logic.
154-155
: Improved state destructuring patternThe code now follows a more structured approach by separately destructuring position properties from
selectState.rect
and assigningcomponentName
directly fromselectState
. This change improves readability and maintainability by clearly distinguishing between position data and component metadata.packages/canvas/render/src/runner.ts (1)
79-79
: Plugin chain updated to include GetComponentByDomNode.The application setup correctly includes the new plugin in the chain.
packages/canvas/container/src/components/CanvasHover.vue (1)
1-19
: Good component structure for hover visualization.The template provides distinct visual treatments for active vs. inactive hover states, with the component name displayed in the top-left corner. The active state allows selection via clicking the component name.
packages/canvas/container/src/components/CanvasRouterJumper.vue (1)
45-62
:❓ Verification inconclusive
Simplified hover state watching logic.
The watch function has been streamlined to only monitor the component name from the hover state, making the code more focused and easier to understand.
Ensure that removing the watch on
inactiveHoverState
doesn't affect any edge cases where inactive router links still need to be processed. Run this verification:
🏁 Script executed:
#!/bin/bash # Check if there are any usages of inactiveHoverState with router components rg "inactiveHoverState.*RouterLink" --type ts --type vueLength of output: 86
Below is the updated review comment after additional verification:
Simplified hover state watching logic – Verify Edge Case Handling for Inactive Router Links
The logic for watching only the component name appears solid. However, please note that our automated search (re-run using file globs for both TypeScript and Vue files) found no references to
inactiveHoverState
used in combination withRouterLink
. Nonetheless, please manually verify that removing the watch oninactiveHoverState
doesn’t inadvertently bypass any edge cases where inactive router links still require processing.
- Confirm that no other parts of the codebase rely on
inactiveHoverState
to trigger router link updates.- Double-check any conditional flows that might be affected by this change.
packages/canvas/container/src/components/CanvasViewerSwitcher.vue (1)
124-148
: Improved hover state management with better null checkingThe refactored hover state logic is more straightforward and robust. By removing the inactiveHoverState prop and implementing proper null checking with an early return when the element doesn't exist, the code is now more maintainable and less error-prone.
The validation logic has been improved while maintaining the guarantee that
state.usedHoverState.element
will have a value whenhandleClick
is called, which aligns with established patterns.packages/canvas/container/src/components/CanvasInsertLine.vue (1)
1-7
: Good implementation of the insertion line componentThis new component effectively extracts the insertion line rendering logic into a dedicated component, which aligns with the PR objective to optimize rendering by extracting hover and insertion logic into separate components.
The conditional rendering and class bindings are well implemented to handle various states and positions.
packages/canvas/container/src/components/CanvasAction.vue (2)
3-10
: Improved structure using rect object for dimensionsThe refactored code now consistently accesses positioning properties through the
selectState.rect
object instead of directly fromselectState
. This structural change improves code organization and maintainability.
116-118
: Well-executed transition to hook-based selection APIThe code properly adopts the new hook-based approach by:
- Importing
useSelectNode
instead of directselectNode
function- Using
getDocument()
to access the document context- Calling
selectNodeById
through the hookThis aligns with the PR's goal to centralize selection logic and distinguish between Vue canvas and general canvas behaviors.
Also applies to: 205-207
packages/plugins/tree/src/Main.vue (2)
74-75
: Updated to use the new node selection mechanismThe code has been successfully refactored to use
useSelectNode
instead ofuseMultiSelect
, which aligns with the PR's goal of centralizing selection logic. The computed propertyselectedIds
has been properly updated to map over the new data structure.
202-208
:❓ Verification inconclusive
Note: Node selection after drop has been removed
The selectNode call after insertion has been removed from the handleDrop method. This aligns with the PR objective that mentions "introducing a delay in selecting nodes after insertion to ensure the canvas updates correctly." This behavior change prevents selection issues after dragging nodes.
Verify that there's another mechanism for selecting the node after insertion or that this is an intentional UX change by running:
🏁 Script executed:
#!/bin/bash # Check if there's any code that selects nodes after insertion elsewhere rg -A 3 "insertNode\(" | rg "selectNode"Length of output: 40
Action Required: Confirm Node Selection Behavior
The explicit
selectNode
call has been removed in thehandleDrop
method (packages/plugins/tree/src/Main.vue, lines 202-208) to introduce a delay in node selection, as outlined by the PR objective. However, our initial search did not reveal an alternate mechanism invokingselectNode
after node insertion.
- Removed Code: The
selectNode
call immediately afterinsertNode
has been removed.- Verification Needed: Please verify that node selection is now being handled as intended (e.g., via a delayed mechanism elsewhere) or that this behavior is an intentional UX change to avoid selection issues on drop.
packages/canvas/container/src/interactions/default-interactions.ts (4)
1-20
: Well-documented purpose and limitationsThe file header provides a clear explanation of the default node hover and selection logic for HTML canvases, including its steps and limitations. This is excellent documentation that helps developers understand the purpose and constraints of the implementation.
32-37
: Good initialization patternThe state initialization properly creates a new object with spread operator to avoid reference issues. This ensures that the hover state is initialized with all necessary properties and a clean rectangle object.
91-100
: Conditional hover state updateThe
updateHoverNode
logic correctly checks if a node is already selected before updating the hover state, which prevents hovering over selected elements. This implements one of the PR objectives - fixing the bug where "hovering over components was ineffective when no components were initially selected."
124-180
: Complex selection logic handles both single and multiple selectionThe
updateSelectedNode
function correctly implements the PR objective of handling element selection, including:
- Support for multi-selection with ctrl/meta key
- Special handling for inactive nodes
- Proper scrolling to the selected node
- Emitting appropriate events based on selection state
This effectively addresses the issue where "Element Plus DatePicker and disabled components could not be selected".
packages/canvas/container/src/keyboard.ts (4)
14-17
: Good refactoring to use the new interaction hooksThe import changes correctly reflect the move to using the new selection and hover hooks, which aligns with the PR objective of centralizing selection and hover logic.
28-51
: Consistent refactoring of keyboard handlersAll keyboard navigation handlers (handlerLeft, handlerRight, handlerUp, handlerDown) have been consistently updated to use the new
selectNodeById
function fromuseSelectNode
. This provides a unified approach to selection across the application.
54-68
: Improved delete handler with hover state cleanupThe refactored
handlerDelete
function now properly clears both selection and hover states, and checks if the deleted node is currently being hovered over. This prevents UI artifacts when deleting nodes.
116-130
: Effective clipboard cut handlerThe clipboard cut handler has been properly updated to use the new selection state API, maintaining the same functionality while integrating with the refactored selection system.
packages/canvas/container/src/interactions/vue-rect.ts (4)
1-6
: Well-documented source of inspirationThe file header properly acknowledges the inspiration from Vue.js DevTools, including the repo link and specific file location. This is good practice for attributing external influences.
31-46
: Clean implementation of rectangle creationThe
createRect
function uses computed properties (getters) for width and height, which is a clean approach that ensures these values are always calculated from the current dimensions.
68-97
: Comprehensive fragment rectangle calculationThe
getFragmentRect
function correctly handles different Vue component structures and edge cases, calculating a combined rectangle that encompasses all child elements. This supports the PR objective to "enhance element positioning logic for Vue canvases."
99-117
: Thorough recursive implementation for element rect calculationThe
getElementRectByInstance
function implements a comprehensive recursive approach to handle different Vue instance types, which is essential for accurately determining element positions in a Vue component tree. This supports the PR objective of "allowing selection of disabled components and those that cannot mount the data-uid attribute."packages/canvas/container/src/interactions/common.ts (2)
6-18
: Well-structured interface for hover and selection statesThe
HoverOrSelectState
interface clearly defines the structure needed for tracking hover and selection states, including rectangle dimensions, node reference, and other necessary properties.
21-49
: Comprehensive Vue instance internal structure definitionThe
VueInstanceInternal
andHTMLElementWithVue
interfaces provide detailed typing for Vue component structures, which is essential for the rectangle calculation functions and traversing the component tree.packages/canvas/container/src/CanvasContainer.vue (12)
2-2
: Refactor to use single-state iteration
ReplacingmultiSelectedStates
withselectState
looks consistent with the new logic. Ensurestate.id
is guaranteed to be unique across potential multiple selections in future expansions.
12-15
: New hover and line insertion components
Introducing<canvas-hover>
,<canvas-insert-line>
,<canvas-router-jumper>
, and<canvas-viewer-switcher>
to handle different aspects of interaction is a solid move. Confirm that each component performs lightweight work to avoid potential performance overhead with many children.
64-65
: Additional imports for hover and insert line
Glad to see these new components are being cleanly imported. Ensure they're only registered here if they're not reused in other modules.
79-79
: Switch to composable-based interactions
UsinguseHoverNode
anduseSelectNode
consolidates hover/selection logic into composables. This should improve maintainability.
89-91
: Registering CanvasViewerSwitcher, CanvasHover, and CanvasInsertLine
These additions keep the component registry consistent with newly introduced functionalities. Good job ensuring all references are declared in thecomponents
field.
114-118
: Fallback to default selection state
These checks correctly handle the single node scenario versus a fallback. This keeps UI elements consistent, preventing potential null references if no node is selected.
121-140
: Async node interaction handler
The newhandleNodeInteractions
function correctly updates the selected node and differentiates left/right clicks. However, consider handling the middle-click event (event.button === 1
) or ignoring it explicitly if not supported. Also confirm that errors during async selection (e.g., ifupdateSelectedNode
fails) are gracefully handled.
163-167
: Clearing hover on drag start
Storing drag offset data followed by an immediate hover clear is logical—this helps avoid stale hover states during a fresh drag operation. Keep an eye out for potential user confusion if a slight delay is needed before forcibly clearing hover.
196-199
: Resetting insert states on mousedown
NullifyinginsertPosition
andinsertContainer
prevents lingering insertion panels. Ensure no race conditions withhandleNodeInteractions
if the user quickly toggles multiple insertion actions.
204-213
: Contextmenu event listener
This approach selectively handles right-click logic in the iframe. Skipping the menu when the target is the document element (line 206) allows precise control over the canvas background. Carefully confirm that it aligns with user expectations for a blank-area contextmenu.
251-258
: Mouseover handling and line-state reset
Clearing the drag line when hovering simplifies the logic. Verify no conflicts occur if a user quickly re-engages a drag mid-hover.
293-293
: Dragenter event
Clearing line state ondragenter
helps maintain consistent UI. Confirm that external drags (e.g., from outside the canvas) also behave correctly, especially if partial data is dropped.packages/canvas/container/src/interactions/vue-interactions.ts (7)
1-11
: License header
License and attribution blocks are properly included. No issues here.
13-22
: Documentation for Vue canvas hover logic
Clearly outlining the rationale behind referencing__vueComponent
is helpful. The explanation of how it resolves issues with components that can’t carry adata-uid
attribute ensures future readers understand the approach.
38-56
: Recursive search for nearest Vue instance
getClosedVueElement
effectively climbs the DOM to find the closest__vueComponent
. Watch out for potential cyclical references in unusual conditions. Otherwise, this approach is straightforward.
58-61
: Global hover state
InitializingcurHoverState
as a globalref
is convenient. Confirm that splitting this among multiple canvases (if needed) is handled properly, so states don’t interfere.
146-154
: Clear selection side effects
clearSelect()
resets both local state andcanvasState.current/parent
via'remove'
event. Ensure downstream listeners are aware that'remove'
also signals a selection clear.
160-222
: Updating selected node
updateSelectedNode
properly supports multi-selection with Ctrl/Meta keys. Good approach to skip a “non-active” node ifisInactiveNode
is set. Validate that multi-selection edge cases (e.g., selecting the same node repeatedly) do not cause inconsistent states.
250-259
: Composables for selection
useSelectNode
centralizes selection logic, making it reusable. ReturningdefaultSelectState
frominitialHoverState
is a clever reuse. Keep an eye on potential confusion if “hover” usage diverges from “select” usage in the future.packages/canvas/container/src/container.ts (4)
14-14
: Refactored import
Using consolidated imports from'../../common'
forNODE_UID
andNODE_TAG
is consistent with other changes.
20-20
: Hover and select composables
Switching touseHoverNode
anduseSelectNode
from older multi-select patterns. This aligns the container’s logic with the new approach.
316-316
: Selection clearing on node removal
After removing a node, callingclearSelect
ensures no stale references remain. Perfect for avoiding residual highlights or errors.
832-847
: canvasApi abstraction
Exposing new wrappers likeclearSelect
,selectNodeById
, andhoverNodeById
is consistent with the official hooks. Perfectly keeps a single entry point for external systems.
packages/canvas/container/src/interactions/default-interactions.ts
Outdated
Show resolved
Hide resolved
packages/canvas/container/src/interactions/default-interactions.ts
Outdated
Show resolved
Hide resolved
packages/canvas/container/src/interactions/default-interactions.ts
Outdated
Show resolved
Hide resolved
…tioning and interaction methods 重构: 优化 Vue 画布交互逻辑,提取元素定位和交互方法
bc010f1
to
59b9695
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.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
packages/canvas/container/src/CanvasContainer.vue (2)
121-141
:⚠️ Potential issueDrag logic only works for the first selected node – multi‑select is ignored
handleNodeInteractions
always dereferencesselectState.value[0]
, even when the user Ctrl/⌘‑clicks to build a multi‑selection (isMultipleSelect === true
).
This will:
- Use the wrong rectangle when the primary click occurred on a node that is not the first in the array.
- Prevent intuitive drag actions for multi‑selection (TODO left in place).
Consider:
- const node = selectState.value[0]?.node + const primaryState = selectState.value.find(({ element }) => element === event.target) + const node = primaryState?.nodeand bail out early if no suitable state is found.
Alternatively, disable drag until a dedicated multi‑drag implementation lands to avoid broken UX.
314-320
:⚠️ Potential issueSeveral iframe‑scoped listeners are never removed – potential memory leak
onUnmounted
only detaches the hot‑key and message listeners. The handlers added incanvasReady
(mousedown
,contextmenu
,scroll
,dragover
,drop
,mouseover
, etc.) stay bound to the
iframe’scontentWindow
, surviving hot reloads and keeping large reactive graphs alive.Expose an
offCanvasReady()
helper mirroringcanvasReady()
and invoke it insideonUnmounted
after verifyingiframe.value
.
Failing to do so will gradually degrade performance during long editing sessions.
♻️ Duplicate comments (1)
packages/canvas/container/src/interactions/vue-interactions.ts (1)
58-61
:structuredClone
again needs a polyfillSame portability concern as raised for
common.ts
. Factor out the fallback util or import from a
shared helper module to avoid duplication.
🧹 Nitpick comments (11)
packages/canvas/container/src/components/CanvasHover.vue (3)
6-6
: Avoid creating a new function on every renderWriting an inline arrow function inside the template means Vue has to create a brand‑new callback on every render, which slightly hurts performance and breaks template caching. You can simplify the markup and avoid the extra allocation:
- <div class="corner-mark-left" @click="(e) => handleSelectHoverNode(e)"> + <div class="corner-mark-left" @click="handleSelectHoverNode">
31-48
: Call the composable once, not inside the event handler
useSelectNode()
is a composable; calling it inside the click handler violates the “call composables at the top ofsetup
” guideline and re‑evaluates dependency injection each time. Cache its return value once and reuse it:setup(props) { - const handleSelectHoverNode = (e) => { - const node = props.hoverState.node - const element = props.hoverState.element - - if (!node) { - return - } - - const { selectNodeById, updateSelectedNode } = useSelectNode() + const { selectNodeById, updateSelectedNode } = useSelectNode() + + const handleSelectHoverNode = (e) => { + const node = props.hoverState.node + const element = props.hoverState.element + + if (!node) { + return + }This keeps composable usage compliant and removes redundant work at runtime.
9-17
: Hard‑coded Chinese label breaks i18n
拖放元素到容器内
is baked into the template, preventing localisation. Consider pulling the text from an i18n resource or providing a prop so consumers can translate it.packages/canvas/container/src/keyboard.ts (1)
28-37
: Cache selection helpers to avoid repeated composable look‑ups
useSelectNode()
is invoked inside every arrow‑key handler. Although the composable is idempotent, the repeated calls are unnecessary. Capture its return value once at module scope:const { selectNodeById } = useSelectNode() function handlerLeft({ parent }) { selectNodeById(parent?.id, '', false) } …This trim downs function overhead for every key press.
packages/canvas/container/src/CanvasContainer.vue (2)
251-259
: High‑frequencymouseover
handler can flood the render pipelineReplacing the old
mousemove
withmouseover
still fires on every DOM edge crossing and quickly saturates the event loop on dense component trees.
A small throttle (≈ 16 ms) keeps the UI responsive without sacrificing precision:- win.addEventListener('mouseover', (ev) => { - ... - }) + const handleHover = throttle((ev) => { + updateHoverNode(ev) + lineState.position = '' + lineState.width = 0 + }, 16) + win.addEventListener('mouseover', handleHover)(You already depend on
lodash-es
elsewhere –throttle
is available.)
112-118
:multiStateLength
is a computed ref but the prop still reads “multi”With the new single‑source
selectState
,multiStateLength
is simply
selectState.value.length
. The name suggests multi‑selection only; if that is the intention keep
it, otherwise consider renaming toselectedCount
for clarity.packages/canvas/container/src/interactions/vue-interactions.ts (2)
118-126
: Optional chaining can shorten the guard clauseStatic analysis already hinted at this; applying it makes the intent clearer:
-if (!res || (res?.node?.id && selectState.value.some((state) => state?.node?.id === res.node.id))) { +if (!res || res.node?.id && selectState.value.some((s) => s.node?.id === res.node.id)) {No behavioural change, just readability.
217-237
: Relying on nakedsetTimeout
for rect refresh is fragile
setTimeout(..., 0)
runs after every micro‑task; rapid successive DOM
mutations will queue many redundant jobs.
Consider usingnextTick
(already imported in container.ts) or a debounced job via
requestAnimationFrame
.-const updateSelectedRect = (): void => { - setTimeout(() => { +const updateSelectedRect = (): void => { + queueMicrotask(() => { ... - }, 0) + }) }packages/canvas/container/src/container.ts (3)
480-483
: Avoid instantiating hooks in the hot drag‑move loop
updateLineState
is executed on everydragMove
. Each time it callsuseHoverNode()
which, while memoised internally, still incurs function overhead and obscures intent.Extract the hook result once at module scope (or cache it in the outer closure) and reuse it:
-const updateLineState = (element?: Element, data?: Node | null) => { - if (!element) { - const { clearHover } = useHoverNode() +const { clearHover } = useHoverNode() + +const updateLineState = (element?: Element, data?: Node | null) => { + if (!element) {A small optimisation that keeps the hot‑path lean.
551-556
: Guard againstundefined
IDs when syncing hover state
hoverNodeById
is called unconditionally withcurHoverState.value?.node?.id
.
When no node is currently hovered,id
isundefined
, resulting in a redundant DOM query ([data-uid="undefined"]
). Add a simple guard:- const { hoverNodeById, curHoverState } = useHoverNode() - hoverNodeById(curHoverState.value?.node?.id) + const { hoverNodeById, curHoverState } = useHoverNode() + const hoverId = curHoverState.value?.node?.id + if (hoverId) { + hoverNodeById(hoverId) + }This prevents unnecessary work and yields clearer intent.
831-846
: Memoise hook wrappers insidecanvasApi
canvasApi.clearSelect
,selectNodeById
, andhoverNodeById
create a new wrapper on every call, internally re‑enteringuseSelectNode
/useHoverNode
. Although the hooks themselves singleton‑cache their impl, wrapping once at definition time is simpler and marginally faster:- clearSelect: () => { - const { clearSelect } = useSelectNode() - return clearSelect() - }, + clearSelect: useSelectNode().clearSelect, - selectNodeById: (id: string, type: string, isMultipleSelect = false) => { - const { selectNodeById } = useSelectNode() - return selectNodeById(id, type, isMultipleSelect) - }, + selectNodeById: useSelectNode().selectNodeById, - hoverNodeById: (id: string) => { - const { hoverNodeById } = useHoverNode() - return hoverNodeById(id) - }, + hoverNodeById: useHoverNode().hoverNodeById,Makes the API thinner and avoids per‑call indirection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
designer-demo/engine.config.js
(1 hunks)designer-demo/public/mock/bundle.json
(2 hunks)packages/canvas/container/index.ts
(1 hunks)packages/canvas/container/src/CanvasContainer.vue
(9 hunks)packages/canvas/container/src/components/CanvasAction.vue
(6 hunks)packages/canvas/container/src/components/CanvasDivider.vue
(2 hunks)packages/canvas/container/src/components/CanvasHover.vue
(1 hunks)packages/canvas/container/src/components/CanvasInsertLine.vue
(1 hunks)packages/canvas/container/src/components/CanvasResizeBorder.vue
(2 hunks)packages/canvas/container/src/components/CanvasRouterJumper.vue
(1 hunks)packages/canvas/container/src/components/CanvasViewerSwitcher.vue
(1 hunks)packages/canvas/container/src/composables/useMultiSelect.ts
(0 hunks)packages/canvas/container/src/container.ts
(15 hunks)packages/canvas/container/src/interactions/common.ts
(1 hunks)packages/canvas/container/src/interactions/html-interactions.ts
(1 hunks)packages/canvas/container/src/interactions/index.ts
(1 hunks)packages/canvas/container/src/interactions/vue-interactions.ts
(1 hunks)packages/canvas/container/src/interactions/vue-rect.ts
(1 hunks)packages/canvas/container/src/keyboard.ts
(5 hunks)packages/canvas/render/src/render.ts
(0 hunks)packages/canvas/render/src/runner.ts
(2 hunks)packages/plugins/tree/src/Main.vue
(3 hunks)packages/toolbars/save/src/js/index.ts
(2 hunks)
💤 Files with no reviewable changes (2)
- packages/canvas/render/src/render.ts
- packages/canvas/container/src/composables/useMultiSelect.ts
✅ Files skipped from review due to trivial changes (2)
- packages/canvas/container/src/interactions/index.ts
- packages/canvas/container/src/interactions/vue-rect.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/canvas/container/src/components/CanvasResizeBorder.vue
- designer-demo/engine.config.js
- packages/toolbars/save/src/js/index.ts
- packages/canvas/render/src/runner.ts
- packages/canvas/container/src/components/CanvasDivider.vue
- packages/canvas/container/src/components/CanvasViewerSwitcher.vue
- designer-demo/public/mock/bundle.json
- packages/canvas/container/src/components/CanvasRouterJumper.vue
- packages/plugins/tree/src/Main.vue
- packages/canvas/container/index.ts
- packages/canvas/container/src/components/CanvasInsertLine.vue
- packages/canvas/container/src/components/CanvasAction.vue
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/canvas/container/src/interactions/html-interactions.ts (5)
packages/canvas/container/src/interactions/common.ts (7)
HoverOrSelectState
(6-18)initialHoverState
(51-62)clearHover
(64-66)getClosedElementHasUid
(68-91)getWindowRect
(93-102)hoverNodeById
(104-110)selectNodeById
(112-123)packages/register/src/hooks.ts (1)
useCanvas
(77-77)packages/canvas/container/src/container.ts (5)
getConfigure
(321-332)canvasState
(53-64)getDocument
(70-70)scrollToNode
(359-384)querySelectById
(334-352)packages/canvas/container/src/interactions/vue-interactions.ts (3)
updateHoverNode
(118-126)useHoverNode
(208-215)useSelectNode
(239-248)packages/canvas/container/src/interactions/index.ts (2)
useHoverNode
(42-48)useSelectNode
(50-56)
packages/canvas/container/src/interactions/common.ts (2)
packages/canvas/container/src/container.ts (2)
getWindow
(72-72)querySelectById
(334-352)packages/canvas/container/src/interactions/vue-interactions.ts (1)
updateHoverNode
(118-126)
packages/canvas/container/src/container.ts (7)
packages/canvas/container/src/interactions/index.ts (2)
useHoverNode
(42-48)useSelectNode
(50-56)packages/canvas/container/src/interactions/html-interactions.ts (2)
useHoverNode
(107-114)useSelectNode
(212-221)packages/canvas/render/src/render.ts (1)
getController
(321-321)packages/register/src/hooks.ts (1)
useCanvas
(77-77)packages/canvas/container/src/interactions/common.ts (3)
clearHover
(64-66)hoverNodeById
(104-110)selectNodeById
(112-123)packages/canvas/render/src/material-function/configure.ts (1)
configure
(1-1)packages/canvas/types.ts (1)
Node
(1-6)
packages/canvas/container/src/interactions/vue-interactions.ts (7)
packages/canvas/container/src/interactions/common.ts (8)
HTMLElementWithVue
(46-49)VueInstanceInternal
(21-43)HoverOrSelectState
(6-18)initialHoverState
(51-62)clearHover
(64-66)getWindowRect
(93-102)hoverNodeById
(104-110)selectNodeById
(112-123)packages/canvas/container/src/interactions/vue-rect.ts (1)
getElementRectByInstance
(99-117)packages/register/src/hooks.ts (1)
useCanvas
(77-77)packages/canvas/render/src/material-function/configure.ts (1)
configure
(1-1)packages/canvas/container/src/container.ts (5)
getConfigure
(321-332)canvasState
(53-64)getDocument
(70-70)scrollToNode
(359-384)querySelectById
(334-352)packages/canvas/container/src/interactions/index.ts (2)
useHoverNode
(42-48)useSelectNode
(50-56)packages/canvas/container/src/interactions/html-interactions.ts (2)
useHoverNode
(107-114)useSelectNode
(212-221)
🪛 Biome (1.9.4)
packages/canvas/container/src/interactions/vue-interactions.ts
[error] 116-119: 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 (4)
packages/canvas/container/src/interactions/html-interactions.ts (1)
35-41
:structuredClone
might not be available in all build targets
structuredClone
is still missing in Node ≤ 16 and some browsers behind enterprise evergreen.
If your tool‑chain transpiles only TS, you may need a ponyfill (e.g.,import 'core-js/actual/structured-clone'
) or fall back toJSON.parse(JSON.stringify(obj))
for maximum compatibility.packages/canvas/container/src/container.ts (3)
321-333
: Guard against undefined component names & missing materials ingetConfigure
getConfigure
can be invoked with anundefined
targetName
(see the new call site inquerySelectById
).
When that happens,controller.getMaterial(undefined)
will run, and the subsequent optional‑chaining returns{}
– this is fine, but the intent is unclear and it silently hides a potential misuse. Adding an explicit early‑return improves readability and prevents accidental calls with falsy names.export const getConfigure = (targetName: string) => { - const material = getController().getMaterial(targetName) + if (!targetName) { + // Gracefully degrade instead of passing an invalid name down the line + return {} + } + + const material = getController().getMaterial(targetName)[ suggest_essential_refactor ]
335-350
:rootSelector
fallback may select the wrong element when multiple instances existThe TODO comment highlights the risk:
doc.querySelector(rootSelector)
picks the first match, which is unreliable once the same component is rendered several times.
Consider:
querySelectorAll(rootSelector)
to fetch all matches.- Narrow the result set (e.g., by traversing from the hovered target toward ancestors, or by comparing against runtime‐kept IDs).
- Fall back only when the original lookup failed and only one unambiguous match exists.
Failing to disambiguate may cause hover/selection to jump to a sibling instance.
If you need help devising a deterministic strategy, let me know—happy to sketch one.
[ suggest_optional_refactor ]
386-392
:✅ Verification successful
updateRect
signature changed – ensure all call sites are updated
updateRect
no longer accepts anid
parameter. Please double‑check for stale invocations elsewhere in the codebase that still pass an argument; TypeScript will error during build, but a quick grep avoids surprises.
🏁 Script executed:
#!/bin/bash # Find external call sites that still pass parameters to updateRect rg -n "updateRect\\(" | grep -v "container.ts"Length of output: 1047
🏁 Script executed:
#!/bin/bash # Find call sites of updateRect with any arguments (non-empty parentheses) rg -n 'updateRect\(\s*\S' | grep -v "container.ts"Length of output: 1052
No stale
updateRect
arguments detected—change is safeI ran a repo‑wide search for any
updateRect(...)
calls passing parameters and found none. All usages invokeupdateRect()
with empty parentheses, so the updated signature is already reflected everywhere.
packages/canvas/container/src/interactions/html-interactions.ts
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/canvas/container/src/interactions/vue-interactions.ts (1)
64-118
:⚠️ Potential issueUnbounded
while
loop may hang on malformed component trees.
getRectAndNode
climbs the Vue-instance chain until it finds an instance withNODE_UID
/NODE_TAG === 'RouterView'
. If the chain is cyclic or extremely deep (e.g. large recursive components) this could freeze the UI.Add a depth guard to prevent potential infinite loops:
let closedVueEle: VueInstanceInternal | undefined = instance +let depth = 0 +const MAX_DEPTH = 100 // Reasonable limit to prevent infinite loops -while (closedVueEle && !(closedVueEle.attrs?.[NODE_UID] || closedVueEle.attrs?.[NODE_TAG] === 'RouterView')) { +while (closedVueEle && + !(closedVueEle.attrs?.[NODE_UID] || closedVueEle.attrs?.[NODE_TAG] === 'RouterView') && + depth < MAX_DEPTH) { closedVueEle = closedVueEle.parent + depth++ } +if (depth >= MAX_DEPTH) { + console.warn('Maximum depth reached when searching for component with NODE_UID or RouterView tag') + res.rect = { ...windowRect } + return res +}
🧹 Nitpick comments (3)
packages/canvas/container/src/CanvasContainer.vue (2)
126-126
: TODO comment needs to be addressed.This TODO indicates there's still work needed to support multi-selection dragging logic, which aligns with the PR objectives to improve Vue canvas interaction.
Would you like help implementing the multi-selection drag functionality?
309-312
: Redundant method needs cleanup.The
selectSlot
method has been stripped of its functionality (commented out code) but is still exported and called. Since it no longer does anything meaningful, consider removing it or documenting why it's kept as a no-op.-// TODO: 需要确认下该事件还是否需要 -const selectSlot = (_slotName) => { - // hoverState.slot = slotName -} +// If this method is truly needed as a placeholder for future implementation: +const selectSlot = (slotName) => { + // Method intentionally left empty until slot selection requirements are confirmed + console.debug('selectSlot called with:', slotName); +} + +// Or if it's not needed at all, consider removing it and its referencespackages/canvas/container/src/interactions/vue-interactions.ts (1)
120-128
: Use optional chaining for safer null handling.The condition could be simplified and made safer by using optional chaining.
-if (!res || (res?.node?.id && selectState.value.some((state) => state?.node?.id === res.node.id))) { +if (!res || (res?.node?.id && selectState.value.some((state) => state?.node?.id === res?.node?.id))) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/canvas/container/src/CanvasContainer.vue
(9 hunks)packages/canvas/container/src/interactions/common.ts
(1 hunks)packages/canvas/container/src/interactions/html-interactions.ts
(1 hunks)packages/canvas/container/src/interactions/vue-interactions.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/canvas/container/src/interactions/html-interactions.ts
- packages/canvas/container/src/interactions/common.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/canvas/container/src/interactions/vue-interactions.ts
[error] 118-121: 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 (17)
packages/canvas/container/src/CanvasContainer.vue (10)
2-2
: Good fix for the key issue in v-for!This properly implements the recommendation from the previous review by using
state.node?.id || state.componentName
instead ofstate.id
, ensuring unique and stable keys for Vue's rendering optimization.
12-15
: Well-structured component separation for improved maintainability.Moving hover, insertion line, router jumper, and viewer switcher functionality into separate components follows good component design principles and separation of concerns.
79-79
: Good architectural improvement with composable hooks.Using specialized composable hooks (
useHoverNode
anduseSelectNode
) centralizes state management and interaction logic, making the code more maintainable and testable.
110-112
: Improved state management with composable hooks.The refactoring replaces direct state management with composable hooks, providing better encapsulation and reusability of selection and hover logic.
121-141
: Good consolidation of node interaction logic.
handleNodeInteractions
centralizes previously scattered interaction logic into a single, well-organized method. This improves code readability and maintainability.
197-201
: Improved event handling with the consolidated interaction method.The mousedown event handler now properly clears insertion states before delegating to the centralized
handleNodeInteractions
method, making the flow clearer.
205-214
: Right-click selection now works correctly for disabled components.This implementation now allows right-click selection of nodes even when components are disabled, addressing one of the PR objectives.
218-227
: Improved scroll handling with debounce.Adding a timeout to manage the scrolling state prevents potential race conditions and improves user experience during scroll operations.
251-259
: Mouse event optimization from mousemove to mouseover.Switching from
mousemove
tomouseover
is more efficient as it reduces the number of event triggers while still providing the needed hover functionality.
344-345
: Proper state exposure in the component.The new state variables are correctly exposed in the return object, making them available to the template and child components.
packages/canvas/container/src/interactions/vue-interactions.ts (7)
59-62
: Good state initialization using Vue's reactivity system.The reactive state setup properly leverages Vue's ref API for managing hover and selection states, making them reactive throughout the component tree.
39-57
: Well-implemented DOM traversal to find Vue components.The
getClosedVueElement
function efficiently traverses the DOM tree to find Vue component instances, enabling proper interaction with Vue components in the canvas.
144-204
: Well-implemented selection logic with multi-select support.The
updateSelectedNode
function correctly handles both single and multiple selection modes, properly manages inactive nodes, and emits appropriate events. This addresses the PR objective of refactoring selection logic.
219-239
: Smart rectangle update with setTimeout to handle DOM changes.Using
setTimeout
to update rectangle coordinates after DOM changes is a good practice as it ensures the DOM has been fully updated before measuring. The code also handles cases where elements might have been removed.
210-217
: Clean composable API for hover state management.The
useHoverNode
composable provides a well-designed API with clear methods for managing hover state, following Vue's composition API best practices.
241-250
: Clean composable API for selection state management.The
useSelectNode
composable provides a well-designed API with clear methods for managing selection state, following Vue's composition API best practices.
152-167
: Good handling of inactive nodes in selection logic.The code correctly differentiates between regular nodes and inactive nodes (those not in the current schema), providing appropriate fallback behavior in each case.
English | 简体中文
PR 重构: 优化 Vue 画布交互逻辑,提取元素定位和交互方法
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?
【功能修改点说明】:
【重构修改说明】
isInactiveNode
进行区分当前 hover 的是否是当前画布的节点。CanvasAction
组件中分别抽离到CanvasHover
、CanvasInsertLine
组件,避免在多选的情况下,重复渲染多个 hover 节点。【通用画布元素定位新逻辑说明】
getBoundingClientRect
计算DOM 节点的矩形信息。适配:与画布技术栈无关,通用性强
缺陷:如果画布无法挂载 data-uid 属性到 DOM 节点上,那么该节点无法反查到对应的 node 节点,导致 hover 、选中等逻辑无法生效。
【Vue 画布元素定位新逻辑说明】
前置依赖:需要 vue 画布注入
__vueComponent
到当前 DOM 节点中。__vueComponent
的节点,通过 __vueComponent 获取到 vue 实例。Does this PR introduce a breaking change?
Other information
测试页面 schema:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores