chore: simplify mouse UI drag orchestration#1638
Conversation
Centralize path and raycast lookup through drag-handler resolution while preserving action-specific no-handler responses and one-shot failure precedence. Add synchronous coverage for bypass handlers, missing handlers, and path failures.
Move DragStart, DragMove, and DragEnd orchestration into a focused executor so SimulateMouseUiUseCase remains a thin action dispatcher. Keep drag state ownership, cleanup ordering, scheduler timing, and response behavior unchanged.
📝 WalkthroughWalkthroughThis PR extracts drag target resolution into a new ChangesDrag target resolution and executor refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SimulateMouseUiUseCase
participant MouseUiIncrementalDragExecutor
participant MouseUiDragTargetResolver
participant MouseDragState
participant MouseUiOverlayState
SimulateMouseUiUseCase->>MouseUiIncrementalDragExecutor: ExecuteDragStart()
MouseUiIncrementalDragExecutor->>MouseUiDragTargetResolver: Resolve(bypass or raycast)
MouseUiDragTargetResolver-->>MouseUiIncrementalDragExecutor: startRaycast, target, failure
MouseUiIncrementalDragExecutor->>MouseDragState: set Target/PointerData
MouseUiIncrementalDragExecutor->>MouseUiOverlayState: update start overlay
SimulateMouseUiUseCase->>MouseUiIncrementalDragExecutor: ExecuteDragMove()
MouseUiIncrementalDragExecutor->>MouseDragState: ValidateDragStillActive()
MouseUiIncrementalDragExecutor->>MouseUiOverlayState: update move overlay
SimulateMouseUiUseCase->>MouseUiIncrementalDragExecutor: ExecuteDragEnd()
MouseUiIncrementalDragExecutor->>MouseDragState: FinalizeDrag / clear
MouseUiIncrementalDragExecutor->>MouseUiOverlayState: dissipate overlay
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
Assets/Tests/Editor/MouseUiDragTargetResolverTests.cs (1)
38-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the non-bypass (raycast) resolution path.
All three tests use
BypassRaycast = true, leaving the raycast branch (resolver lines 45-49) uncovered at the unit level. A test that sets up a Canvas with a raycast-target Graphic and callsResolvewithBypassRaycast = falsewould verify theUiRaycastHelper.RaycastUI→ExecuteEvents.GetEventHandler<IDragHandler>flow directly through the new resolver API.🤖 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 `@Assets/Tests/Editor/MouseUiDragTargetResolverTests.cs` around lines 38 - 117, Add a unit test that covers the non-bypass raycast path in MouseUiDragTargetResolver.Resolve, since the current tests only exercise the bypass flow via BypassRaycast = true. Create a Canvas with a raycast-target Graphic, call Resolve with BypassRaycast = false, and assert the raycast result and handler resolution from UiRaycastHelper.RaycastUI through ExecuteEvents.GetEventHandler<IDragHandler>. Use MouseUiDragTargetResolver.Resolve and the existing test helpers as the entry points to keep the new test aligned with the current suite.
🤖 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.
Nitpick comments:
In `@Assets/Tests/Editor/MouseUiDragTargetResolverTests.cs`:
- Around line 38-117: Add a unit test that covers the non-bypass raycast path in
MouseUiDragTargetResolver.Resolve, since the current tests only exercise the
bypass flow via BypassRaycast = true. Create a Canvas with a raycast-target
Graphic, call Resolve with BypassRaycast = false, and assert the raycast result
and handler resolution from UiRaycastHelper.RaycastUI through
ExecuteEvents.GetEventHandler<IDragHandler>. Use
MouseUiDragTargetResolver.Resolve and the existing test helpers as the entry
points to keep the new test aligned with the current suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f44ae6d-d0b3-45f9-ac8e-53e6122b5fdc
⛔ Files ignored due to path filters (3)
Assets/Tests/Editor/MouseUiDragTargetResolverTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiDragTargetResolver.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiIncrementalDragExecutor.cs.metais excluded by none and included by none
📒 Files selected for processing (5)
Assets/Tests/Editor/MouseUiDragTargetResolverTests.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiDragTargetResolver.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiIncrementalDragExecutor.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiOneShotDragExecutor.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.cs
There was a problem hiding this comment.
No issues found across 8 files
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Summary
User Impact
Changes
IDragHandlerlookup.DragStartwhile preserving the observable failure order: start-target failure, drop-target failure, then no-handler fallback.DragStart,DragMove,DragEnd, and active-drag validation intoMouseUiIncrementalDragExecutor.MouseDragStateownership unchanged and pass the main-thread cleanup scheduler explicitly.SimulateMouseUiUseCasefrom 434 lines to 137 lines.Verification
0/3) and pass after implementation (3/3).141/141).32/32).0errors and0warnings.FinalizeDragbeforeMouseDragState.Clearwith the same captures and timing.Compatibility