fix: Input simulation no longer hangs when Editor updates stall#1306
Merged
Conversation
Replace the old update-driven EditorDelay primitive with a focused EditorFrameWaiter for frame waits, and route timeout-sensitive waits through TimerDelay so tool calls can return even when Editor updates stall. - Add timed-out input simulation outcomes with deferred main-thread cleanup. - Use EditorFrameWaiter for frame-based editor waits and TimerDelay for wall-clock waits. - Update focused editor and input simulation tests for the new wait contracts.
Ensure delayed main-thread actions fault the returned task when the action throws, and make input apply timeouts win only before an input update starts applying state. Add regression coverage for delayed action exception propagation.
Contributor
|
Warning Review limit reached
More reviews will be available in 8 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRemoves EditorDelay/EditorDelayManager and adds EditorFrameWaiter. Refactors TimerDelay and TaskExtensions. Input simulation now returns explicit wait outcomes (Completed/Paused/TimedOut) with per-frame vs wall-clock racing; keyboard/mouse flows and tests updated accordingly. ChangesEditorDelay to EditorFrameWaiter migration and timeout-aware input simulation
Sequence Diagram(s)sequenceDiagram
participant Simulate as SimulateUseCase
participant Apply as ApplyOnNextConfiguredUpdate
participant InputUpdate as InputSystemConfiguredUpdate
participant Timeout as WallClockTimeout
Simulate->>Apply: request apply(action) + start remaining-time timeout
par wait for configured update
Apply->>InputUpdate: subscribe callback
and wall-clock racing
Apply->>Timeout: race against remaining time
end
alt InputUpdate fires first
InputUpdate->>Apply: callback runs -> Apply returns Completed
Apply-->>Simulate: Completed
else Timeout first
Timeout->>Apply: timeout elapsed -> Apply returns TimedOut
Apply-->>Simulate: TimedOut
else cancellation
Simulate->>Apply: cancellation token fires -> Apply returns Paused/Cancelled
Apply-->>Simulate: Paused
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Editor/FirstPartyTools/SimulateKeyboard/Skill/SKILL.md (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWorkflow steps across three files contradict the updated Debug Break section headings.
The workflow steps in
SimulateKeyboard,SimulateMouseInput, andSimulateMouseUiall describe Debug Break inspection as "an optional follow-up," but their Debug Break section headings were renamed to "(Standard for E2E)" and the body text positions paused-frame proof as the standard approach. This systematic inconsistency creates conflicting guidance about whether debug breaks are optional or standard practice for E2E verification.📝 Suggested alignment pattern for all three files
For each workflow step, replace "If exact-frame proof would reduce uncertainty, treat Debug Break inspection as an optional follow-up" with language that positions it as standard for state-transition verification, such as:
For state transitions driven by simulated input, use Debug Break inspection as standard frame proof per the section below; supplement with logs, screenshots, and durable state🤖 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 `@Packages/src/Editor/FirstPartyTools/SimulateKeyboard/Skill/SKILL.md` at line 17, Update the workflow step sentence in the three files SimulateKeyboard, SimulateMouseInput, and SimulateMouseUi to match the renamed Debug Break section "(Standard for E2E)"; specifically replace the line "If exact-frame proof would reduce uncertainty, treat Debug Break inspection as an optional follow-up" with wording that treats Debug Break as the standard for state-transition verification (e.g., "For state transitions driven by simulated input, use Debug Break inspection as standard frame proof per the section below; supplement with logs, screenshots, and durable state"); ensure this change appears in the workflow steps that reference the Debug Break section to keep the guidance consistent with the "(Standard for E2E)" heading.
🧹 Nitpick comments (3)
Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md (1)
3-3: 💤 Low valueConsider shortening the frontmatter description.
The
descriptionfield spans multiple sentences with detailed implementation guidance. YAML frontmatter descriptions are typically concise summaries. Consider moving the detailed guidance ("Whenever you verify behavior...") to the main body and keeping only the core directive in the description.🤖 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 `@Packages/src/Editor/CliOnlyTools`~/PausePoint/Skill/SKILL.md at line 3, The frontmatter description is too long and contains implementation guidance; shorten the YAML `description` in Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md to a concise summary (e.g., "Standard paused-frame proof for Unity PlayMode/E2E gameplay verification.") and move the detailed guidance text starting with "Whenever you verify behavior..." into the main document body as a separate paragraph or section; update only the `description` field in the frontmatter and place the longer verification steps and examples in the markdown content below.Packages/src/Editor/ToolContracts/EditorFrameWaiter.cs (1)
156-159: ⚡ Quick winRedundant null validation combines Debug.Assert and throw.
Lines 156-159 validate
completionSourceandremoveRequestwith bothDebug.Assertandthrow, duplicating the null check. Based on learnings, Debug.Assert alone is sufficient for programmer preconditions in this internal class, as the fail-fast policy expects assertions to halt during development.♻️ Proposed simplification
public EditorFrameWaitRequest( TaskCompletionSource<bool> completionSource, int targetFrameCount, Action<EditorFrameWaitRequest> removeRequest) { Debug.Assert(completionSource != null, "completionSource must not be null"); Debug.Assert(removeRequest != null, "removeRequest must not be null"); - _completionSource = completionSource ?? throw new ArgumentNullException(nameof(completionSource)); + _completionSource = completionSource; _targetFrameCount = targetFrameCount; - _removeRequest = removeRequest ?? throw new ArgumentNullException(nameof(removeRequest)); + _removeRequest = removeRequest; }🤖 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 `@Packages/src/Editor/ToolContracts/EditorFrameWaiter.cs` around lines 156 - 159, The constructor in EditorFrameWaiter redundantly validates completionSource and removeRequest with both Debug.Assert and null-coalescing throws; replace the null-coalescing throw checks with simple assignments and keep the Debug.Assert preconditions only. In the EditorFrameWaiter constructor, assert Debug.Assert(completionSource != null) and Debug.Assert(removeRequest != null), then assign _completionSource = completionSource and _removeRequest = removeRequest (leave _targetFrameCount assignment unchanged) so the class uses assertions as the fail-fast policy for these internal preconditions.Source: Learnings
Packages/src/Editor/ToolContracts/TimerDelay.cs (1)
54-62: ⚡ Quick winRedundant null validation combines Debug.Assert and throw.
Lines 58-61 validate
synchronizationContextwith bothDebug.Assertandthrow. Based on learnings, Debug.Assert alone suffices for programmer preconditions, though this specific case validates a runtime state (whether the method was called from the correct thread), so the exception may be appropriate. However, the duplication is still redundant.♻️ Proposed simplification (if treating as precondition)
SynchronizationContext synchronizationContext = SynchronizationContext.Current; Debug.Assert( synchronizationContext != null, "WaitThenExecuteOnMainThread must start from Unity's main-thread synchronization context."); -if (synchronizationContext == null) -{ - throw new InvalidOperationException( - "WaitThenExecuteOnMainThread must start from Unity's main-thread synchronization context."); -}Note: Keep the throw if this is considered runtime validation rather than a programmer precondition.
🤖 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 `@Packages/src/Editor/ToolContracts/TimerDelay.cs` around lines 54 - 62, The code redundantly checks SynchronizationContext with both Debug.Assert and an exception in WaitThenExecuteOnMainThread; choose one validation: if this is a programmer precondition remove the throw/InvalidOperationException and keep only Debug.Assert(synchronizationContext != null, "..."); if this is runtime validation remove the Debug.Assert and keep the throw(new InvalidOperationException("WaitThenExecuteOnMainThread must start from Unity's main-thread synchronization context.")); update the remaining message to match the chosen behavior and remove the duplicate null check block.Source: Learnings
🤖 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.
Inline comments:
In `@Packages/src/Editor/ToolContracts/TimerDelay.cs`:
- Around line 47-66: WaitThenExecuteOnMainThread captures
SynchronizationContext.Current but then calls action() after awaiting
Wait(milliseconds, ct) without marshaling back, so the callback can run off the
main thread; fix by invoking the captured SynchronizationContext to execute the
action on the main thread (e.g., use synchronizationContext.Post or Send to
dispatch action) after the await completes, preserving the null checks already
present and ensuring the action runs on the captured SynchronizationContext
rather than directly calling action().
---
Outside diff comments:
In `@Packages/src/Editor/FirstPartyTools/SimulateKeyboard/Skill/SKILL.md`:
- Line 17: Update the workflow step sentence in the three files
SimulateKeyboard, SimulateMouseInput, and SimulateMouseUi to match the renamed
Debug Break section "(Standard for E2E)"; specifically replace the line "If
exact-frame proof would reduce uncertainty, treat Debug Break inspection as an
optional follow-up" with wording that treats Debug Break as the standard for
state-transition verification (e.g., "For state transitions driven by simulated
input, use Debug Break inspection as standard frame proof per the section below;
supplement with logs, screenshots, and durable state"); ensure this change
appears in the workflow steps that reference the Debug Break section to keep the
guidance consistent with the "(Standard for E2E)" heading.
---
Nitpick comments:
In `@Packages/src/Editor/CliOnlyTools`~/PausePoint/Skill/SKILL.md:
- Line 3: The frontmatter description is too long and contains implementation
guidance; shorten the YAML `description` in
Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md to a concise summary
(e.g., "Standard paused-frame proof for Unity PlayMode/E2E gameplay
verification.") and move the detailed guidance text starting with "Whenever you
verify behavior..." into the main document body as a separate paragraph or
section; update only the `description` field in the frontmatter and place the
longer verification steps and examples in the markdown content below.
In `@Packages/src/Editor/ToolContracts/EditorFrameWaiter.cs`:
- Around line 156-159: The constructor in EditorFrameWaiter redundantly
validates completionSource and removeRequest with both Debug.Assert and
null-coalescing throws; replace the null-coalescing throw checks with simple
assignments and keep the Debug.Assert preconditions only. In the
EditorFrameWaiter constructor, assert Debug.Assert(completionSource != null) and
Debug.Assert(removeRequest != null), then assign _completionSource =
completionSource and _removeRequest = removeRequest (leave _targetFrameCount
assignment unchanged) so the class uses assertions as the fail-fast policy for
these internal preconditions.
In `@Packages/src/Editor/ToolContracts/TimerDelay.cs`:
- Around line 54-62: The code redundantly checks SynchronizationContext with
both Debug.Assert and an exception in WaitThenExecuteOnMainThread; choose one
validation: if this is a programmer precondition remove the
throw/InvalidOperationException and keep only
Debug.Assert(synchronizationContext != null, "..."); if this is runtime
validation remove the Debug.Assert and keep the throw(new
InvalidOperationException("WaitThenExecuteOnMainThread must start from Unity's
main-thread synchronization context.")); update the remaining message to match
the chosen behavior and remove the duplicate null check block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad7354b1-da40-4ff4-a1c8-5cafb3567fcf
⛔ Files ignored due to path filters (4)
Assets/Tests/Editor/EditorFrameWaiterTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Common/InputSystem/UnityCLILoop.FirstPartyTools.Common.InputSystem.Editor.asmdefis excluded by none and included by nonePackages/src/Editor/ToolContracts/EditorDelay.cs.metais excluded by none and included by nonePackages/src/Editor/ToolContracts/EditorFrameWaiter.cs.metais excluded by none and included by none
📒 Files selected for processing (24)
Assets/Editor/EditorDelayManualTests.csAssets/Tests/Editor/EditorDelayTests.csAssets/Tests/Editor/EditorFrameWaiterTests.csAssets/Tests/Editor/StaticFacadeStateGuardTests.csAssets/Tests/PlayMode/SimulateKeyboardTests.csAssets/Tests/PlayMode/SimulateMouseInputTests.csPackages/src/Editor/Application/ApplicationEditorStartup.csPackages/src/Editor/Application/SessionRecoveryService.csPackages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/Common/InputSystem/InputSystemUpdateHelper.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCodeDomainReloadWaitSignal.csPackages/src/Editor/FirstPartyTools/Screenshot/EditorWindowCaptureUtility.csPackages/src/Editor/FirstPartyTools/Screenshot/ScreenshotUseCase.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputUseCase.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/Skill/SKILL.mdPackages/src/Editor/ToolContracts/EditorDelay.csPackages/src/Editor/ToolContracts/EditorDelayManager.csPackages/src/Editor/ToolContracts/EditorFrameWaiter.csPackages/src/Editor/ToolContracts/TaskExtensions.csPackages/src/Editor/ToolContracts/TimerDelay.cs
💤 Files with no reviewable changes (3)
- Assets/Tests/Editor/EditorDelayTests.cs
- Packages/src/Editor/ToolContracts/EditorDelayManager.cs
- Packages/src/Editor/ToolContracts/EditorDelay.cs
There was a problem hiding this comment.
6 issues found across 28 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Add regression coverage showing that WaitThenExecuteOnMainThread resumes its delayed action on the SynchronizationContext captured before the timer wait. This documents the intended behavior behind the PR review discussion without changing production code.
Ensure Editor frame waits do not run continuations inline from Editor update, defer zero-duration TimerDelay main-thread actions until an Editor update, and surface Input System apply failures through the awaited task so timeout paths do not wait on orphaned completions. Also align mouse skill workflow text with the Debug Break E2E guidance.
Merged
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User Impact
Changes
Verification