Support dependency map in resource config
#2476
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2476 +/- ##
===========================================
- Coverage 68.42% 68.38% -0.04%
===========================================
Files 923 923
Lines 95940 95904 -36
Branches 8140 8134 -6
===========================================
- Hits 65647 65588 -59
- Misses 30040 30062 +22
- Partials 253 254 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/asset/ResourceManager.ts (1)
559-560: Add checks or validations for the_dependencyMapusage.While this property correctly tracks resource dependencies mapped to their references, consider adding runtime checks when assigning or retrieving its entries. This prevents unexpected errors if a key is not found or if a value is incorrectly typed (e.g.,
nullorundefined).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(3 hunks)
🧰 Additional context used
🪛 eslint
packages/core/src/asset/ResourceManager.ts
[error] 637-637: Replace ·virtualPath:·string;·path:·string;·type:·string;·id:·string,·depndentAssets:·{·[key:·string]:·string·}· with ⏎··virtualPath:·string;⏎··path:·string;⏎··type:·string;⏎··id:·string;⏎··depndentAssets:·{·[key:·string]:·string·};⏎
(prettier/prettier)
🪛 GitHub Check: lint
packages/core/src/asset/ResourceManager.ts
[failure] 637-637:
Replace ·virtualPath:·string;·path:·string;·type:·string;·id:·string,·depndentAssets:·{·[key:·string]:·string·}· with ⏎··virtualPath:·string;⏎··path:·string;⏎··type:·string;⏎··id:·string;⏎··depndentAssets:·{·[key:·string]:·string·};⏎
| @@ -631,7 +634,7 @@ | |||
| "g" | |||
| ); | |||
|
|
|||
| type EditorResourceItem = { virtualPath: string; path: string; type: string; id: string }; | |||
| type EditorResourceItem = { virtualPath: string; path: string; type: string; id: string, depndentAssets: { [key: string]: string } }; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address ESLint’s formatting recommendations and fix the property name.
Use multiline for readability, and correct the property name to dependentAssets.
Below is a suggested diff:
- type EditorResourceItem = { virtualPath: string; path: string; type: string; id: string, depndentAssets: { [key: string]: string } };
+ type EditorResourceItem = {
+ virtualPath: string;
+ path: string;
+ type: string;
+ id: string;
+ dependentAssets: { [key: string]: string };
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type EditorResourceItem = { virtualPath: string; path: string; type: string; id: string, depndentAssets: { [key: string]: string } }; | |
| type EditorResourceItem = { | |
| virtualPath: string; | |
| path: string; | |
| type: string; | |
| id: string; | |
| dependentAssets: { [key: string]: string }; | |
| }; |
🧰 Tools
🪛 eslint
[error] 637-637: Replace ·virtualPath:·string;·path:·string;·type:·string;·id:·string,·depndentAssets:·{·[key:·string]:·string·}· with ⏎··virtualPath:·string;⏎··path:·string;⏎··type:·string;⏎··id:·string;⏎··depndentAssets:·{·[key:·string]:·string·};⏎
(prettier/prettier)
🪛 GitHub Check: lint
[failure] 637-637:
Replace ·virtualPath:·string;·path:·string;·type:·string;·id:·string,·depndentAssets:·{·[key:·string]:·string·}· with ⏎··virtualPath:·string;⏎··path:·string;⏎··type:·string;⏎··id:·string;⏎··depndentAssets:·{·[key:·string]:·string·};⏎
| @@ -593,6 +595,7 @@ | |||
| initVirtualResources(config: EditorResourceItem[]): void { | |||
| config.forEach((element) => { | |||
| this._virtualPathMap[element.virtualPath] = element.path; | |||
| this._dependencyMap[element.virtualPath] = element.depndentAssets; | |||
There was a problem hiding this comment.
Spelling mismatch in the key property depndentAssets.
It appears the property name is missing an “e.” Renaming it to dependentAssets throughout the code helps with clarity and consistency, since “depndent” could be mistaken for a typographical error.
Apply the following diff to rename the property:
- this._dependencyMap[element.virtualPath] = element.depndentAssets;
+ this._dependencyMap[element.virtualPath] = element.dependentAssets;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this._dependencyMap[element.virtualPath] = element.depndentAssets; | |
| this._dependencyMap[element.virtualPath] = element.dependentAssets; |
| @@ -593,6 +595,7 @@ export class ResourceManager { | |||
| initVirtualResources(config: EditorResourceItem[]): void { | |||
| config.forEach((element) => { | |||
| this._virtualPathMap[element.virtualPath] = element.path; | |||
| this._dependencyMap[element.virtualPath] = element.depndentAssets; | |||
There was a problem hiding this comment.
What's the value of element.dependentAssets if asset has not dependentAssets. if it is undefined, still set?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/asset/ResourceManager.ts (1)
559-560: Consider adding JSDoc comments for the dependency map.Since this is a critical feature for internal editor use, consider adding detailed JSDoc comments explaining:
- The structure and format of the dependency map
- The relationship between virtual paths and their dependencies
- Usage examples and constraints
Add documentation like this:
/** @internal */ + /** + * Maps virtual paths to their dependent assets. + * @example + * { + * "virtualPath/to/asset": { + * "dependency1": "path/to/dependency1", + * "dependency2": "path/to/dependency2" + * } + * } + */ _dependencyMap: Record<string, { [key: string]: string }> = Object.create(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/asset/ResourceManager.ts
[error] 598-598: Invalid assignment to element.depndentAssets && this._dependencyMap[element.virtualPath]
This expression cannot be assigned to
(parse)
🔇 Additional comments (2)
packages/core/src/asset/ResourceManager.ts (2)
559-560: LGTM! Property declaration follows class conventions.
The _dependencyMap property is well-typed and consistently initialized using Object.create(null).
637-638: Fix type definition formatting and property name.
The type definition needs formatting improvements and the property name should be corrected from "depndent" to "dependent".
| @@ -593,6 +595,7 @@ export class ResourceManager { | |||
| initVirtualResources(config: EditorResourceItem[]): void { | |||
| config.forEach((element) => { | |||
| this._virtualPathMap[element.virtualPath] = element.path; | |||
| element.depndentAssets && this._dependencyMap[element.virtualPath] = element.depndentAssets; | |||
There was a problem hiding this comment.
Fix assignment syntax and add null check.
The current implementation has several issues:
- The assignment syntax is incorrect
- Missing null check before accessing
depndentAssets - Property name has a typo ("depndent" should be "dependent")
Apply this diff to fix the issues:
- element.depndentAssets && this._dependencyMap[element.virtualPath] = element.depndentAssets;
+ if (element.dependentAssets) {
+ this._dependencyMap[element.virtualPath] = element.dependentAssets;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| element.depndentAssets && this._dependencyMap[element.virtualPath] = element.depndentAssets; | |
| if (element.dependentAssets) { | |
| this._dependencyMap[element.virtualPath] = element.dependentAssets; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 598-598: Invalid assignment to element.depndentAssets && this._dependencyMap[element.virtualPath]
This expression cannot be assigned to
(parse)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
66-69: Modernize lint-staged configurationThe lint-staged configuration uses older syntax with explicit
git add. Modern versions of lint-staged automatically stage files after fixes."lint-staged": { - "*.{ts,tsx}": [ - "eslint --fix", - "git add" - ] + "*.{ts,tsx}": "eslint --fix" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)packages/core/src/asset/ResourceManager.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/asset/ResourceManager.ts
🔇 Additional comments (1)
package.json (1)
66-66: Verify the necessity of adding TSX support
Let's verify if this change is related to the dependency map feature and if there are any TSX files in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/asset/ResourceManager.ts (2)
599-601: Add documentation and validation for dependency mapping.While the implementation is correct, consider:
- Adding JSDoc comments explaining the purpose and structure of dependencies
- Adding validation for the dependency map contents
Example documentation:
+ /** + * Maps virtual paths to their dependent assets. + * @param element.dependentAssetMap - Map of asset keys to their dependency paths + */ if (element.dependentAssetMap) { this._dependencyMap[element.virtualPath] = element.dependentAssetMap; }
639-645: Enhance type definition with documentation and specific types.Consider improving the type definition:
- Add TSDoc comments for the type and its properties
- Make
dependentAssetMaptype more specific than{ [key: string]: string }+/** + * Configuration for editor-specific resource items. + */ type EditorResourceItem = { + /** Virtual path used for resource mapping */ virtualPath: string; + /** Actual file system path */ path: string; + /** Resource type identifier */ type: string; + /** Unique identifier for the resource */ id: string; + /** Map of asset identifiers to their dependency paths */ - dependentAssetMap: { [key: string]: string }; + dependentAssetMap: Record<AssetKey, DependencyPath>; }; +/** Asset identifier type */ +type AssetKey = string; + +/** Path to a dependency resource */ +type DependencyPath = string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(3 hunks)
🔇 Additional comments (2)
packages/core/src/asset/ResourceManager.ts (2)
559-560: LGTM! Property declaration follows best practices.
The _dependencyMap property is well-typed and follows the established patterns in the codebase for internal map properties.
Line range hint 559-601: Verify dependency cleanup in resource destruction.
The _destroy() method should be updated to clean up the _dependencyMap. Let's verify the current implementation:
Consider adding cleanup in the _destroy() method:
private _destroy(): void {
this.cancelNotLoaded();
this._gc(true);
this._assetPool = null;
this._assetUrlPool = null;
this._referResourcePool = null;
this._graphicResourcePool = null;
this._contentRestorerPool = null;
this._loadingPromises = null;
+ this._dependencyMap = null;
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/asset/ResourceManager.ts (2)
597-601: Consider enhancing the method documentation.While the implementation is solid, the method's documentation could be improved to explain:
- The purpose of
dependentAssetMap- The relationship between virtual paths and dependencies
- Usage examples for editor integration
Add JSDoc comments like this:
/** * @internal * @beta Just for internal editor, not recommended for developers. + * @param config - Array of editor resource items + * @remarks + * This method initializes the virtual resource system by: + * 1. Mapping virtual paths to actual resources + * 2. Storing resource configurations by ID + * 3. Setting up dependency relationships between assets */
639-645: Consider making some properties optional for better flexibility.The type definition is well-structured, but consider making
dependentAssetMapoptional since not all resources will have dependencies.Apply this diff:
type EditorResourceItem = { virtualPath: string; path: string; type: string; id: string; - dependentAssetMap: { [key: string]: string }; + dependentAssetMap?: { [key: string]: string }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(4 hunks)
🔇 Additional comments (1)
packages/core/src/asset/ResourceManager.ts (1)
558-560: LGTM! Well-structured property declarations.
Good practices observed:
- Using
Object.create(null)for map-like objects - Clear type definitions
- Proper internal marking
| @@ -184,7 +184,7 @@ export class ResourceManager { | |||
| * @internal | |||
| */ | |||
| _getRemoteUrl(url: string): string { | |||
| return this._virtualPathMap[url] ?? url; | |||
| return this._virtualPathMap[url].path ?? url; | |||
There was a problem hiding this comment.
Add null check to prevent potential runtime errors.
The current implementation might throw if virtualPathMap[url] is undefined.
Apply this diff to add null checks:
- return this._virtualPathMap[url].path ?? url;
+ return this._virtualPathMap[url]?.path ?? url;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return this._virtualPathMap[url].path ?? url; | |
| return this._virtualPathMap[url]?.path ?? url; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/asset/ResourceManager.ts (2)
558-560: Consider adding JSDoc comments for the new properties.While these properties are marked as
@internal, adding detailed JSDoc comments would help other developers understand their purpose and usage./** @internal */ + /** Map of virtual paths to their corresponding resource configurations */ _virtualPathMap: EditorResourceConfig = Object.create(null); /** @internal */ + /** Map of virtual paths to their dependent asset configurations */ _dependencyMap: Record<string, { [key: string]: string }> = Object.create(null);
639-645: Consider strengthening the EditorResourceItem type.The type definition could be improved in several ways:
- Add readonly modifiers to prevent accidental modifications
- Consider using a more specific type for
dependentAssetMapvalues- Add JSDoc comments for better documentation
type EditorResourceItem = { - virtualPath: string; - path: string; - type: string; - id: string; - dependentAssetMap: { [key: string]: string }; + /** Virtual path used for resource identification */ + readonly virtualPath: string; + /** Actual file system path or URL */ + readonly path: string; + /** Resource type identifier */ + readonly type: string; + /** Unique identifier for the resource */ + readonly id: string; + /** Map of dependent asset identifiers to their paths */ + readonly dependentAssetMap: Readonly<Record<string, string>>; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(6 hunks)
🔇 Additional comments (3)
packages/core/src/asset/ResourceManager.ts (3)
187-187: LGTM: Improved null safety with optional chaining.
The use of optional chaining (?.) here properly handles cases where _virtualPathMap[url] might be undefined.
209-209: LGTM: Consistent null safety pattern.
The optional chaining pattern is consistently applied here, matching the implementation in _getRemoteUrl.
364-364: LGTM: Maintained consistency in remote URL handling.
The same null-safe pattern is correctly applied here, maintaining consistency across the codebase.
| this._virtualPathMap[element.virtualPath] = element; | ||
| this._editorResourceConfig[element.id] = element; | ||
| if (element.dependentAssetMap) { | ||
| this._dependencyMap[element.virtualPath] = element.dependentAssetMap; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add cleanup mechanism and validation for dependent assets
Based on the verification results, there are legitimate concerns:
- The
dependentAssetMaplacks runtime validation before being added to_dependencyMap - There's no cleanup mechanism for
_dependencyMapentries when resources are removed - The
_destroy()method doesn't clear the_dependencyMap
Suggested improvements:
- Add runtime validation for
dependentAssetMapstructure before assignment - Implement cleanup for
_dependencyMapentries when resources are removed - Include
_dependencyMapcleanup in the_destroy()method
🔗 Analysis chain
Verify dependent asset initialization.
The code now handles dependent assets, but there are a few considerations:
- No validation is performed on the
dependentAssetMapstructure - The dependency map could grow unbounded as more virtual resources are added
Consider adding validation and implementing a cleanup mechanism.
Let me gather more information about the cleanup mechanisms and validation.
Let me check one more aspect about resource cleanup and the relationship between virtual paths and resources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential memory leaks by finding all calls that might add to _dependencyMap
ast-grep --pattern 'this._dependencyMap[$_] = $_'
# Look for any cleanup calls
rg -A 5 '_dependencyMap.*=.*null'
Length of output: 650
Script:
#!/bin/bash
# Check for cleanup methods in ResourceManager
ast-grep --pattern 'function $_($$$) {
$$$
this._dependencyMap[$$$] = $$$
$$$
}'
# Look for methods that might handle resource removal
rg -A 10 "removeResource|destroy|dispose|clear" packages/core/src/asset/ResourceManager.ts
# Check if there's validation for dependentAssetMap
ast-grep --pattern 'interface $_Resource {
$$$
dependentAssetMap$_
$$$
}'
Length of output: 852
Script:
#!/bin/bash
# Check for virtual path cleanup
rg -A 5 "_virtualPathMap.*=.*null|delete.*_virtualPathMap|delete.*_dependencyMap" packages/core/src/asset/ResourceManager.ts
# Look for resource removal or cleanup methods
ast-grep --pattern 'function $_($$$) {
$$$
delete this._virtualPathMap[$_]
$$$
}'
# Check the type definition of dependentAssetMap
rg -A 3 "dependentAssetMap.*:" packages/core/src/asset/ResourceManager.ts
Length of output: 637
package.json
Outdated
| @@ -63,7 +63,7 @@ | |||
| "vitest": "2.1.3" | |||
| }, | |||
| "lint-staged": { | |||
| "*.{ts}": [ | |||
| "*.{ts,tsx}": [ | |||
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/core/src/asset/ResourceManager.ts (1)
Line range hint
479-486: Add cleanup for new resource maps in _destroy method.The _destroy method should clean up all class properties, including the newly added resource maps.
_destroy(): void { this.cancelNotLoaded(); this._gc(true); this._assetPool = null; this._assetUrlPool = null; this._referResourcePool = null; this._graphicResourcePool = null; this._contentRestorerPool = null; this._loadingPromises = null; + this._virtualPathResourceMap = null; + this._idResourceMap = null; }
🧹 Nitpick comments (3)
packages/core/src/asset/ResourceManager.ts (3)
187-187: Add null check to prevent potential runtime errors.The optional chaining operator is correctly used for the
pathproperty, but we should also add a null check for_virtualPathResourceMap[url]itself.- return this._virtualPathResourceMap[url]?.path ?? url; + return this._virtualPathResourceMap?.[url]?.path ?? url;
556-558: Consider adding runtime type validation for map entries.The maps are initialized with
Object.create(null), which is good for avoiding prototype pollution. However, consider adding runtime type validation when entries are added to these maps to ensure type safety.Consider creating a validation helper:
private _validateEditorResourceItem(item: EditorResourceItem): void { if (!item.virtualPath || !item.path || !item.type || !item.id) { throw new Error('Invalid EditorResourceItem: missing required properties'); } }
571-573: Enhance error handling with more descriptive messages.The warning message could be more informative by including additional context about the resource being requested.
- Logger.warn(`refId:${refId} is not find in this._idResourceMap.`); + Logger.warn( + `Resource not found: refId "${refId}" does not exist in resource map.` + + ` Available resources: ${Object.keys(this._idResourceMap).length}` + );
| this._virtualPathResourceMap[element.virtualPath] = element; | ||
| this._idResourceMap[element.id] = element; | ||
| if (element.dependentAssetMap) { | ||
| this._virtualPathResourceMap[element.virtualPath].dependentAssetMap = element.dependentAssetMap; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and cleanup mechanism.
The current implementation has several potential issues:
- No validation of the input array
- No cleanup of existing entries before initialization
- Redundant assignment of dependentAssetMap
Consider this improved implementation:
initVirtualResources(config: EditorResourceItem[]): void {
+ if (!Array.isArray(config)) {
+ throw new Error('Config must be an array of EditorResourceItem');
+ }
+ // Clear existing entries
+ this._virtualPathResourceMap = Object.create(null);
+ this._idResourceMap = Object.create(null);
+
config.forEach((element) => {
+ this._validateEditorResourceItem(element);
this._virtualPathResourceMap[element.virtualPath] = element;
this._idResourceMap[element.id] = element;
- if (element.dependentAssetMap) {
- this._virtualPathResourceMap[element.virtualPath].dependentAssetMap = element.dependentAssetMap;
- }
});
}Committable suggestion skipped: line range outside the PR's diff.
GuoLei1990
changed the title
resource config
GuoLei1990
added
enhancement
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Refactor