Fix isRoot was not set correctly when set entity.parent
#2597
Conversation
WalkthroughThe changes introduce a new static method Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)tests/src/core/Entity.test.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2597 +/- ##
==========================================
- Coverage 68.98% 68.91% -0.07%
==========================================
Files 961 961
Lines 100339 100272 -67
Branches 8681 8662 -19
==========================================
- Hits 69215 69101 -114
- Misses 30864 30911 +47
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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: 0
🧹 Nitpick comments (2)
packages/core/src/Entity.ts (2)
22-124: Consider splitting_moveEntityTointo smaller helper methods.
This method handles multiple operations—removal from old parent/scene, updating_isRoot, toggling active states, etc. Breaking it down into smaller functions could improve readability and maintainability. Also ensure thorough test coverage for all condition branches (e.g. old parent vs. new parent, old scene vs. new scene).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-29: packages/core/src/Entity.ts#L29
Added line #L29 was not covered by tests
[warning] 31-32: packages/core/src/Entity.ts#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 41-42: packages/core/src/Entity.ts#L41-L42
Added lines #L41 - L42 were not covered by tests
[warning] 55-56: packages/core/src/Entity.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 66-70: packages/core/src/Entity.ts#L66-L70
Added lines #L66 - L70 were not covered by tests
[warning] 77-123: packages/core/src/Entity.ts#L77-L123
Added lines #L77 - L123 were not covered by tests
532-538: Consider reusing_moveEntityToinclearChildren.
Here, the code manually detaches and inactivates each child. For consistency,_moveEntityTo(child, null, null)could replicate the same steps in a standardized manner unless there's a specific need to handle it directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Entity.ts(8 hunks)packages/core/src/Scene.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/core/src/Scene.ts (1)
packages/core/src/Entity.ts (1)
Entity(21-807)
🪛 GitHub Check: codecov/patch
packages/core/src/Scene.ts
[warning] 321-321: packages/core/src/Scene.ts#L321
Added line #L321 was not covered by tests
packages/core/src/Entity.ts
[warning] 29-29: packages/core/src/Entity.ts#L29
Added line #L29 was not covered by tests
[warning] 31-32: packages/core/src/Entity.ts#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 41-42: packages/core/src/Entity.ts#L41-L42
Added lines #L41 - L42 were not covered by tests
[warning] 55-56: packages/core/src/Entity.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 66-70: packages/core/src/Entity.ts#L66-L70
Added lines #L66 - L70 were not covered by tests
[warning] 77-123: packages/core/src/Entity.ts#L77-L123
Added lines #L77 - L123 were not covered by tests
[warning] 451-451: packages/core/src/Entity.ts#L451
Added line #L451 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (10)
packages/core/src/Scene.ts (4)
302-302: Add test coverage forcreateRootEntityusage.
The new call toEntity._moveEntityTo(entity, null, this)is not covered by tests per static analysis. Please confirm or consider adding unit tests ensuring thatisRootis set correctly and that the entity’s state is properly initialized.
321-321: Verify index bounds inaddRootEntity(index: number, entity: Entity).
_moveEntityTo()throws an error on out-of-bounds indices, which is correct. Consider adding a test that deliberately uses an invalid index to ensure proper error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 321-321: packages/core/src/Scene.ts#L321
Added line #L321 was not covered by tests
323-323: Clean refactor for simple addition of root entities.
This direct_moveEntityTocall is consistent with the overall refactoring. No immediate concerns noted.
333-333: Consider coverage for entity removal from root.
Calling_moveEntityTo(entity)with no parent/scene detaches the entity from the scene. Please verify through tests that the entity no longer appears in_rootEntitiesand thatisRootis reset tofalse.packages/core/src/Entity.ts (6)
310-310: Parent setter refactor confirmation.
Replacing internal parent logic with_moveEntityTounifies entity movement. Confirm or add tests covering edge cases such as settingparenttonull, reassigning the same parent, and cross-parent transfers.
451-454: RefactoredaddChildimplementation.
This approach centralizes child addition logic under_moveEntityTo. It’s simpler to maintain, provided that tests confirm the new child’ssiblingIndexand_parentreferences are set correctly.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 451-451: packages/core/src/Entity.ts#L451
Added line #L451 was not covered by tests
462-464: Validation inremoveChild.
Early-returning if the child’s_parentis different prevents unintended moves. The subsequent_moveEntityTo(child)detaches the child as expected. Please verify that removal flows are tested, including removing children from different hierarchies or lacking a valid parent link.
516-521: Unified flow increateChild.
Calling_moveEntityTo(child, this, this._scene)keeps all logic consistent. Make sure there are tests confirming that newly created children have the correctparent,scene, and active states right after creation.
638-639: Detach entity upon destroy.
Invoking_moveEntityTo(this)before settingthis.isActive = false;neatly removes the entity from its parent or scene. Please confirm coverage so that tests verify the entity is fully detached and deactivated in the destruction workflow.
695-695: Refined_onParentChangehook.
This ensures theTransformupdates appropriately whenever the parent changes. Confirm there’s adequate testing around transform updates in nested entity hierarchies.
|
原先把逻辑收口在
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/Entity.ts (1)
22-125:❓ Verification inconclusive
Well-implemented centralized entity repositioning logic.
This new
_moveEntityTostatic method provides a centralized place for handling entity parent-scene relationships, which simplifies the code and ensures consistent behavior across different operations. It correctly handles the four scenarios mentioned in the PR comments:
- Becoming a child node of a free node (when
sceneis null andparentis not null)- Becoming a child node within a scene (when both
sceneandparentare not null)- Becoming a free node without a parent (when both
sceneandparentare null)- Becoming a root node (when
sceneis not null andparentis null)There's significant code duplication between the two main branches (when
oldScene !== scenevs whenoldScene === scene). Consider extracting common logic to reduce duplication and improve maintainability.static _moveEntityTo(entity: Entity, parent: Entity = null, scene: Scene = null, index?: number): void { const { _parent: oldParent, _scene: oldScene } = entity; - if (oldScene !== scene) { - if (oldParent) { - Entity._removeFromChildren(oldParent._children, entity); - oldParent._dispatchModify(EntityModifyFlags.Child, oldParent); - } else if (oldScene) { - Entity._removeFromChildren(oldScene._rootEntities, entity); - } - // ...rest of the first block - } else { - if (oldParent !== parent) { - if (oldParent) { - Entity._removeFromChildren(oldParent._children, entity); - oldParent._dispatchModify(EntityModifyFlags.Child, oldParent); - } - // ...rest of the second block - } else { - !!parent && index !== undefined && (entity.siblingIndex = index); - } - } + // First handle the case when nothing changes except the index + if (oldParent === parent && oldScene === scene) { + !!parent && index !== undefined && (entity.siblingIndex = index); + return; + } + + // Remove from old parent or scene + if (oldParent) { + Entity._removeFromChildren(oldParent._children, entity); + oldParent._dispatchModify(EntityModifyFlags.Child, oldParent); + } else if (oldScene) { + Entity._removeFromChildren(oldScene._rootEntities, entity); + } + + // Then handle the remaining logic for cross-scene or same-scene movement + // ...rest of the method using common functions for active state changes }Several branches in this method aren't covered by tests according to code coverage reports. Run the following script to identify the missing test coverage:
🏁 Script executed:
#!/bin/bash # Check which lines in _moveEntityTo aren't covered by tests # List uncovered lines in the method echo "Uncovered lines in _moveEntityTo method:" ast-grep --pattern '$$$static _moveEntityTo$$$' | xargs rg -n "warning" "packages/core/src/Entity.ts#L" --with-filenameLength of output: 278
Please verify that all code branches in
_moveEntityToare covered by tests and consider refactoring duplicated logic.The centralized entity repositioning logic is well implemented and correctly supports the four scenarios (free node child, scene child, free node without parent, and root node). However, there remains significant code duplication across the branches handling cross-scene versus same-scene changes. This redundancy not only affects maintainability but also makes it harder to be fully confident that all cases are exercised by tests.
Actionable points:
- Code duplication: Consider extracting the common logic (for example, the removal from the old parent/scene and processing of active state changes) into helper functions to reduce duplication.
- Test coverage: The automated script intended to identify uncovered lines did not produce conclusive output. Please manually verify that tests exercise all branches of this method, especially:
- The transition of an entity between different scenes.
- Parent changes within the same scene.
- Updates where only the sibling index is modified.
static _moveEntityTo(entity: Entity, parent: Entity = null, scene: Scene = null, index?: number): void { const { _parent: oldParent, _scene: oldScene } = entity; - if (oldScene !== scene) { - if (oldParent) { - Entity._removeFromChildren(oldParent._children, entity); - oldParent._dispatchModify(EntityModifyFlags.Child, oldParent); - } else if (oldScene) { - Entity._removeFromChildren(oldScene._rootEntities, entity); - } - // ...rest of the first block - } else { - if (oldParent !== parent) { - if (oldParent) { - Entity._removeFromChildren(oldParent._children, entity); - oldParent._dispatchModify(EntityModifyFlags.Child, oldParent); - } - // ...rest of the second block - } else { - !!parent && index !== undefined && (entity.siblingIndex = index); - } - } + // First, if only the sibling index changes, update it and return early. + if (oldParent === parent && oldScene === scene) { + !!parent && index !== undefined && (entity.siblingIndex = index); + return; + } + + // Remove from the old parent or scene + if (oldParent) { + Entity._removeFromChildren(oldParent._children, entity); + oldParent._dispatchModify(EntityModifyFlags.Child, oldParent); + } else if (oldScene) { + Entity._removeFromChildren(oldScene._rootEntities, entity); + } + + // Handle remaining logic for repositioning and active state updates... + // (consider extracting common routines here to reduce duplication) }Note: The automated check for uncovered lines did not yield clear output, so please ensure via manual verification or an updated testing strategy that each branch (especially those handling different activation states) is adequately covered.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-30: packages/core/src/Entity.ts#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-33: packages/core/src/Entity.ts#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 44-45: packages/core/src/Entity.ts#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 58-59: packages/core/src/Entity.ts#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 67-71: packages/core/src/Entity.ts#L67-L71
Added lines #L67 - L71 were not covered by tests
[warning] 78-124: packages/core/src/Entity.ts#L78-L124
Added lines #L78 - L124 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Entity.ts(9 hunks)tests/src/core/Entity.test.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/src/core/Entity.test.ts (2)
packages/core/src/Entity.ts (4)
scene(332-334)Entity(21-812)parent(306-308)parent(310-312)packages/core/src/Scene.ts (1)
Scene(27-491)
🪛 GitHub Check: codecov/patch
packages/core/src/Entity.ts
[warning] 29-30: packages/core/src/Entity.ts#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-33: packages/core/src/Entity.ts#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 44-45: packages/core/src/Entity.ts#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 58-59: packages/core/src/Entity.ts#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 67-71: packages/core/src/Entity.ts#L67-L71
Added lines #L67 - L71 were not covered by tests
[warning] 78-124: packages/core/src/Entity.ts#L78-L124
Added lines #L78 - L124 were not covered by tests
[warning] 348-348: packages/core/src/Entity.ts#L348
Added line #L348 was not covered by tests
[warning] 457-457: packages/core/src/Entity.ts#L457
Added line #L457 was not covered by tests
🔇 Additional comments (11)
packages/core/src/Entity.ts (8)
311-311: LGTM! Simplified parent setter uses the new centralized method.The parent setter now delegates to the
_moveEntityTomethod, which ensures_isRootis properly set based on the parent-scene relationship.
347-354: Improved sibling index handling.The code now properly considers whether the entity is a root entity (in a scene) or a child entity (has a parent) when setting the sibling index.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 348-348: packages/core/src/Entity.ts#L348
Added line #L348 was not covered by tests
455-460: Simplified addChild method using the centralized approach.The
addChildmethod now consistently uses_moveEntityToto handle the parent-child relationship, ensuring proper_isRootproperty setting.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 457-457: packages/core/src/Entity.ts#L457
Added line #L457 was not covered by tests
467-470: Added safety check for removeChild operation.Good defensive programming with the check
if (child._parent !== this) return;which prevents unnecessary operations when removing a child that doesn't belong to this entity.
526-526: Consistent usage of _moveEntityTo in createChild.The createChild method now uses the centralized
_moveEntityTomethod to ensure consistency in how entities are added to the hierarchy.
538-544: Improved clearChildren implementation with explicit active state handling.The updated implementation ensures proper handling of active state flags and scene owner during children clearing.
644-644: Simplified entity cleanup during destruction.Using
_moveEntityTowith no arguments effectively detaches the entity from its parent and scene, which is the correct behavior during destruction.
701-704: Simplified _onParentChange method.The method is now streamlined to just handle transform parent changes and dispatch modify events, since the
_isRootsetting is handled by_moveEntityTo.tests/src/core/Entity.test.ts (3)
469-524: Great test for entity modification flags.This test thoroughly verifies that entity modification events are properly dispatched when:
- A child is added to a parent
- A child's sibling index is changed
- A child is moved from one parent to another
It ensures the modification tracking system works correctly with the new
_moveEntityTomethod.
526-621: Comprehensive test for entity activation states.This test covers complex scenarios involving:
- Free entities (without scene or parent)
- Entities with parent but no scene
- Entities in a scene
- Entities moved between scenes
It verifies that enable/disable events are properly triggered, which is critical when changing an entity's parent or scene.
623-636: Direct test for the isRoot property handling.This test specifically validates the fix described in the PR - ensuring
_isRootis set correctly when:
- An entity is added as a root entity to a scene (
_isRoot = true)- An entity is set as a child of another entity (
_isRoot = false)- An entity is re-added as a root entity to a scene (
_isRoot = true)The test confirms that all the logic related to maintaining the
_isRootproperty is working correctly.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Refactor
Tests