feat: pause-point round 9 feedback improvements#1967
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds ChangesPause-point orchestration
Keyboard recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectRunner
participant PausePointWait
participant Unity
participant Trigger
ProjectRunner->>PausePointWait: enable-pause-point --await --resume-play --trigger
PausePointWait->>PausePointWait: confirm marker armed
PausePointWait->>Unity: Status
Unity-->>PausePointWait: IsPaused
PausePointWait->>Unity: Play when paused
Unity-->>PausePointWait: ResumePlayResult
PausePointWait->>Trigger: dispatch after resume
Trigger-->>PausePointWait: TriggerResult
sequenceDiagram
participant InputUpdateApplier
participant KeyboardActionExecutor
participant KeyboardCleanup
participant InputSystem
InputUpdateApplier->>InputUpdateApplier: detect pause
InputUpdateApplier-->>KeyboardActionExecutor: Paused outcome
KeyboardActionExecutor->>KeyboardCleanup: release interrupted state
KeyboardCleanup->>InputSystem: explicit release and latch synchronization
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go (1)
306-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a combined not-armed case covering both
--resume-playand--triggertogether.Existing not-armed tests exercise
resumePlayalone (this test) andtriggerCommandalone (TestWaitForPausePointSkipsTriggerWhenNotArmedin pause_point_trigger_test.go), but no test sets both flags simultaneously against a not-armed marker to assert bothresumeResult.ErrorandtriggerResult.Errorare populated together (pause_point_wait_poll.go lines 60-77).🤖 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 `@cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go` around lines 306 - 352, Extend TestWaitForPausePointSkipsResumeWhenNotArmed to set triggerCommand alongside resumePlay, then assert both resumeResult.Error and triggerResult.Error are populated while the marker is not enabled. Preserve the existing assertions that neither action executes and the not-enabled state is returned.
🤖 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 @.agents/skills/uloop-simulate-keyboard/SKILL.md:
- Around line 29-31: Update the command synopsis for the keyboard simulation CLI
to represent --key as optional for ReleaseAll, using conditional or separate
command forms so the no-key recovery command is unambiguous. Apply the same
correction at .agents/skills/uloop-simulate-keyboard/SKILL.md lines 29-31 and
.claude/skills/uloop-simulate-keyboard/SKILL.md lines 29-31; keep the existing
parameter-table behavior unchanged.
In
`@Packages/src/Editor/FirstPartyTools/Common/InputSystem/InputSystemConfiguredUpdateApplier.cs`:
- Around line 134-136: Update the timeout handling around
WatchForPauseDiscardAsync so the linked timeoutCts is not disposed while the
watcher is still using its token: await the watcher’s completion before leaving
the timeoutCts scope, or move the watcher to a lifetime that outlasts
timeoutCts. Preserve the existing pause-discard behavior and cancellation flow.
---
Nitpick comments:
In `@cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go`:
- Around line 306-352: Extend TestWaitForPausePointSkipsResumeWhenNotArmed to
set triggerCommand alongside resumePlay, then assert both resumeResult.Error and
triggerResult.Error are populated while the marker is not enabled. Preserve the
existing assertions that neither action executes and the not-enabled state is
returned.
🪄 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 Plus
Run ID: bb36d5d9-88f6-48ae-9250-a08b02ef79ef
⛔ Files ignored due to path filters (2)
Assets/Tests/Editor/KeyboardKeyStateServiceTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/UnityCLILoop.Tests.Editor.asmdefis excluded by none and included by none
📒 Files selected for processing (34)
.agents/skills/uloop-pause-point/SKILL.md.agents/skills/uloop-simulate-keyboard/SKILL.md.claude/skills/uloop-pause-point/SKILL.md.claude/skills/uloop-simulate-keyboard/SKILL.mdAssets/RegressionHarness/KeyStateAfterPauseInterruption/SpaceHoldPoller.csAssets/Tests/Editor/KeyboardKeyStateServiceTests.csAssets/Tests/PlayMode/KeyboardInputSimulationResponseFactoryTests.csPackages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/Common/InputSimulation/UnityCliLoopInputSimulationTypes.csPackages/src/Editor/FirstPartyTools/Common/InputSystem/InputSystemConfiguredUpdateApplier.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/Application/KeyboardKeyState.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/KeyboardInputActionExecutor.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/KeyboardInputMainThreadCleanup.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/KeyboardInputSimulationResponseFactory.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardResponse.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/Skill/SKILL.mdcli/common/tools/default-tools.jsoncli/project-runner/internal/projectrunner/list_output.gocli/project-runner/internal/projectrunner/list_output_test.gocli/project-runner/internal/projectrunner/native_command_help.gocli/project-runner/internal/projectrunner/native_command_help_test.gocli/project-runner/internal/projectrunner/pause_point_enable.gocli/project-runner/internal/projectrunner/pause_point_enable_test.gocli/project-runner/internal/projectrunner/pause_point_resume_play.gocli/project-runner/internal/projectrunner/pause_point_resume_play_test.gocli/project-runner/internal/projectrunner/pause_point_trigger_test.gocli/project-runner/internal/projectrunner/pause_point_types.gocli/project-runner/internal/projectrunner/pause_point_wait.gocli/project-runner/internal/projectrunner/pause_point_wait_poll.gocli/project-runner/internal/projectrunner/pause_point_wait_test.godocs/regression-harness.mdscripts/regression-harness-key-state-after-pause-interruption.shscripts/regression-harness-resume-play-paused-arm.sh
Cancel the apply timeout CTS on the timeout path so the pause watcher stops instead of polling a disposed token. Clarify ReleaseAll synopsis and cover the combined not-armed resume+trigger skip path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed CodeRabbit review items in c118b8d:
Local verification: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Feedback mapping
#1964—--resume-playonenable-pause-point --await/await-pause-point: arm confirm → resume → trigger in one call; harnessregression-harness-resume-play-paused-arm.sh#1965— dispose pending apply on pause; force-sync stale press latch;simulate-keyboard --action ReleaseAll(paused-tolerant); harness scenarios for interrupt / 5s resume / paused ReleaseAllTryGetCapturedValuetuple shape undocumented (CS0030 on first use); Fast-Progressing Games still taught split resume+input#1966— skills document Pause → build →--await --resume-play --trigger, tuple deconstruct sample, andReleaseAllrecoveryChild PRs (already squash-merged into this branch)
feat: add --resume-play to pause-point await flowfix: recover keyboard key state after pause-point interruption with ReleaseAlldocs: document paused-scenario workflow and TryGetCapturedValue signature in skillsUser Impact
ReleaseAllrestores a clean keyboard state (including while still paused for inspection).Changes
--resume-play+ResumePlayResult; trigger skip/failure semantics when resume fails.ReleaseAllaction + responseReleasedKeys.ReleaseAll; regenerated.claude/.agentscopies.Verification
scripts/check-go-cli.shexit 0,uloop compile0 err/0 warn, harness runs) — repo CI does not fire on PRs targeting a feature branch; full CI runs on this PR.scripts/regression-harness-resume-play-paused-arm.sh,scripts/regression-harness-key-state-after-pause-interruption.shscripts/check-go-cli.sh/uloop compileon the child PRs