refact: getRect logic #1132
refact: getRect logic #1132
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR updates several canvas modules by refining how hover states and node selections are handled. In the CanvasContainer component, mouse event handling has been modified to use a newly imported hover state. A set of helper functions and reactive variables is added in the component-selection module to support Vue component detection and bounding rectangle computations. Additionally, container logic and event handling in drag-and-drop operations are updated to better integrate the new hover state functions. The Vue application initialization is also enhanced with a new mixin that attaches component instances to DOM elements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CanvasContainer
participant CompSel
participant VueState
User->>CanvasContainer: Mouseover event
CanvasContainer->>CompSel: updateHoverNode(event)
CompSel->>VueState: Update currentHoverNode, currentHoverInstance, currentHoverRect
VueState-->>CanvasContainer: Return updated hover state
sequenceDiagram
participant AppCreator
participant VueApp
participant GetComponentByDomNode
participant TinyI18nHost
AppCreator->>VueApp: createApp(Main)
VueApp->>GetComponentByDomNode: Install mixin
GetComponentByDomNode->>VueApp: Attach __vueComponent on mount
VueApp->>TinyI18nHost: Use translation host
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/canvas/container/src/container.js (1)
543-619: 🛠️ Refactor suggestionClean up debug logs and implement drag logic.
Several issues need attention:
- Remove console.log statements
- Implement the TODO for drag logic
- The hover state updates are commented out without clear replacement
- console.log('updateLineState element', element) - console.log('updateLineState', element, data)Would you like me to help implement the drag logic or create an issue to track this task?
🧹 Nitpick comments (5)
packages/canvas/container/src/component-selection.js (2)
43-136: Optimize rectangle calculation performance.The rectangle calculation functions create new objects and perform multiple DOM operations. Consider caching results and minimizing reflows.
Consider these optimizations:
+const rectCache = new WeakMap() export const getElementRect = (instance) => { + if (rectCache.has(instance)) { + return rectCache.get(instance) + } let rect if (instance?.type?.description === 'v-fgt') { rect = getFragmentRect(instance) } else if (instance.el?.nodeType === 1) { rect = instance.el.getBoundingClientRect() } // ... rest of the conditions + rectCache.set(instance, rect) return rect } +// Clear cache when needed +export const clearRectCache = () => { + rectCache = new WeakMap() +}
138-190: Enhance hover state management with debouncing.The hover state updates could trigger frequent recalculations. Consider debouncing the updates.
Add debouncing to hover updates:
+import { ref } from 'vue' + +const debounce = (fn, wait) => { + let timeout + return (...args) => { + clearTimeout(timeout) + timeout = setTimeout(() => fn(...args), wait) + } +} export const updateHoverNode = debounce((e) => { // ... existing implementation }, 16) // ~60fpspackages/canvas/container/src/CanvasContainer.vue (1)
214-224: Remove commented-out mousemove listener.The mousemove event listener is commented out but should be removed since it's replaced by mouseover handling.
Remove the commented code:
-// win.addEventListener('mousemove', (ev) => { -// handleCanvasEvent(() => { -// dragMove(ev, true) -// }) -// })packages/canvas/container/src/container.js (2)
215-237: Remove commented out code.Since
getElementis now imported from the component-selection module, this commented-out implementation should be removed to maintain code cleanliness.-// export const getElement = (element) => { -// // 如果当前元素是body -// if (element === element.ownerDocument.body) { -// return element -// } - -// // 如果当前元素是画布的html，返回画布的body -// if (element === element.ownerDocument.documentElement) { -// return element.ownerDocument.body -// } - -// if (!element || element.nodeType !== 1) { -// return undefined -// } - -// if (element.getAttribute(NODE_UID)) { -// return element -// } else if (element.parentElement) { -// return getElement(element.parentElement) -// } - -// return undefined -// }
342-349: Address the TODO for rootSelector + loopId handling.The fallback mechanism using rootSelector is a good addition, but the TODO indicates that the interaction between rootSelector and loopId needs to be implemented.
Would you like me to help implement the rootSelector + loopId handling logic or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/canvas/container/src/CanvasContainer.vue(4 hunks)packages/canvas/container/src/component-selection.js(1 hunks)packages/canvas/container/src/container.js(11 hunks)packages/canvas/render/src/render.ts(2 hunks)packages/canvas/render/src/runner.ts(2 hunks)
🔇 Additional comments (5)
packages/canvas/render/src/runner.ts (2)
48-58: Consider potential memory leaks in the Vue mixin.The mixin stores Vue component instances in DOM elements via
__vueComponent. While this enables component lookup, it could prevent garbage collection of component instances if the DOM elements persist longer than expected.Consider cleaning up the reference in the
unmountedhook:const GetComponentByDomNode = { install: (Vue) => { Vue.mixin({ mounted() { this.$el.__vueComponent = this?._ + }, + unmounted() { + if (this.$el) { + delete this.$el.__vueComponent + } } }) } }
76-76: LGTM! The app creation sequence is correct.The order of plugin installation is appropriate, with
GetComponentByDomNodeprecedingTinyI18nHost.packages/canvas/render/src/render.ts (1)
101-101: LGTM! Removal of mouseover event binding is correct.The removal of
bindProps.onMouseover = stopEventaligns with the new hover state management implemented incomponent-selection.js.packages/canvas/container/src/container.js (2)
27-30: LGTM! Good modularization of element-related functionality.Moving element-related functions to a separate module improves code organization and maintainability.
743-764: LGTM! Good enhancement to support instance-based selection.The function has been properly updated to handle both element and instance-based selection, maintaining compatibility with existing code while adding new functionality.
| export const getClosedVueElement = (element) => { | ||
| if (!element) { | ||
| return element | ||
| } | ||
|
|
||
| if (element === element.ownerDocument.body) { | ||
| return element | ||
| } | ||
|
|
||
| if (element.__vueComponent) { | ||
| return element.__vueComponent | ||
| } | ||
|
|
||
| if (element.parentElement) { | ||
| return getClosedVueElement(element.parentElement) | ||
| } | ||
| } | ||
|
|
||
| export const getElement = (element) => { | ||
| let closedVueEle = getClosedVueElement(element) | ||
|
|
||
| if (!closedVueEle) { | ||
| return | ||
| } | ||
|
|
||
| while(closedVueEle && !closedVueEle?.[NODE_UID]) { | ||
| closedVueEle = closedVueEle.parent | ||
| } | ||
|
|
||
| if (closedVueEle) { | ||
| return closedVueEle | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in component lookup functions.
The recursive component lookup functions lack proper error boundaries and type checking.
Add type checks and error handling:
export const getClosedVueElement = (element) => {
+ if (!(element instanceof Element) && element !== null) {
+ return null
+ }
+
if (!element) {
return element
}
// ... rest of the function
}
export const getElement = (element) => {
+ if (!(element instanceof Element)) {
+ return null
+ }
+
let closedVueEle = getClosedVueElement(element)
// ... rest of the function
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getClosedVueElement = (element) => { | |
| if (!element) { | |
| return element | |
| } | |
| if (element === element.ownerDocument.body) { | |
| return element | |
| } | |
| if (element.__vueComponent) { | |
| return element.__vueComponent | |
| } | |
| if (element.parentElement) { | |
| return getClosedVueElement(element.parentElement) | |
| } | |
| } | |
| export const getElement = (element) => { | |
| let closedVueEle = getClosedVueElement(element) | |
| if (!closedVueEle) { | |
| return | |
| } | |
| while(closedVueEle && !closedVueEle?.[NODE_UID]) { | |
| closedVueEle = closedVueEle.parent | |
| } | |
| if (closedVueEle) { | |
| return closedVueEle | |
| } | |
| } | |
| export const getClosedVueElement = (element) => { | |
| if (!(element instanceof Element) && element !== null) { | |
| return null | |
| } | |
| if (!element) { | |
| return element | |
| } | |
| if (element === element.ownerDocument.body) { | |
| return element | |
| } | |
| if (element.__vueComponent) { | |
| return element.__vueComponent | |
| } | |
| if (element.parentElement) { | |
| return getClosedVueElement(element.parentElement) | |
| } | |
| } | |
| export const getElement = (element) => { | |
| if (!(element instanceof Element)) { | |
| return null | |
| } | |
| let closedVueEle = getClosedVueElement(element) | |
| if (!closedVueEle) { | |
| return | |
| } | |
| while (closedVueEle && !closedVueEle?.[NODE_UID]) { | |
| closedVueEle = closedVueEle.parent | |
| } | |
| if (closedVueEle) { | |
| return closedVueEle | |
| } | |
| } |
| const setCurrentNode = async (event) => { | ||
| const { clientX, clientY } = event | ||
| const element = getElement(event.target) | ||
| // const element = getElement(event.target) | ||
| closeMenu() | ||
| let node = getCurrent().schema | ||
| // let node = getCurrent().schema | ||
| let node = currentHoverNode.value | ||
|
|
||
| if (element) { | ||
| const currentElement = querySelectById(getCurrent().schema?.id) | ||
| if (node) { | ||
| // const currentElement = querySelectById(getCurrent().schema?.id) | ||
| // const currentElement = element | ||
|
|
||
| if (!currentElement?.contains(element) || event.button === 0) { | ||
| const loopId = element.getAttribute(NODE_LOOP) | ||
| if (loopId) { | ||
| node = await selectNode(element.getAttribute(NODE_UID), `loop-id=${loopId}`) | ||
| } else { | ||
| node = await selectNode(element.getAttribute(NODE_UID)) | ||
| } | ||
| } | ||
| // const loopId = element[NODE_LOOP] | ||
|
|
||
| if (event.button === 0 && element !== element.ownerDocument.body) { | ||
| const { x, y } = element.getBoundingClientRect() | ||
| // if (loopId) { | ||
| // node = await selectNode(element.getAttribute(NODE_UID), element, `loop-id=${loopId}`) | ||
| // } else { | ||
| // } | ||
|
|
||
| node = await selectNode(node.id, currentHoverInstance.value) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify node selection logic.
The node selection logic has been updated to use currentHoverNode but retains commented-out code.
Clean up the implementation:
const setCurrentNode = async (event) => {
const { clientX, clientY } = event
- // const element = getElement(event.target)
closeMenu()
- // let node = getCurrent().schema
let node = currentHoverNode.value
if (node) {
- // const currentElement = querySelectById(getCurrent().schema?.id)
- // const currentElement = element
- // const loopId = element[NODE_LOOP]
- // if (loopId) {
- // node = await selectNode(element.getAttribute(NODE_UID), element, `loop-id=${loopId}`)
- // } else {
- // }
node = await selectNode(node.id, currentHoverInstance.value)
// ... rest of the function
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setCurrentNode = async (event) => { | |
| const { clientX, clientY } = event | |
| const element = getElement(event.target) | |
| // const element = getElement(event.target) | |
| closeMenu() | |
| let node = getCurrent().schema | |
| // let node = getCurrent().schema | |
| let node = currentHoverNode.value | |
| if (element) { | |
| const currentElement = querySelectById(getCurrent().schema?.id) | |
| if (node) { | |
| // const currentElement = querySelectById(getCurrent().schema?.id) | |
| // const currentElement = element | |
| if (!currentElement?.contains(element) || event.button === 0) { | |
| const loopId = element.getAttribute(NODE_LOOP) | |
| if (loopId) { | |
| node = await selectNode(element.getAttribute(NODE_UID), `loop-id=${loopId}`) | |
| } else { | |
| node = await selectNode(element.getAttribute(NODE_UID)) | |
| } | |
| } | |
| // const loopId = element[NODE_LOOP] | |
| if (event.button === 0 && element !== element.ownerDocument.body) { | |
| const { x, y } = element.getBoundingClientRect() | |
| // if (loopId) { | |
| // node = await selectNode(element.getAttribute(NODE_UID), element, `loop-id=${loopId}`) | |
| // } else { | |
| // } | |
| node = await selectNode(node.id, currentHoverInstance.value) | |
| const setCurrentNode = async (event) => { | |
| const { clientX, clientY } = event | |
| closeMenu() | |
| let node = currentHoverNode.value | |
| if (node) { | |
| node = await selectNode(node.id, currentHoverInstance.value) | |
| // ... rest of the function | |
| } | |
| } |
| export const hoverNode = (id, data) => { | ||
| const element = querySelectById(id) | ||
| element && setHoverRect(element, data) | ||
| // element && updateLineState(element, data) | ||
| if (element) { | ||
| updateHoverNode({ target: element }) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clean up commented code and verify hover behavior.
The hover state management has been updated to use updateHoverNode, but the old implementation is still present in comments.
- // element && updateLineState(element, data)Run this script to verify all hover state updates use the new system:
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining uses of old hover state management
rg "updateLineState.*element.*data" -A 2Length of output: 708
Clean up unused commented code and verify hover behavior
- Remove the commented-out line (
// element && updateLineState(element, data)) in thehoverNodefunction located inpackages/canvas/container/src/container.js. - Confirm that the new hover behavior implemented via
updateHoverNodeworks as expected. - Additionally, review the
updateLineStatefunction: if it’s no longer needed elsewhere, consider removing it to avoid confusion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const hoverNode = (id, data) => { | |
| const element = querySelectById(id) | |
| element && setHoverRect(element, data) | |
| // element && updateLineState(element, data) | |
| if (element) { | |
| updateHoverNode({ target: element }) | |
| } | |
| } | |
| export const hoverNode = (id, data) => { | |
| const element = querySelectById(id) | |
| if (element) { | |
| updateHoverNode({ target: element }) | |
| } | |
| } |
| const setSelectRect = (element, instance) => { | ||
| let calcElement = element | ||
|
|
||
| if (!element && !instance) { | ||
| calcElement = getDocument().body | ||
| } | ||
|
|
||
| let left = 0 | ||
| let top = 0 | ||
| let width = 0 | ||
| let height = 0 | ||
|
|
||
| if (calcElement) { | ||
| const { left: l, top: t, width: w, height: h } = getRect(calcElement) | ||
|
|
||
| left = l | ||
| top = t | ||
| width = w | ||
| height = h | ||
| } | ||
|
|
||
| if (instance) { | ||
| const { left: l, top: t, width: w, height: h } = getElementRect(instance) | ||
| left = l | ||
| top = t | ||
| width = w | ||
| height = h | ||
| } | ||
|
|
||
| const { left, height, top, width } = getRect(element) | ||
|
|
||
| // const { left, height, top, width } = getRect(element) | ||
| const componentName = getCurrent().schema?.componentName || '' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clean up commented code and verify instance-based calculations.
The function has been improved to handle both element and instance-based calculations, but there are two issues:
- Remove the commented out code on line 421
- Verify that instance-based calculations using
getElementRectwork correctly with all component types
- // const { left, height, top, width } = getRect(element)Run this script to verify instance-based calculations work with different component types:
🏁 Script executed:
#!/bin/bash
# Description: Find all component types that use getElementRect
# Look for patterns where components might override or customize rectangle calculations
ast-grep --pattern 'getElementRect($_)'Length of output: 574
Action Required: Clean Up Commented Code & Confirm Instance Calculation Consistency
-
Remove the obsolete commented-out line (line 421) in
packages/canvas/container/src/container.js:- // const { left, height, top, width } = getRect(element) -
Our verification confirms that using
getElementRectfor instance-based calculations is consistent across multiple components (see its usage incomponent-selection.jsandcontainer.js). No further adjustments are needed for instance handling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setSelectRect = (element, instance) => { | |
| let calcElement = element | |
| if (!element && !instance) { | |
| calcElement = getDocument().body | |
| } | |
| let left = 0 | |
| let top = 0 | |
| let width = 0 | |
| let height = 0 | |
| if (calcElement) { | |
| const { left: l, top: t, width: w, height: h } = getRect(calcElement) | |
| left = l | |
| top = t | |
| width = w | |
| height = h | |
| } | |
| if (instance) { | |
| const { left: l, top: t, width: w, height: h } = getElementRect(instance) | |
| left = l | |
| top = t | |
| width = w | |
| height = h | |
| } | |
| const { left, height, top, width } = getRect(element) | |
| // const { left, height, top, width } = getRect(element) | |
| const componentName = getCurrent().schema?.componentName || '' | |
| const setSelectRect = (element, instance) => { | |
| let calcElement = element | |
| if (!element && !instance) { | |
| calcElement = getDocument().body | |
| } | |
| let left = 0 | |
| let top = 0 | |
| let width = 0 | |
| let height = 0 | |
| if (calcElement) { | |
| const { left: l, top: t, width: w, height: h } = getRect(calcElement) | |
| left = l | |
| top = t | |
| width = w | |
| height = h | |
| } | |
| if (instance) { | |
| const { left: l, top: t, width: w, height: h } = getElementRect(instance) | |
| left = l | |
| top = t | |
| width = w | |
| height = h | |
| } | |
| const componentName = getCurrent().schema?.componentName || '' | |
| } |
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit