feat: add canvas route bar #967
Conversation
…to feat/route-bar
…to feat/route-bar
WalkthroughThis pull request introduces significant updates to the canvas and routing components, enhancing navigation and layout functionality. Key changes include the addition of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/plugins/page/src/composable/usePage.js (2)
182-182: Ensure proper error handling
While fetching page data, consider handling potential rejections fromhttp.fetchPageList. At a minimum, log errors or provide a fallback flow.
249-256: Clearing canvas state
clearCurrentStateeffectively resets important canvas references. Verify if any additional context (e.g., undo history, local caches) needs clearing for a consistent reset experience.packages/canvas/layout/src/CanvasLayout.vue (1)
36-37: Hardcoded margin could hinder dynamic toggling
The comment correctly points out that the32pxmargin is a fixed offset for the route bar. If route-bar visibility will be toggled, consider a more responsive or computed approach.packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (2)
36-42: Lazy page loading
Ifpages.valueis not an array, you fetch pages. Adding error handling or a loading indicator can improve the user experience in slow network scenarios.
44-63: Recursive flattening for tree data
flattenTreeDatais well-structured for constructing a uniform list from nested trees. Watch for large nested structures that may impact performance; consider lazy loading if needed.packages/canvas/route-bar/src/CanvasRouteBar.vue (2)
20-20: Consider addressing the TODO regarding small-scale height issues.
When scale is very small, this TODO suggests that the height calculation may not display correctly. Consider adding a minimum height or a conditional check to gracefully handle extremely small scaling factors.
66-86: Ensure error handling or fallback for pageId changes.
Within the watch block, if thegetAncestorscall or the page-state tree mapping fails, it might cause an unhandled error. Adding error-handling or a graceful fallback for scenarios wherepageIdor related data is missing would improve robustness.packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
58-58: Components destructuring and return statement are consistent.
Declaring and returningCanvasRouteBarsimplifies usage and organization. Consider adding unit tests or integration tests to validate that the new header area and route bar functionality work as expected, especially in dynamic or large-scale pages.Also applies to: 224-224
packages/canvas/container/src/components/CanvasMenu.vue (1)
224-228: Validate target route during route jump.
Currently, theroute()method callsswitchPagewithout validating thattargetis a valid page. Consider adding logic to handle invalid or missingtargetroute names, possibly displaying an error or a user-friendly message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/canvas/DesignCanvas/src/DesignCanvas.vue(3 hunks)packages/canvas/container/src/components/CanvasMenu.vue(5 hunks)packages/canvas/index.js(2 hunks)packages/canvas/layout/src/CanvasLayout.vue(3 hunks)packages/canvas/render/src/builtin/CanvasRouterLink.vue(2 hunks)packages/canvas/render/src/builtin/builtin.json(2 hunks)packages/canvas/route-bar/index.js(1 hunks)packages/canvas/route-bar/src/CanvasRouteBar.vue(1 hunks)packages/configurator/src/index.js(2 hunks)packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue(1 hunks)packages/plugins/page/src/PageTree.vue(6 hunks)packages/plugins/page/src/composable/usePage.js(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/canvas/route-bar/index.js
🔇 Additional comments (30)
packages/plugins/page/src/composable/usePage.js (4)
16-17: Imported methods add clarity and modularity
The addition of getCanvasStatus and other imports from @opentiny/tiny-engine-common/js/canvas and @opentiny/tiny-engine-meta-register neatly separates concerns, ensuring this composable can leverage canvas and layout functionalities.
20-20: Contextual constants improve readability
Destructuring ELEMENT_TAG and COMPONENT_NAME at the top of the file makes constants immediately discoverable. Please ensure they are used consistently.
222-222: Avoid repeated calls if pages are loaded
The await getPageList() is invoked only if pageSettingState.pages.length === 0. Confirm that re-invoking it when pages are already fetched is unnecessary.
258-265: URL updates and history management
updateUrlPageId updates pageid and removes blockid, which is consistent with the new flow. Validate that external links or direct reloads remain functional after discarding these query parameters.
packages/canvas/index.js (1)
12-12: New CanvasRouteBar import and registration
Adding CanvasRouteBar to the default export broadens the canvas component set. Ensure that any usage is properly documented, especially if it requires configuration in other modules.
Also applies to: 25-25
packages/canvas/layout/src/CanvasLayout.vue (2)
3-3: New named header slot
This slot broadens layout flexibility and allows top-level elements (e.g., a route bar) to be placed outside the main container.
17-17: Adjusted layout height offset
Changing from 36px to 68px reflects the new layout requirements. Verify that the new offset won't introduce overlapping for smaller viewports.
packages/canvas/render/src/builtin/CanvasRouterLink.vue (3)
13-13: Prop types ensure clarity
Switching to PropType<{ name: string }> makes the routing structure explicit. Good improvement for type safety and future extensibility.
25-28: Prop type upgrade
The updated to prop allows for named routes or extended objects. This fosters advanced routing scenarios.
33-47: Computed states prevent runtime errors
The checks for pageAncestor array and props.to?.name prevent potential undefined references. This robust approach avoids edge-case crashes.
packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (6)
1-12: Dynamic dropdown for page routing
The template showcases a tiny-select that updates state.selected and emits changes. This is a clean approach to linking user selections with the parent component.
14-23: Properly defined props
Accepting modelValue as a string or array is flexible. However, confirm that both data types are handled consistently if the parent always expects an object shape (with name).
26-30: Using reactive state effectively
The selected property merges local state with the prop, ensuring two-way data binding. This straightforward approach simplifies the user’s experience.
32-34: Auto-sync with watchEffect
Automatically updating state.selected when modelValue changes ensures the component remains in sync if an external event updates the prop.
65-70: Filtering for valid pages
.filter((node) => node.rawData.isPage) ensures only real pages appear in the dropdown. Makes sense to separate folders vs. pages for clarity.
72-74: Emit updated value with name property
Emitting { name: state.selected } integrates seamlessly with components expecting that shape. Make sure upstream components handle the new structure.
packages/canvas/route-bar/src/CanvasRouteBar.vue (1)
92-93: Logic looks good for route navigation.
The handleClickRoute method effectively delegates page-switching to switchPage. Make sure any potential errors from switchPage are handled at the higher level if needed.
packages/configurator/src/index.js (1)
23-23: New RouterSelectConfigurator import and export are consistent.
The addition of RouterSelectConfigurator aligns well with the existing pattern. Consider adding test coverage to ensure correctness and to verify that this configurator functions as intended in the broader application.
Also applies to: 58-58
packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
3-5: Header slot for CanvasRouteBar is thoughtfully placed.
Integrating the route bar within the header slot is logical. Ensure that any relevant styling or container constraints do not negatively impact the layout, especially if the scale factor or page dimension changes.
packages/canvas/container/src/components/CanvasMenu.vue (1)
124-128: Conditional rendering of “路由跳转” is appropriate for RouterLink.
You’ve correctly implemented a show() function to ensure this menu item appears only when the current component is RouterLink. This is a good approach to preventing irrelevant options.
packages/plugins/page/src/PageTree.vue (8)
58-58: Good addition of lifecycle imports.
Using onMounted and onUnmounted makes the subscription/unsubscription logic more organized. The approach is sound.
79-79: Constant usage is clear.
Using PAGE_STATUS from constants helps maintain clarity in code references to page states.
99-99: Destructuring from useCanvas.
The code is more readable by destructuring only the needed properties: pageState, isBlock, isSaved. Ensure no leftover references to removed properties.
108-108: Renaming to switchPageById for clarity.
Renaming the function to switchPageById effectively distinguishes between the local switchPage scope and the composable’s logic.
119-122: Initialization of message subscription variables.
Declaring subscriber = null ahead of time and referencing subscribe and unsubscribe keeps message-based logic maintainable.
123-134: Subscribing to locationHistoryChanged.
Subscribing to this topic on mount and updating currentNodeData is correct. Confirm that any other components publishing locationHistoryChanged maintain consistency in payload structure.
135-139: Unsubscribing on unmount.
Excellent practice: preventing memory leaks by cleaning up the subscription in onUnmounted.
152-155: Updated switchPage method.
Encapsulating the low-level logic in switchPageById is a clean separation of responsibilities. The sequence of updating state.currentNodeData, closing setting panels, and then switching pages is logically consistent.
packages/canvas/render/src/builtin/builtin.json (2)
181-181: Replacing InputConfigurator with RouterSelectConfigurator.
This change streamlines routing setup by providing users a specialized UI for route selection.
631-631: Empty props for RouterLink snippet.
Leaving props empty sets a clear baseline for more advanced configurations. Ensure that code references rely on the new to property usage as needed.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
packages/plugins/page/src/composable/usePage.js (1)
313-314: Consider internationalizing confirmation messagesThe confirmation message is hardcoded in Chinese. Consider using a translation system for better internationalization support.
packages/plugins/page/src/PageGeneral.vue (1)
57-59: Confirm i18n strategy for the label "设为默认页".This checkbox label is hardcoded in Chinese. If your project targets an international audience, consider adding an internationalization mechanism or a prop for label text to maintain consistency across different languages.
packages/canvas/layout/src/CanvasLayout.vue (2)
14-14: Define ROUTE_BAR_HEIGHT as a single source of truth.Extracting this constant is a good step. Consider documenting it or making it configurable if multiple components rely on the same dimension.
41-41: Use consistent margin naming.You changed
.site-canvasto have amargin-bottom: 18px;. If other components also rely on margin or spacing, consider standardizing this to maintain a coherent design system.packages/canvas/route-bar/src/CanvasRouteBar.vue (1)
85-86: Handle switchPageWithConfirm() error states.For example, if the operation fails or the user cancels, ensure the user interface updates accordingly or logs the reason.
packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (1)
61-71: Potential for multi-level page nesting.
getNodeIconreturns null for the root node, a page icon ifdata.isPage, otherwise a folder icon. If more node types are introduced, consider an extensible approach or a mapping strategy.packages/canvas/container/src/components/CanvasMenu.vue (1)
224-228: Consider extracting navigation logic to a composableThe route operation contains business logic that could be extracted to a dedicated composable function. This would:
- Improve testability by isolating navigation logic
- Make the component more focused on UI concerns
- Allow reuse of navigation logic across components
Consider creating a
useRouteNavigationcomposable:// composables/useRouteNavigation.js export function useRouteNavigation() { const page = usePage() const modal = useModal() const navigateToTarget = async (target) => { if (!target) { modal.message({ message: '路由目标未设置', status: 'error' }) return } try { await page.switchPageWithConfirm(target) } catch (error) { modal.message({ message: `导航失败: ${error.message}`, status: 'error' }) } } return { navigateToTarget } }Then use it in the component:
route() { - const target = getCurrent().schema.props.to?.name - if (target) { - usePage().switchPageWithConfirm(target) - } + const { navigateToTarget } = useRouteNavigation() + const target = getCurrent()?.schema?.props?.to?.name + navigateToTarget(target) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/canvas/DesignCanvas/src/DesignCanvas.vue(5 hunks)packages/canvas/container/src/components/CanvasMenu.vue(5 hunks)packages/canvas/layout/src/CanvasLayout.vue(3 hunks)packages/canvas/route-bar/src/CanvasRouteBar.vue(1 hunks)packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue(1 hunks)packages/plugins/page/src/PageGeneral.vue(4 hunks)packages/plugins/page/src/composable/usePage.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/canvas/DesignCanvas/src/DesignCanvas.vue
🔇 Additional comments (22)
packages/plugins/page/src/composable/usePage.js (6)
16-24: LGTM: Imports are well-organized and properly utilized
The new imports are correctly structured and all imported functions are used within the implementation.
Line range hint 189-229: LGTM: Improved flexibility with optional appId parameter
The function now gracefully handles cases where appId is not provided by falling back to the base info id.
Line range hint 229-255: LGTM: Simplified ancestor retrieval logic
The function has been streamlined by leveraging the improved getPageList functionality.
303-330: LGTM: Proper implementation of unsaved changes handling
The switchPageWithConfirm function properly implements the handling of unsaved changes, addressing the previous review comment.
347-348: LGTM: Properly exported new functions
The new functions are correctly exported following the existing pattern.
292-293: Address TODO comment about Breadcrumb plugin
The TODO comment indicates technical debt regarding Breadcrumb plugin implementation.
packages/plugins/page/src/PageGeneral.vue (3)
57-59: Add unit tests or integration tests covering "Set as Default Page" logic.
Since this feature changes flow behavior (e.g., there's now a “default page” concept), ensure tests validate:
- The fallback behavior when no page is set as default.
- The case where multiple pages attempt to set themselves as default, if applicable.
69-69: Imports and component registration look good.
You have correctly imported and registered Checkbox as TinyCheckbox. No issues here.
Also applies to: 81-82
207-207: Validate that each node label is properly escaped or sanitized.
Although unlikely in normal usage, if node labels originate from user input, ensure you sanitize or escape them to prevent any potential XSS vulnerabilities.
packages/canvas/layout/src/CanvasLayout.vue (2)
3-3: Slots for header, container, and footer significantly enhance flexibility.
The usage of named slots for structural layout is a best practice to allow consumers of your component to insert custom content. This looks good.
18-19: Conditional route bar height logic is well-structured.
The calculation of isBlock combined with a dynamic routeBarHeight ensures a responsive layout. Nothing further to address.
Also applies to: 21-23
packages/canvas/route-bar/src/CanvasRouteBar.vue (5)
4-9: Ensure uniqueness of the route key in v-for.
Using :key="route.id" is correct if every route has a guaranteed unique id. If the same route ID can appear in multiple contexts, consider a more specific key.
25-31: Use guard clauses or null checks.
pageId is initially derived via getBaseInfo(). If getBaseInfo fails, consider a fallback or guard if pageId is missing. Also confirm other code handles empty states safely.
43-47: Unsubscribe logic is correct.
Good job cleaning up the subscription on unmount to avoid memory leaks. Looks fine here.
67-76: Careful with route transformations.
You replace multiple slashes and remove leading/trailing slashes. Confirm that this does not break any special paths (e.g., “//comment-page” or trailing slashes needed for some frameworks).
90-120: Solid styling.
This route bar styling is consistent, and the hover effect is clear. No further notes.
packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (2)
39-43: Trigger getPageList() only if pages are not an array.
This is a good approach to ensure your data structure is loaded.
93-95: Emit shaped data object for consistency.
Emitting { name: state.selected } is correct if your parent always expects an object with a name field. Validate that it aligns with how the parent uses modelValue.
packages/canvas/container/src/components/CanvasMenu.vue (4)
5-5: LGTM: Template update for filtered menu items
The change from menus to filteredMenus correctly implements the filtering mechanism in the template.
35-35: LGTM: Required imports added
The new imports (computed and usePage) are necessary for implementing the route jump functionality and menu filtering.
Also applies to: 37-37
130-137: LGTM: Clean implementation of menu filtering
The computed property implementation for menu filtering is well-structured and follows Vue best practices.
258-258: LGTM: Computed property correctly exposed
The filteredMenus computed property is properly exposed in the component's return object.
packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue
Show resolved
Hide resolved
| menus.value.unshift({ | ||
| name: '路由跳转', | ||
| code: 'route', | ||
| show: () => getCurrent().schema?.componentName === 'RouterLink' | ||
| }) |
There was a problem hiding this comment.
Add null safety check for RouterLink component check
The show condition might throw an error if getCurrent() returns null or if the schema is undefined.
Add null checks to prevent potential runtime errors:
menus.value.unshift({
name: '路由跳转',
code: 'route',
- show: () => getCurrent().schema?.componentName === 'RouterLink'
+ show: () => getCurrent()?.schema?.componentName === 'RouterLink'
})📝 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.
| menus.value.unshift({ | |
| name: '路由跳转', | |
| code: 'route', | |
| show: () => getCurrent().schema?.componentName === 'RouterLink' | |
| }) | |
| menus.value.unshift({ | |
| name: '路由跳转', | |
| code: 'route', | |
| show: () => getCurrent()?.schema?.componentName === 'RouterLink' | |
| }) |
| route() { | ||
| const target = getCurrent().schema.props.to?.name | ||
| if (target) { | ||
| usePage().switchPageWithConfirm(target) | ||
| } |
There was a problem hiding this comment.
Add error handling and null safety checks to route operation
The route operation lacks proper error handling and null safety checks. Consider the following improvements:
- Add null safety checks for the 'to' property
- Handle navigation errors
- Consider cases where 'to' exists but 'name' is undefined
Here's a safer implementation:
route() {
- const target = getCurrent().schema.props.to?.name
+ const current = getCurrent()
+ if (!current?.schema?.props?.to) {
+ useModal().message({
+ message: '路由目标未设置',
+ status: 'error'
+ })
+ return
+ }
+ const target = current.schema.props.to.name
if (target) {
- usePage().switchPageWithConfirm(target)
+ usePage().switchPageWithConfirm(target).catch(error => {
+ useModal().message({
+ message: `导航失败: ${error.message}`,
+ status: 'error'
+ })
+ })
+ } else {
+ useModal().message({
+ message: '路由名称未设置',
+ status: 'error'
+ })
}
}📝 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.
| route() { | |
| const target = getCurrent().schema.props.to?.name | |
| if (target) { | |
| usePage().switchPageWithConfirm(target) | |
| } | |
| route() { | |
| const current = getCurrent() | |
| if (!current?.schema?.props?.to) { | |
| useModal().message({ | |
| message: '路由目标未设置', | |
| status: 'error' | |
| }) | |
| return | |
| } | |
| const target = current.schema.props.to.name | |
| if (target) { | |
| usePage().switchPageWithConfirm(target).catch(error => { | |
| useModal().message({ | |
| message: `导航失败: ${error.message}`, | |
| status: 'error' | |
| }) | |
| }) | |
| } else { | |
| useModal().message({ | |
| message: '路由名称未设置', | |
| status: 'error' | |
| }) | |
| } | |
| } |
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
Improvements
Bug Fixes
Chores