fix(loader): prefer virtualPathResourceMap type over URL extension inference#3035
fix(loader): prefer virtualPathResourceMap type over URL extension inference#3035cptbtptpbcptdtptp wants to merge 6 commits into
Conversation
…ference When loading an asset by URL, prefer the type registered in _virtualPathResourceMap over inferring from the URL file extension. This ensures editor-registered assets with non-standard extensions (e.g. virtual paths without extensions) resolve to the correct loader. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesResourceManager type resolution and sub-asset loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3035 +/- ##
===========================================
+ Coverage 77.31% 79.16% +1.85%
===========================================
Files 914 914
Lines 101723 101660 -63
Branches 10437 11211 +774
===========================================
+ Hits 78647 80480 +1833
+ Misses 22892 20993 -1899
- Partials 184 187 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/asset/ResourceManager.ts`:
- Around line 343-347: The lookup at line 343 uses the raw assetInfo.url
directly to query _virtualPathResourceMap, which fails to match entries when the
URL contains query parameters like ?q=. Normalize the URL before performing the
_virtualPathResourceMap lookup by extracting the base URL without query
parameters, then use the normalized URL as the key for the lookup while still
preserving the original assetInfo.url for any subsequent operations that need
the full URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec63d091-d98a-4f29-b6bf-49e62f8c41e9
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Strip the query (?q=) before looking up `_virtualPathResourceMap`, so a sub-asset virtual path resolves its type the same way `_loadSingleItem` resolves its path via `assetBaseURL`. Keep explicit `type` taking precedence over the map (the map only overrides URL-extension inference, matching the PR title), and guard against an undefined `url` for urls-only loads such as TextureCube. Add ResourceManager tests covering extensionless virtual path, sub-asset query path, and explicit-type precedence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/core/resource/ResourceManager.test.ts (1)
91-121: ⚡ Quick winAdd a regression test for url-less
LoadItemhandling.These tests validate virtual-path precedence well, but they don’t cover the undefined-
urlpath mentioned in the PR objective. Add one case that passes aLoadItemwithouturl(e.g.,urls-only shape) and asserts_assignDefaultOptionsdoes not throw unexpectedly from type inference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/resource/ResourceManager.test.ts` around lines 91 - 121, Add a regression test case within the "assignDefaultOptions virtualPath type" describe block to cover the url-less LoadItem scenario. Create a new test that calls _assignDefaultOptions with a LoadItem object that omits the url property (for example, containing urls instead), then assert that the method does not throw an error and handles type inference gracefully when url is undefined or missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/asset/ResourceManager.ts`:
- Line 345: The call to ResourceManager._getTypeByUrl(assetInfo.url) at line 345
can execute with an undefined URL in url-list-based inputs, causing a runtime
crash when split is called on undefined before reaching the proper error
handling. Add a guard condition to only call _getTypeByUrl when assetInfo.url is
defined, allowing the assignment to evaluate to undefined and reach the intended
"asset type should be specified" validation path.
---
Nitpick comments:
In `@tests/src/core/resource/ResourceManager.test.ts`:
- Around line 91-121: Add a regression test case within the
"assignDefaultOptions virtualPath type" describe block to cover the url-less
LoadItem scenario. Create a new test that calls _assignDefaultOptions with a
LoadItem object that omits the url property (for example, containing urls
instead), then assert that the method does not throw an error and handles type
inference gracefully when url is undefined or missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 587064c4-8119-4061-baef-ad75dee58b78
📒 Files selected for processing (2)
packages/core/src/asset/ResourceManager.tstests/src/core/resource/ResourceManager.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…oading Resolve asset type and remote path from a single _parseURL + virtualPath lookup, and move baseUrl resolution after the lookup so virtual paths are no longer polluted by baseUrl. _assignDefaultOptions now takes the resolved remoteConfig instead of querying the map itself. - Merge `urls` into `url` before _parseURL — fixes urls-only (TextureCube) throw - Always strip the `?q=` query from item.url so the sub-asset query no longer leaks into the cache key / loadingPromise dedup / request url - Move virtualPath type checks to the public load() layer (drop tests that poked the private _assignDefaultOptions); sync the _parseURL field rename in the existing queryPath tests Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/core/resource/ResourceManager.test.ts (1)
91-120: 💤 Low valueConsider cleaning up test state to improve isolation.
The tests modify shared state (
_virtualPathResourceMapviainitVirtualResources) and leave unresolved promises inloadingPromises. While the tests correctly validate the refactored behavior, this could cause flakiness or interference with other tests.Consider adding cleanup in
afterEachor within each test:Suggested cleanup pattern
+ afterEach(() => { + // Clean up virtual path registrations + // `@ts-ignore` + engine.resourceManager._virtualPathResourceMap = Object.create(null); + // `@ts-ignore` + engine.resourceManager._loadingPromises = {}; + }); + it("infers loader type from virtualPathResourceMap when type is omitted", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/resource/ResourceManager.test.ts` around lines 91 - 120, The tests in the "virtualPath loading" describe block are not cleaning up shared state after execution, which can cause test interference and flakiness. Add an afterEach hook within the describe block that clears the virtual resource map (by calling initVirtualResources with an empty array or resetting _virtualPathResourceMap) and clears any pending loading promises from ResourceManager to ensure proper test isolation. Alternatively, add cleanup at the end of each test (after loaderSpy.mockRestore() calls) to reset the resourceManager state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/src/core/resource/ResourceManager.test.ts`:
- Around line 91-120: The tests in the "virtualPath loading" describe block are
not cleaning up shared state after execution, which can cause test interference
and flakiness. Add an afterEach hook within the describe block that clears the
virtual resource map (by calling initVirtualResources with an empty array or
resetting _virtualPathResourceMap) and clears any pending loading promises from
ResourceManager to ensure proper test isolation. Alternatively, add cleanup at
the end of each test (after loaderSpy.mockRestore() calls) to reset the
resourceManager state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9cf71b29-ac19-4a28-b5b1-695a6abf9f56
📒 Files selected for processing (2)
packages/core/src/asset/ResourceManager.tstests/src/core/resource/ResourceManager.test.ts
…eturn
- _parseURL returns { assetBaseURL } again — clearer than `url`, which
collided with the `item.url` reassigned in _loadSingleItem
- _assignDefaultOptions returns void; it mutates in place and the return
value was no longer used after the lookup moved to the caller
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The added test referenced a non-existent "Texture2D" loader key; the texture type is AssetType.Texture = "Texture", so vi.spyOn got undefined and threw "Cannot convert undefined or null to object". Use the AssetType enum for both loader keys to avoid the typo class. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…for type - _assignDefaultOptions: type ??= ... (was ||=) so an explicit type is only defaulted when actually unset, not when it is a falsy string - add load()-layer tests: urls merged before parse (KTXCube urls-only), explicit type wins over the virtualPath map type, and a virtualPath still resolves via the map when baseUrl is set Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
第四轮（e78e08bbe → 89e5b2868，3 个 commit）相对上一轮全是收尾打磨，无功能改动：_parseURL 返回字段 url 改名 assetBaseURL（消除与 _loadSingleItem 里被重赋值的 item.url 的命名碰撞，并与 _onSubAssetSuccess/_onSubAssetFail 已有的 assetBaseURL 形参命名对齐）、_assignDefaultOptions 返回值改 void（查表搬到调用方后该返回值已无人消费）、||=→??=、补 4 个 load() 层测试。逐项核对均为等价 / 纯净化改动，无回归。可以合。
上轮问题状态
- [P2] 虚拟路径查表用未解析原始 url — ✅ 上一轮已解决（
_parseURL先于查表跑），本轮无变动，保持关闭。 - CodeRabbit「
_getTypeByUrl(undefined)crash」/「normalize url before lookup」 — ✅ 已结构化兜住（item.url ?? item.urls.join(",")前置补齐 +_parseURL剥 query），CodeRabbit 自己也标了 "Addressed in e78e08b / 66d7929"，保持关闭。 - [P3]
shares the main asset across sub-asset queries是 characterization 非回归 — 上轮已记，本轮无变动；本轮新增的merges urls into a single url before parsing属同一类——base 上_assignDefaultOptions早就有assetInfo.url ?? assetInfo.urls.join(",")（base :350，在_parseURL之前），KTXCube urls-only 在 base/PR 行为一致，这条测试 base 同样 pass，是既有行为的 characterization。无害，归入同一条 P3，不另起新问题。 ||=→??=— 上轮我已就||=把type:""当未设处理这点表态"不是问题"。本轮翻成??=。逐查AssetType全部为非空串、且公开 APItype?: AssetType构造不出""，两种写法仅在type===""这一不可达分支发散，均非真实路径，不再就此发散。
关键核对（确认无回归）
- 字段改名
url → assetBaseURL：_parseURL唯一消费点 :356 同步解构，数据流 1:1 不变，纯命名一致性改进。 _assignDefaultOptions返回void：唯一调用点 :361 不使用返回值，确为死返回值清除。- 新增 4 测试：均经公开
load()入口触发、spy_loaders断言公开输出（type / call count），是链路测试不戳私有；@ts-ignore仅用于访问静态_loadersmap，与本文件既有测试（:21/:42/:50/:58/:67）同一惯例。
问题
无新问题。本轮 delta 全是命名 / 死代码 / 测试覆盖的收尾打磨，前序问题已全部闭环。
简化建议
无。代码已是上一轮本质解的精简形态，本轮进一步收紧了命名与返回签名，干净。
2 participants
Summary
_assignDefaultOptions, check_virtualPathResourceMapfirst for the asset type before falling back to URL extension inference.type.Test plan
pnpm run testpasses_virtualPathResourceMaptype🤖 Generated with Claude Code
Summary by CodeRabbit
?materials[...]variants reuse a single main asset load and apply consistent fallback behavior.