Fix reource load issue in local downloaded npm package #2436
Conversation
Sway007
added
Bug
WalkthroughThe changes involve a modification to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceManager
Client->>ResourceManager: getResourceByRef(ref)
ResourceManager->>ResourceManager: construct url
ResourceManager-->>Client: return resource (possibly cloned)
Possibly related PRs
Suggested reviewers
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 @@
## main #2436 +/- ##
==========================================
- Coverage 69.44% 69.43% -0.01%
==========================================
Files 524 524
Lines 27484 27482 -2
Branches 4112 4111 -1
==========================================
- Hits 19085 19081 -4
Misses 6894 6894
- Partials 1505 1507 +2
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: 1
🧹 Outside diff range and nitpick comments (1)
packages/core/src/asset/ResourceManager.ts (1)
576-578: Document URL handling behavior in method comments.Since this is a @beta API, it's important to document the URL handling behavior, especially how relative URLs are processed.
Apply this diff to improve documentation:
/** * @internal * @beta Just for internal editor, not recommended for developers. + * @param ref.refId - The reference ID to look up in the resource config + * @param ref.key - Optional key for sub-resource access + * @param ref.isClone - Whether to clone the resource + * @remarks + * The method handles both absolute and relative URLs by using a base URL for parsing. + * Query parameters from the original URL are preserved. */🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/asset/ResourceManager.ts
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
🔇 Additional comments (1)
packages/core/src/asset/ResourceManager.ts (1)
577-577: LGTM! Verify URL handling for various formats.
The addition of "http://localhost" as a base URL is a good fix for handling relative URLs. However, let's verify it works correctly with different URL formats.
Let's verify the URL handling behavior:
✅ Verification successful
URL handling implementation is safe and consistent with existing patterns
The verification shows that:
- The URL construction pattern is consistent with other URL usages in the codebase (e.g., in ShaderLab, Utils)
- The
remoteUrlhandling follows an established pattern across the codebase, particularly in resource loading - The change is isolated to query string extraction and doesn't affect the overall URL resolution strategy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for different URL usage patterns in the codebase
# to ensure the change won't break existing code.
# Look for URL constructions and usages
rg -A 2 "new URL\("
# Look for potential remoteUrl patterns in tests
rg -A 2 "remoteUrl.*['\"]" "test/"
Length of output: 1475
Script:
#!/bin/bash
# Let's analyze the usage context of getResourceByRef and remoteUrl patterns
# Find the getResourceByRef implementation and its usage
ast-grep --pattern 'getResourceByRef($$$) { $$$ }'
# Look for test files in the entire repository
fd -e test.ts -e spec.ts
# Search for remoteUrl patterns in the codebase
rg -A 2 "remoteUrl.*="
Length of output: 3928
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
| @@ -574,7 +574,7 @@ | |||
| return Promise.resolve(null); | |||
| } | |||
| const remoteUrl = resourceConfig.path; | |||
| const queryPath = new URL(remoteUrl).search; | |||
| const queryPath = new URL(remoteUrl, "http://localhost").search; | |||
There was a problem hiding this comment.
Add test coverage for URL handling scenarios.
The modified URL handling logic lacks test coverage. Please add tests for:
- Relative URLs
- URLs with query parameters
- URLs without scheme/host
- Invalid URLs
Would you like me to help generate test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(1 hunks)
🔇 Additional comments (2)
packages/core/src/asset/ResourceManager.ts (2)
583-589: Add test coverage for resource resolution scenarios
Referring to the previous review comment about test coverage, we still need tests for:
- Resource resolution with various key formats
- Cloning behavior
- Error handling scenarios
576-582: Verify path handling for different environments
While simplifying URL handling by directly using resourceConfig.path might fix local package loading, we should ensure it works correctly across different environments.
Let's verify the path handling:
| let resource = item; | ||
| if (key) { | ||
| const paths = this._parseQueryPath(key); | ||
| resource = this._getResolveResource(item, paths); | ||
| } | ||
| return isClone ? resource.clone() : resource; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for resource resolution and cloning
The current implementation should handle potential failures in:
- Resource resolution with invalid keys
- Clone operation failures
Consider wrapping the operations in try-catch:
return promise.then((item) => {
+ try {
let resource = item;
if (key) {
const paths = this._parseQueryPath(key);
resource = this._getResolveResource(item, paths);
}
return isClone ? resource.clone() : resource;
+ } catch (error) {
+ Logger.warn(`Failed to resolve/clone resource: ${error.message}`);
+ return null;
+ }
});Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/src/asset/ResourceManager.ts (1)
577-577: Document the URL handling behavior.Since this is a behavior change in how URLs are handled for local packages, please add JSDoc comments explaining:
- The purpose of using a base URL
- How different URL formats are handled
- Any limitations or considerations for local packages
const resourceConfig = this._editorResourceConfig[refId]; if (!resourceConfig) { Logger.warn(`refId:${refId} is not find in this._editorResourceConfig.`); return Promise.resolve(null); } const remoteUrl = resourceConfig.path; + // Use base URL to handle relative URLs in local packages + // This ensures consistent URL parsing regardless of the URL format const queryPath = new URL(remoteUrl, "http://localhost").search;🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/asset/ResourceManager.ts
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
🔇 Additional comments (3)
packages/core/src/asset/ResourceManager.ts (3)
Line range hint 517-524: LGTM! URL parameter parsing logic is well-implemented.
The implementation correctly handles URL parameters, including proper decoding of query parameters.
577-577: Add test coverage for URL handling.
The URL parsing logic lacks test coverage. This was previously flagged in the review comments.
Would you like me to help generate test cases for:
- Relative URLs with local packages
- URLs with query parameters
- URLs without scheme/host
- Invalid URLs
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
577-577: Implementation looks good, but verify URL handling edge cases.
The use of "http://localhost" as a base URL is a good practice for parsing relative URLs. However, let's verify the behavior with different URL formats.
✅ Verification successful
Let me analyze the URL usage patterns in the codebase to verify the implementation. I'll check the ResourceManager test file to see if URL handling is covered.
URL handling implementation is verified and well-tested
The implementation is safe and properly tested:
- The
queryPathtest suite in ResourceManager.test.ts covers various URL scenarios including:- URLs with query parameters
- URLs with encoded characters
- URLs without query strings
- The use of "http://localhost" as base URL is appropriate as it's only used for query string extraction via
.search - The extracted query parameters are correctly appended to the virtual path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for different URL formats in the codebase to ensure compatibility
# Search for URL usage patterns
echo "Checking URL patterns in the codebase..."
rg -A 2 'new URL\(' --type ts
# Look for potential URL-related test files
echo "Looking for URL-related test files..."
fd -e test.ts -e spec.ts -e test.js -e spec.js
Length of output: 4310
Script:
#!/bin/bash
# Check ResourceManager tests for URL handling coverage
cat tests/src/core/resource/ResourceManager.test.ts
# Check the full context of URL usage in ResourceManager
rg -B 5 -A 5 'new URL\(' packages/core/src/asset/ResourceManager.ts
Length of output: 3280
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 577-577: packages/core/src/asset/ResourceManager.ts#L577
Added line #L577 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/asset/ResourceManager.ts (2)
576-579: Document the URL construction behavior changeSince this changes how resource URLs are constructed, please update the method's documentation to:
- Explain how the URL is constructed from virtualPath
- Document the query parameter format when a key is provided
Example documentation:
/** * @internal * @beta Just for internal editor, not recommended for developers. + * @param ref.refId - The reference ID for the resource + * @param ref.key - Optional key to append as a query parameter + * @param ref.isClone - Whether to clone the resource + * @returns Promise resolving to the requested resource + * @remarks URLs are constructed by using virtualPath directly and appending the key as a 'q' parameter if provided */🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 576-576: packages/core/src/asset/ResourceManager.ts#L576
Added line #L576 was not covered by tests
576-579: Add input validation for URL constructionThe current implementation should validate inputs to prevent potential issues:
- Check if virtualPath is defined
- Validate key format if provided
let url = resourceConfig.virtualPath; + if (!url) { + Logger.warn(`Invalid virtualPath for refId:${refId}`); + return Promise.resolve(null); + } if (key) { + // Validate key format + if (typeof key !== 'string') { + Logger.warn(`Invalid key type for refId:${refId}`); + return Promise.resolve(null); + } url += (url.indexOf("?") > -1 ? "&" : "?") + "q=" + key; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 576-576: packages/core/src/asset/ResourceManager.ts#L576
Added line #L576 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/asset/ResourceManager.ts
[warning] 576-576: packages/core/src/asset/ResourceManager.ts#L576
Added line #L576 was not covered by tests
🔇 Additional comments (2)
packages/core/src/asset/ResourceManager.ts (2)
Line range hint 517-524: LGTM: URL parameter parsing logic is correct
The implementation properly handles URL parameter extraction and decoding.
576-579:
Add test coverage for URL construction logic
The modified URL construction logic lacks test coverage. Please add tests for:
- Basic URL without query parameters
- URL with existing query parameters
- URL with special characters in the key
#!/bin/bash
# Check for existing tests of getResourceByRef
rg -l "getResourceByRef.*test" Would you like me to help generate test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 576-576: packages/core/src/asset/ResourceManager.ts#L576
Added line #L576 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/src/asset/ResourceManager.ts (1)
576-578: Consider architectural implications for local npm package resources.Since this change affects how local resources are loaded in npm packages:
- Consider adding support for handling file system paths differently from URLs
- The current implementation might need to handle path separators consistently across platforms
- Local paths might require different validation rules than URLs
Consider implementing a dedicated path resolver for local npm package resources that:
- Handles both URL and file system paths
- Normalizes path separators
- Validates paths according to the context (local vs remote)
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 576-576: packages/core/src/asset/ResourceManager.ts#L576
Added line #L576 was not covered by tests
[warning] 578-578: packages/core/src/asset/ResourceManager.ts#L578
Added line #L578 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/asset/ResourceManager.ts
[warning] 576-576: packages/core/src/asset/ResourceManager.ts#L576
Added line #L576 was not covered by tests
[warning] 578-578: packages/core/src/asset/ResourceManager.ts#L578
Added line #L578 was not covered by tests
🔇 Additional comments (1)
packages/core/src/asset/ResourceManager.ts (1)
576-578:
Add test coverage and error handling for URL construction.
The URL construction logic has been simplified, but there are several areas that need attention:
- The changes lack test coverage as indicated by static analysis
- Missing error handling for invalid
virtualPathor malformedkey - Documentation needs to be updated to reflect the URL construction changes
Let's verify the impact of these changes:
Would you like me to help:
- Generate test cases covering various URL scenarios?
- Add error handling for invalid inputs?
- Update the documentation?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 576-576: packages/core/src/asset/ResourceManager.ts#L576
Added line #L576 was not covered by tests
[warning] 578-578: packages/core/src/asset/ResourceManager.ts#L578
Added line #L578 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/src/core/resource/ResourceManager.test.ts(2 hunks)
🔇 Additional comments (1)
tests/src/core/resource/ResourceManager.test.ts (1)
84-96: Expand test coverage for resource loading scenarios
The current test coverage appears insufficient for the changes described in the PR objectives. Consider adding these test cases:
- Local npm package resource loading (main PR objective)
- Edge cases for URL handling:
- URLs with multiple query parameters
- URLs with special characters
- Relative vs absolute paths
- Different resource types
Let's check the current test coverage:
| expect(loadRes).to.equal(undefined); | ||
| }); | ||
|
|
||
| // TODO: case for gltf loader load invalid q url, expect to throw |
There was a problem hiding this comment.
Implement the TODO test case before merging
Given that this PR addresses resource loading issues, the TODO test case for GLTF loader error handling should be implemented now rather than deferred. This test is important for validating the error handling behavior of the resource loading fix.
Would you like me to help implement the test case for GLTF loader error handling? Here's a suggested structure:
it("should throw error when loading GLTF with invalid query URL", async () => {
try {
await engine.resourceManager.load({
url: "/test-assets/model.gltf?q=invalid[query]",
type: AssetType.GLTF
});
expect.fail("Should have thrown an error");
} catch (error) {
expect(error).to.be.instanceOf(Error);
expect(error.message).to.include("Invalid query parameter");
}
});| describe("gltf subAsset load", () => { | ||
| it("invalid q case", async () => { | ||
| const loadRes = await engine.resourceManager.load({ | ||
| // contains invalid q value cdn url. | ||
| url: "https://mdn.alipayobjects.com/huamei_aftkdx/afts/file/A*_Ao1QZtL9fMAAAAAAAAAAAAADteEAQ/mock-project.json", | ||
| type: AssetType.Project | ||
| }); | ||
| expect(loadRes).to.equal(undefined); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test case needs improvement for reliability and clarity
Several issues with the current test implementation:
- Using an external CDN URL makes the test unreliable and dependent on network connectivity. Consider using a mock URL or local test fixture.
- The test description and implementation don't clearly document what constitutes an "invalid q value" or why
undefinedis the expected return value. - The test case should be expanded to better validate the local npm package resource loading fix mentioned in the PR objectives.
Consider refactoring like this:
describe("gltf subAsset load", () => {
- it("invalid q case", async () => {
+ it("should handle invalid query parameters in resource URLs", async () => {
const loadRes = await engine.resourceManager.load({
- // contains invalid q value cdn url.
- url: "https://mdn.alipayobjects.com/huamei_aftkdx/afts/file/A*_Ao1QZtL9fMAAAAAAAAAAAAADteEAQ/mock-project.json",
+ url: "/test-assets/mock-project.json?q=invalid[query]", // Use local test asset
type: AssetType.Project
});
+ // Document why undefined is expected
+ // Consider adding more assertions to validate the error handling
expect(loadRes).to.equal(undefined);
});Committable suggestion skipped: line range outside the PR's diff.
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
Bug Fixes
Refactor
getResourceByRefmethod for improved resource fetching, simplifying the URL construction process when loading resources.