Repository navigation
fix(ui): correct root-canvas demotion state and listener bookkeeping - #3070
cptbtptpbcptdtptp wants to merge 8 commits into
Conversation
Three coupled fixes ported from galacean#3068 (fix/shaderlab): - The demote loop dirtied the canvas itself instead of each element (setRootCanvasDirty(this) -> (element)), so demoted elements kept stale _rootCanvas/_indexInRootCanvas and never re-homed: the new root's walk re-assignment is gated on the element's dirty flag. - _registerListener truncated a shrinking listening chain without unregistering the dropped tail, leaking listeners on ex-ancestors. - Releasing that leak exposed that nested-canvas demotion relied on it: UpdateFlagManager.dispatch iterates a backward snapshot, so a listener appended mid-dispatch is never invoked by that dispatch, and the enabling canvas claims root status only after its dispatch returns — the cascade's search could not find it and promoted nested canvases to root. The demote cascade now receives the enabling canvas as an explicit successor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. Walkthrough
ChangesListener cleanup
Root canvas transitions
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change corrects root-canvas demotion state and listener cleanup within packages/ui. No concrete merge-blocking risk was found, and regression tests cover the root reassignment paths. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes stay within UI canvas lifecycle handling, with no identified new security boundary or attack path. Normal reassignment is tested, but listener removal during a notification leaves a limited ownership-consistency risk to verify. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 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. A rabbit checks the canvas tree Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3070 +/- ##
===========================================
+ Coverage 86.49% 86.67% +0.18%
===========================================
Files 809 809
Lines 94310 94394 +84
Branches 11737 11731 -6
===========================================
+ Hits 81570 81819 +249
+ Misses 12651 12488 -163
+ Partials 89 87 -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.
🧹 Nitpick comments (1)
tests/src/ui/UICanvas.test.ts (1)
354-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a second element to prevent hiding concurrent modification bugs.
Testing the demote cascade with only a single nested element (
image) masks concurrent modification bugs (such as removing elements from an array while looping over it, which succeeds for length 1 but skips elements for length 2+). Adding a second element ensures the logic robustly processes all children and would successfully catch theforEachiteration bug inUICanvas.ts.♻️ Proposed test update
it("re-homes elements to the new root canvas when theirs loses root status", () => { const outerEntity = root.createChild("outer"); const innerEntity = outerEntity.createChild("inner"); const innerCanvas = innerEntity.addComponent(UICanvas); const imageEntity = innerEntity.createChild("image"); const image = imageEntity.addComponent(Image); + const image2Entity = innerEntity.createChild("image2"); + const image2 = image2Entity.addComponent(Image); // Collected by the inner canvas while it is the root. // `@ts-ignore` innerCanvas._getRenderers(); // `@ts-ignore` expect(image._getRootCanvas()).to.eq(innerCanvas); + // `@ts-ignore` + expect(image2._getRootCanvas()).to.eq(innerCanvas); // `@ts-ignore` expect(innerCanvas._disorderedElements.length).to.be.greaterThan(0); // Enabling a canvas on an ancestor demotes the inner one. const outerCanvas = outerEntity.addComponent(UICanvas); // `@ts-ignore` expect(innerCanvas._isRootCanvas).to.be.false; // `@ts-ignore` expect(outerCanvas._isRootCanvas).to.be.true; // The element's canvas state must be reset (the walk's re-assignment is gated on the // dirty flag), otherwise it keeps rendering into the demoted canvas's dead queue. // `@ts-ignore` expect(image._isRootCanvasDirty).to.be.true; // `@ts-ignore` expect(image._rootCanvas).to.be.null; + // `@ts-ignore` + expect(image2._isRootCanvasDirty).to.be.true; + // `@ts-ignore` + expect(image2._rootCanvas).to.be.null; // The new root adopts it on its next walk. // `@ts-ignore` outerCanvas._getRenderers(); // `@ts-ignore` expect(image._getRootCanvas()).to.eq(outerCanvas); + // `@ts-ignore` + expect(image2._getRootCanvas()).to.eq(outerCanvas); outerEntity.destroy(); });🤖 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/ui/UICanvas.test.ts` around lines 354 - 389, Add a second child UI element with an Image component in the test “re-homes elements to the new root canvas when theirs loses root status,” then assert both images are marked dirty, have null root canvases after demotion, and are reassigned to outerCanvas after its renderer walk. Keep the existing assertions and cleanup behavior intact.
🤖 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/ui/UICanvas.test.ts`:
- Around line 354-389: Add a second child UI element with an Image component in
the test “re-homes elements to the new root canvas when theirs loses root
status,” then assert both images are marked dirty, have null root canvases after
demotion, and are reassigned to outerCanvas after its renderer walk. Keep the
existing assertions and cleanup behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fdadae4d-fb3c-4d95-a316-e04f616e96b7
📒 Files selected for processing (3)
packages/ui/src/Utils.tspackages/ui/src/component/UICanvas.tstests/src/ui/UICanvas.test.ts
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
把 #3068 的三处 packages/ui 修复原样 backport 到 dev/2.0。我在 #3068 已逐轮审过这三处（round-3 setRootCanvasDirty(this)→(element) 的 #2375-class bug、round-6 listener tail-release 泄漏、round-7 hosting 状态机 successor 线程化），本轮核心是校验：前像 bug 真在 dev/2.0 上、修复与 #3068 逐字等价、类型适配到 mainline ui 接口名正确、两个新测试可反向证伪。全部通过，无 P0/P1/P2/P3，可合。
核验结论（逐条）
1. Utils.ts — listener 链缩短的 tail 泄漏
dev/2.0前像确认：_registerListener走完 while 后直接listeningEntities.length = count，链变短时[count, oldLength)的尾部实体仍挂着_registerModifyListener→ 永久泄漏。- 修复补的
for (i=count; i<n) listeningEntities[i]._unRegisterModifyListener(listener)与 #3068 tip3c6c88c0逐字节相同（含注释）。✓
2. UICanvas.ts:414 — CanvasEnableInScene 分支缺 successor
- 前像
_setIsRootCanvas(false)无 successor → 嵌套 canvas demote cascade 里searchRootCanvasInParents(element)看不到「刚 enable、尚未 claim root」的祖先 canvas → 嵌套 canvas 被误提升为 root。 - 修复
_setIsRootCanvas(false, <UICanvas>param)+_setIsRootCanvas新增successor形参、cascade 里searchRootCanvasInParents(element) ?? successor，逻辑与 #3068 等价（#3068 用IUIElement/IUIGroupAble是它 round-5 把接口上移 core 后的名字，dev/2.0无此重构，backport 正确沿用 mainline 的IElement/IGroupAble，行为一致）。✓
3. UICanvas.ts:656 — 非 canvas 元素 setRootCanvasDirty(this)→(element)
- 前像 dirty 的是「正在 demote 的 canvas 自己」，而紧接着
disorderedElements.length = 0把这张 canvas 的列表清空 → 对元素 re-home 是 no-op；元素保留 stale_rootCanvas/_indexInRootCanvas+ 干净的 dirty flag，新 root 的 walk（以元素 dirty flag 为门）跳过它 → 渲染进已死队列、永不 re-home。 - 修复改为 dirty
element本身（setRootCanvasDirty内部置_isRootCanvasDirty=true+_registerRootCanvas(element,null)把_rootCanvas清 null），正解。✓
测试（两个新增均可反向证伪）
re-homes elements...：断言image._isRootCanvasDirty===true+image._rootCanvas===null。前像走setRootCanvasDirty(this)→ image 状态不动 →_isRootCanvasDirty仍 false → 前像 fail。✓re-roots nested canvases...：断言inner._isRootCanvas===false+inner._getRootCanvas()===outerCanvas。前像无 successor →searchRootCanvasInParents(inner)返 null →_setIsRootCanvas(!null)把 inner 误提升为 root → 断言 fail。✓Image经component/index.ts:6→index.ts export * from "./component"导出，IElement在dev/2.0声明了_rootCanvas/_indexInRootCanvas/_isRootCanvasDirty/_getRootCanvas()，测试引用符号全部存在。✓
补充说明
- #3068 里的「ui/Utils + spine 双源同步」债在此不适用：
dev/2.0无 spine 包（spine-in-UICanvas 是 #3068 stacked 在fix/shaderlab上的工作），backport 只需改ui/Utils.ts一处，已完整。 - 新增注释与 #3068 已接受版本逐字相同（多句解释性
//块，非 JSDoc），风格随 ported 代码一致，不另提。
LGTM。
Reorder _onEnableInScene to claim root status before broadcasting CanvasEnableInScene (Unity's AddToManager order), so demote cascades always find the new root via the ancestor search. Drops the newRootCanvas successor param entirely and merges the enable branch into the Parent branch.
Covers: enabler nested under an existing higher root, disable promoting the inner canvas back, and a disable/enable round-trip
关于分支上的后续改动:successor 参数 → 先认领后广播@cptbtptpbcptdtptp 分支上那几个 原修法的问题:正确,但 hack——补偿的是自己造的时序窗口原修法保持 具体表现为它补偿的窗口是实现顺序自己造出来的——整个 ui 包里, 这带来两个实际代价:
新修法:对齐"先应用、再派发"_onEnableInScene(): void {
const rootCanvas = Utils.searchRootCanvasInParents(this);
this._setIsRootCanvas(!rootCanvas); // 先认领(应用)
Utils.setRootCanvas(this, rootCanvas);
entity._dispatchModify(CanvasEnableInScene, this); // 后广播(派发)
}级联里的搜索在广播期间永远能命中已认领的启用 canvas,于是 successor 参数、 性能:无劣化,深树更优操作集合完全相同,差异只在级联内搜索的行走长度(以本 PR 测试层级 root/outer/mid/inner 计):
旧版每条级联搜索都走满到场景根 O(D);新版在启用 canvas 处早退 O(k)(k = 元素到启用者的层数)。30 层深、启用者在上方 2 层的树:30 跳 vs 2 跳。canvas 启停本身是冷路径,不涉每帧开销。代码量净 -3 行(13+/16−),删掉一个参数、一个分支、一个兜底。 bug 面:消除 2 项,新增 0 项
新增风险审计:全库消费 验证数据
对原始修复的三处诊断(脏错对象、尾部泄漏、时序依赖)没有任何改动,bug 1/2 的修法保持原样;这次只替换了 bug 3 的补偿方式。 |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
无阻塞问题，未发现新的 P0/P1/P2 finding。本次提交 COMMENTED review（gh pr review --comment），目标 HEAD：8ffaf5958da8bb9a5774993cd98cbc87fa109bb3。自动 CR 不替代人工 Reviewer 的合入门禁，是否合入仍由人工确认。
基于 2026-07-16 的审查结论做增量复审：核对实际 PR diff，并区分合入 dev/2.0 的既有变更与本 PR 新增变更；重点检查 enable 顺序调整、successor 路径删除、listener 释放及三项新增边界测试。
已关闭问题清单
- Listener 链缩短的尾部泄漏：
Utils._registerListener仍在截断数组前逐项注销尾部 listener；本轮只精简注释、移除已不需要的类型抑制，释放行为未回退。 - Demote 时错误地 dirty canvas 自身：仍对每个非 canvas
element清除旧 root 注册并标记 dirty，新 root 的 walk 可以重新收集元素。 - 嵌套 canvas 找不到正在启用的祖先：仍已解决，但实现改为先确定 root 状态和注册，再广播
CanvasEnableInScene。祖先搜索现在能直接找到新 root，原 successor 参数及 fallback 已删除，无需恢复。
架构、熵增与测试治理
- 上游事实与事件：检查
Entity.addComponent、Component.enabled/_setActive和UpdateFlagManager.dispatch。组件先进入 entity 的组件列表并进入 enable 阶段；UICanvas._onEnableInScene再完成 root 状态和注册后广播。级联不再依赖广播期间新增 listener 被执行，也不依赖泄漏 listener 收敛。 - 权威 owner 与下游消费：root 身份由 UICanvas 管理，root/group 的引用、索引及 listener 链统一经 Utils 注册和释放；ComponentsManager 只消费 canvas 注册结果，renderer/group/interactive 经既有 dirty/getter/walk 路径刷新派生归属。核对 demote 遍历中的删除语义，DisorderedArray 的延迟删除不会跳过待迁移元素。
- 复杂度净变化：相对上轮删除 successor 参数传递、
?? successorfallback 及 enable 专用赋值分支；没有新增持久状态、第三份 root 真相、平行状态机、转换/校验层或兼容 wrapper。应保留现有 root 状态与统一注册入口；本轮已完成同根因冗余的删除，未发现需要额外同步层或进一步删除的 actionable 治理问题。 - 测试契约：原两项回归测试继续从添加组件触发，断言元素重新归属及嵌套 canvas 的 root 结果；旧 successor 时序注释已按新契约更新。新增三项覆盖已有更高 root、禁用外层后的提升与元素迁移、disable/enable 往返；没有为旧 fixture/mock/snapshot 在生产路径保留 compatibility branch。
验证复用目标 HEAD 的 CI 日志：先完成 b:module 与全包 b:types，再运行 Chromium 测试；UICanvas 9/9、UICanvasEdge 3/3，全套 131 个文件、1746 项测试通过。lint、三平台 build 与四组 e2e 也全部成功。本轮按只读边界未在本地构建或重跑测试。
Summary
Ports three coupled
packages/uifixes from #3068 (fix/shaderlab) todev/2.0— all three bug sites are byte-identical here.UICanvas._setIsRootCanvas(false)calledUtils.setRootCanvasDirty(this)instead of(element), so demoted elements kept a stale_rootCanvas/_indexInRootCanvasand a clean dirty flag — but the new root's walk re-assignment is gated on the element's dirty flag, so they kept rendering into the demoted canvas's dead queue and never re-homed.Utils._registerListenertruncated a shrinking listening chain (listeningEntities.length = count) without unregistering the dropped tail, permanently leaking listeners on ex-ancestor entities.UpdateFlagManager.dispatchiterates a backward snapshot (a listener appended mid-dispatch is never invoked by that dispatch), and the enabling canvas claims root status only after itsCanvasEnableInScenedispatch returns — so the demote cascade'ssearchRootCanvasInParentscould not find it and promoted nested canvases to root. Convergence previously depended on a leaked listener happening to sit at a low index of the backward iteration. The cascade now receives the enabling canvas as an explicitsuccessor(_setIsRootCanvas(false, successor)).(2) and (3) must land together: fixing the leak alone breaks nested-canvas re-rooting.
Tests
Two regression tests added to
tests/src/ui/UICanvas.test.ts:re-homes elements to the new root canvas when theirs loses root status— pins (1)re-roots nested canvases to the enabling ancestor canvas— pins (3), exercised through (2)Negative validation: on unfixed
dev/2.0(rebuilt dist without the source fixes) both new tests fail and all 6 existing tests pass; with the fixes the full ui suite is green (66/66, node22b:module+b:types, vitest fromtests/).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests