Fixed the problem of modifying component.enable when dispatching UI events, resulting in error reporting
#2549
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes refactor the pointer event handling in the Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
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 (
|
cptbtptpbcptdtptp
requested review from
GuoLei1990,
singlecoder and
johanzhu
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2549 +/- ##
==========================================
+ Coverage 68.59% 68.92% +0.32%
==========================================
Files 961 961
Lines 100219 100266 +47
Branches 8598 8679 +81
==========================================
+ Hits 68749 69111 +362
+ Misses 31210 30895 -315
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. |
![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 (1)
packages/ui/src/input/UIPointerEventEmitter.ts (1)
21-24: Consider using more descriptive field names.
Although these temporary data structures_tempSet,_path,_tempArray0, and_tempArray1serve a clear internal purpose, they might be more readable if named according to their usage context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/input/UIPointerEventEmitter.ts(3 hunks)tests/src/ui/UIEvent.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/ui/src/input/UIPointerEventEmitter.ts
[warning] 105-106: packages/ui/src/input/UIPointerEventEmitter.ts#L105-L106
Added lines #L105 - L106 were not covered by tests
[warning] 154-157: packages/ui/src/input/UIPointerEventEmitter.ts#L154-L157
Added lines #L154 - L157 were not covered by tests
[warning] 159-163: packages/ui/src/input/UIPointerEventEmitter.ts#L159-L163
Added lines #L159 - L163 were not covered by tests
[warning] 166-166: packages/ui/src/input/UIPointerEventEmitter.ts#L166
Added line #L166 was not covered by tests
[warning] 170-170: packages/ui/src/input/UIPointerEventEmitter.ts#L170
Added line #L170 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (16)
packages/ui/src/input/UIPointerEventEmitter.ts (11)
26-28: Good separation for pointer state tracking.
These newly introduced instance arrays (_enteredPath,_pressedPath,_draggedPath) help clarify which entities are in each pointer state. This approach is more maintainable than a single shared structure.
103-106: Add a test case for drag events.
Lines 105-106 trigger_fireDragand then cleardraggedPath. However, static analysis indicates these lines aren’t covered by tests, suggesting no scenario exercises the “drag” logic.Would you like to add or expand an existing unit test to ensure these lines are validated?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-106: packages/ui/src/input/UIPointerEventEmitter.ts#L105-L106
Added lines #L105 - L106 were not covered by tests
111-120: Verify shared references in_pressedPathand_draggedPath.
Here,_pressedPathand_draggedPathare assigned the same entities as_enteredPath. Ensure that this is intentional and that triggering down or beginDrag events for both simultaneously won’t lead to unexpected side effects.
125-151: Click and drop logic looks consistent.
The processUp logic checks for common elements to fire click events and handles drop events. Overall, this flow is sound.
154-157: Add test coverage for pointer leave with entered entities.
These lines handle pointer leave events, but according to static analysis, they’re not tested. A scenario where the pointer leaves the UI while over an entity would confirm correct firing of exit events.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 154-157: packages/ui/src/input/UIPointerEventEmitter.ts#L154-L157
Added lines #L154 - L157 were not covered by tests
159-163: Cover pointer leave during drag scenario.
This block resets any dragged elements when the pointer leaves. Add a test scenario for a pointer leaving while dragging to ensure correct dispatch of endDrag events.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 159-163: packages/ui/src/input/UIPointerEventEmitter.ts#L159-L163
Added lines #L159 - L163 were not covered by tests
166-166: Resetting_pressedPath.
Clearing_pressedPathhere ensures state consistency when the pointer leaves, but static analysis indicates it’s unverified by tests. Please add coverage for this condition if possible.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 166-166: packages/ui/src/input/UIPointerEventEmitter.ts#L166
Added line #L166 was not covered by tests
170-170: Dispose method lacks coverage.
Clearing all pointer state arrays upon disposal is crucial for memory safety, but it’s untested. Consider adding or extending tests to confirm resource cleanup.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 170-170: packages/ui/src/input/UIPointerEventEmitter.ts#L170
Added line #L170 was not covered by tests
174-192: Efficient path comparison for enter/exit events.
This_updateRaycastlogic contrasts the current path with the previously entered path to decide which entities need enter/exit events. The overall approach is solid for performance and clarity.
209-225:_findCommonInPathis straightforward.
The logic for identifying intersection of two entity arrays is readable and efficient for the intended purpose.
227-254:_findDiffInPathapproach ensures accurate addition/removal detection.
The two-pass set-based difference detection is clear. This will correctly flag new and removed entities in large hierarchy transitions.tests/src/ui/UIEvent.test.ts (5)
1-74: Solid foundation for UI event testing.
You’ve established a thorough test setup, including a customTestScriptclass to track multiple pointer event types. This improves confidence in the new event emitter logic.
95-132: Comprehensive coverage of down/up lifecycle.
Tests confirm pointer “down” events on the first image, followed by pointer “up.” Great job ensuring click, drag, and drop events are triggered or not triggered as expected.
133-198: Chained assertions validate intermediate UI states well.
Your progression from pointer down → pointer up → pointer move checks multiple script counters for each image. This ensures that events bubble and propagate correctly.
199-230: Handling disabled elements tested thoroughly.
Disablingimage3ensures that the pointer exit event is fired and that further interactions no longer occur on it. This verifies correct opt-out from further pointer events.
233-242: Good utility for generating PointerEvents.
ThegeneratePointerEventhelper function is concise and flexible for orchestrating test scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/src/ui/UIEvent.test.ts (3)
28-73: Consider adding reset functionality for test counters.The TestScript class effectively tracks event counts, but lacks a way to reset counters between tests. This could lead to test interdependence if multiple test cases are added in the future.
Add a reset method to clean up between tests:
class TestScript extends Script { + reset(): void { + this.enterCount = 0; + this.exitCount = 0; + this.downCount = 0; + this.clickCount = 0; + this.beginDragCount = 0; + this.dragCount = 0; + this.endDragCount = 0; + this.upCount = 0; + this.dropCount = 0; + }
75-90: Consider using type guard for transform access.The type assertions for UITransform could be replaced with a more type-safe approach.
Consider using a type guard:
-(<UITransform>imageEntity1.transform).size.set(300, 300); +const transform1 = imageEntity1.transform; +if (transform1 instanceof UITransform) { + transform1.size.set(300, 300); +}
91-226: Consider breaking down assertions into smaller test cases.The test case contains many assertions and tests multiple behaviors. This could make it harder to identify which specific behavior failed.
Consider splitting into multiple test cases:
- Test pointer down events
- Test pointer up events
- Test pointer move events
- Test disabled component behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/ui/UIEvent.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (3)
tests/src/ui/UIEvent.test.ts (3)
1-20: LGTM! Test setup is well-structured.The imports and initial setup are comprehensive, including all necessary dependencies for UI event testing.
21-27: LGTM! Canvas configuration is appropriate for testing.The canvas setup with ScreenSpaceOverlay mode and resolution settings provides a good testing environment.
229-238: LGTM! Helper function is well-implemented.The generatePointerEvent helper is well-typed and provides good defaults for optional parameters.
| it("ui enter", () => { | ||
| // @ts-ignore | ||
| const { _pointerManager: pointerManager } = inputManager; | ||
| const { _target: target } = pointerManager; | ||
| const { left, top } = target.getBoundingClientRect(); | ||
| target.dispatchEvent(generatePointerEvent("pointerdown", 2, left + 2, top + 2)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid @ts-ignore by properly typing test utilities.
Using @ts-ignore to access private members makes the test brittle. Consider creating test utilities that properly expose these members for testing.
Create a test utility:
// test-utils.ts
export function getPointerManager(inputManager: any) {
return {
pointerManager: inputManager._pointerManager,
target: inputManager._pointerManager._target
};
}
Summary by CodeRabbit
Refactor
Tests