Fix entity find path #2541
Fix entity find path #2541
Conversation
|
Warning Rate limit exceeded@GuoLei1990 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes refactor the entity search functionality by enhancing the recursive lookup mechanism. The Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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
🧹 Nitpick comments (1)
packages/core/src/Scene.ts (1)
421-423: Align empty path handling infindEntityByPathwithEntity.findByPathWhen the input path is empty,
findEntityByPathreturnsnull, whereasEntity.findByPathreturns the current entity. For consistency and intuitive API behavior, consider returningnullor the root entity consistently in both methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Entity.ts(2 hunks)packages/core/src/Scene.ts(1 hunks)
🔇 Additional comments (3)
packages/core/src/Entity.ts (2)
406-408: EnsurefindByPathcorrectly handles empty pathsThe
findByPathmethod now returnsthiswhen the input path is empty, which correctly allows an empty path to refer to the current entity.
37-39: Review the recursive call logic in_findChildByNameThe recursive call at lines 37-39 may lead to infinite recursion or incorrect traversal due to the adjustment of
pathIndexandchildIndex. Verify that decrementingpathIndexand incrementingsiblingIndexin the recursive call correctly backtracks to search sibling entities at higher levels.Run the following script to test the method with various entity hierarchies and paths:
packages/core/src/Scene.ts (1)
426-429: Confirm the usage of_findChildByNameinfindEntityByPathThe method now uses
Entity._findChildByNamefor searching entities by path. Ensure that this change integrates well with the overall entity hierarchy traversal and that access to the_findChildByNamemethod (which is marked as internal) is appropriate.Review the access modifiers and consider whether
_findChildByNameshould remain internal or be exposed more appropriately for use inScene.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2541 +/- ##
==========================================
+ Coverage 68.47% 68.52% +0.04%
==========================================
Files 961 961
Lines 100221 100233 +12
Branches 8559 8576 +17
==========================================
+ Hits 68630 68685 +55
+ Misses 31331 31288 -43
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/src/core/Entity.test.ts (2)
297-309: Enhance test coverage with additional scenarios.While the current test cases verify basic functionality, consider adding tests for:
- Case sensitivity in path segments
- Invalid paths (e.g., non-existent segments)
- Path segments with special characters
Example test cases to add:
expect(parent.findByPath("/child")).eq(child); expect(parent.findByPath("child/grandson")).eq(grandson2); +// Case sensitivity +expect(parent.findByPath("CHILD/grandson")).eq(null); +// Invalid paths +expect(parent.findByPath("child/nonexistent")).eq(null); +expect(parent.findByPath("child//grandson")).eq(null); +// Special characters +expect(parent.findByPath("child/../child/grandson")).eq(null);🧰 Tools
🪛 ESLint
[error] 305-306: Delete
⏎(prettier/prettier)
23-24: Fix formatting: Remove extra empty lines.Remove the extra empty lines to comply with the project's formatting rules.
Also applies to: 30-33, 305-306
🧰 Tools
🪛 ESLint
[error] 23-24: Delete
⏎(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Scene.ts(1 hunks)tests/src/core/Entity.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Scene.ts
🧰 Additional context used
🪛 ESLint
tests/src/core/Entity.test.ts
[error] 23-24: Delete ⏎
(prettier/prettier)
[error] 30-33: Delete ⏎⏎⏎
(prettier/prettier)
[error] 305-306: Delete ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
tests/src/core/Entity.test.ts (1)
24-26: LGTM! Good test coverage for edge cases.The new test cases effectively verify the behavior of
findEntityByPathfor empty paths and root paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/src/core/Entity.test.ts (3)
17-24: Consider using unique names for test entities.Having multiple entities named "parent" (
parentX,parent,parentY) could lead to ambiguous test results and make debugging more difficult.Consider using more descriptive names that reflect their role in the test:
-const parentX = new Entity(engine, "parent"); -const parent = new Entity(engine, "parent"); -const parentY = new Entity(engine, "parent"); +const firstParent = new Entity(engine, "first_parent"); +const middleParent = new Entity(engine, "middle_parent"); +const lastParent = new Entity(engine, "last_parent");
309-318: Improve test clarity with unique entity names and descriptive comments.Having multiple entities named "child" makes the test harder to understand. Consider using unique names that describe the entity's role in the test hierarchy.
-const child2 = new Entity(engine, "child"); -const child3 = new Entity(engine, "child"); +const secondChild = new Entity(engine, "second_child"); +const thirdChild = new Entity(engine, "third_child"); -const grandson = new Entity(engine, "grandsonX"); -const grandson2 = new Entity(engine, "grandson"); +const firstGrandson = new Entity(engine, "first_grandson"); +const secondGrandson = new Entity(engine, "second_grandson"); // Add descriptive comments // Test hierarchy: // - parent // - child // - firstGrandson // - secondChild // - secondGrandson // - thirdChild
33-33: Fix formatting issue.Remove the extra whitespace on this empty line to comply with the project's formatting rules.
- +🧰 Tools
🪛 ESLint
[error] 33-34: Delete
⏎·····(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/core/Entity.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/src/core/Entity.test.ts
[error] 45-45: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🪛 ESLint
tests/src/core/Entity.test.ts
[error] 33-34: Delete ⏎·····
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
Please check if the PR fulfills these requirements
Summary by CodeRabbit