Feat: Add dependency map #2476
Feat: Add dependency map #2476
Conversation
WalkthroughThe changes introduce a new property Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2476 +/- ##
========================================
Coverage 68.43% 68.44%
========================================
Files 922 922
Lines 95661 95672 +11
Branches 8121 8122 +1
========================================
+ Hits 65466 65479 +13
+ Misses 29942 29940 -2
Partials 253 253
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;
}
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
Chores
Documentation