feat: materials new protocol #940
Conversation
WalkthroughThis pull request introduces a comprehensive restructuring of dependency management across multiple components and files. The changes primarily focus on how component libraries, scripts, and styles are imported, managed, and referenced. Key modifications include updating the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Learnings (2)packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)packages/plugins/materials/src/composable/useMaterial.js (1)⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (11)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (3)
packages/canvas/common/src/utils.js (2)
Line range hint
107-116: Handle undefined modules when setting componentsWhen
dynamicImportComponentsreturns an empty object,modules[exportName]may beundefined. This could lead to issues when assigning components. Consider checking ifmodules[exportName]exists before assigning it.Apply this diff to fix the issue:
export const setComponents = async ({ package: pkg, script, components }) => { if (!pkg) return const modules = await dynamicImportComponents(pkg, script) Object.entries(components).forEach(([componentId, exportName]) => { if (!window.TinyLowcodeComponent[componentId]) { + if (modules[exportName]) { window.TinyLowcodeComponent[componentId] = modules[exportName] + } else { + console.warn(`Component "${exportName}" not found in module "${pkg}".`) + } } }) }
Line range hint
125-131: Fix incorrect argument passing todynamicImportComponentsIn
updateDependencies, thescripts.map(dynamicImportComponents)call passes the entire script object as the first argument, which doesn't match the expected parameters. Destructure the script objects to pass the correct arguments.Apply this diff to fix the issue:
export const updateDependencies = ({ detail }) => { const { scripts = [], styles = [] } = detail || {} const { styles: canvasStyles } = window.componentsDepsMap const newStyles = [...styles].filter((item) => !canvasStyles.has(item)) newStyles.forEach((item) => canvasStyles.add(item)) - const promises = [...newStyles].map((src) => addStyle(src)).concat(scripts.map(dynamicImportComponents)) + const promises = [...newStyles] + .map((src) => addStyle(src)) + .concat(scripts.map(({ package: pkg, script }) => dynamicImportComponents(pkg, script))) Promise.allSettled(promises) }packages/plugins/materials/src/composable/useMaterial.js (1)
Line range hint
417-437: Avoid mutating state while iterating ingetBlockDepsModifying
materialState.componentsDepsMap.scriptswhile iterating overscriptsmay lead to unexpected behavior. Collect new scripts separately before updating the state.Apply this diff:
const getBlockDeps = (dependencies = {}) => { const { scripts = [], styles = [] } = dependencies + const newScripts = [] scripts.length && scripts.forEach((npm) => { const { package: pkg, script, css, components } = npm const npmInfo = materialState.componentsDepsMap.scripts.find((item) => item.package === pkg) if (!npmInfo || !npmInfo.script) { - materialState.componentsDepsMap.scripts.push({ package: pkg, script, css, components }) + newScripts.push({ package: pkg, script, css, components }) } else { const components = npmInfo.components || {} npmInfo.components = { ...components, ...npm.components } } }) + materialState.componentsDepsMap.scripts.push(...newScripts) styles?.forEach((item) => materialState.componentsDepsMap.styles.add(item)) }
🧹 Nitpick comments (10)
packages/canvas/common/src/utils.js (1)
79-100: Improve error handling indynamicImportComponentsIf neither the import map includes the package nor a script URL is provided, the
modulesobject remains empty, which may cause issues downstream. Consider adding a warning or error handling to notify about the missing module.You can add a warning message when the module fails to load:
const dynamicImportComponents = async (pkg, script) => { if (window.TinyComponentLibs[pkg]) { return window.TinyComponentLibs[pkg] } let modules = {} try { // 优先从importmap导入，兼容npm.script字段定义的cdn地址 if (getImportMapKeys().includes(pkg)) { modules = await import(/* @vite-ignore */ pkg) } else if (script) { modules = await import(/* @vite-ignore */ script) + } else { + console.warn(`Module "${pkg}" not found in import map and no script URL provided.`) } } catch (error) { modules = {} } window.TinyComponentLibs[pkg] = modules return modules }packages/plugins/materials/src/composable/useMaterial.js (1)
295-318: Consolidate dependency aggregation logicThe logic in
setCanvasDepsfor aggregating dependencies is similar to other parts of the code. Consider creating a utility function to handle dependency aggregation to avoid duplication.packages/canvas/DesignCanvas/src/importMap.js (2)
Line range hint
3-46: Update function documentation to reflect new parameterThe function
getImportMapDatanow accepts a second parametercanvasDeps. Update the JSDoc comment to include this parameter and its purpose.Apply this diff:
/** * Generates import map data for the canvas. + * @param {Object} overrideVersions - Optional versions to override default import map versions. + * @param {Object} canvasDeps - Dependencies from the canvas, including scripts and styles. */ export function getImportMapData(overrideVersions = {}, canvasDeps = { scripts: [], styles: [] }) {
30-35: InitializematerialRequireimports properlyEnsure that
materialRequire.importsis always an object to prevent potentialundefinederrors when spreading intoimportMap.imports.Apply this diff:
const materialRequire = canvasDeps.scripts.reduce((imports, { package: pkg, script }) => { imports[pkg] = script return imports }, {}) const importMap = { imports: { vue: `${VITE_CDN_DOMAIN}/vue@${importMapVersions.vue}/dist/vue.runtime.esm-browser.prod.js`, 'vue-i18n': `${VITE_CDN_DOMAIN}/vue-i18n@${importMapVersions.vueI18n}/dist/vue-i18n.esm-browser.js`, ...blockRequire.imports, ...tinyVueRequire.imports, + ...materialRequire } }packages/vue-generator/src/plugins/genDependenciesPlugin.js (1)
48-54: Consider adding package validationWhile the package processing logic is correct, it might benefit from additional validation to ensure package integrity.
Consider adding validation:
packages.forEach((item) => { const { package: packageName, version } = item + if (!packageName) { + console.warn('Invalid package entry:', item) + return + } + + if (version && !isValidSemver(version)) { + console.warn(`Invalid version '${version}' for package '${packageName}'`) + return + } if (packageName && !resDeps[packageName]) { resDeps[packageName] = version || 'latest' } })You'll need to add the following import:
import { valid as isValidSemver } from 'semver'packages/canvas/DesignCanvas/src/DesignCanvas.vue (3)
29-29: Consider grouping related importsConsider grouping
useMessagewith other related hooks for better code organization.
61-74: Consider extracting initialization logicThe canvas initialization logic could be extracted into a separate function for better maintainability and reusability.
+const initCanvasDeps = (deps) => { + if (canvasSrc) { + return + } + const { importMap, importStyles } = getImportMapData( + getMergeMeta('engine.config')?.importMapVersion, + deps + ) + canvasSrcDoc.value = initCanvas(importMap, importStyles).html +} + useMessage().subscribe({ topic: 'init_canvas_deps', - callback: (deps) => { - if (canvasSrc) { - return - } - const { importMap, importStyles } = getImportMapData( - getMergeMeta('engine.config')?.importMapVersion, - deps - ) - canvasSrcDoc.value = initCanvas(importMap, importStyles).html - } + callback: initCanvasDeps })
Line range hint
82-147: Consider extracting complex conditionsThe watch logic contains complex nested conditions. Consider extracting these into named functions or constants for better readability.
Example approach:
const shouldShowConfirm = (isSaved, pageSchema, oldPageSchema) => (!isSaved || pageSchema !== oldPageSchema) && !showModal const isValidPageStatus = (pageStatus, pageSchema) => ![PAGE_STATUS.Guest, PAGE_STATUS.Occupy].includes(pageStatus.state) && pageSchema?.componentNamedesigner-demo/public/mock/bundle.json (2)
43-43: Consider using object notation for shortcuts and contextMenu actions.The current array syntax for properties and actions could be converted to object notation for better maintainability and type safety.
- "properties": ["type", "size"] + "properties": { + "type": true, + "size": true + } - "actions": ["copy", "remove", "insert", "updateAttr", "bindEevent", "createBlock"] + "actions": { + "copy": true, + "remove": true, + "insert": true, + "updateAttr": true, + "bindEevent": true, + "createBlock": true + }Also applies to: 46-46, 49-49
Line range hint
13258-13271: Consider adding value validation for checkbox group.The default values and options should be validated to ensure they match.
"modelValue": ["name1", "name2"], +"validator": { + "type": "array", + "items": { + "enum": ["name1", "name2", "name3"] + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
designer-demo/public/mock/bundle.json(51 hunks)mockServer/src/mock/get/app-center/v1/apps/schema/918.json(1 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(2 hunks)packages/canvas/DesignCanvas/src/importMap.js(2 hunks)packages/canvas/common/src/utils.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(1 hunks)packages/canvas/container/src/container.js(1 hunks)packages/canvas/render/src/RenderMain.js(1 hunks)packages/canvas/render/src/runner.js(2 hunks)packages/common/js/preview.js(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/plugins/bridge/src/js/resource.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(9 hunks)packages/toolbars/generate-code/src/Main.vue(1 hunks)packages/vue-generator/src/plugins/genDependenciesPlugin.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/canvas/container/src/container.js
🔇 Additional comments (13)
packages/plugins/materials/src/composable/useMaterial.js (1)
403-413:
Initialize materialState.dependencies to ensure it is an array
In fetchMaterial, materialState.dependencies may not be properly initialized if packages is undefined. Ensure it is always an array to prevent potential errors.
Apply this diff:
const packages = []
materials.forEach((response) => {
if (response.status === 'fulfilled' && response.value.materials) {
addMaterials(response.value.materials)
packages.push(...(response.value.materials.packages || []))
}
})
- materialState.dependencies = packages
+ materialState.dependencies = packages || []
setCanvasDeps()Likely invalid or redundant comment.
packages/common/js/preview.js (1)
Line range hint 29-36: LGTM! Verify dependency mapping consistency
The renaming from thirdPartyDeps to componentsDepsMap is consistent with the PR's objectives. The logic for processing scripts and styles is maintained correctly.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Dependency mapping change is consistently implemented
The renaming from thirdPartyDeps to componentsDepsMap has been thoroughly verified. The new componentsDepsMap is properly integrated across the codebase:
- Used in material state management (
useMaterial.js) - Correctly propagated to canvas components
- Properly handled in preview functionality
- No lingering references to the old
thirdPartyDepsname
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to thirdPartyDeps
rg "thirdPartyDeps" --type js --type vue
# Verify the new componentsDepsMap usage
rg "componentsDepsMap" --type js --type vue
Length of output: 140
Script:
#!/bin/bash
# Check for any remaining references to thirdPartyDeps, searching all text files
rg "thirdPartyDeps"
# Verify the new componentsDepsMap usage, searching all text files
rg "componentsDepsMap"
Length of output: 1749
packages/canvas/render/src/runner.js (1)
14-14: LGTM! Import statement updated correctly
The import statement has been updated to include setComponents, maintaining consistency with the new dependency management approach.
packages/vue-generator/src/plugins/genDependenciesPlugin.js (2)
25-25: LGTM! Schema destructuring updated correctly
The addition of the packages array to the schema destructuring aligns with the new material asset package protocol.
48-54: Verify package resolution strategy
The current implementation might need to consider package resolution conflicts between different sources (utils, componentsMap, and packages).
Let's analyze the package resolution strategy:
✅ Verification successful
Let me analyze the current implementation and verification results:
- The code in question handles package dependencies by storing them in
resDepsobject - From the search results, I can see:
- The package resolution happens in multiple places (utils, componentsMap, packages)
- There are multiple components using the same package with different versions (e.g. @opentiny/vue has versions 0.1.16, 0.1.17, 3.2.0, 3.4.1)
Let me check the full implementation of the dependency resolution:
After analyzing the full implementation, I can now provide a final response:
Package resolution strategy is correctly implemented
The current implementation has a well-defined precedence order that prevents conflicts:
- Utils dependencies are processed first, with duplicate prevention via
resDeps[packageName]check - Component dependencies are added only if:
- The package isn't already in resDeps
- The component is actually used (verified via componentsSet)
- Additional packages are added last, also with duplicate prevention
- Special handling for @opentiny/vue-icon to match @opentiny/vue version
This strategy ensures that:
- No duplicate packages are added
- Earlier declarations take precedence
- Only dependencies for used components are included
- Version consistency is maintained for related packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential package conflicts in the codebase
rg -A 5 "package: .*," --type js --type json
# Look for existing version resolution logic
rg "version.*latest" --type js
Length of output: 85133
Script:
#!/bin/bash
# Check the full implementation of genDependenciesPlugin
cat packages/vue-generator/src/plugins/genDependenciesPlugin.js
Length of output: 2744
packages/plugins/bridge/src/js/resource.js (3)
14-21: LGTM! Well-organized imports
The addition of useMaterial import aligns with the PR's objective of modifying the material asset package protocol.
188-221: Well-structured async/await implementation with proper error handling
The conversion to async/await and addition of try/catch improves error handling. The conditional handling of npm type resources with setCanvasDeps() aligns with the new material package protocol.
224-227: LGTM! Clear user feedback implementation
The success notification implementation provides clear feedback with dynamic messages based on the action type.
packages/design-core/src/preview/src/preview/Preview.vue (1)
83-83: LGTM! Simplified import map handling
The direct usage of store.setImportMap simplifies the code by removing unnecessary intermediate steps, making it more maintainable.
packages/toolbars/generate-code/src/Main.vue (1)
148-149: LGTM: Addition of packages array to appSchema
The addition of the packages array aligns with the PR objectives to support the new material asset package protocol.
Let's verify the packages array structure in other files:
✅ Verification successful
LGTM: Package array structure is consistent across the codebase
Based on the search results, the packages array structure is consistently defined and used across the codebase:
- Mock data in
designer-demo/public/mock/bundle.jsonshows the expected structure withname,package,version, andscriptproperties - The structure is properly consumed in
vue-generatorfor dependency generation - The implementation in
useMaterial.jscorrectly handles and propagates the packages array
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the structure of packages array across the codebase
# Expected: Consistent structure with name, npm package name, version, and CDN links
# Search for packages array definitions
rg -A 5 "packages.*\["
Length of output: 3097
mockServer/src/mock/get/app-center/v1/apps/schema/918.json (1)
2095-2096: Verify the axios version for security concerns
The addition of CDN link for axios is good, but we should verify if version 1.7.9 is the latest stable version without known vulnerabilities.
Let's check the version:
✅ Verification successful
Axios version 1.7.9 is secure and up-to-date
The version check reveals that:
- 1.7.9 is the current latest stable version from npm
- The most recent high severity vulnerability affected versions <= 1.7.3 (patched in 1.7.4)
- The specified version 1.7.9 is above the vulnerable range and includes all security patches
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify axios version security
# Expected: No known vulnerabilities for the specified version
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "axios", severities: [CRITICAL, HIGH]) {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Get latest version from npm
curl -s https://registry.npmjs.org/axios/latest | jq '.version'
Length of output: 1066
designer-demo/public/mock/bundle.json (2)
14081-14088: Verify package dependency configuration.
The new packages array introduces element-plus dependency configuration. While the structure looks good, we should verify the version consistency across components using this package.
✅ Verification successful
Package version consistency verified across the codebase
The version "2.4.2" of element-plus is consistently used across all instances in the codebase, including:
- designer-demo/public/mock/bundle.json
- mockServer/src/assets/json/appinfo.json
- packages/engine-cli/template/designer/public/mock/bundle.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all element-plus component definitions use consistent version
rg -A 1 '"package": "element-plus"' | grep -A 1 'version'
Length of output: 1561
635-635: Validate form item nesting rules.
The nesting rules for ElForm should be verified to ensure proper form structure.
Also applies to: 644-644, 647-647, 650-650
✅ Verification successful
Form nesting rules are properly defined in the configuration
The configuration correctly specifies that ElFormItem components are allowed as children within ElForm components through the childWhitelist property. This is consistently defined across multiple configuration files in the codebase, indicating proper form structure validation rules are in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ElFormItem is properly nested under ElForm
ast-grep --pattern 'componentName: "ElForm" {
$$$
componentName: "ElFormItem"
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Search for form nesting rules in bundle.json
rg -A 5 '"childWhitelist".*ElFormItem' public/mock/bundle.json
Length of output: 128
Script:
#!/bin/bash
# Search for component definitions related to ElForm and ElFormItem
rg -A 10 '"componentName":\s*"ElForm"' public/mock/bundle.json
Length of output: 128
Script:
#!/bin/bash
# Look for any validation or nesting rules
rg -i "form.*item.*nest|nest.*form.*item" public/mock/bundle.json
Length of output: 131
Script:
#!/bin/bash
# Find the correct path to bundle.json
fd bundle.json
Length of output: 216
Script:
#!/bin/bash
# Search for ElForm in all json files
rg -A 5 '"componentName":\s*"ElForm"' --type json
Length of output: 574
Script:
#!/bin/bash
# Search for childWhitelist in all json files
rg -A 5 '"childWhitelist".*ElFormItem' --type json
Length of output: 1522
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/canvas/common/src/utils.js (1)
Line range hint
110-117: Add error handling for undefined components and module exports to prevent runtime exceptionsIn the
setComponentsfunction, there's a possibility thatcomponentsormodules[exportName]may beundefined. Attempting to access or assign these values without validation could lead to runtime errors. Please add checks to ensure that these variables are defined before proceeding.Apply this diff to add necessary checks:
export const setComponents = async ({ package: pkg, script, components }) => { if (!pkg) return const modules = await dynamicImportComponents(pkg, script) + if (!components || typeof components !== 'object') { + console.error('Components object is undefined or invalid') + return + } Object.entries(components).forEach(([componentId, exportName]) => { + if (!modules || !modules[exportName]) { + console.error(`Module '${exportName}' not found in package '${pkg}'`) + return + } if (!window.TinyLowcodeComponent[componentId]) { window.TinyLowcodeComponent[componentId] = modules[exportName] } }) }
🧹 Nitpick comments (4)
packages/canvas/common/src/utils.js (1)
63-71: Add a null check fordocument.querySelectorto prevent potential errors when the import map is missingIn the
getImportMapKeysfunction, if the<script type="importmap">element does not exist in the document,document.querySelectorwill returnnull, leading to aTypeErrorwhen attempting to accesstextContent. While thetry...catchblock handles the error by returning an empty array, it's better to explicitly check for the element's existence to avoid unnecessary exceptions.Apply this diff to add a null check:
const getImportMapKeys = () => { try { - const importMaps = document.querySelector('script[type="importmap"]').textContent + const importMapScript = document.querySelector('script[type="importmap"]') + if (!importMapScript) { + return [] + } + const importMaps = importMapScript.textContent const importMapObject = JSON.parse(importMaps) return Object.keys(importMapObject.import) } catch (error) { return [] } }packages/plugins/materials/src/composable/useMaterial.js (2)
Line range hint
212-255: RenamesetThirdPartyDepstosetComponentsDepsfor consistencyThe function
generateThirdPartyDepshas been renamed togenerateComponentsDeps, butsetThirdPartyDepsstill retains the old naming convention. For consistency and clarity, consider renamingsetThirdPartyDepstosetComponentsDeps.Apply this diff to rename the function:
-const setThirdPartyDeps = (components) => { +const setComponentsDeps = (components) => { const { scripts = [], styles = [] } = generateComponentsDeps(components) - materialState.componentsDepsMap.scripts.push(...scripts) + materialState.componentsDepsMap.scripts.push(...scripts) styles.forEach((item) => materialState.componentsDepsMap.styles.add(item)) }Don't forget to update all references to
setThirdPartyDepsin the codebase:#!/bin/bash # Description: Update references from `setThirdPartyDeps` to `setComponentsDeps` # Replace all instances in the codebase rg 'setThirdPartyDeps' --files-with-matches | xargs sed -i 's/setThirdPartyDeps/setComponentsDeps/g'
295-319: Handle duplicate packages when setting canvas dependenciesIn the
setCanvasDepsfunction, when reducingallPackages, duplicate packages are filtered out based on the package name. However, if different versions or scripts are provided for the same package, this could lead to inconsistent behavior. Consider handling version conflicts or consolidating scripts for the same package.Apply this diff to improve duplicate package handling:
- if (scripts.find((item) => item.package === pkg)) { + const existingPackage = scripts.find((item) => item.package === pkg) + if (existingPackage) { + // Merge components and other properties if necessary + existingPackage.components = { ...existingPackage.components, ...npm.components } + return { scripts, styles } + }designer-demo/public/mock/bundle.json (1)
Line range hint
13258-13271: Consider adding validation for checkbox group values.The default modelValue array contains strings but the schema doesn't enforce value types. Consider adding type validation.
"modelValue": ["name1", "name2"], +"type": { + "type": "array", + "items": { + "type": "string" + } +}, "type": "checkbox",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
designer-demo/public/mock/bundle.json(51 hunks)mockServer/src/mock/get/app-center/v1/apps/schema/918.json(1 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(2 hunks)packages/canvas/DesignCanvas/src/importMap.js(2 hunks)packages/canvas/common/src/utils.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(1 hunks)packages/canvas/container/src/container.js(1 hunks)packages/canvas/render/src/RenderMain.js(1 hunks)packages/canvas/render/src/runner.js(2 hunks)packages/common/js/preview.js(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/plugins/bridge/src/js/resource.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(9 hunks)packages/toolbars/generate-code/src/Main.vue(1 hunks)packages/vue-generator/src/plugins/genDependenciesPlugin.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/canvas/container/src/container.js
🔇 Additional comments (15)
packages/canvas/common/src/utils.js (2)
79-101: Verify that removing the export statement from dynamicImportComponents does not affect external modules
The dynamicImportComponents function was previously exported but is now declared without the export keyword. This change could break functionality in other modules that import this function. Please verify that no external modules rely on this export, or consider re-exporting the function if necessary.
Run the following script to check for external imports of dynamicImportComponents:
125-127: Ensure window.componentsDepsMap is properly initialized before accessing its properties
In the updateDependencies function, window.componentsDepsMap is used to access styles. Please verify that window.componentsDepsMap is initialized and contains the expected structure to prevent potential undefined errors.
Run the following script to check the initialization of window.componentsDepsMap:
packages/canvas/DesignCanvas/src/importMap.js (1)
Line range hint 3-47: LGTM!
The getImportMapData function has been correctly updated to accept canvasDeps, allowing dynamic inclusion of external dependencies in the import map and styles. The default parameters and object destructuring are used appropriately.
packages/common/js/preview.js (1)
Line range hint 29-36: Verify handling of empty or malformed dependency maps
The code assumes componentsDepsMap will always have valid scripts and styles arrays. Consider adding defensive checks.
- const { scripts, styles } = useMaterial().materialState.componentsDepsMap
+ const { scripts = [], styles = [] } = useMaterial().materialState.componentsDepsMap || {}
params.scripts = {}
scripts
.filter((item) => item.script)
.forEach((item) => {
+ if (!item.package) return
params.scripts[item.package] = item.script
})
params.styles = [...styles]Let's verify the structure of componentsDepsMap across the codebase:
packages/vue-generator/src/plugins/genDependenciesPlugin.js (1)
48-54: Consider version conflict resolution strategy
The current implementation might lead to version conflicts when the same package is defined in multiple places (utils, componentsMap, packages).
Let's verify potential version conflicts in existing material definitions:
Consider implementing a version resolution strategy:
- Define priority order (e.g., packages > componentsMap > utils)
- Add version compatibility checks
- Log warnings for potential conflicts
packages/plugins/bridge/src/js/resource.js (3)
14-21: LGTM! Clean import organization
The addition of useMaterial aligns with the new material asset package protocol implementation.
189-203:
Fix variable shadowing in try block
The result variable is declared both in the outer scope and inside the try block, which could lead to unexpected behavior.
Apply this diff to fix the variable shadowing:
- const result = await requestUpdateReSource(data)
+ result = await requestUpdateReSource(data)
- const result = await requestAddReSource(data)
+ result = await requestAddReSource(data)Likely invalid or redundant comment.
213-221: Verify canvas dependency updates
The conditional handling of npm resources with setCanvasDeps() looks good, but let's verify that all canvas instances are properly updated.
✅ Verification successful
Let me gather more information about the canvas dependency update mechanisms to ensure completeness.
Based on the analysis of the shell script results, I can now provide the final response:
Canvas dependency updates are properly handled
The verification shows that the canvas dependency update mechanism is correctly implemented:
setCanvasDeps()inuseMaterial()handles npm dependencies by updating both material dependencies and utility dependencies- The
elsebranch withupdateUtils([result])correctly handles non-npm resources by updating the canvas utility functions - Both paths trigger appropriate canvas refreshes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of setCanvasDeps across the codebase
# Expected: setCanvasDeps should be called consistently after npm resource changes
# Check for setCanvasDeps usage
rg -A 5 "setCanvasDeps"
# Check for other npm resource handling patterns
ast-grep --pattern 'if ($expr.type === "npm") { $$$ }'
Length of output: 1945
Script:
#!/bin/bash
# Check for updateUtils usage and related canvas update patterns
rg -A 5 "updateUtils"
# Check for canvas refresh/update related functions
rg -A 5 "canvasApi"
# Look for npm type handling in material state
ast-grep --pattern 'type === "npm"'
Length of output: 46980
packages/design-core/src/preview/src/preview/Preview.vue (1)
83-83: Verify cleanup of old import map utilities
The simplified import map management looks good, but we should verify that all references to the old utility functions have been removed.
✅ Verification successful
Based on the search results from our verification attempts, we can make a conclusion:
Import map utilities have been properly cleaned up
The searches show that:
- Only the new
setImportMapmethod is being used in Preview.vue - No instances of the old
addUtilsImportMapwere found anywhere in the codebase - The import map handling is now centralized through the store's
setImportMapmethod - All other import map related code appears to be using the new simplified approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to old import map utilities
# Expected: No references to addUtilsImportMap should remain
# Search for any remaining references to the old function
rg "addUtilsImportMap"
# Check for any similar utility functions that might need updating
ast-grep --pattern 'function $name($_, $_) {
$$$
importMap
$$$
}'
Length of output: 97
Script:
#!/bin/bash
# Let's try a broader search for import map related functions and their usage
# Search for files containing 'importMap' keyword
rg -l "importMap"
# Then let's look for specific patterns related to import map manipulation
rg "\.importMap" -A 2 -B 2
# Search for store operations related to import maps
rg "setImportMap|addImportMap|updateImportMap" -A 2 -B 2
Length of output: 2076
Script:
#!/bin/bash
# Let's check the history of the importMap.js files to understand the changes
rg -A 10 -B 10 "addUtilsImportMap|setImportMap" packages/design-core/src/preview/src/preview/importMap.js packages/canvas/DesignCanvas/src/importMap.js
# Also check the store implementation to see how import maps are handled
rg -A 5 -B 5 "class.*Store" packages/design-core/src/preview/src/preview/
# Look for any utility functions related to import maps
ast-grep --pattern 'function $name($_) {
$$$
importMap
$$$
}'
Length of output: 299
packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
61-61: LGTM! Good use of Vue's reactivity system
Converting canvasSrcDoc to a ref ensures proper reactivity for canvas updates.
mockServer/src/mock/get/app-center/v1/apps/schema/918.json (1)
2095-2096: Verify axios CDN link compatibility.
The addition of the CDN link for axios should be verified for:
- Version compatibility with the application
- CORS policy compliance
- Availability and reliability of the CDN
designer-demo/public/mock/bundle.json (2)
14081-14088: Verify the package configuration for element-plus.
The package configuration looks good but let's verify the unpkg URLs are accessible and the version is stable.
✅ Verification successful
Element-plus package configuration is secure and accessible
The verification shows:
- Both CDN URLs (CSS and MJS) are accessible with 200 status codes
- Version 2.4.2 is well above the patched version (2.0.6) for the only known vulnerability
- The vulnerability was from April 2022 and has been addressed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify element-plus package URLs and version
curl -I "https://unpkg.com/element-plus@2.4.2/dist/index.full.mjs"
curl -I "https://unpkg.com/element-plus@2.4.2/dist/index.css"
# Check for any security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "element-plus") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 2676
635-638: Verify nesting rule configuration for form components.
The nesting rules for ElForm and ElFormItem look correct, but let's verify there are no other form components that should be included in the whitelist.
Also applies to: 768-771
✅ Verification successful
Nesting rules for form components are correctly configured
The verification shows that only ElForm and ElFormItem are the form-related components in the bundle, and the current nesting rule configuration correctly allows ElFormItem as a child of ElForm. No additional form components need to be included in the whitelist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other form-related components that may need to be included in nesting rules
ast-grep --pattern 'component: "El$_Form$_"'
Length of output: 46
Script:
#!/bin/bash
# Search for form-related components in the bundle.json file
rg -A 5 '"component":\s*"El.*Form' public/mock/bundle.json
# Also search for any form-related nesting rules
rg -A 5 '"childWhitelist".*Form' public/mock/bundle.json
Length of output: 247
Script:
#!/bin/bash
# Search for form-related components in the bundle.json file
rg -A 5 '"component":\s*"El.*Form' designer-demo/public/mock/bundle.json
# Also search for any form-related nesting rules
rg -A 5 '"childWhitelist".*Form' designer-demo/public/mock/bundle.json
Length of output: 804
packages/toolbars/generate-code/src/Main.vue (1)
148-149: Verify handling of new packages property in code generation
The addition of packages property to appSchema aligns with the new material asset protocol. However, we should ensure proper handling in the code generation process.
Let's verify the code generation handling:
Consider adding JSDoc documentation for the appSchema structure to help maintainers understand the new protocol format.
✅ Verification successful
The packages property is properly handled in code generation
The code review reveals that the packages property is safely integrated into the code generation flow:
- It's part of the
appSchemaobject which is passed togenerateAppCode - The spread operator with fallback (
[...(appData.packages || [])]) ensures safe handling of undefined values - The property is used alongside other essential schema properties like
componentsMap,pageSchema, andblockSchema - The generated code is properly processed through
generateAppCodeand transformed into file content for local saving
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the code generation process handles the new packages property
# Search for packages property usage in code generation
rg -A 5 "packages.*=.*appSchema\.packages"
# Search for potential validation or processing of packages
ast-grep --pattern 'function $_(appSchema) {
$$$
packages
$$$
}'
Length of output: 117
Script:
#!/bin/bash
# Let's broaden our search to understand the usage of packages in code generation
# Search for any references to packages in the codebase
rg -A 5 "packages.*=" packages/toolbars/generate-code/
# Look for appSchema usage patterns
rg -A 5 "appSchema\." packages/toolbars/generate-code/
# Search for code generation related functions
ast-grep --pattern 'function generate$_($$$) {
$$$
}'
# Check for any schema or type definitions
fd "\.d\.ts$|types?\.ts$" packages/toolbars/generate-code/ --exec cat {}
Length of output: 248
Script:
#!/bin/bash
# Let's check the broader context of the file and its usage
# Get the content of Main.vue to understand the context
cat packages/toolbars/generate-code/src/Main.vue
# Look for files that might consume the generated output
fd . packages/toolbars/generate-code/ -e .vue -e .ts -e .js --exclude "*.test.*" --exclude "*.spec.*" --exec rg -l "getPreGenerateInfo|appSchema"
# Search for any package-related types or interfaces
rg -g "*.ts" -g "*.d.ts" -g "*.vue" "interface.*App.*Schema|type.*App.*Schema" packages/toolbars/generate-code/
Length of output: 7776
packages/canvas/container/src/CanvasContainer.vue (1)
116-116:
Breaking Change: Verify migration from thirdPartyDeps to componentsDepsMap
The change from thirdPartyDeps to componentsDepsMap is a breaking change that requires careful verification of all consumers.
Let's verify the migration completeness:
Please ensure all consumers of this window property are updated. Consider:
- Adding a migration guide for downstream dependencies
- Documenting the new
componentsDepsMapstructure - Adding a deprecation warning for any transitional period
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/canvas/render/src/RenderMain.js (3)
70-82: Consider enhancing error handling for function utilities.The default function
() => {}might silently hide errors when the function generation fails. Consider logging these failures or notifying users about malformed function definitions.- const defaultFn = () => {} - utilsCollection[item.name] = generateFunction(item.content.value, context) || defaultFn + try { + const fn = generateFunction(item.content.value, context) + if (!fn) { + globalNotify({ + type: 'warning', + message: `Function utility "${item.name}" could not be generated` + }) + } + utilsCollection[item.name] = fn || (() => {}) + } catch (error) { + globalNotify({ + type: 'error', + message: `Failed to generate function utility "${item.name}": ${error.message}` + }) + utilsCollection[item.name] = () => {} + }
84-102: Consider internationalizing error messages and adding retry mechanism.The implementation handles npm imports well but has a few areas for improvement:
- Error message is hardcoded in Chinese
- No retry mechanism for failed imports
- globalNotify({ - type: 'error', - message: `工具类 ${npmUtils[index].name} 加载失败，请检查CDN地址是否配置正确` - }) + const message = t('error.npm_utility_load_failed', { + name: npmUtils[index].name + }) + globalNotify({ + type: 'error', + message, + action: { + text: t('action.retry'), + onClick: () => retryImport(npmUtils[index]) + } + })
Line range hint
441-486: Document breaking changes in API signatures.The transition of
setUtilsand related functions from synchronous to asynchronous is a breaking change that affects API consumers. This needs to be clearly documented.Consider:
- Adding JSDoc comments to document the async nature of these functions
- Updating the API documentation
- Adding migration guides for existing consumers
Example JSDoc:
+/** + * Sets utility functions and npm modules in the canvas context. + * @async + * @param {Array} data - Array of utility configurations + * @param {boolean} clear - Whether to clear existing utilities + * @param {boolean} isForceRefresh - Whether to force a canvas refresh + * @returns {Promise<void>} + * @throws {Error} When npm module import fails + */ const setUtils = async (data, clear, isForceRefresh) => {packages/canvas/common/src/utils.js (2)
59-61: Enhance JSDoc documentation.The current documentation could be more descriptive. Consider adding
@returns {string[]}and more detailed description./** - * 从页面importmap中获取模块的名称 - * @returns importmap中模块的名称集合 + * Retrieves module names from the page's import map configuration + * @returns {string[]} Array of module names defined in the import map, or empty array if not found + */
Line range hint
113-124: Add error handling for missing exports and consider cache invalidation.The function should verify that the requested component exports exist and consider cache invalidation strategy.
Consider these improvements:
export const getComponents = async ({ package: pkg, script, components }) => { if (!pkg) return const modules = await dynamicImportComponentLib({ pkg, script }) Object.entries(components).forEach(([componentId, exportName]) => { + if (!(exportName in modules)) { + console.warn(`Component "${exportName}" not found in package "${pkg}"`) + return + } if (!window.TinyLowcodeComponent[componentId]) { window.TinyLowcodeComponent[componentId] = modules[exportName] } }) }Also, consider adding a cache invalidation mechanism to handle component updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/common/src/utils.js(2 hunks)packages/canvas/render/src/RenderMain.js(1 hunks)
🔇 Additional comments (2)
packages/canvas/render/src/RenderMain.js (1)
Line range hint 109-120: Add missing await keywords to async function calls.
The updateUtils and deleteUtils functions call the async setUtils without awaiting its completion. This could lead to race conditions.
Apply this diff to fix the async calls:
-const updateUtils = (data) => {
+const updateUtils = async (data) => {
- setUtils(data, false, true)
+ await setUtils(data, false, true)
}
-const deleteUtils = (data) => {
+const deleteUtils = async (data) => {
data?.forEach((item) => {
if (utils[item.name]) {
delete utils[item.name]
}
})
- setUtils([], false, true)
+ await setUtils([], false, true)
}packages/canvas/common/src/utils.js (1)
73-73: Verify the correct property name in importMapObject.
The code accesses importMapObject.imports but comments mention "import". Verify that "imports" is the correct property name according to the Import Maps specification.
✅ Verification successful
Let me gather more information about import maps usage and specification in the codebase.
Let me check the content of the import map JSON file to verify the property name.
Property name "imports" is correct according to Import Maps specification
The code correctly uses importMapObject.imports as the property name. This is verified by:
- The import map JSON file which uses "imports" as the root property
- Multiple consistent usages across the codebase in files like
externalDeps.js,copyImportMap.js, and others - This follows the standard Import Maps specification where "imports" is the correct property name for specifying module specifier remapping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other import map usages to verify property name
rg -g '*.{js,ts,vue}' -A 2 -B 2 'importmap.*imports'
Length of output: 408
Script:
#!/bin/bash
# Search for importmap related files and configurations
rg -g '*.{js,ts,vue,json}' -i 'importmap'
# Look for any import map JSON files
fd -e json 'importmap'
# Search for import map related documentation or comments
rg -g '*.{js,ts,vue,md}' -i 'import map' -C 3
Length of output: 11260
Script:
#!/bin/bash
# Check the content of the import map JSON file
cat packages/design-core/src/preview/src/preview/importMap.json
# Look for any Import Maps specification references in documentation
rg -g '*.md' -i 'import maps? specification'
Length of output: 1893
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
Line range hint
135-143: Consider improving promise chain handlingThe current promise chain could be optimized for better error handling and race condition prevention:
- }).then(() => { + }).then(async () => { isRefresh.value = true state.searchValue = '' selectedBlockArray.value.length = 0 - useResource().fetchResource({ isInit: false }) // 添加区块分组，不需要重新init页面或者区块。 + await useResource().fetchResource({ isInit: false }) // 添加区块分组，不需要重新init页面或者区块。 useNotify({ message: '添加区块成功', type: 'success' })This change ensures that:
- The resource fetch completes before showing the success notification
- Any errors in resource fetching are properly caught by the catch block
packages/plugins/materials/src/composable/useMaterial.js (1)
Line range hint
451-471: Improve type safety ingetBlockDepsThe function should validate input types and handle potential undefined values.
Apply this diff:
const getBlockDeps = (dependencies = {}) => { const { scripts = [], styles = [] } = dependencies + if (!Array.isArray(scripts) || !Array.isArray(styles)) { + return + } scripts.length && scripts.forEach((npm) => { + if (!npm?.package) { + return + } const { package: pkg, script, css, components } = npm const npmInfo = materialState.componentsDepsMap.scripts.find((item) => item.package === pkg) if (!npmInfo || !npmInfo.script) { materialState.componentsDepsMap.scripts.push({ package: pkg, script, css, components }) } else { const components = npmInfo.components || {} npmInfo.components = { ...components, ...npm.components } } }) styles?.forEach((item) => materialState.componentsDepsMap.styles.add(item)) }
🧹 Nitpick comments (4)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (2)
Line range hint
28-39: Consider extracting block map logic to a composableThe block map initialization logic could be moved to a separate composable for better reusability and maintainability.
Consider creating a new composable
useBlockMap.js:// useBlockMap.js import { ref } from 'vue' export function useBlockMap() { const blockMap = ref(new Map()) const initGroupBlockMap = (groups = []) => { blockMap.value.clear() for (let group of groups) { const groupBlock = group?.blocks || [] for (let block of groupBlock) { blockMap.value.set(block.id, block) } } } const includesBlockInGroups = (blockId) => blockMap.value.has(blockId) return { initGroupBlockMap, includesBlockInGroups } }This would:
- Improve code organization
- Make the functionality more reusable
- Make testing easier
Line range hint
213-234: Consider optimizing API calls and data processingThe current implementation makes multiple separate API calls and processes results individually. This could be optimized for better performance.
- Promise.allSettled([fetchTenants(), fetchUsers(), fetchTags()]).then((results) => { + const staticTenants = [ + { name: '对所有组织开放', id: '1' }, + { name: '对当前组织开放', id: '2' } + ] + + Promise.all([fetchUsers(), fetchTags()]) + .then(([users, tags]) => { state.filters[0].children = [ - { - name: '对所有组织开放', - id: '1' - }, - { - name: '对当前组织开放', - id: '2' - } + ...staticTenants ] - state.filters[1].children = - results[1].status === 'fulfilled' - ? results[1].value.map((item) => ({ - name: item?.username, - id: item?.id - })) - : [] - state.filters[2].children = - results[2].status === 'fulfilled' ? results[2].value.map((item) => ({ name: item })) : [] - blockUsers.value = state.filters[1].children + state.filters[1].children = users.map(item => ({ + name: item?.username, + id: item?.id + })) + state.filters[2].children = tags.map(item => ({ name: item })) + blockUsers.value = state.filters[1].children + }) + .catch(error => { + message({ + message: `Failed to fetch filters: ${error.message || error}`, + status: 'error' + }) })This change:
- Removes unnecessary
fetchTenantscall since the data is static- Uses
Promise.allinstead ofPromise.allSettledfor simpler error handling- Adds proper error handling
- Simplifies the data processing logic
packages/plugins/materials/src/composable/useMaterial.js (2)
446-447: Consider error handling infetchMaterialThe function should handle potential errors when calling
setCanvasDeps.Apply this diff:
- setCanvasDeps() + try { + setCanvasDeps() + } catch (error) { + console.error('Failed to set canvas dependencies:', error) + }
485-485: Ensure safe access to canvas APIThe optional chaining on
canvasApi.valueis good, but consider adding a null check fordetail.Apply this diff:
- useCanvas().canvasApi.value?.canvasDispatch('updateDependencies', { detail: materialState.componentsDepsMap }) + const deps = materialState.componentsDepsMap + if (deps?.scripts?.length || deps?.styles?.size) { + useCanvas().canvasApi.value?.canvasDispatch('updateDependencies', { detail: deps }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/canvas/container/src/CanvasContainer.vue(2 hunks)packages/canvas/container/src/container.js(0 hunks)packages/canvas/render/src/runner.js(2 hunks)packages/common/js/preview.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(8 hunks)packages/plugins/materials/src/composable/useResource.js(1 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(1 hunks)packages/vue-generator/src/plugins/genDependenciesPlugin.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/canvas/container/src/container.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/canvas/render/src/runner.js
- packages/common/js/preview.js
- packages/vue-generator/src/plugins/genDependenciesPlugin.js
- packages/canvas/container/src/CanvasContainer.vue
🔇 Additional comments (4)
packages/plugins/materials/src/composable/useResource.js (1)
36-37: LGTM: New state property for canvas dependencies
The addition of canvasDeps with separate collections for scripts and styles is a good approach. Using Set for styles ensures uniqueness and prevents duplicates.
packages/plugins/materials/src/composable/useMaterial.js (3)
41-42: Update state management for dependencies
Good refactoring to replace thirdPartyDeps with componentsDepsMap. The new structure better represents the relationship between components and their dependencies.
274-288:
Enhance error handling in getUtilsDependencies
The function should handle potential undefined properties in the utility content.
Apply this diff:
return utils
- .filter((item) => item.type === 'npm' && item.content.cdnLink)
+ .filter((item) => item.type === 'npm' && item.content?.cdnLink)
.map((item) => {
- const { package: pkg, version, cdnLink } = item.content
+ const { package: pkg = '', version = '', cdnLink = '' } = item.content || {}
return {
package: pkg,
version,
script: cdnLink
}
})Likely invalid or redundant comment.
293-309: Verify dependency updates in setCanvasDeps
The function needs to ensure that duplicate scripts are not added and that the message payload is properly structured.
✅ Verification successful
Duplicate prevention and message structure are properly implemented
The code implementation in setCanvasDeps is correct and safe:
- It properly checks for duplicates using
scripts.find()before adding new utilities - The message payload structure is consistent with its consumer in DesignCanvas.vue
- The scripts array is properly initialized and managed throughout the file
- The spread operator
[...styles]creates a new array, preventing unintended mutations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential duplicate script entries
ast-grep --pattern 'useMessage().publish({ topic: "init_canvas_deps", data: $_ })'
Length of output: 84
Script:
#!/bin/bash
# Let's try a different pattern to match the message publishing
ast-grep --pattern 'useMessage().publish({
topic: "init_canvas_deps",
data: $$$
})'
# Also search for other usages of init_canvas_deps
rg "init_canvas_deps" -A 3
# Check the structure of getUtilsDependencies
ast-grep --pattern 'getUtilsDependencies = () => $$$'
Length of output: 796
Script:
#!/bin/bash
# Let's check how scripts array is populated and used
rg "scripts\." -A 3 packages/plugins/materials/src/composable/useMaterial.js
# Check the structure of useResource and resState
ast-grep --pattern 'const { scripts, styles } = useResource().resState.canvasDeps'
# Look for the definition of scripts array
rg "scripts =" packages/plugins/materials/src/composable/useMaterial.js
Length of output: 1489
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/canvas/render/src/RenderMain.js (4)
99-103: Internationalize error messagesThe error message is hardcoded in Chinese. Consider using i18n for better internationalization support.
- message: `工具类 ${npmUtils[index].name} 加载失败，请检查CDN地址是否配置正确` + message: t('utils.loadError', { name: npmUtils[index].name })
97-105: Enhance error handling and loggingConsider adding detailed error logging and structured error handling.
results.forEach((res, index) => { if (res.status !== 'fulfilled') { + console.error(`Failed to load utility ${npmUtils[index].name}:`, res.reason) globalNotify({ type: 'error', - message: `工具类 ${npmUtils[index].name} 加载失败，请检查CDN地址是否配置正确` + message: t('utils.loadError', { name: npmUtils[index].name }), + details: res.reason?.message }) return }
87-92: Validate function-type utilitiesConsider adding validation for function-type utilities to ensure they meet expected interfaces.
data .filter((item) => item.type === 'function') .forEach((item) => { const defaultFn = () => {} + const fn = generateFunction(item.content.value, context) + if (fn && typeof fn !== 'function') { + console.warn(`Utility ${item.name} is not a function`) + return + } - utilsCollection[item.name] = generateFunction(item.content.value, context) || defaultFn + utilsCollection[item.name] = fn || defaultFn })
106-111: Validate npm module exportsConsider adding validation for npm module exports to ensure they meet expected interfaces.
const module = res.value const { name, content } = npmUtils[index] const { exportName, destructuring } = content +const exportedValue = destructuring ? module[exportName] : module.default +if (exportedValue === undefined) { + console.warn(`Export ${exportName} not found in module ${name}`) + return +} -utilsCollection[name] = destructuring ? module[exportName] : module.default +utilsCollection[name] = exportedValuepackages/plugins/materials/src/composable/useMaterial.js (2)
304-320: Add debouncing for canvas dependency updates.The
setCanvasDepsfunction might be called frequently during initialization. Consider debouncing the message publishing.Apply this diff to add debouncing:
+const debouncedPublish = utils.debounce((data) => { + useMessage().publish({ + topic: 'init_canvas_deps', + data + }) +}, 100) const setCanvasDeps = () => { const { scripts, styles } = useResource().resState.canvasDeps getUtilsDependencies().forEach((util) => { if (scripts.find((item) => util.package === item.package)) { return } scripts.push(util) }) - useMessage().publish({ - topic: 'init_canvas_deps', - data: { scripts: scripts, styles: [...styles] } - }) + debouncedPublish({ scripts: scripts, styles: [...styles] }) }
468-479: Ensure atomic updates for dependency maps.The block dependency handling should ensure atomic updates to prevent race conditions.
Apply this diff to ensure atomic updates:
- materialState.componentsDepsMap.scripts.push({ package: pkg, script, css, components }) + const newDeps = [...materialState.componentsDepsMap.scripts] + newDeps.push({ package: pkg, script, css, components }) + materialState.componentsDepsMap.scripts = newDepsdesigner-demo/public/mock/bundle.json (3)
14041-14047: Consider adding integrity hashes for CDN resourcesFor the TinyVue package CDN resources, consider adding integrity hashes to prevent tampering:
{ "name": "TinyVue组件库", "package": "@opentiny/vue", "version": "3.14.0", "destructuring": true, "script": "https://unpkg.com/@opentiny/vue@~3.14/runtime/tiny-vue.mjs", + "scriptIntegrity": "<sha384-hash>", "css": "https://unpkg.com/@opentiny/vue-theme@~3.14/index.css", + "cssIntegrity": "<sha384-hash>" }
14048-14054: Consider pinning exact versions for CDN URLsThe Element Plus package uses tilde ranges in CDN URLs which could potentially pull in breaking changes. Consider pinning to exact versions:
{ "name": "element-plus组件库", "package": "element-plus", "version": "2.4.2", - "script": "https://unpkg.com/element-plus@2.4.2/dist/index.full.mjs", + "script": "https://unpkg.com/element-plus@2.4.2/dist/index.full.mjs", "css": "https://unpkg.com/element-plus@2.4.2/dist/index.css" }
14039-14054: Consider adding package validation metadataThe packages section could benefit from additional metadata to help validate package integrity and compatibility:
"packages": [ { "name": "TinyVue组件库", "package": "@opentiny/vue", "version": "3.14.0", + "engines": { + "node": ">=14.0.0", + "npm": ">=6.0.0" + }, + "peerDependencies": { + "vue": "^3.0.0" + } } // ... other packages ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
designer-demo/public/mock/bundle.json(33 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/DesignCanvas/src/importMap.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(2 hunks)packages/canvas/container/src/container.js(0 hunks)packages/canvas/render/src/RenderMain.js(2 hunks)packages/canvas/render/src/runner.js(2 hunks)packages/common/js/preview.js(2 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/plugins/materials/src/composable/useMaterial.js(8 hunks)packages/plugins/materials/src/composable/useResource.js(1 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(1 hunks)packages/toolbars/generate-code/src/Main.vue(1 hunks)
💤 Files with no reviewable changes (1)
- packages/canvas/container/src/container.js
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/toolbars/generate-code/src/Main.vue
- packages/canvas/DesignCanvas/src/importMap.js
- packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue
- packages/canvas/container/src/CanvasContainer.vue
- packages/common/js/preview.js
- packages/plugins/materials/src/composable/useResource.js
🔇 Additional comments (10)
packages/canvas/render/src/RenderMain.js (1)
Line range hint
71-114: Consider handling importMap existence scenariosBased on the previous review comments, the code should handle two scenarios:
- When importMap exists
- When importMap doesn't exist (should use cdnLink)
Let's verify the importMap handling:
Consider refactoring the npm utils import logic to handle both scenarios:
const npmUtils = data.filter((item) => item.type === 'npm' && item.content.cdnLink) -const results = await Promise.allSettled(npmUtils.map((item) => import(/* @vite-ignore */ item.content.package))) +const results = await Promise.allSettled(npmUtils.map(async (item) => { + const { content } = item + const importPath = window.host?.importMap?.[content.package] || content.cdnLink + return import(/* @vite-ignore */ importPath) +}))packages/canvas/render/src/runner.js (2)
14-14: LGTM! Import statement updated to support new dependency management.The import statement has been updated to include new utilities that support the revised dependency management approach.
84-85: 🛠️ Refactor suggestionVerify error handling for dependency loading.
While the dependency loading logic is sound, consider adding error handling to gracefully handle component loading failures.
Apply this diff to improve error handling:
Promise.all([ - ...window.componentsDeps.map(getComponents), - ...scripts.map((src) => addScript(src)).concat(styles.map((src) => addStyle(src))) + ...window.componentsDeps.map(dep => + getComponents(dep).catch(err => { + console.error(`Failed to load component:`, err); + return null; + }) + ), + ...scripts.map(src => + addScript(src).catch(err => { + console.error(`Failed to load script: ${src}`, err); + return null; + }) + ), + ...styles.map(src => + addStyle(src).catch(err => { + console.error(`Failed to load style: ${src}`, err); + return null; + }) + ) ]).finally(() => create(config))packages/design-core/src/preview/src/preview/Preview.vue (1)
83-83: LGTM! Simplified import map handling.The direct usage of
store.setImportMap(importMapData)improves code clarity by removing unnecessary complexity.packages/canvas/DesignCanvas/src/DesignCanvas.vue (2)
62-62: LGTM! Enhanced reactivity with ref().Converting
canvasSrcDocto useref()ensures proper reactivity in Vue's composition API.
64-75: 🛠️ Refactor suggestionAdd error handling for dependency initialization.
The message subscription implementation should handle potential errors during import map generation.
Apply this diff to improve error handling:
useMessage().subscribe({ topic: 'init_canvas_deps', callback: (deps) => { if (canvasSrc) { return } + try { const { importMap, importStyles } = getImportMapData(getMergeMeta('engine.config')?.importMapVersion, deps) canvasSrcDoc.value = initCanvas(importMap, importStyles).html + } catch (error) { + console.error('Failed to initialize canvas dependencies:', error) + // Consider emitting an error event to notify the parent + useMessage().publish({ + topic: 'canvas_init_error', + data: { error } + }) + } } })Likely invalid or redundant comment.
packages/plugins/materials/src/composable/useMaterial.js (3)
42-43: LGTM! Improved state management structure.The new
componentsDepsMapstructure provides better organization for scripts and styles dependencies.
285-299: 🛠️ Refactor suggestionAdd validation for utility dependencies.
The
getUtilsDependenciesfunction should validate the utility structure before accessing properties.Apply this diff to add validation:
const getUtilsDependencies = () => { const { utils } = useResource().resState return utils - .filter((item) => item.type === 'npm' && item.content.cdnLink) + .filter((item) => { + return item?.type === 'npm' && + item?.content?.cdnLink && + item?.content?.package + }) .map((item) => { const { package: pkg, version, cdnLink } = item.content return { package: pkg, version, script: cdnLink } }) }Likely invalid or redundant comment.
323-366: 🛠️ Refactor suggestionAdd input validation for material dependencies.
The
parseMaterialsDependenciesfunction should validate the input structure.Apply this diff to add validation:
const parseMaterialsDependencies = (materialBundle) => { + if (!materialBundle || typeof materialBundle !== 'object') { + console.warn('Invalid material bundle provided') + return + } const { packages = [], components = [] } = materialBundle + if (!Array.isArray(packages) || !Array.isArray(components)) { + console.warn('Invalid packages or components structure') + return + } const { scripts: scriptsDeps, styles: stylesDeps } = useResource().resState.canvasDeps packages?.forEach((pkg) => { + if (!pkg?.package) { + console.warn('Invalid package structure:', pkg) + return + } if (scriptsDeps.find((item) => item.package === pkg.package)) { return } scriptsDeps.push(pkg) if (!pkg.css) { return } if (Array.isArray(pkg.css)) { pkg.css.map(stylesDeps.add) } else { stylesDeps.add(pkg.css) } })Likely invalid or redundant comment.
designer-demo/public/mock/bundle.json (1)
14040-14054: Verify package versions and security advisoriesThe package configurations reference specific versions that should be verified for security and compatibility.
✅ Verification successful
Consider updating package versions for improved features and security
Both packages have newer versions available that include bug fixes and improvements:
- @opentiny/vue: 3.14.0 → 3.21.0
- element-plus: 2.4.2 → 2.9.2
No critical security vulnerabilities found in current versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the specified packages # Check latest versions echo "Checking @opentiny/vue latest version..." curl -s https://registry.npmjs.org/@opentiny/vue/latest | jq '.version' echo "Checking element-plus latest version..." curl -s https://registry.npmjs.org/element-plus/latest | jq '.version' # Check for security advisories echo "Checking security advisories..." gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "element-plus") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 998
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/bridge/src/js/resource.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/plugins/bridge/src/js/resource.js (3)
14-14: LGTM! Well-structured importsThe import statement is properly organized and includes all necessary utilities from the meta-register package.
178-179: LGTM! Improved code readabilityGood use of async/await and the
isEditvariable makes the code more maintainable.Also applies to: 181-182
215-221: 🛠️ Refactor suggestionMove success notification after all operations complete
The success notification should only be shown after confirming all operations (including canvas updates) have completed successfully.
Consider moving the success notification and state updates into a
finallyblock:- useNotify({ - type: 'success', - message: `${isEdit ? '修改' : '创建'}成功` - }) - emit('refresh', state.type) - state.refresh = true - callback() + try { + // ... existing try blocks ... + } catch (error) { + // ... error handling ... + } finally { + if (!error) { + useNotify({ + type: 'success', + message: `${isEdit ? '修改' : '创建'}成功` + }) + emit('refresh', state.type) + state.refresh = true + callback() + } + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/plugins/materials/src/composable/useMaterial.js (3)
42-43: Consider using TypeScript interfaces for state objects.The
componentsDepsMapandpackagesproperties would benefit from TypeScript interfaces to ensure type safety and provide better documentation of the expected structure.interface ComponentsDepsMap { scripts: Array<{ package: string; script: string; components?: Record<string, string>; }>; styles: Set<string>; } interface MaterialState { components: any[]; blocks: any[]; componentsDepsMap: ComponentsDepsMap; packages: Array<{ package: string; version: string; script?: string; css?: string | string[]; }>; }
304-320: Consider adding error handling for message publishing.The function should handle potential errors during message publishing and provide feedback.
const setCanvasDeps = () => { const { scripts, styles } = useResource().appSchemaState.canvasDeps getUtilsDependencies().forEach((util) => { if (scripts.find((item) => util.package === item.package)) { return } scripts.push(util) }) + try { useMessage().publish({ topic: 'init_canvas_deps', data: { scripts: scripts, styles: [...styles] } }) + } catch (error) { + console.error('Failed to publish canvas dependencies:', error) + } }
467-471: Consider memory management for Set operations.The Set of styles could grow indefinitely. Consider implementing a cleanup mechanism.
const getBlockDeps = (dependencies = {}) => { const { scripts = [], styles = [] } = dependencies + const MAX_STYLES = 1000 // Adjust based on your needs scripts.length && scripts.forEach((npm) => { const { package: pkg, script, css, components } = npm const npmInfo = materialState.componentsDepsMap.scripts.find((item) => item.package === pkg) if (!npmInfo || !npmInfo.script) { materialState.componentsDepsMap.scripts.push({ package: pkg, script, css, components }) } else { const components = npmInfo.components || {} npmInfo.components = { ...components, ...npm.components } } }) - styles?.forEach((item) => materialState.componentsDepsMap.styles.add(item)) + styles?.forEach((item) => { + materialState.componentsDepsMap.styles.add(item) + if (materialState.componentsDepsMap.styles.size > MAX_STYLES) { + const iterator = materialState.componentsDepsMap.styles.values() + materialState.componentsDepsMap.styles.delete(iterator.next().value) + } + }) }Also applies to: 479-479
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/canvas/container/src/CanvasContainer.vue(2 hunks)packages/common/js/preview.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/common/js/preview.js
- packages/canvas/container/src/CanvasContainer.vue
🔇 Additional comments (2)
packages/plugins/materials/src/composable/useMaterial.js (2)
285-299:⚠️ Potential issueAdd null checks for nested properties.
The function assumes
item.contentalways exists and has acdnLinkproperty. This could lead to runtime errors.Apply this diff to add proper null checks:
return utils - .filter((item) => item.type === 'npm' && item.content.cdnLink) + .filter((item) => item.type === 'npm' && item.content?.cdnLink) .map((item) => { - const { package: pkg, version, cdnLink } = item.content + const { package: pkg, version = '', cdnLink = '' } = item.content || {} return { package: pkg, version, script: cdnLink } })Likely invalid or redundant comment.
323-366:⚠️ Potential issueAdd input validation and error handling.
The function should validate the input structure and handle potential errors.
Apply this diff to add proper validation:
const parseMaterialsDependencies = (materialBundle) => { + if (!materialBundle || typeof materialBundle !== 'object') { + console.warn('Invalid material bundle provided') + return + } const { packages = [], components = [] } = materialBundle const { scripts: scriptsDeps, styles: stylesDeps } = useResource().appSchemaState.canvasDeps packages?.forEach((pkg) => { + if (!pkg?.package) { + console.warn('Invalid package configuration:', pkg) + return + } if (scriptsDeps.find((item) => item.package === pkg.package)) { return }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/plugins/materials/src/composable/useMaterial.js (1)
493-493:⚠️ Potential issueAdd error handling for canvas dispatch.
The canvas dispatch operation should handle potential errors when the canvas API is not available.
🧹 Nitpick comments (2)
packages/plugins/materials/src/composable/useMaterial.js (2)
42-43: Consider using TypeScript interfaces for state objects.The reactive state objects would benefit from TypeScript interfaces to clearly define the shape of
componentsDepsMapandpackages. This would improve maintainability and provide better IDE support.interface ComponentsDepsMap { scripts: Array<{ package: string; script: string; components?: Record<string, string>; }>; styles: Set<string>; } interface MaterialState { components: any[]; blocks: any[]; componentsDepsMap: ComponentsDepsMap; packages: any[]; // Define specific package structure }
540-540: Document the new exported functions.The newly exported
setCanvasDepsfunction lacks JSDoc documentation. Add documentation to maintain consistency with other exported functions.+ /** + * Sets canvas dependencies including material and utility dependencies. + * Notifies the canvas to refresh the importmap. + */ setCanvasDeps,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/materials/src/composable/useMaterial.js(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/plugins/materials/src/composable/useMaterial.js (3)
285-299:⚠️ Potential issueAdd error handling for missing or malformed utils.
The
getUtilsDependenciesfunction should handle potential errors when accessingitem.contentproperties.const getUtilsDependencies = () => { const { utils } = useResource().appSchemaState return utils - .filter((item) => item.type === 'npm' && item.content.cdnLink) + .filter((item) => item.type === 'npm' && item.content?.cdnLink) .map((item) => { + if (!item.content?.package) { + console.warn(`Invalid util item: missing package name`, item) + return null + } const { package: pkg, version, cdnLink } = item.content return { package: pkg, version, script: cdnLink } - }) + }) + .filter(Boolean)Likely invalid or redundant comment.
468-479:⚠️ Potential issueEnsure atomic updates for reactive state.
Direct mutations to
materialState.componentsDepsMapmay not trigger reactivity properly. Consider using atomic updates.-materialState.componentsDepsMap.scripts.push({ package: pkg, script, css, components }) +const updatedScripts = [...materialState.componentsDepsMap.scripts, { package: pkg, script, css, components }] +materialState.componentsDepsMap.scripts = updatedScripts -styles?.forEach((item) => materialState.componentsDepsMap.styles.add(item)) +const updatedStyles = new Set([...materialState.componentsDepsMap.styles, ...(styles || [])]) +materialState.componentsDepsMap.styles = updatedStylesLikely invalid or redundant comment.
323-366:⚠️ Potential issueAdd validation for material bundle structure.
The
parseMaterialsDependenciesfunction should validate the material bundle structure before processing.const parseMaterialsDependencies = (materialBundle) => { + if (!materialBundle || typeof materialBundle !== 'object') { + console.error('Invalid material bundle:', materialBundle) + return + } + const { packages, components } = materialBundle const { scripts: scriptsDeps, styles: stylesDeps } = useResource().appSchemaState.canvasDeps packages?.forEach((pkg) => { + if (!pkg?.package) { + console.warn('Invalid package in material bundle:', pkg) + return + } if (scriptsDeps.find((item) => item.package === pkg.package)) { return }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/plugins/materials/src/composable/useMaterial.js (3)
285-299:⚠️ Potential issueEnsure
item.contentis defined ingetUtilsDependenciesIn the
getUtilsDependenciesfunction, ifitem.contentis undefined, accessingitem.content.cdnLinkcan lead to a runtime error. Please add a check to confirm thatitem.contentexists before accessing its properties.Apply this diff to fix the issue:
return utils .filter((item) => item.type === 'npm' && item.content && item.content.cdnLink) .map((item) => { const { package: pkg, version, cdnLink } = item.content return { package: pkg, version, script: cdnLink } })
327-343:⚠️ Potential issueAdd validation for
pkg.packageinparseMaterialsDependenciesIn the
packages?.forEachloop, ifpkg.packageis undefined, it could cause unexpected behavior when pushing toscriptsDeps. Please add a check to ensurepkg.packageexists before proceeding.Apply this diff:
packages?.forEach((pkg) => { + if (!pkg?.package) { + return + } if (scriptsDeps.find((item) => item.package === pkg.package)) { return } scriptsDeps.push(pkg) // Rest of the code...
492-492:⚠️ Potential issueAdd error handling for
canvasDispatchmethodThe
canvasDispatchmethod might be undefined ifcanvasApi.valueis not available, which can lead to runtime errors. Please add a conditional check before callingcanvasDispatch.Apply this diff:
- useCanvas().canvasApi.value?.canvasDispatch('updateDependencies', { detail: materialState.componentsDepsMap }) + const canvasApi = useCanvas().canvasApi.value + if (canvasApi?.canvasDispatch) { + canvasApi.canvasDispatch('updateDependencies', { detail: materialState.componentsDepsMap }) + } else { + console.error('Canvas API is not available') + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/canvas/DesignCanvas/src/importMap.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(2 hunks)packages/common/js/preview.js(1 hunks)packages/plugins/bridge/src/js/resource.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(8 hunks)packages/plugins/materials/src/composable/useResource.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/common/js/preview.js
- packages/plugins/materials/src/composable/useResource.js
- packages/plugins/bridge/src/js/resource.js
- packages/canvas/container/src/CanvasContainer.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/canvas/common/src/utils.js (1)
114-123: Enhance error handling and cache management.The function should handle potential errors from
dynamicImportComponentLiband implement cache invalidation.Apply this diff to improve error handling and caching:
export const getComponents = async ({ package: pkg, script, components }) => { if (!pkg) return + try { const modules = await dynamicImportComponentLib({ pkg, script }) Object.entries(components).forEach(([componentId, exportName]) => { + if (!modules[exportName]) { + console.warn(`Component ${exportName} not found in package ${pkg}`) + return + } if (!window.TinyLowcodeComponent[componentId]) { window.TinyLowcodeComponent[componentId] = modules[exportName] } }) + } catch (error) { + console.error(`Failed to load components from package ${pkg}:`, error) + throw error + } }packages/plugins/materials/src/composable/useMaterial.js (2)
360-364: Validate array before iteration.The code assumes
stylesis an array without validation.Apply this diff to add array validation:
- if (Array.isArray(styles)) { - styles.forEach((item) => stylesDeps.add(item)) - } else { - stylesDeps.add(styles) - } + if (!styles) return + + const stylesToAdd = Array.isArray(styles) ? styles : [styles] + stylesToAdd.forEach((item) => { + if (item) stylesDeps.add(item) + })
478-478: Improve array handling and reactivity.The current implementation might have issues with array validation and reactive updates.
Apply this diff to improve array handling:
- styles?.forEach((item) => materialState.componentsDepsMap.styles.add(item)) + if (Array.isArray(styles)) { + styles.forEach((item) => { + if (item) materialState.componentsDepsMap.styles.add(item) + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/canvas/common/src/utils.js(1 hunks)packages/canvas/render/src/runner.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/canvas/render/src/runner.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/canvas/common/src/utils.js (2)
85-88: 🛠️ Refactor suggestionAdd input validation for required parameters.
The function should validate the
pkgparameter before proceeding.Apply this diff to add input validation:
const dynamicImportComponentLib = async ({ pkg, script }) => { + if (!pkg) { + throw new Error('Package name is required') + } + if (window.TinyComponentLibs[pkg]) { return window.TinyComponentLibs[pkg] }Likely invalid or redundant comment.
100-102: 🛠️ Refactor suggestionImprove error handling in dynamic imports.
The current error handling silently fails by returning an empty object. This could mask critical issues.
Apply this diff to improve error handling:
} catch (error) { - modules = {} + console.error(`Failed to load component library ${pkg}:`, error) + throw error }Likely invalid or redundant comment.
packages/plugins/materials/src/composable/useMaterial.js (2)
322-324: 🛠️ Refactor suggestionAdd input validation for material bundle.
The function should validate the input object before processing.
Apply this diff to add input validation:
const parseMaterialsDependencies = (materialBundle) => { + if (!materialBundle || typeof materialBundle !== 'object') { + console.warn('Invalid material bundle') + return + } const { packages, components } = materialBundleLikely invalid or redundant comment.
288-298: 🛠️ Refactor suggestionAdd property validation in utility dependency parsing.
The function assumes properties exist without proper validation, which could lead to runtime errors.
Apply this diff to add property validation:
return utils - .filter((item) => item.type === 'npm' && item.content.cdnLink) + .filter((item) => item.type === 'npm' && item.content?.cdnLink) .map((item) => { + if (!item.content?.package) { + console.warn('Invalid utility dependency: missing package name') + return null + } const { package: pkg, version, cdnLink } = item.content return { package: pkg, version, script: cdnLink } - }) + }) + .filter(Boolean)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/plugins/materials/src/composable/useResource.js (1)
36-37: Add JSDoc documentation for the new materialsDeps property.While the structure is well-designed (using Set for styles to prevent duplicates), please add JSDoc documentation to describe the purpose and structure of the
materialsDepsproperty.+/** + * @typedef {Object} MaterialsDeps + * @property {Array<string>} scripts - Array of script URLs for material dependencies + * @property {Set<string>} styles - Set of style URLs for material dependencies + */ const appSchemaState = reactive({ dataSource: [], pageTree: [], langs: {}, utils: {}, globalState: [], materialsDeps: { scripts: [], styles: new Set() } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/common/js/preview.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(7 hunks)packages/plugins/materials/src/composable/useResource.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/common/js/preview.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (7)
packages/plugins/materials/src/composable/useMaterial.js (5)
22-23: LGTM! Good separation of concerns.The new imports and state management changes improve the code organization by:
- Separating message handling and resource management
- Introducing a clear structure for component dependencies
Also applies to: 42-43
503-504: LGTM! Good documentation of exported functions.The new exported functions are well-documented and maintain consistency with the existing exports.
457-457:⚠️ Potential issueVerify that styles is an array before iterating.
When using
styles?.forEach(...), it's important to check thatstylesis an array to prevent potential runtime errors.Apply this diff:
- styles?.forEach((item) => materialState.componentsDepsMap.styles.add(item)) + if (Array.isArray(styles)) { + styles.forEach((item) => materialState.componentsDepsMap.styles.add(item)) + }Likely invalid or redundant comment.
293-298:⚠️ Potential issueAdd error handling for message publishing.
The
updateCanvasDepsfunction should handle potential failures in message publishing.Apply this diff to add proper error handling:
const updateCanvasDeps = () => { + try { + const deps = getCanvasDeps() + if (!deps.scripts.length && !deps.styles.length) { + console.warn('No dependencies to update') + return + } useMessage().publish({ topic: 'init_canvas_deps', - data: getCanvasDeps() + data: deps }) + } catch (error) { + console.error('Failed to update canvas dependencies:', error) + } }Likely invalid or redundant comment.
301-344:⚠️ Potential issueAdd validation for material bundle structure.
The
parseMaterialsDependenciesfunction needs input validation and proper error handling.Apply this diff to add proper validation:
const parseMaterialsDependencies = (materialBundle) => { + if (!materialBundle || typeof materialBundle !== 'object') { + console.warn('Invalid materialBundle') + return + } const { packages, components } = materialBundle + if (!Array.isArray(packages) && !Array.isArray(components)) { + console.warn('No valid dependencies found in materialBundle') + return + } const { scripts: scriptsDeps, styles: stylesDeps } = useResource().appSchemaState.materialsDeps + if (!scriptsDeps || !stylesDeps) { + console.error('Invalid dependency state structure') + return + } packages?.forEach((pkg) => { + try { if (!pkg.script || scriptsDeps.find((item) => item.package === pkg.package)) { return } scriptsDeps.push(pkg) if (!pkg.css) { return } if (Array.isArray(pkg.css)) { - pkg.css.forEach((item) => stylesDeps.add(item)) + pkg.css.filter(Boolean).forEach((item) => stylesDeps.add(item)) } else { stylesDeps.add(pkg.css) } + } catch (error) { + console.error(`Error processing package ${pkg?.package}:`, error) + } })Likely invalid or redundant comment.
packages/plugins/materials/src/composable/useResource.js (2)
192-192: LGTM!The export of
getUtilsDepsfollows the existing pattern correctly.
36-37: Verify integration with canvas dependencies.Please ensure that
materialsDepsis properly initialized and consumed by the canvas. Let's verify the integration:Also applies to: 178-187
✅ Verification successful
✓ Integration with canvas dependencies verified
The
materialsDepsstructure is properly initialized and consistently accessed across the codebase throughuseResource().appSchemaState. The canvas integration is correctly implemented in CanvasContainer.vue, maintaining the expected structure for scripts and styles.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how materialsDeps is used across the codebase # Check for materialsDeps usage echo "Checking materialsDeps usage:" rg -l "materialsDeps" # Check for getUtilsDeps usage echo -e "\nChecking getUtilsDeps usage:" rg -l "getUtilsDeps" # Look for potential initialization points echo -e "\nChecking potential initialization points:" ast-grep --pattern 'appSchemaState.materialsDeps = $_'Length of output: 591
Script:
#!/bin/bash # Check how materialsDeps is used in each file echo "materialsDeps usage in CanvasContainer.vue:" rg "materialsDeps" packages/canvas/container/src/CanvasContainer.vue -A 2 echo -e "\nmaterialsDeps usage in useMaterial.js:" rg "materialsDeps" packages/plugins/materials/src/composable/useMaterial.js -A 2 echo -e "\nInitialization in useResource.js:" rg "materialsDeps.*=|const.*materialsDeps" packages/plugins/materials/src/composable/useResource.js -A 2Length of output: 923
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/plugins/bridge/src/js/resource.js (2)
200-206: 🛠️ Refactor suggestionImprove error handling in the create flow.
Similar to the edit flow, the create flow should have better error handling and input validation.
} else { const result = await requestAddReSource(data) - if (result) { - useResource().appSchemaState[data.category].push(result) + if (!result) { + throw new Error('Add resource request failed') } + useResource().appSchemaState[data.category].push(result) }
208-216: 🛠️ Refactor suggestionHandle async operations in success flow.
The canvas update and subsequent operations should handle potential async failures.
- generateBridgeUtil(getAppId()) + await generateBridgeUtil(getAppId()) useNotify({ type: 'success', message: `${isEdit ? '修改' : '创建'}成功` }) emit('refresh', state.type) state.refresh = true callback()
🧹 Nitpick comments (1)
packages/plugins/bridge/src/js/resource.js (1)
217-221: Enhance error handling with specific error types and logging.Consider adding:
- Error type classification for different failure scenarios
- Error logging for debugging
- Cleanup operations in case of failure
} catch (error) { + console.error('Resource operation failed:', error) useNotify({ type: 'error', - message: `工具类${isEdit ? '修改' : '创建'}失败：${error.message}` + message: `工具类${isEdit ? '修改' : '创建'}失败：${ + error.code === 'NETWORK_ERROR' ? '网络错误' : + error.code === 'VALIDATION_ERROR' ? '验证失败' : + error.message + }` }) + // Cleanup if needed + if (!isEdit && state.resource.id) { + try { + await requestDeleteReSource(`app=${getAppId()}&id=${state.resource.id}`) + } catch (cleanupError) { + console.error('Cleanup failed:', cleanupError) + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/canvas/DesignCanvas/src/importMap.js(2 hunks)packages/canvas/common/src/utils.js(1 hunks)packages/plugins/bridge/src/js/resource.js(2 hunks)packages/plugins/materials/src/composable/useMaterial.js(7 hunks)packages/plugins/materials/src/composable/useResource.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/canvas/DesignCanvas/src/importMap.js (1)
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/importMap.js:51-51
Timestamp: 2025-01-13T03:46:13.817Z
Learning: The `getImportMapData` function in `packages/canvas/DesignCanvas/src/importMap.js` has default parameter handling that initializes `canvasDeps` with empty arrays for `scripts` and `styles`, making additional null checks unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (16)
packages/canvas/DesignCanvas/src/importMap.js (4)
3-3: LGTM! Function signature updated with proper defaults.The new parameter
canvasDepsis well-structured with appropriate default values for both scripts and styles.
48-49: LGTM! Dependencies are properly merged.The import map correctly combines dependencies from multiple sources using the spread operator.
53-53: LGTM! Styles are properly combined.The styles from both block requirements and canvas dependencies are correctly merged.
35-41: Verify script URLs before adding to import map.The function processes external scripts but should validate URLs before adding them to the import map to prevent security issues.
Run this script to check for potentially unsafe URLs:
packages/canvas/common/src/utils.js (3)
62-77: LGTM! Import map keys are safely extracted.The function includes proper error handling and null checks for the import map element.
115-124: 🛠️ Refactor suggestionAdd validation for component exports.
The function should validate that the exported components exist in the modules before caching.
Apply this diff to add validation:
export const getComponents = async ({ package: pkg, script, components }) => { if (!pkg) return const modules = await dynamicImportComponentLib({ pkg, script }) Object.entries(components).forEach(([componentId, exportName]) => { + if (!modules[exportName]) { + console.warn(`Component "${exportName}" not found in package "${pkg}"`) + return + } if (!window.TinyLowcodeComponent[componentId]) { window.TinyLowcodeComponent[componentId] = modules[exportName] } }) }Likely invalid or redundant comment.
85-107: 🛠️ Refactor suggestionAdd timeout handling for dynamic imports.
The function should handle potential timeouts during dynamic imports to prevent hanging.
Apply this diff to add timeout handling:
const dynamicImportComponentLib = async ({ pkg, script }) => { if (window.TinyComponentLibs[pkg]) { return window.TinyComponentLibs[pkg] } let modules = {} + const importWithTimeout = (url) => { + return Promise.race([ + import(/* @vite-ignore */ url), + new Promise((_, reject) => + setTimeout(() => reject(new Error(`Import timeout: ${url}`)), 30000) + ) + ]) + } try { const importMapKeys = getImportMapKeys() if (importMapKeys.includes(pkg)) { - modules = await import(/* @vite-ignore */ pkg) + modules = await importWithTimeout(pkg) } else if (script) { - modules = await import(/* @vite-ignore */ script) + modules = await importWithTimeout(script) } } catch (error) { + console.error(`Failed to load module ${pkg}:`, error) modules = {} } window.TinyComponentLibs[pkg] = modules return modules }Likely invalid or redundant comment.
packages/plugins/materials/src/composable/useResource.js (2)
36-37: LGTM! State management is properly structured.The
materialsDepsproperty is well-organized with separate arrays for scripts and styles.
179-187: 🛠️ Refactor suggestionAdd validation for utility dependencies.
The function should validate the structure of utility items before processing.
Apply this diff to add validation:
const getUtilsDeps = () => { + if (!Array.isArray(appSchemaState.utils)) { + console.warn('Utils is not properly initialized') + return [] + } return appSchemaState.utils.map((item) => { + if (!item?.content?.package || !item?.content?.cdnLink) { + console.warn(`Invalid utility structure:`, item) + return null + } return { ...item, package: item.content.package, script: item.content.cdnLink } - }) + }).filter(Boolean) }Likely invalid or redundant comment.
packages/plugins/materials/src/composable/useMaterial.js (5)
42-43: LGTM! Dependencies are properly organized.The
componentsDepsMapuses appropriate data structures with an array for scripts and a Set for styles to prevent duplicates.
281-288: 🛠️ Refactor suggestionAdd error handling for resource state access.
The function should handle cases where the material dependencies state is not properly initialized.
Apply this diff to add error handling:
const getCanvasDeps = () => { + if (!useResource().appSchemaState?.materialsDeps) { + console.warn('Material dependencies not initialized') + return { scripts: [], styles: [] } + } const { scripts, styles } = useResource().appSchemaState.materialsDeps return { scripts: [...scripts].filter((item) => item.script), styles: [...styles] } }Likely invalid or redundant comment.
293-298: 🛠️ Refactor suggestionAdd error handling for canvas updates.
The function should handle potential failures when publishing updates to the canvas.
Apply this diff to add error handling:
const updateCanvasDeps = () => { + try { + const deps = getCanvasDeps() + if (!deps.scripts.length && !deps.styles.length) { + console.warn('No dependencies to update') + return + } useMessage().publish({ topic: 'init_canvas_deps', - data: getCanvasDeps() + data: deps }) + } catch (error) { + console.error('Failed to update canvas dependencies:', error) + } }Likely invalid or redundant comment.
447-460: 🛠️ Refactor suggestionAdd validation for block dependencies.
The function should validate the structure of block dependencies before processing.
Apply this diff to add validation:
scripts.forEach((npm) => { + if (!npm?.package) { + console.warn('Invalid package structure:', npm) + return + } const { package: pkg, script, css, components } = npm const npmInfo = materialState.componentsDepsMap.scripts.find((item) => item.package === pkg) if (!npmInfo || !npmInfo.script) { materialState.componentsDepsMap.scripts.push({ package: pkg, script, css, components }) } else { const components = npmInfo.components || {} npmInfo.components = { ...components, ...npm.components } } }) -if (Array.isArray(styles)) { +if (Array.isArray(styles) && styles.length > 0) { styles.forEach((item) => materialState.componentsDepsMap.styles.add(item)) }Likely invalid or redundant comment.
300-345: 🛠️ Refactor suggestionAdd validation for material bundle structure.
The function should validate the material bundle structure before processing dependencies.
Apply this diff to add validation:
const parseMaterialsDependencies = (materialBundle) => { + if (!materialBundle || typeof materialBundle !== 'object') { + console.warn('Invalid material bundle') + return + } const { packages, components } = materialBundle + if (!Array.isArray(packages) && !Array.isArray(components)) { + console.warn('No valid dependencies found in material bundle') + return + } const { scripts: scriptsDeps, styles: stylesDeps } = useResource().appSchemaState.materialsDeps packages?.forEach((pkg) => { + if (!pkg?.package) { + console.warn('Invalid package entry:', pkg) + return + } if (!pkg.script || scriptsDeps.find((item) => item.package === pkg.package)) { return } // ... rest of the function })Likely invalid or redundant comment.
packages/plugins/bridge/src/js/resource.js (2)
178-180: LGTM! Clean async/await conversion.The function signature has been properly converted to async/await pattern, and the edit state check is cleanly extracted into a constant.
Line range hint
181-200: Enhance error handling in the edit flow.The current implementation has nested conditions that make the code harder to follow. Consider restructuring for better error handling.
if (isEdit) { data.id = state.resource.id const result = await requestUpdateReSource(data) - if (result) { - const index = useResource().appSchemaState[data.category].findIndex((item) => item.name === result.name) - - if (index === -1) { - useNotify({ - type: 'error', - message: '修改失败' - }) - - return - } - - useResource().appSchemaState[data.category][index] = result + if (!result) { + throw new Error('Update resource request failed') } + + const index = useResource().appSchemaState[data.category].findIndex((item) => item.name === result.name) + if (index === -1) { + throw new Error('Resource not found in app schema') + } + + useResource().appSchemaState[data.category][index] = result
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/design-core/src/preview/src/preview/Preview.vue (1)
Line range hint
164-177: Add input validation for code transformation.The code transformation logic using
transformSynccould be vulnerable to code injection. Consider:
- Validating and sanitizing the input before transformation
- Adding content security policies
- Implementing rate limiting for transformations
Add input validation before code transformation:
+ const validateScript = (script) => { + // Add validation logic here + if (!script || typeof script !== 'string') { + throw new Error('Invalid script content'); + } + // Add more validation as needed + } const transformedScript = transformSync(p1, { + validateScript(p1); babelrc: false, plugins: [[vueJsx, { pragma: 'h' }]], sourceMaps: false, configFile: false })
♻️ Duplicate comments (3)
packages/plugins/materials/src/composable/useResource.js (1)
185-196: 🛠️ Refactor suggestionAdd error handling and type documentation for getUtilsDeps.
The function needs defensive programming and better documentation.
+/** + * Transforms utils array into a format compatible with material dependencies. + * @returns {Array<{name: string, package: string, script: string}>} Array of utility dependencies + * @throws {Error} If utils array is not properly initialized + */ const getUtilsDeps = () => { + if (!Array.isArray(appSchemaState.utils)) { + console.warn('Utils array is not properly initialized') + return [] + } return appSchemaState.utils .filter((item) => item.type === 'npm') .map((item) => { + if (!item?.content) { + console.warn(`Missing content for util: ${item?.name}`) + return null + } return { ...item, package: item.content?.package, script: item.content?.cdnLink } }) + .filter(Boolean) }packages/canvas/common/src/utils.js (2)
59-71: 🛠️ Refactor suggestionAdd timeout handling and improve error reporting.
The dynamic import lacks timeout handling and proper error reporting.
Apply this improvement:
const dynamicImportComponentLib = async ({ pkg, script }) => { + if (!pkg) { + throw new Error('Package name is required') + } + if (window.TinyComponentLibs[pkg]) { return window.TinyComponentLibs[pkg] } if (!script) { + console.warn(`No script provided for package "${pkg}"`) return {} } + const importWithTimeout = (url) => { + const timeout = 30000 // 30 seconds + return Promise.race([ + import(/* @vite-ignore */ url), + new Promise((_, reject) => + setTimeout(() => reject(new Error(`Import timeout: ${url}`)), timeout) + ) + ]) + }
90-99: 🛠️ Refactor suggestionImprove validation and error handling for component exports.
The function needs better parameter validation and error handling.
Apply this improvement:
export const getComponents = async ({ package: pkg, script, components }) => { - if (!pkg) return + if (!pkg || !components || typeof components !== 'object') { + throw new Error('Package name and components mapping are required') + } const modules = await dynamicImportComponentLib({ pkg, script }) Object.entries(components).forEach(([componentId, exportName]) => { + if (!modules[exportName]) { + console.warn(`Component export "${exportName}" not found in package "${pkg}"`) + return + } if (!window.TinyLowcodeComponent[componentId]) { window.TinyLowcodeComponent[componentId] = modules[exportName] } }) }
🧹 Nitpick comments (3)
packages/plugins/materials/src/composable/useResource.js (1)
37-38: Document and optimize the materialsDeps property.Consider the following improvements:
- Add JSDoc to document the purpose and structure of
materialsDeps- Based on the retrieved learning about component dependencies, consider making
materialsDepsnon-reactive if changes are handled explicitly+/** + * @typedef {Object} MaterialsDeps + * @property {Array<string>} scripts - Array of script dependencies + * @property {Set<string>} styles - Set of unique style dependencies + */ const appSchemaState = reactive({ dataSource: [], pageTree: [], langs: {}, utils: {}, globalState: [], - materialsDeps: { scripts: [], styles: new Set() } }) + +// Non-reactive material dependencies since changes are handled explicitly +const materialsDeps = { scripts: [], styles: new Set() }packages/canvas/common/src/utils.js (1)
86-88: Add JSDoc type annotations.The JSDoc comment lacks type information for the parameters.
Improve the documentation:
/** * 获取组件对象并缓存，组件渲染时使用 - * @param {object} param0 组件的依赖： { package： 包名，script：js文件cdn, components：组件id和导出组件名的映射关系} + * @param {object} param0 组件的依赖 + * @param {string} param0.package 包名 + * @param {string} param0.script js文件cdn地址 + * @param {Record<string, string>} param0.components 组件id和导出组件名的映射关系 * @returns {Promise<void>} */packages/design-core/src/preview/src/preview/Preview.vue (1)
Line range hint
129-134: Consider improving error handling and documentation.The block schema processing section contains TODs and complex logic that would benefit from:
- Proper error handling for failed block schema fetches
- Documentation explaining the block schema processing flow
- Resolution of the TODOs regarding cascade generation and built-in blocks
Consider extracting the block schema processing logic into a separate service to improve maintainability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/common/src/utils.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(2 hunks)packages/canvas/container/src/container.js(0 hunks)packages/canvas/render/src/runner.ts(2 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/plugins/materials/src/composable/useResource.js(2 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(1 hunks)
💤 Files with no reviewable changes (1)
- packages/canvas/container/src/container.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue
- packages/canvas/container/src/CanvasContainer.vue
🧰 Additional context used
📓 Learnings (1)
packages/plugins/materials/src/composable/useResource.js (1)
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/plugins/materials/src/composable/useMaterial.js:0-0
Timestamp: 2025-01-13T07:49:12.136Z
Learning: In the materials system, componentsDepsMap.scripts in materialState is used as a data store for tracking component dependencies and doesn't require Vue reactivity as changes are handled explicitly through updateCanvasDeps().
🔇 Additional comments (5)
packages/plugins/materials/src/composable/useResource.js (1)
201-201: LGTM!The addition of
getUtilsDepsto the exports is correct.packages/canvas/render/src/runner.ts (1)
14-14: LGTM!The import statement has been updated to reflect the new component loading strategy, removing unused imports.
packages/canvas/DesignCanvas/src/DesignCanvas.vue (2)
68-68: LGTM! Good use of Vue's reactivity system.The change from a regular variable to a reactive reference using
ref()is appropriate for Vue's reactivity system.
70-81: Add error handling for dependency initialization.The message subscription implementation should handle potential errors during import map generation.
useMessage().subscribe({ topic: 'init_canvas_deps', callback: (deps) => { if (canvasSrc) { return } + try { const { importMap, importStyles } = getImportMapData(getMergeMeta('engine.config')?.importMapVersion, deps) canvasSrcDoc.value = initCanvas(importMap, importStyles).html + } catch (error) { + console.error('Failed to initialize canvas dependencies:', error) + // Consider showing a user-friendly error message + } } })packages/design-core/src/preview/src/preview/Preview.vue (1)
126-126: Verify import map completeness and add validation.The direct setting of import map data without utility processing could potentially miss some dependencies. Consider:
- Adding validation to ensure importMapData contains all required dependencies
- Documenting the expected structure of importMapData
- Adding error handling for malformed import maps
Let's verify if all required dependencies are properly included in the import map:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/canvas/render/src/application-function/utils.ts (1)
62-77: Enhance error handling and type safety.While the error handling is good, consider these improvements:
- Add more detailed error information
- Implement type checking for loaded modules
- Add debug logging
npmUtils.forEach((res, index) => { const { name, content } = data[index] const { exportName, destructuring, cdnLink } = content if (res.status !== 'fulfilled') { globalNotify({ type: 'error', - message: `加载工具类"${name}"失败，请检查cdn链接是否正确，${cdnLink}` + message: `Failed to load utility "${name}". Error: ${res.reason}. CDN: ${cdnLink}` }) return } const module = res.value + // Type check the loaded module + if (destructuring && !(exportName in module)) { + globalNotify({ + type: 'error', + message: `Export "${exportName}" not found in module "${name}"` + }) + return + } + + // Debug logging in development + if (process.env.NODE_ENV === 'development') { + console.debug(`Loaded utility "${name}" from ${cdnLink}`) + } utilsCollection[name] = destructuring ? module[exportName] : module.default })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/render/src/application-function/utils.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/canvas/render/src/application-function/utils.ts (2)
5-5: LGTM! Import added for error notification.The addition of
globalNotifyimport is appropriate for handling error cases in the async utility loading.
22-22: Verify callers handle the async setUtils.The function is now async, which is necessary for dynamic imports. Ensure all callers properly await this function.
✅ Verification successful
Watcher callback safely handles async setUtils
The only usage found is in a Vue watcher callback in
RenderMain.ts. Vue's reactivity system automatically handles async callbacks in watchers, making this usage safe without explicit await. No changes needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for setUtils calls that might need updating rg "setUtils\(" --type ts --type vue -B 2 -A 2Length of output: 76
Script:
#!/bin/bash # Search for setUtils calls in TypeScript files rg "setUtils\(" --type ts -B 2 -A 2 # Search for potential method references and assignments ast-grep --pattern 'setUtils($_)'Length of output: 458
Script:
#!/bin/bash # Get more context around the usage rg "setUtils" packages/canvas/render/src/RenderMain.ts -B 10 -A 5Length of output: 1276
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
70-82:⚠️ Potential issueAdd error handling and subscription cleanup.
The subscription implementation needs improvements:
- No cleanup on component unmount
- Missing error handling for malformed deps
- Potential race conditions in initialization
Apply this diff to address the issues:
export default { setup() { // ... other code ... + const subscription = ref(null) - useMessage().subscribe({ + subscription.value = useMessage().subscribe({ topic: 'init_canvas_deps', subscriber: 'canvas_design_canvas', callback: (deps) => { + // Protect against race conditions + if (canvasSrcDoc.value) { + return + } + + // Validate deps parameter + if (!deps || typeof deps !== 'object') { + console.error('Invalid dependencies received') + return + } + if (canvasSrc) { return } const { importMap, importStyles } = getImportMapData(getMergeMeta('engine.config')?.importMapVersion, deps) canvasSrcDoc.value = initCanvas(importMap, importStyles).html } }) + + onUnmounted(() => { + if (subscription.value) { + useMessage().unsubscribe(subscription.value) + } + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/DesignCanvas/src/DesignCanvas.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
68-68: LGTM! Good use of Vue 3 Composition API.The change from a regular variable to a reactive reference using
ref()is the correct approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/canvas/render/type.d.ts (2)
13-17: Add JSDoc documentation for the new property.The new
componentsDepsproperty lacks documentation explaining its purpose, usage, and relationship with existing properties likethirdPartyDeps.Add JSDoc comments to improve maintainability:
+ /** + * Manages component dependencies and their relationships. + * Each entry defines a component package with its script source and component mappings. + * @example + * { + * script: "https://unpkg.com/my-component@1.0.0/dist/index.js", + * package: "my-component", + * components: { "MyButton": "MyComponent.Button" } + * } + */ componentsDeps: Array<{ script: string package: string components: Record<string, string> }>
13-17: Consider improving type safety with more specific types.The current type definition could be more specific to prevent potential runtime issues.
Consider these improvements:
componentsDeps: Array<{ - script: string - package: string - components: Record<string, string> + script: `https://${string}` | `http://${string}` | `//${string}` // Ensure valid URLs + package: string // Consider using PackageName type if available + components: { + [componentName: string]: `${string}.${string}` // Enforce dot notation for component paths + } }>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/render/src/runner.ts(2 hunks)packages/canvas/render/type.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/canvas/render/src/runner.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/canvas/render/type.d.ts (1)
Line range hint
4-17: Clarify the relationship betweencomponentsDepsandthirdPartyDeps.The interface now has two seemingly overlapping dependency management properties:
thirdPartyDepsandcomponentsDeps. This could lead to confusion about which property should be used in different scenarios.Let's analyze the usage to understand the distinction:
Consider:
- Can these properties be consolidated?
- Should we deprecate
thirdPartyDepsin favor ofcomponentsDeps?- If both are needed, what's the clear distinction between their purposes?
✅ Verification successful
The distinction between
thirdPartyDepsandcomponentsDepsis architecturally sound
thirdPartyDeps: Simple external resources (styles/scripts) without component mappingcomponentsDeps: Component-specific dependencies that require package and component mapping informationThese properties serve different purposes and should remain separate to maintain clean separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage patterns of both properties echo "=== thirdPartyDeps usage ===" rg "thirdPartyDeps" -A 3 echo -e "\n=== componentsDeps usage ===" rg "componentsDeps" -A 3Length of output: 3316
3 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?
物料资产包协议修改。涉及画布渲染、预览、出码、npm类型工具类的加载
协议内容如下：
json { "data": { "framework": "Vue", "materials": { "components": [ { "id": '', "component": 'TinyButton' "npm": { "pacakge": "@opentiny/vue", // "version": "3.11.0", // 移除 // "script" : "", // 移除 // "css": "", // 移除 "exportName": "Button" } } ], "blocks": [], "snippets": [], "packages": [ // 新增字段 { "name": "TinyVue组件库", // 名称 "package": "@opentiny/vue", // npm包名 "version": "3.11.0", // npm包版本 "script": "https://xxxxx", // esm格式的js文件cdn地址 "css": ["https://xxxxx"], // 样式表文件cdn地址 ] }, { "name": "xxx工具库", // 名称 "package": "lodash", // npm包名 "version": "3.11.0", // npm包版本 "script": "https://xxxxx", // esm格式的js文件cdn地址 "css": ["https://xxxxx"], // 样式表文件cdn地址 "needDeclare": true // 出码时是否需要声明到package.json的依赖，默认为true。当物料npm包中已声明的依赖且不需要在宿主工程声明时，可设置为false }, ] } } }Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
TinyVueandelement-pluslibraries.Improvements
Bug Fixes
Chores