Fix Entity._isRoot error and Entity modify error
#2598
Conversation
WalkthroughThe pull request refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant E as Entity
participant P as Parent
participant S as Scene
E->>E: Set siblingIndex
alt Entity is root
E->>S: Update _rootEntities list
else Entity is non-root
E->>P: Retrieve parent and update _children list
P->>P: Invoke _dispatchModify(EntityModifyFlags.Child)
end
note over E,P: Similar flow when removing from a parent
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)
🔇 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 #2598 +/- ##
==========================================
+ Coverage 68.98% 68.99% +0.01%
==========================================
Files 961 961
Lines 100339 100345 +6
Branches 8681 8687 +6
==========================================
+ Hits 69215 69234 +19
+ Misses 30864 30851 -13
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)
tests/src/core/Entity.test.ts (2)
404-417: Added test for _isRoot property behaviorThis test ensures that an entity's root status is correctly updated when:
- It's created as a root entity
- It's added as a child to another entity (no longer a root)
- It's added back to the scene as a root entity
This directly tests the fix for the
Entity._isRooterror mentioned in the PR title.Consider avoiding direct access to internal properties with
@ts-ignore. Instead, you could test the behavior indirectly by checking if the entity is inscene._rootEntitiesor has a parent:- // @ts-ignore - expect(child._isRoot).eq(true); + expect(child.parent).eq(null); + expect(scene.getRootEntities().includes(child)).eq(true);
531-588: Added test for entity modification notificationsThis test verifies the fix for the Entity modify error by ensuring:
- Parent entities receive Child modification notifications when children are added/removed
- Parent entities receive Child modification notifications when a child's sibling index changes
- Both old and new parents receive appropriate notifications when a child moves between parents
This directly tests the modifications to the
_dispatchModifybehavior in the Entity class.Similar to the isRoot test, consider avoiding direct access to internal methods with
@ts-ignore. You could potentially expose a public test method or test the behavior indirectly by observing the effects of the modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Entity.ts(3 hunks)tests/src/core/Entity.test.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
tests/src/core/Entity.test.ts (2)
packages/core/src/Entity.ts (4)
parent(201-203)parent(205-207)scene(227-229)Entity(21-834)packages/core/src/Scene.ts (1)
Scene(27-542)
packages/core/src/Entity.ts (1)
packages/core/src/index.ts (1)
EntityModifyFlags(42-42)
🪛 GitHub Check: codecov/patch
packages/core/src/Entity.ts
[warning] 244-244: packages/core/src/Entity.ts#L244
Added line #L244 was not covered by tests
[warning] 630-630: packages/core/src/Entity.ts#L630
Added line #L630 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (5)
packages/core/src/Entity.ts (3)
695-695: Fixed _isRoot flag setting when parent is assignedExplicitly setting
_isRoot = falsewhen a parent is assigned ensures that an entity can't be both a root entity and have a parent simultaneously, which resolves the Entity._isRoot error mentioned in the PR title.
243-249:❓ Verification inconclusive
Enhanced siblingIndex setter logic to correctly handle root vs non-root entities
This change properly differentiates between root and non-root entities when setting the sibling index. For root entities, it correctly uses the scene's root entities collection, while for non-root entities it uses the parent's children collection and notifies the parent of the modification.
🏁 Script executed:
#!/bin/bash # Verify test coverage for the siblingIndex setter ast-grep --pattern $'set siblingIndex(value: number) { $$$ if (this._isRoot) { $$$ } else { $$$ } $$$ }'Length of output: 123
Below is an updated review comment reflecting our current findings. Note that the automated script using ast‑grep did not yield output, so please verify manually that the code snippet below indeed starts at line 243 in
packages/core/src/Entity.ts:Enhanced siblingIndex Setter – Verify Manual Test Coverage
The code now properly distinguishes between root and non‑root entities:
- For root entities, it updates via the scene’s
_rootEntitiescollection.- For non‑root entities, it updates using the parent’s
_childrencollection and notifies the parent via_dispatchModify.Since our automated search via ast‑grep produced no output, please confirm the snippet manually. You can use the following command to display lines 240–260:
sed -n '240,260p' packages/core/src/Entity.tsIf the snippet matches the expected logic, the changes appear correctly implemented.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 244-244: packages/core/src/Entity.ts#L244
Added line #L244 was not covered by tests
630-630:✅ Verification successful
Improved modification notification in _removeFromParent
Changed from
this._dispatchModifytooldParent._dispatchModify, ensuring that the parent entity dispatches the modification notification when a child is removed, maintaining consistency with the overall entity notification model.
🏁 Script executed:
#!/bin/bash # Verify test coverage for the _removeFromParent method ast-grep --pattern $'_removeFromParent(): void { $$$ oldParent._dispatchModify(EntityModifyFlags.Child, oldParent); $$$ }'Length of output: 129
Action Required: Verify _removeFromParent Notification Change
The change to call
oldParent._dispatchModify(EntityModifyFlags.Child, oldParent)(instead ofthis._dispatchModify) aims to ensure the parent entity dispatches the modification notification correctly. However, our initial ast-grep check produced no output, so please run the above rg command and manually verify that the intended change is present and correct in the_removeFromParentmethod.
- File under review:
packages/core/src/Entity.ts- Line of interest: 630 (or the corresponding block in
_removeFromParent)- Code snippet to confirm:
oldParent._dispatchModify(EntityModifyFlags.Child, oldParent);Once the output confirms that this modification exists as expected within the
_removeFromParentmethod, you can approve the changes.
🏁 Script executed:
#!/bin/bash # Using rg to verify the _removeFromParent method usage and ensure that # the change to use oldParent._dispatchModify(EntityModifyFlags.Child, oldParent) is present in the file. rg -n '_removeFromParent' packages/core/src/Entity.tsLength of output: 245
Action Required: Manual Verification of Notification Change
The previous verification using
rg -n '_removeFromParent'did not confirm the presence of the updated notification call. Please run the above script, then manually verify that within the_removeFromParentmethod the modified call:oldParent._dispatchModify(EntityModifyFlags.Child, oldParent);is indeed present. This change is intended to ensure the proper dispatch to the parent entity rather than the child entity. If the updated call is present as expected, you can approve the changes.
🏁 Script executed:
#!/bin/bash # Using rg to search for the modification notification in the _removeFromParent method. rg -n 'oldParent\._dispatchModify' packages/core/src/Entity.tsLength of output: 259
Final Verification: Modification Notification Updated Correctly
The change where the parent entity dispatches the modification notification (i.e., using
oldParent._dispatchModify(EntityModifyFlags.Child, oldParent);instead ofthis._dispatchModify) in the_removeFromParentmethod has been verified inpackages/core/src/Entity.tsat line 630. No additional changes are required.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 630-630: packages/core/src/Entity.ts#L630
Added line #L630 was not covered by teststests/src/core/Entity.test.ts (2)
1-1: Added EntityModifyFlags importCorrectly added the EntityModifyFlags import required for the new tests.
419-514: Added comprehensive test for entity activation statesThis test thoroughly verifies entity activation behavior when:
- Adding/removing entities from different parents
- Moving entities between scenes
- Checking the event counts for enable/disable/enableInScene/disableInScene
The test ensures that activation state changes trigger the appropriate callbacks and updates internal state 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