fix(loader): prefer virtualPathResourceMap type over URL extension inference#3035
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.51% +2.19%
===========================================
Files 914 919 +5
Lines 101723 102523 +800
Branches 10437 11371 +934
===========================================
+ Hits 78647 81519 +2872
+ Misses 22892 20818 -2074
- Partials 184 186 +2
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>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…type A registered virtualPath records the asset's actual type; an explicit type passed at load time should not override that fact. Reverse the precedence to `map ?? explicit ?? extension` so a registered asset is always loaded with its recorded type, while explicit type is still honored for unregistered (pro-code) assets where the map has no entry. Also drop the now-redundant explicit type in getResourceByRef, letting the map drive type resolution as the single source of truth.
…ions The method resolves the asset type (with validation that throws when it cannot be determined) in addition to filling in default load options, so "assign default options" undersold what it does. Rename to reflect that it resolves and validates the load item, not just assigns defaults.
The variable holds an EditorResourceItem looked up from _virtualPathResourceMap — a registered virtualPath entry carrying the resource's real path, type and metadata. "remoteConfig" was misleading: it is neither a config nor remote (it is an in-memory map entry that merely contains a remote path field). Rename to reflect that it is an entry of the virtualPath resource map.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Loading a registered virtualPath with only a url (not via getResourceByRef) previously dropped the params recorded in the map. Resolve params from the map entry in _resolveLoadItemOptions, mirroring type: explicit params win, otherwise the map params are used. getResourceByRef no longer forwards params, since the map is now the single source of truth for both type and params.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
第六轮（6de6bc060 → e781728，2 个 commit，PR 已合入 dev/2.0 via 4967220a6）。本轮把 params 也纳入虚拟路径 map 解析：_resolveLoadItemOptions 新增 assetInfo.params ??= virtualResourceEntry?.params，并据此把 getResourceByRef 里冗余的 params: mapped.params 删掉（同前一轮删 type: mapped.type 同理）。逐路径核对后对 getResourceByRef 完全等价，对直接 load(虚拟路径) 是新增能力（之前 params 拿不到）。可以合（已合）。
上轮问题状态
前五轮所有问题保持关闭，本轮无变动：
- [P2] 虚拟路径查表用未解析原始 url — ✅ 保持关闭。
- CodeRabbit
_getTypeByUrl(undefined)crash / normalize url — ✅ 保持关闭。 - type 优先级反转
map ?? explicit ?? extension— ✅ 上轮已核安全，保持关闭。 getResourceByRef删type: mapped.type— ✅ 上轮已核等价，保持关闭。- [P3] 测试回归强度不对等 / characterization — 保持关闭，不另起。
||=→??=/type:""、params:null不可达分支 — 已表态非问题，保持关闭。
关键核对（确认无回归）
1. params ??= virtualResourceEntry?.params（_resolveLoadItemOptions :347）—— 安全且语义自洽
getResourceByRef路径等价：base 是load({url: loadUrl, params: mapped.params})，本轮删 explicit 后params在_resolveLoadItemOptions里被??= mapped.params填回（loadUrl的assetBaseURL剥 query 后回到同一虚拟路径 →virtualResourceEntry === mapped），含子资源?q=路径同样剥回。数据流 1:1。- 直接
load(虚拟路径)是新增能力：base 下params为undefined，本轮补回 map 记录的 params。 - 唯一传 explicit params 的内部调用点
SpriteAtlasLoader:55/GLTFTextureParser:52都走resolveAbsoluteUrl→ 绝对 url → map 必然 miss →??= undefined保留 explicit，不受影响。
2. params 优先级 = explicit 赢（与 type 的 map 赢相反）—— 设计上可接受
type是资源身份（决定走哪个 loader），编辑器记录的即真实身份，故 map 赢、explicit 不能覆盖。params是 loader 调参（mipmap/format/keepMeshData…），map 存的是默认值，调用方传 explicit 是有意 override，故 explicit 赢。- 两者方向相反但各自正确：身份用 map 收口，选项允许覆盖。不是 bug。
3. 两个新测试 —— 回归强度
fills params from virtualPathResourceMap when params is omitted：有效反向见证。base（6de6bc060）_resolveLoadItemOptions无params ??=行 →params为undefined→ 断言{mipmap:false}fail；本轮补回 → pass。✅prefers explicit params over the virtualPath map params：断言本身正确（explicit 赢），但严格说不是反向见证（base 下直接load的 params 是undefined，本轮才是mipmap:false，两版断言{mipmap:true}都 pass，因为 explicit 始终在第一位）。无害，是合理的防回归基线。
问题
无 P0/P1/P2。一条 P3（不阻塞，仅记录；PR 已合，留作后续清理）：
- [P3] tests/.../ResourceManager.test.ts:146 — 注释
mirroring type precedence说反了- 该测试断言 params 是 explicit 赢（
prefers explicit params over the virtualPath map params，:131），而 type 是 map 赢（同文件 :142prefers the virtualPath map type over an explicit type+ :178-179 注释 "an explicit type cannot override the type the editor recorded for it"）。两者优先级相反，不是 "mirroring"（镜像/一致），而是倒置。 - 注释
// Explicit params overrides the map params (overwrite, not merge), mirroring type precedence的后半句与代码事实矛盾——把有意为之的非对称说成对称，是会误导后人的 aspirational 注释。 - 建议改成实话，并点明非对称的理由，例如：
// params is an override (explicit wins) — opposite to type, which the registered map owns; type is identity, params are loader options the caller may tune。
- 该测试断言 params 是 explicit 赢（
简化建议
无。本轮把 params 也收口进 map 解析、删掉 getResourceByRef 最后一处冗余显式字段，方向与前几轮"单一真相源"一致，干净。唯一遗留是上面那条说反的注释。
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.