Feat: Add dependency map #2476
Feat: Add dependency map #2476
Conversation
|
Warning Rate limit exceeded@johanzhu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a new property 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.43% 68.45% +0.02%
===========================================
Files 922 922
Lines 95661 95672 +11
Branches 8121 8126 +5
===========================================
+ Hits 65466 65494 +28
+ Misses 29942 29925 -17
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.
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
Bug Fixes
Documentation