fix(app-preview-importMap): Fix ImportMap Issues, Optimize Popover Trigger, and Improve Variable Binding in Preview#1676
Conversation
WalkthroughReduced popover open delay and changed a preview toolbar trigger from click to hover. Broadened VariableConfigurator initialization check to accept up to 3 keys. Consolidated preview data loading to use a single getBasicData call (including importMapData) and switched app JS generation to consume params.styles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Preview as Preview UI
participant Hook as usePreviewData
participant Src as getBasicData
User->>Preview: Open preview
Preview->>Hook: init()
Hook->>Src: getBasicData(basicFiles, params.scripts)
Src-->>Hook: { importMapData, appData, metaData, ... }
Hook->>Hook: importMap ← importMapData
Hook->>Hook: styles ← params.styles
alt app.js reference present
Hook->>Hook: Generate appJsCode from app.js using params.styles
else no app.js reference
Hook->>Hook: appJsCode ← empty
end
Hook-->>Preview: { importMap, styles, appJsCode, ... }
Preview-->>User: Render preview
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
packages/canvas/container/src/container.ts (1)
512-514: Consider capping the animation wait time.While synchronizing selection visuals with CSS transitions is valuable, unbounded wait times could degrade user experience if elements have very long transitions. Additionally, rapid consecutive calls to
updateRectmight create overlapping timeouts.Consider applying these improvements:
if (id || isBodySelected) { // 获取元素动画持续时间 const waitTime = getElementDurationTime(id) - setTimeout(() => setSelectRect(id), waitTime) + // Cap wait time to prevent indefinite delays + const cappedWaitTime = Math.min(waitTime, 1000) + setTimeout(() => setSelectRect(id), cappedWaitTime) } else {Alternatively, consider debouncing or canceling previous timeouts to handle rapid consecutive calls.
packages/plugins/materials/src/composable/useResource.ts (1)
137-166: Consider adding type annotations for better type safety.The page selection logic is correct and properly implements the prioritized fallback chain. However, adding explicit return type annotations would improve type safety and IDE support.
Apply this diff to add return type annotation:
- const getPageInfo = () => { + const getPageInfo = (): PageInfo => { // 页面是否被他人锁定 (被锁定 且 非当前用户锁定) const isPageOccupierdByOthers = (page: any) => { - return page.meta?.occupier && page.meta.occupier.id !== globalState.userInfo.id + return page.meta?.occupier && page.meta.occupier.id !== globalState.userInfo?.id }packages/design-core/src/preview/src/preview/usePreviewData.ts (1)
418-418: LGTM! This correctly implements dynamic import map collection.Switching from
getImportMaptogetBasicDataensures that app preview now uses the same dynamic dependency collection as page preview, resolving the missing element-plus and user-added dependencies issue.Note:
appDataandmetaDataare destructured but not used in this branch. Consider using a more targeted destructuring or adding an underscore prefix to indicate intentional non-use.Apply this diff to make the unused variables explicit:
- const { importMapData: importMap } = await getBasicData(basicFiles, params.scripts) + const { importMapData: importMap, appData: _appData, metaData: _metaData } = await getBasicData(basicFiles, params.scripts)Or, if you prefer to keep only what's used:
- const { importMapData: importMap } = await getBasicData(basicFiles, params.scripts) + // Only extract importMapData; appData and metaData are fetched but not used in app preview + const { importMapData: importMap } = await getBasicData(basicFiles, params.scripts)packages/canvas/container/src/components/CanvasAction.vue (1)
254-259: Cache schema ID to avoid redundant calls.The code calls
getCurrent().schema.idthree times (lines 254, 256, 257). While unlikely to cause issues, caching the value improves clarity and prevents potential inconsistencies.Apply this diff:
const hide = () => { - if (getCurrent().schema?.id) { + const schemaId = getCurrent().schema?.id + if (schemaId) { const { clearSelect } = useCanvas().canvasApi.value - getRenderer().setCondition(getCurrent().schema.id, false) - useCanvas().pageState.nodesStatus[getCurrent().schema.id] = false + getRenderer().setCondition(schemaId, false) + useCanvas().pageState.nodesStatus[schemaId] = false clearSelect() } updateRect() }packages/canvas/DesignCanvas/src/DesignCanvas.vue (2)
112-114: Consider simplifying the lock message.The updated message is more specific than before (using
componentType[type]instead of "文件"), and adding "您可以创建新页面" provides users with an alternative action, which is good.However, the message is somewhat verbose with the phrase "然后再锁定该${componentType[type]}后编辑" appearing at the end. Consider simplifying to:
-`当前${componentType[type]}被 ${pageInfo?.username || ''} 锁定，您可以创建新页面，如需编辑请先联系他解锁${ - componentType[type] -}，然后再锁定该${componentType[type]}后编辑！` +`当前${componentType[type]}被 ${pageInfo?.username || ''} 锁定。您可以创建新页面，或联系他解锁后进行编辑。`
90-92: Use a dedicated resetProps method instead of getProps(null, null)
At packages/canvas/DesignCanvas/src/DesignCanvas.vue:90-92, replace the hackyuseProperties().getProps(null, null)call with a clearresetProps()(orclearProps()) API onusePropertiesto explicitly reset component properties rather than passingnullto a getter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
designer-demo/public/mock/bundle.json(3 hunks)mockServer/src/mock/get/app-center/v1/apps/schema/1.json(1 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(2 hunks)packages/canvas/container/src/components/CanvasAction.vue(1 hunks)packages/canvas/container/src/container.ts(3 hunks)packages/common/component/toolbar-built-in/ToolbarBaseIcon.vue(1 hunks)packages/common/js/import-map.json(1 hunks)packages/configurator/src/html-attributes-configurator/HtmlAttributesConfigurator.vue(6 hunks)packages/configurator/src/slot-configurator/SlotConfigurator.vue(1 hunks)packages/configurator/src/variable-configurator/VariableConfigurator.vue(0 hunks)packages/design-core/src/preview/src/preview/usePreviewData.ts(2 hunks)packages/plugins/materials/src/composable/useResource.ts(2 hunks)packages/plugins/tree/src/Main.vue(2 hunks)packages/toolbars/preview/src/Main.vue(1 hunks)
💤 Files with no reviewable changes (1)
- packages/configurator/src/variable-configurator/VariableConfigurator.vue
🧰 Additional context used
🧬 Code graph analysis (2)
packages/design-core/src/preview/src/preview/usePreviewData.ts (1)
packages/design-core/src/preview/src/preview/generate.js (2)
processAppJsCode(153-169)processAppJsCode(153-169)
packages/plugins/materials/src/composable/useResource.ts (2)
packages/register/src/common.ts (1)
getMetaApi(20-30)packages/register/src/constants.ts (1)
META_SERVICE(1-24)
🔇 Additional comments (14)
packages/configurator/src/slot-configurator/SlotConfigurator.vue (1)
186-196: Visual tweak LGTMSwitch knob offset adjustment is safe and improves alignment.
packages/canvas/container/src/container.ts (1)
879-887: Return value addition is backward compatible.All 7 call sites across the codebase ignore the return value, confirming the change from void to
Node | nullis safe and does not introduce breaking changes. This improves API consistency.packages/plugins/materials/src/composable/useResource.ts (2)
122-122: LGTM!The global state retrieval is necessary for the new occupancy check logic and follows the established pattern of using
getMetaApi.
167-167: LGTM!The usage of
getPageInfo()correctly retrieves the appropriate page based on the new prioritized selection logic.packages/design-core/src/preview/src/preview/usePreviewData.ts (1)
617-617: Good defensive coding practice.Adding the
|| ''fallback prevents potential issues ifapp.jsis missing fromnewFiles. This makes the app preview branch consistent with the page preview branch at line 407.packages/common/js/import-map.json (1)
20-20: Verify if this change belongs in this PR.The PR objective states this is a bugfix for "importmap missing element-plus and user-added dependencies." However, this change adds
@opentiny/vue-huichartsto the import map, which appears unrelated to the element-plus fix.The actual fix for dynamic import map collection is in
usePreviewData.ts(line 418), which makes app preview retrieve dependencies dynamically instead of relying on the static import map.Is the huicharts addition:
- Part of a separate feature that should be in its own PR?
- An example of a "user-added dependency" that needed to be in the static import map for other reasons?
- Intentionally bundled with this bugfix?
packages/configurator/src/html-attributes-configurator/HtmlAttributesConfigurator.vue (6)
3-3: LGTM: Header text updated appropriately.The change from "自定义属性" (Custom Attributes) to "原生属性" (Native Attributes) better reflects the purpose of configuring native HTML attributes.
14-27: LGTM: Form validation setup is correct.The form configuration properly integrates validation with:
- Form ref for programmatic validation access
- Model binding to state.formData
- Rules binding for validation
- Proper prop attributes on form items
91-101: Verify that max length limits are appropriate for HTML attributes.The validation rules enforce:
- Key: max 20 characters
- Value: max 200 characters
These limits might be too restrictive for some valid HTML attributes:
- Keys like
data-test-automation-idcan exceed 20 characters- Values like URLs or base64-encoded data can exceed 200 characters
Confirm these limits align with the intended use cases.
210-221: LGTM: Exposes necessary refs for template.The return statement correctly exposes
attrFormRefandrulesfor use in the template's form validation.
274-278: LGTM: CSS prevents layout issues with long attributes.The word-wrapping styles ensure long attribute names and values display properly without breaking the layout.
1-224: Verify that this file belongs in this PR.The PR title and description focus on fixing importmap issues with element-plus, but this file implements form validation for the HTML attributes configurator. While the AI-generated summary mentions this change, it's unclear how it relates to the importmap bugfix.
Confirm whether:
- This is an unrelated change that should be in a separate PR, or
- The PR description is incomplete and should mention this validation enhancement
packages/toolbars/preview/src/Main.vue (1)
3-10: Clarify how this change addresses the importMap bug.The PR objectives state this is fixing "importmap missing element-plus when a…" but the changes here only modify UI interaction (hover trigger). If this is an ancillary change bundled with the main fix, consider documenting the relationship or moving unrelated UI improvements to a separate PR for better change tracking.
packages/common/component/toolbar-built-in/ToolbarBaseIcon.vue (1)
4-4: Good UX improvement.Reducing the hover delay from 1000ms to 500ms makes the toolbar more responsive while still providing enough delay to avoid accidental triggers. This aligns well with the hover-based interaction pattern.
packages/configurator/src/html-attributes-configurator/HtmlAttributesConfigurator.vue
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/design-core/src/preview/src/preview/usePreviewData.ts (1)
301-326: Consider adding error handling to getBasicData.If any of the promises in
Promise.allfails (e.g., network error, invalid metadata), the entireupdatePreviewoperation fails without graceful degradation. Consider wrapping in try-catch or usingPromise.allSettledto handle partial failures.Example approach:
const getBasicData = async (basicFilesPromise: Promise<any>, scripts: Record<string, string>) => { + try { const searchParams = new URLSearchParams(location.search) // ... existing code ... const promises = [getAppData(), fetchMetaData(metaDataParams), getImportMap(scripts), basicFilesPromise] const [appData, metaData, importMapData] = await Promise.all(promises) return { appData, metaData, importMapData } + } catch (error) { + console.error('Failed to fetch basic data:', error) + // Return fallback or partial data, or re-throw if appropriate + throw error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/design-core/src/preview/src/preview/usePreviewData.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/design-core/src/preview/src/preview/usePreviewData.ts (3)
packages/common/js/preview.js (1)
params(129-129)packages/design-core/src/preview/src/preview/srcFiles.js (1)
srcFiles(28-28)packages/design-core/src/preview/src/preview/generate.js (2)
processAppJsCode(153-169)processAppJsCode(153-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/design-core/src/preview/src/preview/usePreviewData.ts (3)
363-363: LGTM! Import map now dynamically obtained for both preview types.This change consolidates import map retrieval by calling
getBasicDatabefore thepreviewTypecheck, ensuring both page and app preview use the same dynamic import map that includes element-plus and user-added dependencies. This addresses the core bug mentioned in the PR objectives.
406-408: LGTM! Page preview now uses params.styles.Using
params.stylesdirectly instead of parsing styles from the URL is cleaner and more maintainable. The styles are correctly passed through fromgetMaterialDeps()inloadInitialData().
612-616: LGTM! App preview now uses dynamic import map and params.styles.These changes fix the bug where app preview was missing element-plus and user-added dependencies:
- Line 612 adds the dynamically generated
importMapDatatosrcFiles- Line 615 uses
params.stylesfor consistent style handlingBoth changes align app preview with page preview behavior.
betterdancing
changed the title
2 participants
…pp preview
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
1、应用预览时使用了内置的importMap，内置的importMap不包含element-plus以及用户自行引入的依赖
2、预览图标点击弹出页面和应用预览后，点击任意预览，popover不会消失
3、组件变量绑定后，如果是双向绑定，再次打开绑定变量弹窗，绑定的变量不会回显
What is the current behavior?
Issue Number: N/A
What is the new behavior?
1、修复内置importMap导致没有element-plus组件库的问题，采用和页面预览相同的动态获取依赖集合的方法
2、优化点击预览图标时trigger为click点击后不会自动关闭的问题，改为使用hover方式打开poper
3、解决打开绑定变量弹窗，绑定的变量不会回显的问题，由于object,keys(modelvalue).length === 2，如果是双向绑定这个条件不成立，因此不能回显，修改为<=3
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit