Feat: add refresh life cycles #898
Conversation
WalkthroughThe changes in this pull request involve multiple modifications across different files, primarily focusing on enhancing the handling of design modes in the canvas-related components. Key updates include the introduction of new functions for managing design modes, improvements to event handling based on the design state, and updates to alias resolution for modules. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (2)
packages/build/vite-config/src/vite-plugins/devAliasPlugin.js (1)
57-57: Document the canvas module restructuring.The canvas module has been moved from being part of
design-coreto its own standalone package. This structural change should be documented.Add a comment above the alias to explain this architectural decision:
+ // Canvas module moved to standalone package for better modularity '@opentiny/tiny-engine-canvas': path.resolve(basePath, 'packages/canvas/index.js'),packages/toolbars/refresh/src/Main.vue (1)
43-51: AwaitafterRefreshif it is asynchronous, and markrefreshResourceasasyncIf
afterRefreshis an asynchronous function that returns a promise, you shouldawaitit to ensure it completes before proceeding. Additionally, to useawaitinsiderefreshResource, the function needs to be declared asasync.Apply these changes:
- Mark
refreshResourceasasync:- const refreshResouce = () => { + const refreshResource = async () => {
- Await
afterRefresh():try { - afterRefresh() + await afterRefresh() } catch (error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/build/vite-config/src/vite-plugins/devAliasPlugin.js(1 hunks)packages/toolbars/refresh/src/Main.vue(3 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
packages/canvas/render/src/context.js (3)
59-60: Translate comment to English for consistency.The codebase should maintain consistent use of English in comments for better international collaboration.
-// 是否表现画布内特征的标志，用来控制是否允许拖拽、原生事件是否触发等 +// Canvas feature flag to control drag behavior and native event triggering
64-66: Add type validation in setCanvasFlag.The setter should validate the input to ensure type safety and prevent invalid states.
export const setCanvasFlag = (flag) => { + if (typeof flag !== 'boolean') { + throw new TypeError('Canvas flag must be a boolean value') + } canvasFlag = flag }
60-66: Consider using reactive state for canvasFlag.Using a plain variable for state that affects multiple components could lead to race conditions in concurrent scenarios. Consider using Vue's reactive state management since this is a Vue application.
-let canvasFlag = true +import { ref } from 'vue' + +const canvasFlag = ref(true) export const getCanvasFlag = () => canvasFlag export const setCanvasFlag = (flag) => { + if (typeof flag !== 'boolean') { + throw new TypeError('Canvas flag must be a boolean value') + } - canvasFlag = flag + canvasFlag.value = flag }packages/canvas/container/src/CanvasContainer.vue (3)
144-154: Add error handling forsetCurrentNodecall.While the event handling logic is well-structured, the
setCurrentNodecall could benefit from error handling.Consider wrapping the state updates in a try-catch block:
isCanvasEvent(event, () => { // html元素使用scroll和mouseup事件处理 if (event.target === doc.documentElement) { isScrolling = false return } insertPosition.value = false - setCurrentNode(event) - target.value = event.target + try { + setCurrentNode(event) + target.value = event.target + } catch (error) { + console.error('Error setting current node:', error) + // Reset states on error + insertPosition.value = false + target.value = null + } })
183-185: Document the purpose of the boolean parameter indragMove.The
trueparameter passed todragMoveis not self-documenting and its purpose is unclear.Consider using a named constant or adding a comment:
isCanvasEvent(ev, (e) => { - dragMove(e, true) + const IS_MOUSE_MOVE = true // Indicates event is from mousemove + dragMove(e, IS_MOUSE_MOVE) })
120-128: Consider extracting event handling pattern into a reusable utility.The new
isCanvasEventpattern introduces a consistent way to handle canvas events. Consider extracting this pattern into a reusable utility that other components can leverage.You could create a composable like
useCanvasEvent:// useCanvasEvent.js export function useCanvasEvent() { const handleCanvasEvent = (event, handler) => { // Current isCanvasEvent implementation } return { handleCanvasEvent } } // Usage in components const { handleCanvasEvent } = useCanvasEvent()This would promote consistency across the codebase and make it easier to maintain the event handling logic in one place.
Also applies to: 144-154, 183-185
packages/canvas/render/src/RenderMain.js (1)
Line range hint
1-458: Consider splitting state management concernsThe file currently handles multiple responsibilities including state management, rendering, method context, props management, and now canvas flags. Consider splitting these concerns into separate modules for better maintainability:
- Create a dedicated canvas state manager for flag-related operations
- Extract the data source management logic
- Separate the props and methods management
This would improve code organization and make the refresh lifecycle management more maintainable.
packages/canvas/render/src/render.js (2)
564-567: Consider making event handling behavior configurable.The current implementation always stops event propagation when the canvas flag is set. Consider making this behavior configurable to support cases where event propagation might be desired.
Example implementation:
- if (getCanvasFlag()) { + const canvasConfig = configure[componentName]?.canvas || {} + if (getCanvasFlag() && canvasConfig.stopEvents !== false) { bindProps.onMouseover = stopEvent bindProps.onFocus = stopEvent }
574-576: Document canvas flag behavior.The canvas flag now controls multiple event handling behaviors. Please add JSDoc comments explaining:
- The purpose of the canvas flag
- How it affects event propagation
- When it should be enabled/disabled
Example documentation:
+ /** + * Conditionally applies click capture based on component configuration and canvas state. + * When both conditions are met, clicks will be captured and stopped from propagating. + */ if (clickCapture(componentName) && getCanvasFlag()) { bindProps.onClickCapture = stopEvent }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/canvas/container/src/CanvasContainer.vue(3 hunks)packages/canvas/render/src/RenderMain.js(2 hunks)packages/canvas/render/src/context.js(1 hunks)packages/canvas/render/src/render.js(2 hunks)
🔇 Additional comments (3)
packages/canvas/render/src/RenderMain.js (2)
30-32: LGTM: Import changes are well-structured
The new imports for canvas flag management are properly integrated into the existing destructured import statement.
456-458: Verify canvas flag usage in refresh lifecycle
The new canvas flag management functions are properly exported. However, let's verify their integration with the refresh lifecycle.
✅ Verification successful
Let me try another search strategy to verify the canvas flag usage.
Based on the search results, I can now generate the final response:
Canvas flag integration is properly implemented
The canvas flags are well integrated within the codebase:
- Used in
CanvasContainer.vuefor conditional rendering - Properly integrated in
render.jsfor event handling (mouseover, focus, click capture) - Flags are managed through the context module with clear getter/setter implementations
- No direct coupling found with refresh lifecycle, which is correct as they serve different purposes:
- Canvas flags control event handling and rendering behavior
- Refresh functionality is handled separately through toolbar actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of canvas flags in refresh lifecycle
# Expected: Find references to getCanvasFlag/setCanvasFlag in lifecycle-related code
echo "Searching for canvas flag usage in lifecycle methods..."
rg -l "getCanvasFlag|setCanvasFlag" --type js --type vue
echo "Searching for refresh lifecycle implementations..."
rg -l "refresh.*lifecycle|lifecycle.*refresh" --type js --type vue
echo "Searching for canvas flag state modifications..."
ast-grep --pattern 'setCanvasFlag($expr)'
Length of output: 566
Script:
#!/bin/bash
# Search for canvas flag usage without vue extension
echo "Searching for canvas flag usage..."
rg -l "getCanvasFlag|setCanvasFlag" --type js
echo "Searching for refresh lifecycle implementations..."
rg -l "refresh.*lifecycle|lifecycle.*refresh" --type js
echo "Looking for canvas flag related code..."
rg -A 5 "getCanvasFlag|setCanvasFlag" --type js
echo "Searching for refresh related code..."
rg -A 5 "\brefresh\b" --type js
Length of output: 14866
packages/canvas/render/src/render.js (1)
23-23: LGTM: Import changes are well-organized.
The addition of getCanvasFlag import is properly placed with related context imports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/toolbars/refresh/src/Main.vue (2)
36-51: Add JSDoc comments to document side effectsThe
refreshResourcefunction performs important side effects like clearing block cache and reloading the canvas. These should be documented for maintainability.+/** + * Refreshes canvas resources by clearing block cache and reloading the canvas. + * Also executes the afterRefresh hook if defined. + * @throws {Error} Catches and notifies errors from afterRefresh execution + */ const refreshResource = () => {
75-93: Enhance error handling with specific error typesWhile the error handling is good, it could be more informative by distinguishing between different types of errors.
const refresh = async () => { try { if (typeof beforeRefresh === 'function') { - await beforeRefresh() + try { + await beforeRefresh() + } catch (error) { + useNotify({ + type: 'error', + message: `Error in beforeRefresh: ${error}` + }) + return + } } if (typeof refreshMethod === 'function') { - const stop = await refreshMethod() + try { + const stop = await refreshMethod() + if (stop) { + return + } + } catch (error) { + useNotify({ + type: 'error', + message: `Error in refreshMethod: ${error}` + }) + return + } - if (stop) { - return - } } } catch (error) { useNotify({ type: 'error', - message: `Error in refreshing: ${error}` + message: `Unexpected error during refresh: ${error}` }) + return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/toolbars/refresh/src/Main.vue(4 hunks)
🔇 Additional comments (2)
packages/toolbars/refresh/src/Main.vue (2)
7-15: LGTM: Import changes are well-structured
The new imports for useNotify and getOptions are properly organized and support the enhanced error handling and lifecycle hooks.
34-34: Verify meta.id availability
The lifecycle hooks are obtained using meta.id. Let's ensure this ID is always available when needed.
✅ Verification successful
meta.id is properly defined and used
The verification shows that:
meta.idis properly defined as 'engine.toolbars.refresh' inpackages/toolbars/refresh/meta.js- The file is correctly imported in Main.vue
getOptionsis a registered utility function from '@opentiny/tiny-engine-meta-register'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if meta.id is properly defined and used consistently
# Test 1: Check meta file definition
rg -A 5 "export default.*id:" packages/toolbars/refresh/src/meta.*
# Test 2: Check other usages of meta.id in the codebase
rg "getOptions\(meta\.id\)" --type vue
Length of output: 216
Script:
#!/bin/bash
# Let's try different approaches to verify meta.id usage
# Check for meta file with any extension
fd "meta\." packages/toolbars/refresh/src/
# Look for meta object definition
rg "meta.*=.*\{.*id:" packages/toolbars/refresh/src/
# Check for getOptions usage
rg "getOptions\(" packages/toolbars/refresh/src/
# Look for meta.id usage
rg "meta\.id" packages/toolbars/refresh/src/
# Check Vue file content to understand the context
cat packages/toolbars/refresh/src/Main.vue
Length of output: 3237
Script:
#!/bin/bash
# Let's check the meta import and its definition
# Find the meta file in the parent directory
fd "meta" packages/toolbars/refresh/
# Check the content of meta file if found
fd "meta" packages/toolbars/refresh/ -x cat {}
# Look for getOptions implementation to understand how it uses the id
rg -g "!*.{json,map}" "export.*getOptions" packages/
Length of output: 534
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/canvas/render/src/context.js (2)
59-60: Improve variable naming and documentation for better semantics.The current name
isDesignModecould be more descriptive of its specific purpose. Based on the comment, this flag controls multiple canvas behaviors (drag-and-drop, native events).Consider renaming to something more specific like
isCanvasInteractiveorisCanvasEditableto better reflect its role in controlling canvas interaction features.-// 是否表现画布内特征的标志，用来控制是否允许拖拽、原生事件是否触发等 -let isDesignMode = true +/** + * Flag that controls canvas interaction features: + * - Enables/disables drag and drop functionality + * - Controls native event propagation + * - Manages canvas edit capabilities + * @type {boolean} + */ +let isCanvasInteractive = true
62-62: Add JSDoc documentation for the getter function.For consistency and better code documentation, add JSDoc comments to the getter function.
+/** + * Gets the current canvas interaction mode. + * @returns {boolean} Current state of canvas interactions + */ export const getDesignMode = () => isDesignModepackages/canvas/render/src/render.js (1)
561-568: Consider consolidating event handlers for better maintainability.While the implementation is correct, we can make it more maintainable by combining the event handlers under a single condition.
Consider this refactoring:
[DESIGN_UIDKEY]: id, [DESIGN_TAGKEY]: componentName } if (getDesignMode()) { - bindProps.onMouseover = stopEvent - bindProps.onFocus = stopEvent + Object.assign(bindProps, { + onMouseover: stopEvent, + onFocus: stopEvent + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/canvas/container/src/CanvasContainer.vue(3 hunks)packages/canvas/render/src/RenderMain.js(2 hunks)packages/canvas/render/src/context.js(1 hunks)packages/canvas/render/src/render.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/canvas/container/src/CanvasContainer.vue
- packages/canvas/render/src/RenderMain.js
🔇 Additional comments (3)
packages/canvas/render/src/render.js (3)
23-23: LGTM: Import statement is correctly placed.
The addition of getDesignMode import is well-organized with related context imports.
574-574: LGTM: Click capture condition is well-implemented.
The combined check for both click capture capability and design mode is correctly implemented and follows a consistent pattern.
564-574: Verify event handling behavior in both modes.
The introduction of design mode checks affects event handling. Let's verify the implementation across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/common/js/constants.js (2)
99-102: Remove or translate the Chinese comments.The constant definition looks good, but consider removing the inline Chinese comments or translating them to English to maintain consistency with the rest of the codebase.
export const DESIGN_MODE = { - DESIGN: 'design', // 设计态 - RUNTIME: 'runtime' // 运行态 + DESIGN: 'design', + RUNTIME: 'runtime' }
99-102: Add JSDoc documentation for the DESIGN_MODE constant.Consider adding JSDoc documentation to describe the purpose and usage of this constant, especially since it's used across multiple files for canvas behavior management.
+/** + * Represents the operational modes of the canvas. + * Used to control canvas behavior in different states. + * @constant + * @type {{DESIGN: string, RUNTIME: string}} + */ export const DESIGN_MODE = { DESIGN: 'design', RUNTIME: 'runtime' }packages/canvas/container/src/CanvasContainer.vue (2)
145-155: Consider extracting mousedown logic into a separate function.The mousedown event handler contains nested logic that could be more maintainable if extracted into a named function.
Consider this refactor:
+const handleMouseDown = (event, doc) => { + // html element uses scroll and mouseup events + if (event.target === doc.documentElement) { + isScrolling = false + return + } + + insertPosition.value = false + setCurrentNode(event) + target.value = event.target +} + win.addEventListener('mousedown', (event) => { - handleCanvasEvent(() => { - // html element uses scroll and mouseup events - if (event.target === doc.documentElement) { - isScrolling = false - return - } - - insertPosition.value = false - setCurrentNode(event) - target.value = event.target - }) + handleCanvasEvent(() => handleMouseDown(event, doc)) })
184-186: Consider debouncing mousemove event handler.The mousemove event can fire frequently during drag operations. Consider debouncing the handler to optimize performance.
Consider this improvement:
+const debouncedDragMove = debounce((ev) => dragMove(ev, true), 16) // ~60fps + win.addEventListener('mousemove', (ev) => { - handleCanvasEvent(() => { - dragMove(ev, true) - }) + handleCanvasEvent(() => debouncedDragMove(ev)) })Don't forget to import the debounce utility:
import { debounce } from '@opentiny/tiny-engine-common/js/utils'packages/canvas/render/src/render.js (1)
564-568: Consider adding error handling for event handlers.The event handlers are correctly implemented and the design mode check is explicit. However, consider adding try-catch blocks around the stopEvent calls to ensure robustness.
if (getDesignMode() === DESIGN_MODE.DESIGN) { - bindProps.onMouseover = stopEvent - bindProps.onFocus = stopEvent + bindProps.onMouseover = (event) => { + try { + return stopEvent(event) + } catch (error) { + console.error('Error in mouseover event handler:', error) + return false + } + } + bindProps.onFocus = (event) => { + try { + return stopEvent(event) + } catch (error) { + console.error('Error in focus event handler:', error) + return false + } + } } if (clickCapture(componentName) && getDesignMode() === DESIGN_MODE.DESIGN) { - bindProps.onClickCapture = stopEvent + bindProps.onClickCapture = (event) => { + try { + return stopEvent(event) + } catch (error) { + console.error('Error in click capture event handler:', error) + return false + } + } }Also applies to: 574-576
packages/canvas/container/src/container.js (1)
71-74: Add JSDoc documentation and mode validation.The new design mode functions would benefit from:
- JSDoc documentation explaining their purpose, parameters, and return values
- Input validation for
setDesignModeto ensure valid mode values+/** + * Gets the current design mode of the canvas. + * @returns {string} The current design mode + */ export const getDesignMode = () => getRenderer()?.getDesignMode() +/** + * Sets the design mode of the canvas. + * @param {string} mode - The design mode to set + * @throws {Error} If the mode is invalid + */ export const setDesignMode = (mode) => { + if (!mode || typeof mode !== 'string') { + throw new Error('Invalid design mode') + } getRenderer()?.setDesignMode(mode) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/canvas/container/src/CanvasContainer.vue(4 hunks)packages/canvas/container/src/container.js(2 hunks)packages/canvas/render/src/context.js(1 hunks)packages/canvas/render/src/render.js(2 hunks)packages/common/js/constants.js(1 hunks)packages/toolbars/refresh/src/Main.vue(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/canvas/render/src/context.js
- packages/toolbars/refresh/src/Main.vue
🔇 Additional comments (5)
packages/canvas/container/src/CanvasContainer.vue (1)
34-34: LGTM: Import of design mode constant.
The import of DESIGN_MODE constant is appropriate for the new design mode checks.
packages/canvas/render/src/render.js (2)
23-23: LGTM: Import changes are well-structured.
The new imports for getDesignMode and DESIGN_MODE are properly organized and align with the design mode event handling changes.
561-563: LGTM: Design mode properties are properly configured.
The design-specific properties (DESIGN_UIDKEY and DESIGN_TAGKEY) are correctly added to bindProps.
packages/canvas/container/src/container.js (2)
900-901: LGTM! API exposure follows established patterns.
The new design mode functions are appropriately exposed through the canvasApi object, maintaining consistency with other canvas operations.
71-74: Verify integration with refresh lifecycles.
Since these changes are part of the "refresh life cycles" feature:
- Ensure that design mode changes trigger appropriate lifecycle events
- Verify that the renderer properly handles design mode transitions
Also applies to: 900-901
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/canvas/common/src/constant.js (2)
5-8: LGTM! Consider adding JSDoc documentation.The
DESIGN_MODEconstant is well-structured and its values clearly indicate their purpose. Since this is a crucial constant used across multiple canvas-related components, consider adding JSDoc documentation to describe its usage and impact.+/** + * Represents the operational modes of the canvas. + * @constant + * @type {{DESIGN: string, RUNTIME: string}} + * @description + * - DESIGN: Design mode for editing and configuration + * - RUNTIME: Runtime mode for production execution + */ export const DESIGN_MODE = { DESIGN: 'design', // 设计态 RUNTIME: 'runtime' // 运行态 }
6-7: Maintain consistent language in comments.For better maintainability and international collaboration, consider using English consistently in comments.
export const DESIGN_MODE = { - DESIGN: 'design', // 设计态 - RUNTIME: 'runtime' // 运行态 + DESIGN: 'design', // Design mode + RUNTIME: 'runtime' // Runtime mode }packages/canvas/container/src/CanvasContainer.vue (1)
144-154: Consider refactoring event handlers for better organization.The event handlers could benefit from better separation of concerns and consistency.
Consider these improvements:
- Extract mousedown logic into named functions:
win.addEventListener('mousedown', (event) => { handleCanvasEvent(() => { - if (event.target === doc.documentElement) { - isScrolling = false - return - } - - insertPosition.value = false - setCurrentNode(event) - target.value = event.target + handleMouseDown(event) }) }) +const handleMouseDown = (event) => { + if (event.target === doc.documentElement) { + isScrolling = false + return + } + + insertPosition.value = false + setCurrentNode(event) + target.value = event.target +}
- Consider applying the same design mode check to mouseup events for consistency:
-win.addEventListener('mouseup', (event) => { +win.addEventListener('mouseup', (event) => { + handleCanvasEvent(() => { if (event.target !== doc.documentElement || isScrolling) { return } insertPosition.value = false setCurrentNode(event) target.value = event.target + }) })Also applies to: 183-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/canvas/common/src/constant.js(1 hunks)packages/canvas/container/src/CanvasContainer.vue(4 hunks)
🔇 Additional comments (1)
packages/canvas/container/src/CanvasContainer.vue (1)
35-35: LGTM: Clean import of DESIGN_MODE constant.
The import is well-organized and properly used for design mode validation.
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
Release Notes
New Features
Bug Fixes
Improvements
Documentation