fix: pause-point disconnect and expiry no longer leave Play Mode stuck paused#1754
Conversation
…nnects mid-request Abandoned pause-point pauses left frame-dependent tools stuck after the agent died. Option B resumes unconditionally on Clear/ClearAll, capture-window expiry (including SingleShot hits), mid-request client disconnect, and bridge DisconnectAllClients—without treating every short-lived command close as a disconnect. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughPause-point handling now resumes the Unity Editor after clears, capture-window expiration, and client disconnects. The runtime adds main-thread processing for pending resume requests and expiration cleanup, while editor and play-mode test doubles implement the new resume contract. ChangesPause-point resume lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClientDisconnectMonitor
participant PausePointRegistry
participant EditorUpdate
participant UnityEditor
ClientDisconnectMonitor->>PausePointRegistry: request pending resume
EditorUpdate->>PausePointRegistry: apply pending resume
PausePointRegistry->>UnityEditor: Resume()
UnityEditor-->>PausePointRegistry: clear editor pause state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Review (Fable) — 1 required fixSpec compliance (approved Option B, triggers = mid-request disconnect + Clear/expiry), the Required fix: disconnect-path Resume calls a Unity API off the main thread
Recommended shape (no new dispatch infra needed): make Please add two pure tests with the existing fake controller: disconnect sets the flag and the update-side application calls Resume exactly once; a stale flag while unpaused is dropped without Resume. Observation (no change requested)While manually paused, an enabled-but-never-hit pause point whose window lapses will now expire via the update pump and clear the manual pause. This is within approved Option B, but it is the one case that can look like a spontaneous resume; if it ever bothers users, restricting expiry-resume to entries that actually Hit is the future knob. Will LGTM once the thread-boundary fix lands. |
Disconnect monitor and DisconnectAllClients run on the thread pool, so ResumeEditorPauseForClientDisconnect only sets a pending flag; OnEditorUpdate applies it before the isPaused gate to avoid unsafe EditorApplication access. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the required thread-safety fix:
Tests:
Verification: compile 0/0, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
LGTM. Verified commit 42b4229: |
2 participants
Summary
User Impact
DisconnectAllClientsall resume Play Modecontrol-play-mode Pauseif one happens to be active—there is no ownership distinctionChanges
Resume()onIUloopPausePointPauseControllerExpiresAtUtc)DisconnectAllClientscallResumeEditorPauseForClientDisconnectawait-pause-pointinspection)Out of scope
isPaused)Verification
uloop compile— 0 errors / 0 warningsuloop run-testsfilterPausePointTests— 53 passeduloop run-testsfilterPausePointCaptureModeTests|PausePointToolModeTests— 12 passedTest plan
clear-pause-point→ Play Mode resumesawait-pause-pointreturns Hit, Unity stays paused until Clear/expiry (short command disconnect must not resume)