Repository navigation
fix: Async setup and input cleanup now carry cancellation tokens - #1594
Conversation
Convert setup wizard async UI handlers to Task-based cores with CancellationToken ct and add a source guard for the R2-5 target files. Add ct parameters to mouse and keyboard release helpers while preserving uncancelable cleanup paths for held input state.
|
Warning Review limit reached
Next review available in: 40 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a static async-method guard for ChangesCancellationToken Propagation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant SetupWizardWindow
participant RefreshUIAsync
participant HandleInstallCliAsync
participant HandleRepairCliPathSetup
participant HandleInstallSkillsAsync
SetupWizardWindow->>RefreshUIAsync: RefreshUIAsync(bool, ct)
RefreshUIAsync->>RefreshUIAsync: ForceRefreshCliVersionAsync(ct)
HandleInstallCliAsync->>HandleRepairCliPathSetup: HandleRepairCliPathSetup(ct)
HandleInstallCliAsync->>HandleInstallCliAsync: InstallGlobalCliAsync(ct)
HandleInstallSkillsAsync->>HandleInstallSkillsAsync: InstallSkillFilesAsync(ct)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Require the R2-5 source guard to match the exact CancellationToken ct parameter name so near-miss names such as cts do not satisfy the convention.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Assets/Tests/Editor/StaticFacadeStateGuardTests.cs (1)
90-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRegex guard has false-negative gaps for method signatures it can't match.
AsyncMethodSignaturePatternrequires an explicit access modifier (private|internal|public|protected) beforeasync, so an implicit-private async method (no leading modifier) would be silently skipped instead of flagged. Similarly,Task(?:<[^>]+>)?can't handle a nested generic return type likeTask<List<string>>— the pattern fails to match the whole signature (since[^>]+stops at the first>), so such a method would also be silently skipped by the guard rather than reported as a violation. Neither case currently exists in the target files, but the guard's whole purpose is preventing future regressions, so a silent skip defeats it.♻️ Possible mitigation
private static readonly Regex AsyncMethodSignaturePattern = new Regex( - @"\b(?:private|internal|public|protected)\s+(?:static\s+)?async\s+(?:void|Task(?:<[^>]+>)?)\s+([A-Za-z_][A-Za-z0-9_]*)\s*\(([^)]*)\)", + @"\basync\s+(?:void|Task(?:<[^)]+>)?)\s+([A-Za-z_][A-Za-z0-9_]*)\s*\(([^)]*)\)", RegexOptions.Compiled | RegexOptions.Singleline);(Dropping the mandatory-modifier requirement widens matches to any
asyncmethod regardless of visibility keyword ordering; nested generics still need a more permissive inner class, e.g.[^)]+up to the method's opening paren, since>no longer needs excluding.)🤖 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/StaticFacadeStateGuardTests.cs` around lines 90 - 97, Update StaticFacadeStateGuardTests so AsyncMethodSignaturePattern does not silently miss async methods with implicit-private visibility or nested generic Task return types. Relax the modifier portion to match async methods regardless of whether a visibility keyword is present, and make the return-type portion permissive enough to capture nested generics like Task<List<string>> before the parameter list. Keep the change localized to AsyncMethodSignaturePattern and verify the guard still identifies async method names for the existing test parsing logic.
🤖 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/StaticFacadeStateGuardTests.cs`:
- Around line 90-97: Update StaticFacadeStateGuardTests so
AsyncMethodSignaturePattern does not silently miss async methods with
implicit-private visibility or nested generic Task return types. Relax the
modifier portion to match async methods regardless of whether a visibility
keyword is present, and make the return-type portion permissive enough to
capture nested generics like Task<List<string>> before the parameter list. Keep
the change localized to AsyncMethodSignaturePattern and verify the guard still
identifies async method names for the existing test parsing logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 354b1e2e-fd7e-4f88-889a-c21605e0fdcc
📒 Files selected for processing (4)
Assets/Tests/Editor/StaticFacadeStateGuardTests.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputUseCase.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.cs
Cover implicit-private async methods and nested generic Task returns so the R2-5 cancellation token guard does not silently skip future async methods.
|
CodeRabbit nitpick about |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Assets/Tests/Editor/StaticFacadeStateGuardTests.cs (1)
90-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRegex still misses generic async methods; could silently skip a violation.
The method-name capture
([A-Za-z_][A-Za-z0-9_]*)\s*\(won't match generic methods likeasync Task<T> Foo<T>(...)(the<T>between name and(breaks the match), so such methods would be silently excluded from the guard scan instead of being flagged for missingCancellationToken ct. Low risk today given the small allowlist, but worth hardening since this guard's whole purpose is to catch exactly this class of omission.♻️ Optional hardening to also capture generic type parameters
- @"\b(?:(?:private|internal|public|protected)\s+)?(?:static\s+)?async\s+(?:void|Task(?:<[^()\n]+>)?)\s+([A-Za-z_][A-Za-z0-9_]*)\s*\(([^)]*)\)", + @"\b(?:(?:private|internal|public|protected)\s+)?(?:static\s+)?async\s+(?:void|Task(?:<[^()\n]+>)?)\s+([A-Za-z_][A-Za-z0-9_]*)\s*(?:<[^()\n]+>)?\s*\(([^)]*)\)",🤖 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/StaticFacadeStateGuardTests.cs` around lines 90 - 92, The AsyncMethodSignaturePattern in StaticFacadeStateGuardTests only matches non-generic async method names, so generic methods like Foo<T> are skipped by the guard scan. Update the regex to allow optional generic type parameter clauses after the method name and before the parameter list, so generic async methods are still captured and checked for the missing CancellationToken ct requirement.
🤖 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/StaticFacadeStateGuardTests.cs`:
- Around line 90-92: The AsyncMethodSignaturePattern in
StaticFacadeStateGuardTests only matches non-generic async method names, so
generic methods like Foo<T> are skipped by the guard scan. Update the regex to
allow optional generic type parameter clauses after the method name and before
the parameter list, so generic async methods are still captured and checked for
the missing CancellationToken ct requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d50b8656-7356-4328-bec7-87336e6a60ac
📒 Files selected for processing (1)
Assets/Tests/Editor/StaticFacadeStateGuardTests.cs
Allow optional generic method type parameters so the cancellation-token source guard cannot skip async methods such as Foo<T>.
|
CodeRabbit follow-up about generic method type parameters was also valid. Fixed in ab3bfa4 by allowing optional generic method clauses such as |
1 participant
Summary
CancellationToken ctinstead of async void bodies.CancellationToken ctwhile preserving uncancelable release paths for held input state.User Impact
Changes
CancellationToken ctparameter.ctparameters to simulate mouse/keyboard release helpers and pass the existing uncancelable cleanup token where cleanup must complete.Verification