fix(entity): clearChildren bug + setSiblingIndex without parent#2991
fix(entity): clearChildren bug + setSiblingIndex without parent#2991cptbtptpbcptdtptp wants to merge 2 commits into
Conversation
WalkthroughEntity hierarchy operations now emit modify events in a deterministic sequence. ChangesEntity Hierarchy Modification Behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/Entity.ts (1)
399-420:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
EntityModifyFlags.Childdispatched per child contradicts the new test (and the documented "single event for the whole clear" intent).
this._dispatchModify(EntityModifyFlags.Child, this)is inside theforloop, so the parent receives oneChildevent per child cleared. The new test assertsparentModifyCount[EntityModifyFlags.Child]).eq(1)(seetests/src/core/Entity.test.ts), which will fail wheneverclearChildren()runs over more than one child (the test itself uses two children).If the intent is a single bulk event per the test's comment ("a single
Childmodify event for the whole clear"), hoist the dispatch out of the loop. If the intent really is one event per child (more consistent with_removeFromParent/_addToChildrenList), the test assertion needs updating instead — but please pick one.🛠️ Suggested fix (single bulk event, matches the test)
clearChildren(): void { const children = this._children; + if (children.length === 0) return; + // Dispatch `Child` to the old parent before `_processInActive` (which unregisters + // UI listeners via `cleanRootCanvas`), so subscribers such as UICanvas can react + // to the hierarchy change while still attached. A single bulk event represents + // the entire clear operation. + this._dispatchModify(EntityModifyFlags.Child, this); for (let i = children.length - 1; i >= 0; i--) { const child = children[i]; child._parent = null; child._siblingIndex = -1; - // Dispatch `Child` to the old parent before `_processInActive` (which unregisters - // UI listeners via `cleanRootCanvas`), so subscribers such as UICanvas can react - // to the hierarchy change while still attached. - this._dispatchModify(EntityModifyFlags.Child, this); let activeChangeFlag = ActiveChangeFlag.None; child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy); child._isActiveInScene && (activeChangeFlag |= ActiveChangeFlag.Scene); activeChangeFlag && child._processInActive(activeChangeFlag); Entity._traverseSetOwnerScene(child, null); // Must after child._processInActive(). child._setParentChange(); } children.length = 0; }🤖 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/Entity.ts` around lines 399 - 420, In clearChildren(), move the call to this._dispatchModify(EntityModifyFlags.Child, this) out of the per-child loop so the parent emits a single bulk Child modify event for the entire clear operation; preserve the existing child cleanup sequence (clearing _parent/_siblingIndex, computing activeChangeFlag and calling child._processInActive(activeChangeFlag), Entity._traverseSetOwnerScene(child, null), and child._setParentChange()) for each child, then after the loop (before or after children.length = 0) invoke this._dispatchModify(EntityModifyFlags.Child, this) once to match the test and documented behavior.tests/src/core/Entity.test.ts (1)
419-425:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStale assertion: lonely entity no longer throws on
siblingIndexset.This test predates the new orphan-entity behavior in
Entity.siblingIndex(theLogger.warnpath added in this PR).entityXhas no parent and is not a root, so the setter now warns instead of throwing —expect(lonelyBadFn).to.throw()will fail.Either assert on the warning (spy
Logger.warn) or remove thethrowexpectation.🛠️ Suggested fix
- // thorw error when set lonely entity - const entityX = new Entity(engine, "entityX"); - var lonelyBadFn = function () { - entityX.siblingIndex = 1; - }; - expect(lonelyBadFn).to.throw(); + // Lonely entity should warn (not throw) when siblingIndex is set + const entityX = new Entity(engine, "entityX"); + const warnSpy = vi.spyOn(Logger, "warn").mockImplementation(() => {}); + expect(() => { entityX.siblingIndex = 1; }).not.toThrow(); + expect(warnSpy).toHaveBeenCalled(); + warnSpy.mockRestore();Note: this also requires importing
Loggerfrom@galacean/engine-coreat the top of the file.🤖 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/Entity.test.ts` around lines 419 - 425, The test currently expects Entity.siblingIndex setter to throw for an orphan entity (entityX), but the setter now warns instead; update the test to spy on Logger.warn (import Logger from '@galacean/engine-core' at the top), set up a spy around Logger.warn, call the setter (e.g., entityX.siblingIndex = 1), assert that Logger.warn was called with an appropriate message, and restore the spy; alternatively remove the expect(lonelyBadFn).to.throw() assertion and replace it with the Logger.warn assertion referencing entityX and the siblingIndex setter.
🤖 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/Entity.ts`:
- Around line 214-224: The setter siblingIndex has an invalid if/else/else
chain; update the second branch to be an else if that checks for this._parent so
orphans fall through to the Logger.warn path: keep the this._isRoot branch that
calls this._setSiblingIndex(this._scene._rootEntities, value), change the middle
branch to else if (this._parent) which calls
this._setSiblingIndex(parent._children, value) and
parent._dispatchModify(EntityModifyFlags.Child, parent), and let the final else
call Logger.warn(`The entity ${this.name} is not in the hierarchy`).
In `@tests/src/core/Entity.test.ts`:
- Around line 332-353: Test expects Entity.clearChildren() to dispatch a single
EntityModifyFlags.Child event for the whole operation, but current
implementation emits Child inside the per-child loop; update the
Entity.clearChildren() method to accumulate removals and call the
modify/dispatch for EntityModifyFlags.Child exactly once after all children are
detached (while still emitting the Parent modify event per detached child and
resetting siblingIndex to -1 for each child); locate the dispatch call(s) in
Entity.clearChildren() and move/condense them so only one Child event is emitted
for the overall clear.
---
Outside diff comments:
In `@packages/core/src/Entity.ts`:
- Around line 399-420: In clearChildren(), move the call to
this._dispatchModify(EntityModifyFlags.Child, this) out of the per-child loop so
the parent emits a single bulk Child modify event for the entire clear
operation; preserve the existing child cleanup sequence (clearing
_parent/_siblingIndex, computing activeChangeFlag and calling
child._processInActive(activeChangeFlag), Entity._traverseSetOwnerScene(child,
null), and child._setParentChange()) for each child, then after the loop (before
or after children.length = 0) invoke
this._dispatchModify(EntityModifyFlags.Child, this) once to match the test and
documented behavior.
In `@tests/src/core/Entity.test.ts`:
- Around line 419-425: The test currently expects Entity.siblingIndex setter to
throw for an orphan entity (entityX), but the setter now warns instead; update
the test to spy on Logger.warn (import Logger from '@galacean/engine-core' at
the top), set up a spy around Logger.warn, call the setter (e.g.,
entityX.siblingIndex = 1), assert that Logger.warn was called with an
appropriate message, and restore the spy; alternatively remove the
expect(lonelyBadFn).to.throw() assertion and replace it with the Logger.warn
assertion referencing entityX and the siblingIndex setter.
🪄 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: 8edd7d34-fe19-44e8-8217-73c91be6bae2
📒 Files selected for processing (3)
packages/core/src/Entity.tspackages/core/src/SceneManager.tstests/src/core/Entity.test.ts
| set siblingIndex(value: number) { | ||
| if (this._siblingIndex === -1) { | ||
| throw `The entity ${this.name} is not in the hierarchy`; | ||
| } | ||
|
|
||
| if (this._isRoot) { | ||
| this._setSiblingIndex(this._scene._rootEntities, value); | ||
| } else { | ||
| const parent = this._parent; | ||
| this._setSiblingIndex(parent._children, value); | ||
| parent._dispatchModify(EntityModifyFlags.Child, parent); | ||
| } else { | ||
| Logger.warn(`The entity ${this.name} is not in the hierarchy`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: invalid if/else/else chain — blocks all CI builds.
The block goes if (this._isRoot) { … } else { … } else { … }, which is not valid TypeScript and breaks every CI job (rollup/swc, ESLint, Biome) at line 221. Per the PR description, the second branch should be else if (this._parent) so orphan entities fall through to the Logger.warn path.
🛠️ Proposed fix
set siblingIndex(value: number) {
if (this._isRoot) {
this._setSiblingIndex(this._scene._rootEntities, value);
- } else {
+ } else if (this._parent) {
const parent = this._parent;
this._setSiblingIndex(parent._children, value);
parent._dispatchModify(EntityModifyFlags.Child, parent);
} else {
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}📝 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.
| set siblingIndex(value: number) { | |
| if (this._siblingIndex === -1) { | |
| throw `The entity ${this.name} is not in the hierarchy`; | |
| } | |
| if (this._isRoot) { | |
| this._setSiblingIndex(this._scene._rootEntities, value); | |
| } else { | |
| const parent = this._parent; | |
| this._setSiblingIndex(parent._children, value); | |
| parent._dispatchModify(EntityModifyFlags.Child, parent); | |
| } else { | |
| Logger.warn(`The entity ${this.name} is not in the hierarchy`); | |
| } | |
| } | |
| set siblingIndex(value: number) { | |
| if (this._isRoot) { | |
| this._setSiblingIndex(this._scene._rootEntities, value); | |
| } else if (this._parent) { | |
| const parent = this._parent; | |
| this._setSiblingIndex(parent._children, value); | |
| parent._dispatchModify(EntityModifyFlags.Child, parent); | |
| } else { | |
| Logger.warn(`The entity ${this.name} is not in the hierarchy`); | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.4.14)
[error] 221-221: Expected a statement but instead found 'else'.
(parse)
🪛 GitHub Actions: CI / 0_e2e (22.x, 2_4).txt
[error] 221-221: Rollup build failed via (plugin swc) with syntax error: "Expression expected" at line 221 (} else {).
🪛 GitHub Actions: CI / 1_build (22.x, ubuntu-latest).txt
[error] 221-221: Build failed during Rollup (plugin swc) with syntax error: "Expression expected" at "} else {".
🪛 GitHub Actions: CI / 2_e2e (22.x, 3_4).txt
[error] 221-223: Build failed in rollup (plugin swc): Syntax Error: "Expression expected" at the closing brace before else.
🪛 GitHub Actions: CI / 3_e2e (22.x, 1_4).txt
[error] 221-221: Build failed in rollup (plugin swc): Syntax Error: 'Expression expected' near '}' / start of 'else' block. See swc error at /Users/runner/work/engine/engine/packages/core/src/Entity.ts:221:1.
🪛 GitHub Actions: CI / 5_lint.txt
[error] 221-221: ESLint parsing error: Declaration or statement expected (Parsing error: Declaration or statement expected).
🪛 GitHub Actions: CI / 6_codecov.txt
[error] 221-221: Build failed during Rollup (plugin swc). Syntax error: Expression expected at line 221 near 'else'.
🪛 GitHub Actions: CI / 7_build (22.x, macos-latest).txt
[error] 221-221: rollup (plugin swc) build failed with Syntax Error: "Expression expected" at } else {
🪛 GitHub Actions: CI / 8_e2e (22.x, 4_4).txt
[error] 221-221: Build failed in rollup (plugin swc): Syntax Error — "Expression expected" at line 221 near "} else {".
🪛 GitHub Actions: CI / build (22.x, macos-latest)
[error] 221-221: Build failed in rollup (plugin swc): Syntax Error: "Expression expected" at line 221 near "} else {".
🪛 GitHub Actions: CI / build (22.x, ubuntu-latest)
[error] 221-221: Rollup (plugin swc) failed with syntax error: "Expression expected" at Entity.ts:221:1 near } else {.
🪛 GitHub Actions: CI / codecov
[error] 221-221: Build failed during rollup compilation with (plugin swc) syntax error: "Expression expected" at the } else { line. Error reported as: "(plugin swc) Error: Expression expected".
🪛 GitHub Actions: CI / e2e (22.x, 1_4)
[error] 221-224: Build failed during Rollup (plugin swc): "Expression expected" Syntax Error at "/Users/runner/work/engine/engine/packages/core/src/Entity.ts:221:1" near "+ } else {"
🪛 GitHub Actions: CI / e2e (22.x, 2_4)
[error] 221-221: Build failed during rollup with plugin swc: Syntax Error — "Expression expected" at line 221 near "} else {".
🪛 GitHub Actions: CI / e2e (22.x, 3_4)
[error] 221-221: Build failed in rollup (plugin swc): 'Expression expected' syntax error at Entity.ts line 221 near '} else {'.
🪛 GitHub Actions: CI / e2e (22.x, 4_4)
[error] 221-223: Build failed during Rollup (plugin swc). swc syntax error in /Users/runner/work/engine/engine/packages/core/src/Entity.ts:221:1: "Expression expected" near "} else {".
🪛 GitHub Actions: CI / lint
[error] 221-221: ESLint parsing error: Declaration or statement expected
🤖 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/Entity.ts` around lines 214 - 224, The setter siblingIndex
has an invalid if/else/else chain; update the second branch to be an else if
that checks for this._parent so orphans fall through to the Logger.warn path:
keep the this._isRoot branch that calls
this._setSiblingIndex(this._scene._rootEntities, value), change the middle
branch to else if (this._parent) which calls
this._setSiblingIndex(parent._children, value) and
parent._dispatchModify(EntityModifyFlags.Child, parent), and let the final else
call Logger.warn(`The entity ${this.name} is not in the hierarchy`).
| const parentModifyCount = [0, 0, 0]; | ||
| const childModifyCount = [0, 0, 0]; | ||
| const child2ModifyCount = [0, 0, 0]; | ||
| // @ts-ignore | ||
| parent._registerModifyListener((flag: EntityModifyFlags) => ++parentModifyCount[flag]); | ||
| // @ts-ignore | ||
| child._registerModifyListener((flag: EntityModifyFlags) => ++childModifyCount[flag]); | ||
| // @ts-ignore | ||
| child2._registerModifyListener((flag: EntityModifyFlags) => ++child2ModifyCount[flag]); | ||
|
|
||
| parent.clearChildren(); | ||
| expect(parent.children.length).eq(0); | ||
|
|
||
| // Parent should receive a single `Child` modify event for the whole clear so | ||
| // listeners (e.g. UICanvas) can invalidate their cached state. | ||
| expect(parentModifyCount[EntityModifyFlags.Child]).eq(1); | ||
| // Each detached child should receive a `Parent` modify event. | ||
| expect(childModifyCount[EntityModifyFlags.Parent]).eq(1); | ||
| expect(child2ModifyCount[EntityModifyFlags.Parent]).eq(1); | ||
| // Sibling index must be reset so the entity is treated as lonely afterwards. | ||
| expect(child.siblingIndex).eq(-1); | ||
| expect(child2.siblingIndex).eq(-1); |
There was a problem hiding this comment.
Heads up: parentModifyCount[EntityModifyFlags.Child]).eq(1) only holds if Entity.clearChildren() dispatches once for the whole operation.
The current implementation in packages/core/src/Entity.ts dispatches the Child event inside the per-child loop, so this assertion will see 2. See the corresponding comment on Entity.clearChildren(); aligning the two is required before this test will pass.
🤖 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/Entity.test.ts` around lines 332 - 353, Test expects
Entity.clearChildren() to dispatch a single EntityModifyFlags.Child event for
the whole operation, but current implementation emits Child inside the per-child
loop; update the Entity.clearChildren() method to accumulate removals and call
the modify/dispatch for EntityModifyFlags.Child exactly once after all children
are detached (while still emitting the Parent modify event per detached child
and resetting siblingIndex to -1 for each child); locate the dispatch call(s) in
Entity.clearChildren() and move/condense them so only one Child event is emitted
for the overall clear.
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查(2026-06-14,HEAD 44e72578)
本轮直接读取 PR HEAD 文件(非 diff 重建)逐一核对。代码自 2026-04-22 最后一次推送以来未变动,两个老 P0 仍在;另发现一个被遗漏的 P0(测试断言与新行为冲突)。
已关闭问题清单
| 问题 | 状态 |
|---|---|
clearChildren 缺少 child._siblingIndex = -1 重置 |
✅ 已修复 |
clearChildren 缺少 child._setParentChange() |
✅ 已修复 |
SceneManager.loadScene destroy 遍历时修改正在遍历的数组 |
✅ 已修复(getLoopArray() 返回稳定快照,destroy 期间从底层 _array 移除不再破坏迭代) |
siblingIndex setter 无父节点时应 warn 而非 throw |
✅ 意图已实现(但实现有语法错误,见下) |
总结
两个 fix 方向都对、且属同一领域(Entity 层级),PR 范围内聚,无需拆分。SceneManager 的 getArray() → getLoopArray() 是正确的并发迭代修复。但 siblingIndex 和 clearChildren 两处实现仍有阻塞性错误,且新增测试与实现/旧测试自相矛盾,CI 必然全红。
问题
[P0] Entity.ts:214-222 — siblingIndex setter 存在 if/else/else 语法错误,无法编译
直接读取 HEAD 文件确认(非 diff 假象):
set siblingIndex(value: number) {
if (this._isRoot) {
this._setSiblingIndex(this._scene._rootEntities, value);
} else {
const parent = this._parent;
this._setSiblingIndex(parent._children, value);
parent._dispatchModify(EntityModifyFlags.Child, parent);
} else { // ← 第二个 else 挂在同一个 if 上,非法 TypeScript
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}三路分支应为 if / else if / else:
set siblingIndex(value: number) {
if (this._isRoot) {
this._setSiblingIndex(this._scene._rootEntities, value);
} else if (this._parent) {
this._setSiblingIndex(this._parent._children, value);
this._parent._dispatchModify(EntityModifyFlags.Child, this._parent);
} else {
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}[P0] Entity.ts:407 — clearChildren 在循环内每个 child 都 dispatch Child,与本 PR 新增测试断言 eq(1) 矛盾
this._dispatchModify(EntityModifyFlags.Child, this) 在 for 循环体内,N 个子节点触发 N 次。但本 PR 新增测试(fixture 有 2 个子节点)断言:
// Parent should receive a single `Child` modify event for the whole clear
expect(parentModifyCount[EntityModifyFlags.Child]).eq(1);实际会得到 2,测试必然失败。约定也支持"批量发一次":_removeFromParent/_addToChildrenList 都是单次操作发一次 Child,clearChildren 作为批量操作整体发一次即可。
注释声称 dispatch 必须在 _processInActive 之前(让 UICanvas 监听器在还挂着时收到事件)——这个约束用"循环前发一次"同样满足,因为循环前任何 child 都还没 _processInActive:
clearChildren(): void {
const children = this._children;
if (children.length === 0) return;
// 循环前发一次:此时所有 child 仍挂在父上,UICanvas 监听器可正常响应
this._dispatchModify(EntityModifyFlags.Child, this);
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
child._parent = null;
child._siblingIndex = -1;
let activeChangeFlag = ActiveChangeFlag.None;
child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy);
child._isActiveInScene && (activeChangeFlag |= ActiveChangeFlag.Scene);
activeChangeFlag && child._processInActive(activeChangeFlag);
Entity._traverseSetOwnerScene(child, null); // Must after child._processInActive().
child._setParentChange();
}
children.length = 0;
}[P0] Entity.test.ts:419-424 — 旧测试仍断言孤立 entity set siblingIndex 会 throw,与本 PR 的新行为冲突
本 PR 把孤立 entity 的 setter 从 throw 改为 Logger.warn,但 sibling index 测试里的旧断言没同步更新:
// thorw error when set lonely entity
const entityX = new Entity(engine, "entityX");
var lonelyBadFn = function () {
entityX.siblingIndex = 1;
};
expect(lonelyBadFn).to.throw(); // ← 新行为是 warn 不再 throw,此断言会失败即使修好上面两个 P0,这条断言仍会让测试红。请改为断言不抛 + spy Logger.warn 被调用,或删除该 throw 断言。(CodeRabbit 也已指出此处。)顺带修一下注释的拼写 thorw → throw、whenless → when less。
简化建议
无新增。三个 P0 修好后即可合入;范围内聚、改动量合理。
Summary
Two small
Entitybugs found during prefab/scene editing:clearChildren缺失边界处理 — fix: ensure children-related state is cleaned up correctly whenclearChildrenis invoked. Adds regression test inEntity.test.ts.siblingIndexsetter 在无父节点时报错 —_setSiblingIndexpath assumed_parentexists when_isRootis false; the new guardelse if (this._parent)skips the operation rather than crashing.Test Plan
Entity.test.tscovers the clearChildren regressionsiblingIndexset without errorSummary by CodeRabbit
Release Notes
Bug Fixes
Tests