chore: Keep Settings and wizard presentation sources under 500 lines - #1787
Merged
hatayama merged 2 commits intoJul 14, 2026
Merged
Conversation
…t signature Extract Class per Fowler: separate foldout/section shell from ListView ownership, row VisualElement binding, and the pure rebuild-signature algorithm. Pin the signature format with characterization tests so list rebuild decisions stay stable. Co-authored-by: Cursor <cursoragent@cursor.com>
…ines Move Method / Extract Class: Settings CLI and skills orchestration into presenters; Setup Wizard CLI/skills workflow into controllers; Migration Wizard operations into a workflow controller. Windows keep shell/DI/static facades. No intended behavior change. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR extracts Setup Wizard, migration-wizard, CLI Settings, and Tool Settings workflow logic into dedicated controllers and presenters. It also adds virtualized tool-list row infrastructure, deterministic layout signatures, expanded-details handling, and characterization tests. ChangesSetup Wizard and Settings workflows
Third-party migration workflow
Tool Settings list rendering
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SetupWizardWindow
participant SetupWizardWorkflowController
participant CliWorkflowController
participant SkillsWorkflowController
SetupWizardWindow->>SetupWizardWorkflowController: Bind UI and start refresh
SetupWizardWorkflowController->>CliWorkflowController: Refresh CLI state
SetupWizardWorkflowController->>SkillsWorkflowController: Refresh skills state
CliWorkflowController-->>SetupWizardWorkflowController: Return CLI installed/path state
SkillsWorkflowController-->>SetupWizardWorkflowController: Return target/install state
SetupWizardWorkflowController-->>SetupWizardWindow: Update UI and resize
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 |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
hatayama
merged commit Jul 14, 2026
2552640
into
feature/god-class-split-integration
1 of 2 checks passed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/Presentation/UnityCliLoopSettingsWindow.cs (1)
352-359: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCapture
_skillsPresenterbefore scheduling the delayed callback
OnDisable/OnDestroyclear_skillsPresenter, so the deferred lambda can hit a null field if the window closes beforedelayCallruns. Capture the presenter into a local before registering the callback.🤖 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 352 - 359, Update HandleToolToggled to copy _skillsPresenter into a local variable before registering the EditorApplication.delayCall callback, then have the deferred lambda invoke ApplyToolToggleSideEffects on that captured presenter instead of the mutable field. Preserve the existing deferred execution and Forget behavior.
🧹 Nitpick comments (4)
Assets/Tests/Editor/ToolSettingsSectionLayoutSignatureTests.cs (1)
10-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test pinning the IsEnabled-invariant.
Consider a third test asserting that toggling
IsEnabledon an item does NOT change the signature. This is the exact invariantToolSettingsSectionListViewController.UpdateToolListrelies on to decide "incremental toggle update" vs "full rebuild" — worth locking down explicitly since it's a critical/easy-to-break characterization test target.🤖 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/ToolSettingsSectionLayoutSignatureTests.cs` around lines 10 - 48, Add a third test in ToolSettingsSectionLayoutSignatureTests that creates equivalent data with an item’s IsEnabled value toggled and asserts ToolSettingsSectionLayoutSignature.Create returns the same signature. Cover the relevant tool group and preserve the existing signature fields, explicitly locking the invariant used by ToolSettingsSectionListViewController.UpdateToolList.Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.cs (1)
41-61: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant full refresh after a direct toggle update.
UpdateSingleTogglesetstoggle.SetValueWithoutNotify(enabled)directly, then immediately callsRefreshToolListView(), which clears_togglesByToolNameand callsRefreshItems()— forcing all visible rows to unbind/rebind, which re-applies the sameSetValueWithoutNotify(item.IsEnabled)viaBindToolListRowElement. The manual direct-set on lines 55-58 is dead work; every single toggle click now pays for a full visible-row rebind it doesn't need.♻️ Proposed fix
public void UpdateSingleToggle(string toolName, bool enabled) { for (int i = 0; i < _toolListRows.Count; i++) { ToolListRowData row = _toolListRows[i]; if (!row.IsTool || row.ToolName != toolName) { continue; } row.IsEnabled = enabled; break; } - if (_togglesByToolName.TryGetValue(toolName, out Toggle toggle)) - { - toggle.SetValueWithoutNotify(enabled); - } - - RefreshToolListView(); + if (_togglesByToolName.TryGetValue(toolName, out Toggle toggle)) + { + toggle.SetValueWithoutNotify(enabled); + } }Please confirm via Unity's
ListView.RefreshItems()docs/behavior that visible items are indeed unbound/rebound on every call (as assumed here) before dropping the refresh.🤖 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/UIToolkit/Components/ToolSettingsSectionListViewController.cs` around lines 41 - 61, Confirm Unity ListView.RefreshItems() behavior for the current UI Toolkit version, specifically whether it unbinds and rebinds visible rows on every call. If confirmed, remove the redundant RefreshToolListView() call from UpdateSingleToggle while preserving the row-model update and direct Toggle.SetValueWithoutNotify update; otherwise retain the refresh and adjust only if needed based on the documented behavior.Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs (1)
201-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate CLI-path-setup refresh logic.
RefreshCliPathSetupInBackgroundandRefreshCliPathSetupAsyncimplement the same shell-visibility check and_needsCliPathSetupassignment. Have the former delegate to the latter to avoid the two implementations drifting apart.♻️ Proposed refactor
internal async Task RefreshCliPathSetupInBackground() { if (_isRefreshingCliPathSetup) { return; } - if (!ShouldCheckCliPathSetup()) - { - _needsCliPathSetup = false; - return; - } - _isRefreshingCliPathSetup = true; RefreshSection(); try { - bool isCliVisibleFromShell = await _cliSetupApplicationService.IsCliVisibleFromShellAsync( - UnityEngine.Application.platform, - CancellationToken.None); - _needsCliPathSetup = !isCliVisibleFromShell; + await RefreshCliPathSetupAsync(CancellationToken.None); } finally { _isRefreshingCliPathSetup = false; RefreshSection(); } }Also applies to: 335-347
🤖 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 201 - 229, Update RefreshCliPathSetupInBackground to delegate the CLI visibility check and _needsCliPathSetup update to RefreshCliPathSetupAsync instead of duplicating that logic. Preserve the existing refresh-state guard and UI refresh behavior, while ensuring the background method awaits the shared implementation.Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs (1)
87-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew workflow controllers depend back on
SetupWizardWindowfor policy decisions and a policy constant. All five sites share one root cause:ShouldRepairCliPathFromPrimaryButton,ShouldCheckCliPathSetupForSetupWizard,CanManageSkills,ShouldUseFirstInstallSkillsUi, andForceFlatSkillInstallareinternal staticmembers on the View (SetupWizardWindow), so the newly-extracted controllers must call back into the Window they're meant to be decoupled from — a dependency direction that didn't exist before this extraction, since the logic previously lived inside the Window itself.CanManageSkillsis a pure identity function (return cliInstalled;), so two of the call sites can simply be inlined.
Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs#L87-L101: callCliSetupPrimaryActionPolicy.ShouldRepairCliPath(...)directly instead ofSetupWizardWindow.ShouldRepairCliPathFromPrimaryButton(...).Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs#L193-L208: callCliPathSetupCheckPolicy.ShouldCheck(...)directly instead ofSetupWizardWindow.ShouldCheckCliPathSetupForSetupWizard(...).Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs#L128-L147: dropSetupWizardWindow.CanManageSkills(cliInstalled)and usecliInstalleddirectly at both call sites.Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs#L161-L165: inline thestring.IsNullOrEmpty(lastSeenSetupWizardVersionBeforeOpen)check instead of callingSetupWizardWindow.ShouldUseFirstInstallSkillsUi(...).Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs#L278-L283: move theForceFlatSkillInstallconstant to a single shared location (e.g. a Domain/Policy class) instead of independently declaring/reading it from bothSetupWizardWindow.csandUnityCliLoopSettingsSkillsPresenter.cs.🤖 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/SetupWizardCliWorkflowController.cs` around lines 87 - 101, Remove the extracted controllers’ policy dependencies on SetupWizardWindow: in Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs#L87-L101 call CliSetupPrimaryActionPolicy.ShouldRepairCli directly, and at `#L193-L208` call CliPathSetupCheckPolicy.ShouldCheck directly; in Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs#L128-L147 use cliInstalled directly, at `#L161-L165` inline the string.IsNullOrEmpty(lastSeenSetupWizardVersionBeforeOpen) check, and at `#L278-L283` move ForceFlatSkillInstall to one shared Domain/Policy location used by both SetupWizardWindow.cs and UnityCliLoopSettingsSkillsPresenter.cs.
🤖 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/Setup/SetupWizardWorkflowController.cs`:
- Around line 131-172: The RefreshUI flow allows ScheduleInitialRefresh and the
Refresh button to run RefreshUIAsync concurrently. Add an in-flight guard or
replaceable cancellation source used by RefreshUI, cancel or ignore any existing
refresh before starting a new one, and pass that cancellation token into
RefreshUIAsync instead of always using CancellationToken.None; ensure the guard
is cleared when the refresh completes.
In
`@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWorkflowController.cs`:
- Around line 138-146: Remove the controller’s dependency on
ThirdPartyToolMigrationWizardWindow by replacing its calls to
ConfirmMigrationApply, ShouldFinishMigrationOnMainThread,
ShouldRefreshAfterInterruptedMigration, ShouldRefreshAfterMigration, and
ShouldRemoveMigrationSkill with ThirdPartyToolMigrationWizardStateRules calls.
Move the inline ConfirmMigrationApply and ShouldRemoveMigrationSkill decision
logic into the rules class first if needed, preserving existing behavior and
signatures at the controller call sites.
In `@Packages/src/Editor/Presentation/UIToolkit/Components/ToolListRowData.cs`:
- Around line 15-16: Remove the UI back-reference and route details toggling
through a callback. In
Packages/src/Editor/Presentation/UIToolkit/Components/ToolListRowData.cs#L15-L16
remove Owner; in
Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.cs#L26-L39
remove the owner constructor parameter and pass ToggleToolDetailsForTool as the
Action<string> callback; in
Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionRowBinder.cs#L79-L94
replace _owner and item.Owner usage with a non-null _onDetailsToggled invocation
and remove the assignment; in
Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSection.cs#L37-L41
stop passing this to the controller.
---
Outside diff comments:
In `@Packages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cs`:
- Around line 352-359: Update HandleToolToggled to copy _skillsPresenter into a
local variable before registering the EditorApplication.delayCall callback, then
have the deferred lambda invoke ApplyToolToggleSideEffects on that captured
presenter instead of the mutable field. Preserve the existing deferred execution
and Forget behavior.
---
Nitpick comments:
In `@Assets/Tests/Editor/ToolSettingsSectionLayoutSignatureTests.cs`:
- Around line 10-48: Add a third test in ToolSettingsSectionLayoutSignatureTests
that creates equivalent data with an item’s IsEnabled value toggled and asserts
ToolSettingsSectionLayoutSignature.Create returns the same signature. Cover the
relevant tool group and preserve the existing signature fields, explicitly
locking the invariant used by
ToolSettingsSectionListViewController.UpdateToolList.
In `@Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs`:
- Around line 87-101: Remove the extracted controllers’ policy dependencies on
SetupWizardWindow: in
Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs#L87-L101
call CliSetupPrimaryActionPolicy.ShouldRepairCli directly, and at `#L193-L208`
call CliPathSetupCheckPolicy.ShouldCheck directly; in
Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs#L128-L147
use cliInstalled directly, at `#L161-L165` inline the
string.IsNullOrEmpty(lastSeenSetupWizardVersionBeforeOpen) check, and at
`#L278-L283` move ForceFlatSkillInstall to one shared Domain/Policy location used
by both SetupWizardWindow.cs and UnityCliLoopSettingsSkillsPresenter.cs.
In
`@Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.cs`:
- Around line 41-61: Confirm Unity ListView.RefreshItems() behavior for the
current UI Toolkit version, specifically whether it unbinds and rebinds visible
rows on every call. If confirmed, remove the redundant RefreshToolListView()
call from UpdateSingleToggle while preserving the row-model update and direct
Toggle.SetValueWithoutNotify update; otherwise retain the refresh and adjust
only if needed based on the documented behavior.
In `@Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs`:
- Around line 201-229: Update RefreshCliPathSetupInBackground to delegate the
CLI visibility check and _needsCliPathSetup update to RefreshCliPathSetupAsync
instead of duplicating that logic. Preserve the existing refresh-state guard and
UI refresh behavior, while ensuring the background method awaits the shared
implementation.
🪄 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: 8f05b6d3-1c4d-469e-aac7-7cf37561a592
⛔ Files ignored due to path filters (10)
Assets/Tests/Editor/ToolSettingsSectionLayoutSignatureTests.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/Setup/SetupWizardWorkflowController.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWorkflowController.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/UIToolkit/Components/ToolListRowData.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionLayoutSignature.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionRowBinder.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/UnityCliLoopSettingsSkillsPresenter.cs.metais excluded by none and included by none
📒 Files selected for processing (15)
Assets/Tests/Editor/ToolSettingsSectionLayoutSignatureTests.csPackages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.csPackages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/Setup/SetupWizardWorkflowController.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWorkflowController.csPackages/src/Editor/Presentation/UIToolkit/Components/ToolListRowData.csPackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSection.csPackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionLayoutSignature.csPackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.csPackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionRowBinder.csPackages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsSkillsPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cs
Comment on lines
+131
to
+172
| internal void RefreshUI(bool refreshSkillsSection = true) | ||
| { | ||
| RefreshUIAsync(refreshSkillsSection, CancellationToken.None).Forget(); | ||
| } | ||
|
|
||
| private void RefreshAutoShowToggle() | ||
| { | ||
| _suppressAutoShowToggle.SetValueWithoutNotify(_editorSettingsPort.GetSuppressSetupWizardAutoShow()); | ||
| } | ||
|
|
||
| private async Task RefreshUIAsync( | ||
| bool refreshSkillsSection, | ||
| CancellationToken ct) | ||
| { | ||
| CancelSkillInstallStateRefresh(); | ||
| ct.ThrowIfCancellationRequested(); | ||
| RefreshAutoShowToggle(); | ||
| ViewDataBinder.SetVisible(_nodejsWarning, false); | ||
| ViewDataBinder.SetVisible(_nodejsOk, false); | ||
| _cliWorkflow.ShowChecking(); | ||
| if (refreshSkillsSection) | ||
| { | ||
| _skillsWorkflow.ShowChecking(); | ||
| } | ||
|
|
||
| await Task.Yield(); | ||
| ct.ThrowIfCancellationRequested(); | ||
|
|
||
| ViewDataBinder.SetVisible(_nodejsWarning, false); | ||
| ViewDataBinder.SetVisible(_nodejsOk, false); | ||
|
|
||
| bool cliInstalled = await _cliWorkflow.RefreshAndUpdateAsync(ct); | ||
|
|
||
| if (!refreshSkillsSection) | ||
| { | ||
| _scheduleResizeToContent(); | ||
| return; | ||
| } | ||
|
|
||
| _skillsWorkflow.ApplyFastSkillsState(cliInstalled); | ||
| _scheduleResizeToContent(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding code.
git ls-files 'Packages/src/Editor/Presentation/Setup/*' 'Packages/src/Editor/Presentation/*Settings*' 'Packages/src/Editor/Presentation/*' | sed -n '1,200p'
echo
echo '--- ast-grep outline: SetupWizardWorkflowController.cs ---'
ast-grep outline Packages/src/Editor/Presentation/Setup/SetupWizardWorkflowController.cs --view expanded || true
echo
echo '--- ast-grep outline: UnityCliLoopSettingsCliSetupPresenter ---'
fd -a 'UnityCliLoopSettingsCliSetupPresenter.*' Packages/src || true
echo
echo '--- rg references for RefreshUI / HandleRefreshCliVersion / _isRefreshingVersion ---'
rg -n "_isRefreshingVersion|HandleRefreshCliVersion|RefreshUIAsync|ScheduleInitialRefresh|ForceRefreshCliVersionAsync|CancelSkillInstallStateRefresh" Packages/src/Editor -SRepository: hatayama/unity-cli-loop
Length of output: 14641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant code slices around the methods and any refresh guards.
for f in \
Packages/src/Editor/Presentation/Setup/SetupWizardWorkflowController.cs \
Packages/src/Editor/Presentation/Setup/UnityCliLoopSettingsCliSetupPresenter.cs
do
if [ -f "$f" ]; then
echo "=== $f ==="
wc -l "$f"
echo
fi
done
echo '--- SetupWizardWorkflowController.cs (lines 1-260) ---'
sed -n '1,260p' Packages/src/Editor/Presentation/Setup/SetupWizardWorkflowController.cs
echo
echo '--- UnityCliLoopSettingsCliSetupPresenter.cs (relevant sections) ---'
sed -n '1,260p' Packages/src/Editor/Presentation/Setup/UnityCliLoopSettingsCliSetupPresenter.csRepository: hatayama/unity-cli-loop
Length of output: 7392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find call sites for the controller refresh path and the initial refresh scheduling.
rg -n "RefreshUI\\(|RefreshUIAsync\\(|ScheduleInitialRefresh\\(|RefreshUI\\b" Packages/src/Editor/Presentation -S
echo
echo '--- controller file around refresh and scheduling methods ---'
sed -n '1,360p' Packages/src/Editor/Presentation/Setup/SetupWizardWorkflowController.csRepository: hatayama/unity-cli-loop
Length of output: 9974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- SetupWizardCliWorkflowController.cs ---'
wc -l Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs
sed -n '1,260p' Packages/src/Editor/Presentation/Setup/SetupWizardCliWorkflowController.cs
echo
echo '--- SetupWizardSkillsWorkflowController.cs ---'
wc -l Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs
sed -n '1,280p' Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.csRepository: hatayama/unity-cli-loop
Length of output: 20760
Serialize RefreshUI ScheduleInitialRefresh() and the Refresh button can overlap, and RefreshUIAsync has no in-flight guard. That lets two refreshes race on _cliWorkflow / _skillsWorkflow and call ForceRefreshCliVersionAsync in parallel. RefreshUI also always passes CancellationToken.None, so the cancellation checks never suppress a prior run. Use a simple _isRefreshing flag or CTS swap to ignore/cancel the previous refresh before starting a new one.
🤖 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/SetupWizardWorkflowController.cs`
around lines 131 - 172, The RefreshUI flow allows ScheduleInitialRefresh and the
Refresh button to run RefreshUIAsync concurrently. Add an in-flight guard or
replaceable cancellation source used by RefreshUI, cancel or ignore any existing
refresh before starting a new one, and pass that cancellation token into
RefreshUIAsync instead of always using CancellationToken.None; ensure the guard
is cleared when the refresh completes.
Comment on lines
+138
to
+146
| internal async Task HandleMigrateThirdPartyTools() | ||
| { | ||
| if (!ThirdPartyToolMigrationWizardWindow.ConfirmMigrationApply( | ||
| _pendingMigrationFilePaths.Length, | ||
| (title, message, ok, cancel) => EditorUtility.DisplayDialog(title, message, ok, cancel))) | ||
| { | ||
| return; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Controller depends back on the EditorWindow for pure decision logic.
ThirdPartyToolMigrationWizardWorkflowController calls ThirdPartyToolMigrationWizardWindow.ConfirmMigrationApply (140-142), .ShouldFinishMigrationOnMainThread (160-165, 168-173), .ShouldRefreshAfterInterruptedMigration (206-208), .ShouldRefreshAfterMigration (221), and .ShouldRemoveMigrationSkill (302-303). This inverts the dependency direction this refactor is meant to establish: the extracted controller now depends on the concrete EditorWindow subclass it was pulled out of, rather than the window depending on the controller. Most of these Window methods already just forward to ThirdPartyToolMigrationWizardStateRules, so the controller could call the rules class directly; ConfirmMigrationApply/ShouldRemoveMigrationSkill (implemented inline on the Window) would need to move to the rules class or the controller itself.
♻️ Example direction for the fix
- bool shouldRemoveMigrationSkill =
- ThirdPartyToolMigrationWizardWindow.ShouldRemoveMigrationSkill(currentInstallState);
+ bool shouldRemoveMigrationSkill =
+ ThirdPartyToolMigrationWizardStateRules.ShouldRemoveMigrationSkill(currentInstallState);(and similarly route the other four call sites through ThirdPartyToolMigrationWizardStateRules, moving ConfirmMigrationApply/ShouldRemoveMigrationSkill there if they aren't already present.)
Also applies to: 160-173, 206-208, 221-221, 292-303
🤖 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/ThirdPartyToolMigrationWizardWorkflowController.cs`
around lines 138 - 146, Remove the controller’s dependency on
ThirdPartyToolMigrationWizardWindow by replacing its calls to
ConfirmMigrationApply, ShouldFinishMigrationOnMainThread,
ShouldRefreshAfterInterruptedMigration, ShouldRefreshAfterMigration, and
ShouldRemoveMigrationSkill with ThirdPartyToolMigrationWizardStateRules calls.
Move the inline ConfirmMigrationApply and ShouldRemoveMigrationSkill decision
logic into the rules class first if needed, preserving existing behavior and
signatures at the controller call sites.
Comment on lines
+15
to
+16
| public bool IsEnabled; | ||
| public ToolSettingsSection Owner; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Row model shouldn't carry a UI-class back-reference — route details-toggle through a callback instead. ToolListRowData.Owner exists only so the details button can call back into ToolSettingsSection, which just forwards to the controller that already created the row binder. This produces an unnecessary 3-hop chain (RowBinder → row.Owner → ToolSettingsSection → controller) when the controller could hand the RowBinder a plain Action<string> callback wired to its own ToggleToolDetailsForTool, exactly like the existing onToolToggled callback.
Packages/src/Editor/Presentation/UIToolkit/Components/ToolListRowData.cs#L15-L16: remove theOwnerfield entirely.Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.cs#L26-L39: drop theownerctor parameter; passToggleToolDetailsForToolas anAction<string>to the row binder instead.Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionRowBinder.cs#L79-L94: replace theowner/_ownerfield anditem.Owner?.ToggleToolDetailsForTool(...)call with a storedAction<string> _onDetailsToggledinvoked directly (no null-conditional, consistent with this file's Debug.Assert fail-fast convention); also drop theitem.Owner = _owner;assignment inBindToolListRowElement.Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSection.cs#L37-L41: stop passingthisinto the controller constructor once theownerparameter is removed.
📍 Affects 4 files
Packages/src/Editor/Presentation/UIToolkit/Components/ToolListRowData.cs#L15-L16(this comment)Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.cs#L26-L39Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionRowBinder.cs#L79-L94Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSection.cs#L37-L41
🤖 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/UIToolkit/Components/ToolListRowData.cs`
around lines 15 - 16, Remove the UI back-reference and route details toggling
through a callback. In
Packages/src/Editor/Presentation/UIToolkit/Components/ToolListRowData.cs#L15-L16
remove Owner; in
Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionListViewController.cs#L26-L39
remove the owner constructor parameter and pass ToggleToolDetailsForTool as the
Action<string> callback; in
Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSectionRowBinder.cs#L79-L94
replace _owner and item.Owner usage with a non-null _onDetailsToggled invocation
and remove the assignment; in
Packages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSection.cs#L37-L41
stop passing this to the controller.
6 tasks
This was referenced Jul 19, 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
uloop compile→ 0/0dotnet test tests/UnityCliLoop.CodeComplexity.Tests→ 12 passedMerge gate
動作チェックOK).Made with Cursor