chore: Keep setup startup coordination out of the window#1563
Conversation
Move setup wizard startup auto-show and migration auto-scan coordination into a dedicated flow object so the window keeps only UI mechanics while preserving startup timing and existing policies.
|
Warning Review limit reached
Next review available in: 24 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 (3)
📝 WalkthroughWalkthroughStartup, version-change, auto-show, and third-party migration auto-scan decision logic previously implemented inline in SetupWizardWindow has been extracted into a new SetupWizardStartupFlow class. SetupWizardWindow now delegates these decisions and state recording via EditorApplication.delayCall, and tests were updated accordingly. ChangesSetupWizardStartupFlow extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EditorStartup
participant SetupWizardWindow
participant SetupWizardStartupFlow
participant SkillDetection
participant SetupWizardWindowUI
EditorStartup->>SetupWizardWindow: InitializeForEditorStartup
SetupWizardWindow->>SetupWizardStartupFlow: register TryShowOnVersionChange on delayCall
SetupWizardStartupFlow->>SetupWizardStartupFlow: EvaluateVersionChange
SetupWizardStartupFlow->>SetupWizardStartupFlow: NeedsCliUpdateForSetupWizardAsync
SetupWizardStartupFlow->>SkillDetection: HasSkillUpdateForSetupWizardAsync (Task.Run)
SkillDetection-->>SetupWizardStartupFlow: skill update result
SetupWizardStartupFlow->>SetupWizardWindowUI: delayCall ShowWindowOnVersionChange
SetupWizardStartupFlow->>SetupWizardWindowUI: delayCall ShowWindowForAutoScan
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 |
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs">
<violation number="1" location="Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs:290">
P2: The extracted `SetupWizardStartupFlow` still depends on `SetupWizardWindow.ForceFlatSkillInstall`, which couples non-UI startup coordination back to the UI window class. This undermines the PR goal of keeping startup coordination out of the window. Consider injecting `forceFlatSkillInstall` through the constructor (consistent with the existing `Action` callback injection pattern) or moving the constant to shared configuration so `SetupWizardStartupFlow` does not reference `SetupWizardWindow`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| List<SkillSetupTargetInfo> targets = await Task.Run( | ||
| () => _skillSetupUseCase.DetectSkillTargetsForLayoutAtProjectRoot( | ||
| projectRoot, | ||
| !SetupWizardWindow.ForceFlatSkillInstall), |
There was a problem hiding this comment.
P2: The extracted SetupWizardStartupFlow still depends on SetupWizardWindow.ForceFlatSkillInstall, which couples non-UI startup coordination back to the UI window class. This undermines the PR goal of keeping startup coordination out of the window. Consider injecting forceFlatSkillInstall through the constructor (consistent with the existing Action callback injection pattern) or moving the constant to shared configuration so SetupWizardStartupFlow does not reference SetupWizardWindow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs, line 290:
<comment>The extracted `SetupWizardStartupFlow` still depends on `SetupWizardWindow.ForceFlatSkillInstall`, which couples non-UI startup coordination back to the UI window class. This undermines the PR goal of keeping startup coordination out of the window. Consider injecting `forceFlatSkillInstall` through the constructor (consistent with the existing `Action` callback injection pattern) or moving the constant to shared configuration so `SetupWizardStartupFlow` does not reference `SetupWizardWindow`.</comment>
<file context>
@@ -0,0 +1,311 @@
+ List<SkillSetupTargetInfo> targets = await Task.Run(
+ () => _skillSetupUseCase.DetectSkillTargetsForLayoutAtProjectRoot(
+ projectRoot,
+ !SetupWizardWindow.ForceFlatSkillInstall),
+ ct);
+ if (ct.IsCancellationRequested)
</file context>
Remove the obsolete setup service registration parameter and split startup guard coverage so each test asserts one scheduling invariant.
Summary
User Impact
Changes
SetupWizardStartupFlowto coordinate startup auto-show, third-party migration auto-scan, version recording, and skill update checks.SetupWizardWindowby injectingActioncallbacks for version-change open and third-party auto-scan.SetupWizardWindow.csandSetupWizardStartupFlow.cs, so delay scheduling guards do not become vacuous after extraction.Timing Invariants
delayCallwithstartupFlow.TryShowOnVersionChangeafter the asset-import-worker and batch-mode guards.delayCall.delayCallonly afterShouldAutoShowForVersionreturns true.Deviations
SetupWizardStartupFlowhas a private compatibility glue helper that callsCliSetupCompatibility.Evaluatewith the injectedCliSetupApplicationServiceminimum version. This avoids reaching back throughSetupWizardWindowstatic service state from the extracted flow.ForceFlatSkillInstallwas widened fromprivate consttointernal constbecause both the setup window UI flow and the extracted startup flow use the shared value; duplicating the value would be worse, and the window-side use remains inSetupWizardWindow.cs.Debug.Assertchecks as boundary contracts, matching the acceptedCliPathSetupPromptpattern.Verification
dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value "(SetupWizardWindowTests|OnionAssemblyDependencyTests)"dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value "(SetupWizardWindowTests|OnionAssemblyDependencyTests)"