fix(loader): preserve virtual resource identity in Editor Preview - #3084
fix(loader): preserve virtual resource identity in Editor Preview#3084luzhuang wants to merge 5 commits into
Conversation
|
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:
WalkthroughVirtual resource loading now preserves logical paths while resolving physical transport URLs. URL handling covers leading slashes, and texture decoding shares cleanup and error reporting across initial loads and content restoration, with expanded atlas and texture tests. ChangesVirtual Resource Loading
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ResourceManager
participant RemoteRequest
participant TextureLoader
ResourceManager->>ResourceManager: Preserve virtual resource URL
ResourceManager->>RemoteRequest: Request physical resource URL
RemoteRequest-->>TextureLoader: Return image bytes
TextureLoader->>TextureLoader: Decode image and revoke object URL
TextureLoader-->>ResourceManager: Return or restore Texture2D
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3084 +/- ##
===========================================
+ Coverage 85.42% 85.55% +0.13%
===========================================
Files 811 811
Lines 94654 94659 +5
Branches 11512 11551 +39
===========================================
+ Hits 80854 80986 +132
+ Misses 13710 13581 -129
- Partials 90 92 +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:
|
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
本轮审查了 bd34daa45612af8b402cd3be916ff181f21ae742...53bb5b4542b6baa51d84247fde54878ddb78eedf 的 4 个提交、8 个文件，并把 348ebfa6e486648036c25d7db101b8ed0f045c70...53bb5b4542b6baa51d84247fde54878ddb78eedf 的三个注释增量、#3079 最终 tree 与当前 dev/2.0 上下游逐项对账。VFS 逻辑身份、Atlas 相对依赖和 Texture 解码 owner 的主体方向正确；当前未发现新的 P0/P1，但 dev/2.0 的公开缓存/取消入口仍保留一处同根因的双口径 P2。当前阻塞级别为 P2（不阻塞），实际 review 动作为 COMMENTED，目标 HEAD 为 53bb5b4542b6baa51d84247fde54878ddb78eedf。自动 CR 不替代人工 Reviewer 的合入门禁，仍需人工审核确认。
已关闭问题清单
baseUrl覆盖虚拟资源逻辑身份 — 已在348ebfa6e486648036c25d7db101b8ed0f045c70闭环：VFS 命中时LoadItem.url保留virtualPath，Atlas 链路测试贯通验证 atlas 与 page 各自命中物理地址。LoadItem.resolvedUrl泄漏派生物理地址并形成双源 — 当前公开类型、ResourceManager 写入及 fixture 均无该字段；物理地址只存在于虚拟资源表和内部 cache/loading/I/O 边界。- Shader 使用物理 URL 作为请求、相对
#include基址或编码判据 — 当前dev/2.0基线继续以逻辑 URL 请求和解析 include，并按 payload 区分源码/预编译表示；本 PR 没有恢复旧的后缀判型或双请求路径。 - Texture 首次解码与内容恢复维护平行 Image 状态机 — 当前两条路径共用
decodeImage，Image 事件、object URL 释放及包含逻辑资源身份和字节数的错误均由同一 helper 拥有；恢复失败测试可反向证伪旧路径。 - 迁移分支夹带 clone 协议及重复 Shader 变更 — #3084 直接基于
dev/2.0，完整 diff 仅含 loader/VFS 修复的 8 个文件，标题、正文和实际变更范围一致。 - 新增注释不符合仓库约定 —
07a8d6a11649e6f95ac1262e1bb9b3e055919930、d5735fa7c60549959513415ffad0621457243656与53bb5b4542b6baa51d84247fde54878ddb78eedf已统一连续///@ts-ignore说明并移除末尾句号；LoadItem的多行 TSDoc 正确保留句号。
问题
- [P2] 让公开缓存查询与取消操作复用 ResourceManager 的虚拟路径映射，删除逻辑/物理 key 双口径 —
packages/core/src/asset/ResourceManager.ts:346-359,421-431现在把虚拟资源按virtualResourceEntry.path写入_assetUrlPool/_loadingPromises，但:106-108的getFromCache(url)和:153-164的cancelNotLoaded(url)仍直接拿调用方 URL 索引这两个物理-key pool。注册Texture/page.tex -> blob:...后，await load("Texture/page.tex")再调用getFromCache("Texture/page.tex")会得到null；加载过程中调用cancelNotLoaded("Texture/page.tex")也找不到对应 promise。它们既违反本 PR 新明确的“请求 URL 是资源身份”契约，也让同一个 ResourceManager 对load与公开管理 API 维护两套 key 语义。应保留_virtualPathResourceMap为唯一映射 owner，把 load/cache/cancel 的 pool-key 派生收口到同一规则，并删除getFromCache/cancelNotLoaded对调用方 URL 的直接索引；不要通过同时写入 virtual/physical 两份镜像 key 来同步。补两条公开链路测试：虚拟资源加载后可按virtualPath取回同一对象，以及 pending 虚拟加载可按virtualPath取消。#3079 最终 tree 已有前一条缓存映射测试，可直接按当前公开契约重放。
架构、熵增与测试治理
向上游一层，Editor manifest / ProjectLoader 拥有 { virtualPath, path, type } 输入事实；ResourceManager 应唯一拥有 virtualPath -> path 映射、loader 选择、cache/loading key 与 I/O 边界，Utils.resolveAbsoluteUrl 只机械派生逻辑依赖路径。向下游一层，SpriteAtlas 从逻辑 atlas 路径派生 page，再回到 ResourceManager 映射；Shader payload 拥有表示格式、逻辑 URL 拥有 include 基址；Texture 的 decodeImage 拥有浏览器 Image/object-URL 生命周期。
相较基线，本 PR 没有新增持久状态、镜像字段、wrapper、legacy fallback 或 compatibility branch：LoadItem 没有引入第二个 resolved URL，Texture 的两套解码状态机反而合并为一套，三个后续提交也只有注释变化。剩余熵增是问题 1 中同一事实的两种公开 key 口径；应保留 ResourceManager 的映射与物理 pool 为 owner，删除公开入口的直接索引，而不是增加第三份 alias 状态。测试侧，Atlas 已从公开 ResourceManager.load() 贯通逻辑身份、相对派生和两个物理请求边界，Texture restoration 守住统一错误契约；当前缺口仅是 cache/cancel 管理链路。未发现为旧测试保留的生产兼容逻辑，也没有应随新契约继续保留的 legacy fixture、mock 或 snapshot。
|
@GuoLei1990 这条 P2 成立，已在 ccf00ca14 闭环。
验证结果：本地全量 124 files / 1658 tests，相关 4 files / 28 tests， |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/src/core/resource/ResourceManager.test.ts (1)
184-204: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the pending load created by this test.
new AssetPromise(() => {})never settles, and the test only restores the loader spy. Because the sameResourceManageris reused across tests, its_loadingPromisesretains this entry indefinitely. Cancel it infinally(usingphysicalPath) or use a self-cleaning resolved mock.Proposed fix
} finally { + resourceManager.cancelNotLoaded(physicalPath); loaderSpy.mockRestore(); }🤖 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 184 - 204, Clean up the unresolved load created in the “preserves the virtual resource identity for loaders” test: in the finally block, cancel the pending resource-manager load using physicalPath before restoring loaderSpy, or replace the mock with a self-cleaning resolved AssetPromise. Ensure _loadingPromises does not retain the test entry.
🤖 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 159-161: Update the cancellation loop in ResourceManager to parse
each logical URL and construct the same physical-path-plus-query key used by
_loadSingleItem before looking up _loadingPromises. Preserve direct URL
cancellation while ensuring virtual sub-assets such as material queries resolve
to their exact pending promise key, and add a regression test covering
cancellation of a virtual sub-asset.
---
Outside diff comments:
In `@tests/src/core/resource/ResourceManager.test.ts`:
- Around line 184-204: Clean up the unresolved load created in the “preserves
the virtual resource identity for loaders” test: in the finally block, cancel
the pending resource-manager load using physicalPath before restoring loaderSpy,
or replace the mock with a self-cleaning resolved AssetPromise. Ensure
_loadingPromises does not retain the test entry.
🪄 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 Plus
Run ID: fe5f665a-1713-4c56-a236-312193947492
📒 Files selected for processing (2)
packages/core/src/asset/ResourceManager.tstests/src/core/resource/ResourceManager.test.ts
| const urls = typeof url === "string" ? [url] : url; | ||
| urls.forEach((url) => { | ||
| this._loadingPromises[this._getRemoteUrl(url)]?.cancel(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Map parsed virtual URLs before cancelling.
For a virtual sub-asset such as Assets/foo?q=materials[0], _loadSingleItem stores the pending promise under a key like <physicalPath>?q=materials[0]. This code maps the full logical string instead, so _getRemoteUrl cannot find the exact virtual-path entry and the load remains uncancelled. Reuse the same query parsing/key construction used by _loadSingleItem, and add a regression test for virtual sub-asset cancellation.
Proposed direction
const urls = typeof url === "string" ? [url] : url;
urls.forEach((url) => {
- this._loadingPromises[this._getRemoteUrl(url)]?.cancel();
+ const { assetBaseURL, queryPath } = this._parseURL(url);
+ const remoteAssetBaseURL = this._getRemoteUrl(assetBaseURL);
+ const paths = queryPath ? this._parseQueryPath(queryPath) : [];
+ let remoteUrl = remoteAssetBaseURL;
+ if (queryPath) {
+ remoteUrl += "?q=" + paths.shift();
+ let index: string;
+ while ((index = paths.shift())) remoteUrl += `[${index}]`;
+ }
+ this._loadingPromises[remoteUrl]?.cancel();
});📝 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.
| const urls = typeof url === "string" ? [url] : url; | |
| urls.forEach((url) => { | |
| this._loadingPromises[this._getRemoteUrl(url)]?.cancel(); | |
| const urls = typeof url === "string" ? [url] : url; | |
| urls.forEach((url) => { | |
| const { assetBaseURL, queryPath } = this._parseURL(url); | |
| const remoteAssetBaseURL = this._getRemoteUrl(assetBaseURL); | |
| const paths = queryPath ? this._parseQueryPath(queryPath) : []; | |
| let remoteUrl = remoteAssetBaseURL; | |
| if (queryPath) { | |
| remoteUrl += "?q=" + paths.shift(); | |
| let index: string; | |
| while ((index = paths.shift())) remoteUrl += `[${index}]`; | |
| } | |
| this._loadingPromises[remoteUrl]?.cancel(); | |
| }); |
🤖 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 `@packages/core/src/asset/ResourceManager.ts` around lines 159 - 161, Update
the cancellation loop in ResourceManager to parse each logical URL and construct
the same physical-path-plus-query key used by _loadSingleItem before looking up
_loadingPromises. Preserve direct URL cancellation while ensuring virtual
sub-assets such as material queries resolve to their exact pending promise key,
and add a regression test covering cancellation of a virtual sub-asset.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
本轮完整增量审查了 53bb5b4542b6baa51d84247fde54878ddb78eedf...ccf00ca14d5da98cd2f0522c8fe52f67cceb9810 的 1 个提交、2 个文件，并沿 load/getFromCache/cancelNotLoaded 的 key 派生向上游 VFS 注册事实、向下游子资源 promise 与 loader I/O 各追一层。ccf00ca14 已闭环主资源按 virtualPath 查询缓存和取消加载的上一轮 P2，且没有引入逻辑/物理 alias；但带 ?q= 的虚拟子资源仍使用另一套 loading key 派生，当前有 1 个 P2。阻塞级别为 P2（不阻塞），实际 review 动作为 COMMENTED，目标 HEAD 为 ccf00ca14d5da98cd2f0522c8fe52f67cceb9810。自动 CR 不替代人工 Reviewer 的合入门禁，仍需人工审核确认。
已关闭问题清单
baseUrl覆盖虚拟资源逻辑身份 — 已由348ebfa6e486648036c25d7db101b8ed0f045c70修复；VFS 命中时 Loader 收到 logical virtualPath，Atlas 相对页面依赖再回到 ResourceManager 映射。LoadItem.resolvedUrl形成逻辑/物理双源 — 当前公开类型、生产写入与 fixture 均无该字段，物理地址只保留在 VFS 表及内部 I/O/cache/loading 边界。- Shader 以物理 URL 作为 include 基址或表示格式判据 — 当前仍以逻辑 URL 解析 include、以 payload 判断源码/预编译表示，本 PR 未恢复后缀判型或第二条请求路径。
- Texture 首次解码与内容恢复维护平行状态机 — 两条路径已统一复用
decodeImage，Image/Object URL 生命周期和错误契约只有一个 owner，恢复失败测试可反向守住。 - 迁移分支夹带 clone/Shader 变更 — #3084 基于
dev/2.0，完整 PR diff 仍仅包含 8 个 loader/VFS 相关文件，标题、正文和变更单元一致。 - 新增注释不符合仓库约定 —
07a8d6a11649e6f95ac1262e1bb9b3e055919930、d5735fa7c60549959513415ffad0621457243656、53bb5b4542b6baa51d84247fde54878ddb78eedf已闭环单行注释、@ts-ignore说明和多行 TSDoc 标点；本增量新增/修改注释也符合该约定。 - 主资源 cache/cancel 的逻辑/物理 key 双口径 —
ccf00ca14已让getFromCache(virtualPath)、cancelNotLoaded(virtualPath)、_onSubAssetSuccess与_loadSingleItem复用_getRemoteUrl；新增两条公开链路测试能在旧实现上失败，且_assetUrlPool/_loadingPromises仍只存物理 key。
问题
- [P2] 让虚拟子资源取消复用
_loadSingleItem的完整 loading-key 派生，删除 query key 的第二套口径 —packages/core/src/asset/ResourceManager.ts:159-162现在把调用方传入的完整字符串直接交给_getRemoteUrl，但该 helper 在:187-189只对完整 key 做 VFS 精确查表；与此同时，:341-373,376-407,479的加载路径会先从Assets/model?q=materials[0]拆出基础 virtualPath，再映射为物理地址，并把子 promise 写到blob:...?q=materials[0]。因此cancelNotLoaded("Assets/model?q=materials[0]")仍会索引逻辑 key，返回的子资源 promise 不会被取消；新增测试tests/src/core/resource/ResourceManager.test.ts:206-229只覆盖无 query 的主资源，无法守住该分支。保留_parseURL为 query 拆分 owner、_virtualPathResourceMap为 virtualPath→physicalPath 事实 owner、物理_loadingPromises为唯一状态；把“解析基础 URL → VFS 映射 → 规范化并拼回 query”的完整 key 派生收口为 load/cancel 共用的机械路径，并删除_loadSingleItem内联拼 key 与cancelNotLoaded直接精确映射这两套实现，不要增加 logical/physical 镜像 alias。补一条从公开load(virtualPath + "?q=...")到cancelNotLoaded的反向测试。本问题也与 CodeRabbit 的现有 discussion#discussion_r3672281364结论一致，本 review 不另发重复 inline comment。
架构、熵增与测试治理
向上游一层，Editor manifest / ProjectLoader 拥有 { virtualPath, path, type } 注册事实；ResourceManager 应唯一拥有 virtualPath→physicalPath、loader 选择和 cache/loading key 派生。向下游一层，Loader 继续消费逻辑身份以解析相对依赖，_request 在叶子 I/O 边界映射物理地址；子资源链由 _parseURL / _parseQueryPath 生成规范 query，并以 _subAssetPromiseCallbacks 协调主/子资源完成状态。
相较上一 HEAD，本增量没有新增持久状态、镜像字段、wrapper、legacy fallback 或 compatibility branch；_getRemoteUrl 替换了 _onSubAssetSuccess 与 _loadSingleItem 的直接查表，owner 数量净减少，主资源公开入口也已统一。剩余熵增仅是完整 loading key 仍由 _loadSingleItem 拼装，而取消入口只做基础精确映射；按问题 1 收口后应只保留一条机械派生路径，不增加第三份真相。测试侧，新增 cache/main-cancel 用例均从公开 API 验证新契约，且新 pending 用例在 finally 清理自身 promise；未发现为了旧测试保留的生产 compatibility branch、legacy fallback、镜像状态或第二套校验。CodeRabbit 已单独跟踪旧 preserves the virtual resource identity for loaders fixture 的 pending promise 清理，本 review 不重复立项。
2 participants
问题
Editor Preview 会将工程文件注册为 VFS 虚拟资源。SpriteAtlas 元数据中的页面路径
./auto-atlas_image_0.tex，必须先基于 Atlas 的逻辑路径SpriteAtlas/.../auto-atlas.atlas推导为SpriteAtlas/.../auto-atlas_image_0.tex，再由ResourceManager映射到 Blob 或 CDN 等物理加载地址。此前
LoadItem.url在进入 Loader 前被改写为物理 URL；后续相对路径推导随之基于物理 URL 运行，VFS key 因而丢失。对于无 scheme 的路径，原先的file://用法还会把首段当作 host 并按 URL 规则小写化，导致SpriteAtlas/...被请求为spriteatlas/...，Preview 无法启动。修复
LoadItem.url始终保留virtualPath；baseUrl只作用于普通、未注册的相对传输 URL。ResourceManager的加载、子资源回调、缓存查询与取消统一通过_getRemoteUrl派生物理 key；内部 cache/loading pool 仍只保存一份物理 key。file:///的 pathname 解析，避免首段进入 host 规范化，并保留 leading slash 语义。覆盖
baseUrl不会改写已注册 VFS key。分支边界
这是 #3079 在
dev/2.0上的等价修复：基于dev/2.0@bd34daa重放 #3079 的最终 loader 改动。后续提交仅对齐项目的英文注释规范并闭环 review 反馈，不包含迁移分支的其他提交。验证
HEADLESS=true pnpm vitest run：124 files / 1658 tests passedpnpm b:modulepnpm b:types：12 个 package project 均通过pnpm lint：0 errors；1622 existing warningspnpm format:check与改动测试文件的 Prettier checkgit diff --checkSummary by CodeRabbit