chore: Harden Settings/Setup async UI and shallow presenter split#1777
Conversation
async void swallows exceptions and complicates UI lifecycle ownership. Convert Settings, migration wizard, and setup startup handlers to async Task, and bridge Action/delegate call sites with Forget() so failures stay logged. Co-authored-by: Cursor <cursoragent@cursor.com>
…enters Keep the EditorWindow as a thin orchestrator for one manual UI check. Move catalog cache/warmup into ToolSettingsPresenter, CLI primary-action mediation into CliSetupPresenter, and extract UnityEditor-independent dialog/PATH policies to Domain with focused tests. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe changes extract settings decision policies, move CLI action and tool-settings state management into presenters, and convert editor async handlers from ChangesSettings presentation refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Packages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cs (1)
100-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant warmup-cancel calls before
SetViewReady(false).
SetViewReady(false)already callsCancelRegistryWarmup()internally when not ready, so the preceding explicit_toolSettingsPresenter?.CancelRegistryWarmup()is a no-op duplicate in bothOnDestroyandOnDisable.ResetRegistryWarmupAttemptCount()right before nulling_toolSettingsPresenteralso has no observable effect.♻️ Simplify cleanup
private void OnDestroy() { CancelDeferredInitialRefresh(); - _toolSettingsPresenter?.CancelRegistryWarmup(); - _toolSettingsPresenter?.ResetRegistryWarmupAttemptCount(); CancelSkillInstallStateRefresh(); _toolSettingsPresenter?.SetViewReady(false); _view?.Dispose();Also applies to: 216-229
🤖 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/Presentation/UnityCliLoopSettingsWindow.cs` around lines 100 - 111, Remove the explicit _toolSettingsPresenter?.CancelRegistryWarmup() and _toolSettingsPresenter?.ResetRegistryWarmupAttemptCount() calls from both OnDestroy and OnDisable. Keep _toolSettingsPresenter?.SetViewReady(false) in the cleanup sequence so it performs the required warmup cancellation before the presenter is cleared.Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs (2)
53-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated cached-CLI-state retrieval within the same class.
ResolveCurrentPrimaryButtonActionre-fetchescliVersion,cliIsDispatcher,cliExecutablePath,canUninstallCli, andrequiredCliVersionfrom_cliSetupApplicationService— the same set already computed inCreateCliSetupData(Line 147-155). Consider extracting a small private helper to avoid the two call sites drifting apart.🤖 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/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs` around lines 53 - 68, The cached CLI state is assembled twice in the presenter, allowing ResolveCurrentPrimaryButtonAction and CreateCliSetupData to diverge. Extract the shared retrieval and canUninstallCli calculation into a private helper, then reuse it from both methods while preserving the existing ResolveCliPrimaryButtonAction behavior.
116-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
CliPathSetupCheckPolicy.ShouldCheckwrapper across two presenters. Both files define an internal static method with the identical body — mappingplatform == RuntimePlatform.WindowsEditortoisWindowsEditorand forwarding toCliPathSetupCheckPolicy.ShouldCheck— under different names. One of these should be the single source of truth, reused by the other call site (and byUnityCliLoopSettingsWindow.ShouldCheckCliPathSetup, which already calls the presenter's copy).
Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs#L116-L123: keep this as the canonical wrapper (or move the mapping directly intoCliPathSetupCheckPolicyitself).Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs#L575-L582: delegateShouldCheckCliPathSetupForSetupWizardtoUnityCliLoopSettingsCliSetupPresenter.ShouldCheckCliPathSetupForPlatforminstead of re-wrapping the policy.🤖 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/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs` around lines 116 - 123, The duplicate policy wrapper should have one source of truth. Keep UnityCliLoopSettingsCliSetupPresenter.ShouldCheckCliPathSetupForPlatform as the canonical mapping to CliPathSetupCheckPolicy.ShouldCheck; in Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs lines 575-582, update ShouldCheckCliPathSetupForSetupWizard to delegate to that presenter method instead of re-wrapping the policy. No direct change is needed at the presenter site.Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs (1)
541-545: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the dialog helper here
HandleInstallSkillsAsyncduplicates theShouldShowSkillsInstalledDialogcheck. Calling the helper keeps the decision in one place; otherwise the wrapper should be removed and its tests updated.🤖 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/Presentation/Setup/SetupWizardWindow.cs` around lines 541 - 545, Update HandleInstallSkillsAsync to call the existing ShouldShowSkillsInstalledDialog helper instead of duplicating its decision logic. Preserve the helper’s behavior and, if the wrapper is no longer needed, remove ShouldShowSkillsInstalledDialog and update its tests accordingly.
🤖 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/Presentation/UnityCliLoopSettingsToolSettingsPresenter.cs`:
- Around line 50-53: Update InvalidateCatalog in
UnityCliLoopSettingsToolSettingsPresenter to reset the registry warmup/retry
budget whenever the catalog is invalidated, in addition to setting
_isCatalogDirty. Ensure invalidation through
UnityCliLoopSettingsWindow.InvalidateToolSettingsCatalog can restart warmup even
after the retry cap was exhausted.
---
Nitpick comments:
In `@Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs`:
- Around line 541-545: Update HandleInstallSkillsAsync to call the existing
ShouldShowSkillsInstalledDialog helper instead of duplicating its decision
logic. Preserve the helper’s behavior and, if the wrapper is no longer needed,
remove ShouldShowSkillsInstalledDialog and update its tests accordingly.
In `@Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs`:
- Around line 53-68: The cached CLI state is assembled twice in the presenter,
allowing ResolveCurrentPrimaryButtonAction and CreateCliSetupData to diverge.
Extract the shared retrieval and canUninstallCli calculation into a private
helper, then reuse it from both methods while preserving the existing
ResolveCliPrimaryButtonAction behavior.
- Around line 116-123: The duplicate policy wrapper should have one source of
truth. Keep
UnityCliLoopSettingsCliSetupPresenter.ShouldCheckCliPathSetupForPlatform as the
canonical mapping to CliPathSetupCheckPolicy.ShouldCheck; in
Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs lines 575-582,
update ShouldCheckCliPathSetupForSetupWizard to delegate to that presenter
method instead of re-wrapping the policy. No direct change is needed at the
presenter site.
In `@Packages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cs`:
- Around line 100-111: Remove the explicit
_toolSettingsPresenter?.CancelRegistryWarmup() and
_toolSettingsPresenter?.ResetRegistryWarmupAttemptCount() calls from both
OnDestroy and OnDisable. Keep _toolSettingsPresenter?.SetViewReady(false) in the
cleanup sequence so it performs the required warmup cancellation before the
presenter is cleared.
🪄 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: 102bd7cf-f52d-4a58-88e0-1305560e16e0
⛔ Files ignored due to path filters (3)
Assets/Tests/Editor/SettingsPresentationDecisionPolicyTests.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/CliPathSetupCheckPolicy.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/SkillInstallDialogPolicy.cs.metais excluded by none and included by none
📒 Files selected for processing (10)
Assets/Tests/Editor/SettingsPresentationDecisionPolicyTests.csAssets/Tests/Editor/UnityCliLoopSettingsWindowCliActionTests.csPackages/src/Editor/Domain/CliPathSetupCheckPolicy.csPackages/src/Editor/Domain/SkillInstallDialogPolicy.csPackages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsToolSettingsPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cs
| internal void InvalidateCatalog() | ||
| { | ||
| _isCatalogDirty = true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e
file='Packages/src/Editor/Presentation/UnityCliLoopSettingsToolSettingsPresenter.cs'
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo "== references =="
rg -n "_registryWarmupAttemptCount|InvalidateCatalog|ScheduleRegistryWarmup|ResetRegistryWarmupAttemptCount" "$file"
echo "== file excerpt =="
nl -ba "$file" | sed -n '1,260p'Repository: hatayama/unity-cli-loop
Length of output: 2407
🏁 Script executed:
rg -n "_registryWarmupAttemptCount|InvalidateCatalog|ScheduleRegistryWarmup|ResetRegistryWarmupAttemptCount" Packages/src/Editor/Presentation/UnityCliLoopSettingsToolSettingsPresenter.csRepository: hatayama/unity-cli-loop
Length of output: 821
🏁 Script executed:
set -e
file='Packages/src/Editor/Presentation/UnityCliLoopSettingsToolSettingsPresenter.cs'
echo "== relevant excerpt =="
sed -n '40,175p' "$file" | cat -nRepository: hatayama/unity-cli-loop
Length of output: 5353
🏁 Script executed:
set -e
file='Packages/src/Editor/Presentation/UnityCliLoopSettingsToolSettingsPresenter.cs'
sed -n '50,175p' "$file" | cat -nRepository: hatayama/unity-cli-loop
Length of output: 5047
🏁 Script executed:
set -e
rg -n "InvalidateCatalog\(" .Repository: hatayama/unity-cli-loop
Length of output: 529
🏁 Script executed:
set -e
file='Packages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cs'
sed -n '360,420p' "$file" | cat -nRepository: hatayama/unity-cli-loop
Length of output: 2515
Reset the warmup budget when invalidating the catalog. Packages/src/Editor/Presentation/UnityCliLoopSettingsToolSettingsPresenter.cs:50 only marks the catalog dirty; if the retry cap is already exhausted, a later invalidation (including UnityCliLoopSettingsWindow.InvalidateToolSettingsCatalog()) can’t restart registry warmup until another reset path runs.
🤖 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/Presentation/UnityCliLoopSettingsToolSettingsPresenter.cs`
around lines 50 - 53, Update InvalidateCatalog in
UnityCliLoopSettingsToolSettingsPresenter to reset the registry warmup/retry
budget whenever the catalog is invalidated, in addition to setting
_isCatalogDirty. Ensure invalidation through
UnityCliLoopSettingsWindow.InvalidateToolSettingsCatalog can restart warmup even
after the retry cap was exhausted.
a9e2798
into
feature/design-review-fixes-integration
Summary
Forget()instead of silentasync voidhandlers.User Impact
async void, so exceptions could disappear and window code kept growing harder to review.Changes
5a: convert remaining Presentationasync voidmethods toasync Taskand bridge Action/delegate call sites withForget().5b: Move Method for catalog cache/warmup intoUnityCliLoopSettingsToolSettingsPresenter, CLI primary-action mediation intoUnityCliLoopSettingsCliSetupPresenter, session restore/save inlined onto the model, and extractSkillInstallDialogPolicy/CliPathSetupCheckPolicy(Domain, no UnityEditor dependency) with focused tests.ConfigureAwait(false)(UI must continue on the main thread).Verification
uloop compile: Success, 0 errors / 0 warnings5a: 2043 passed, 0 failed, 7 skipped5b: 81 passed5b: 2048 passed, 0 failed, 7 skippedManual check required (do not merge until confirmed)
Please verify once in a running Editor:
Review notes
feature/design-review-fixes-integration(notv3-betadirectly)/review(bugbot or security)