chore: Keep setup skills step rendering separate from the window#1565
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughA new ChangesSkills step presenter extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SetupWizardWindow
participant SetupWizardSkillsStepPresenter
participant SkillsTargetList
SetupWizardWindow->>SetupWizardSkillsStepPresenter: ShowChecking(shouldUseFirstInstallSkillsUi)
SetupWizardSkillsStepPresenter->>SkillsTargetList: clear rows
SetupWizardWindow->>SetupWizardSkillsStepPresenter: Update(canManageSkills, isInstallingSkills, targets, options)
SetupWizardSkillsStepPresenter->>SetupWizardSkillsStepPresenter: FilterInstallableSkillTargets / GetSelectedSkillTargetInfo
SetupWizardSkillsStepPresenter->>SkillsTargetList: render rows with status text and CSS class
SetupWizardSkillsStepPresenter->>SetupWizardWindow: enable/disable install button, set label
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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Packages/src/Editor/Presentation/Setup/SetupWizardSkillsStepPresenter.cs (2)
29-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant double null-check in constructor.
Each parameter is null-checked twice: once via
Debug.Assert(lines 29-34) and again via?? throw new ArgumentNullException(lines 36-47). Since the throw always executes on null regardless of build configuration, the asserts add no additional protection here—they duplicate exactly the same check the throw already performs unconditionally.♻️ Simplify by removing the redundant asserts
- Debug.Assert(skillsTargetRow != null, "skillsTargetRow must not be null"); - Debug.Assert(skillsTargetList != null, "skillsTargetList must not be null"); - Debug.Assert(skillsStatusDivider != null, "skillsStatusDivider must not be null"); - Debug.Assert(skillsStatusLabel != null, "skillsStatusLabel must not be null"); - Debug.Assert(installSkillsButton != null, "installSkillsButton must not be null"); - Debug.Assert(onInstallSkillsClicked != null, "onInstallSkillsClicked must not be null"); - _skillsTargetRow = skillsTargetRow ?? throw new System.ArgumentNullException(nameof(skillsTargetRow));🤖 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/SetupWizardSkillsStepPresenter.cs` around lines 29 - 47, Remove the redundant null guards in SetupWizardSkillsStepPresenter’s constructor by deleting the Debug.Assert checks and keeping the existing ArgumentNullException validation for skillsTargetRow, skillsTargetList, skillsStatusDivider, skillsStatusLabel, installSkillsButton, and onInstallSkillsClicked; the constructor already enforces the null checks unconditionally, so the asserts in the initializer block add no extra value.
290-318: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSimplify the no-op ternary
GetSkillInstallStatusClassreturns"setup-target-item__status--different-layout"in both branches, so the ternary can be collapsed to a single return.🤖 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/SetupWizardSkillsStepPresenter.cs` around lines 290 - 318, In GetSkillInstallStatusClass, the final ternary is a no-op because both branches return the same "setup-target-item__status--different-layout" value. Simplify that return in SetupWizardSkillsStepPresenter so the method directly returns the string once after the hasDifferentLayoutSkills check, keeping the existing behavior unchanged.
🤖 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 `@Packages/src/Editor/Presentation/Setup/SetupWizardSkillsStepPresenter.cs`:
- Around line 29-47: Remove the redundant null guards in
SetupWizardSkillsStepPresenter’s constructor by deleting the Debug.Assert checks
and keeping the existing ArgumentNullException validation for skillsTargetRow,
skillsTargetList, skillsStatusDivider, skillsStatusLabel, installSkillsButton,
and onInstallSkillsClicked; the constructor already enforces the null checks
unconditionally, so the asserts in the initializer block add no extra value.
- Around line 290-318: In GetSkillInstallStatusClass, the final ternary is a
no-op because both branches return the same
"setup-target-item__status--different-layout" value. Simplify that return in
SetupWizardSkillsStepPresenter so the method directly returns the string once
after the hasDifferentLayoutSkills check, keeping the existing behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b805ecf-b7c7-4d84-8073-74b6708d37d6
⛔ Files ignored due to path filters (1)
Packages/src/Editor/Presentation/Setup/SetupWizardSkillsStepPresenter.cs.metais excluded by none and included by none
📒 Files selected for processing (3)
Assets/Tests/Editor/SetupWizardWindowTests.csPackages/src/Editor/Presentation/Setup/SetupWizardSkillsStepPresenter.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.cs
There was a problem hiding this comment.
1 issue found across 4 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/SetupWizardSkillsStepPresenter.cs">
<violation number="1" location="Packages/src/Editor/Presentation/Setup/SetupWizardSkillsStepPresenter.cs:74">
P2: Returning users with no existing tool folder can no longer install skills from the setup wizard; the presenter switches to the empty target-list path instead of the target-selection path when `installableTargets.Count == 0`. Consider preserving the previous `shouldUseFirstInstallSkillsUi || installableTargetCount == 0` condition and matching install-target selection logic.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ShouldShowSkillsTargetRowForSetupWizard(shouldUseFirstInstallSkillsUi)); | ||
| ViewDataBinder.SetVisible( | ||
| _skillsTargetList, | ||
| ShouldShowSkillsTargetListForSetupWizard(canManageSkills, shouldUseFirstInstallSkillsUi)); |
There was a problem hiding this comment.
P2: Returning users with no existing tool folder can no longer install skills from the setup wizard; the presenter switches to the empty target-list path instead of the target-selection path when installableTargets.Count == 0. Consider preserving the previous shouldUseFirstInstallSkillsUi || installableTargetCount == 0 condition and matching install-target selection logic.
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/SetupWizardSkillsStepPresenter.cs, line 74:
<comment>Returning users with no existing tool folder can no longer install skills from the setup wizard; the presenter switches to the empty target-list path instead of the target-selection path when `installableTargets.Count == 0`. Consider preserving the previous `shouldUseFirstInstallSkillsUi || installableTargetCount == 0` condition and matching install-target selection logic.</comment>
<file context>
@@ -0,0 +1,328 @@
+ ShouldShowSkillsTargetRowForSetupWizard(shouldUseFirstInstallSkillsUi));
+ ViewDataBinder.SetVisible(
+ _skillsTargetList,
+ ShouldShowSkillsTargetListForSetupWizard(canManageSkills, shouldUseFirstInstallSkillsUi));
+
+ if (!canManageSkills)
</file context>
This was referenced Jul 7, 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
Extract
SetupWizardSkillsStepPresenterfromSetupWizardWindowso the setup wizard skills step rendering is isolated from window orchestration.User Impact
No behavior change intended. The setup wizard should render the skills target/status/install controls exactly as before.
Changes
SetupWizardSkillsStepPresenterfor the skills step target row/list, status label/divider, and install button.SkillSetupUseCase, async target refresh, async install flow, success dialog decision, first-install mode decision, and CLI-to-skills manageability policy onSetupWizardWindow.group-skills-row,group-skills-toggle, andgroup-skills-labelonSetupWizardWindowbecause they mutate window state/settings rather than only render.SetupWizardWindowtoSetupWizardSkillsStepPresenter.Checking-State Mapping
The old skills checking block appeared in
ApplyInitialCheckingStateandRefreshUIwith the same behavior split:ViewDataBinder.SetVisible(_groupSkillsRow, false)remains inSetupWizardWindow._groupSkillsToggle.SetEnabled(false)remains inSetupWizardWindow.UpdateSkillsStatusLabel("Checking installed skills...")moved intoSetupWizardSkillsStepPresenter.ShowChecking(...)._installSkillsButton.SetEnabled(false)moved intoSetupWizardSkillsStepPresenter.ShowChecking(...)._installSkillsButton.text = "Checking..."moved intoSetupWizardSkillsStepPresenter.ShowChecking(...).SetupWizardSkillsStepPresenter.ShowChecking(...).SetupWizardSkillsStepPresenter.ShowChecking(...).Deviations
SetupWizardWindow.BindEventsto the presenter constructor, matching the CLI presenter pattern.UpdateSkillsStatusLabelis now a private presenter helper._groupSkillsToggle.SetEnabled(canManageSkills && !_isInstallingSkills)remains inSetupWizardWindowand is placed immediately beforeSetupWizardSkillsStepPresenter.Update(...)so the group-setting control stays window-owned.ShowCheckingtakesshouldUseFirstInstallSkillsUiso presenter rendering stays parameterized by window-owned state.Verification
dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"-> 0 errors / 0 warningsdist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type exact --filter-value "io.github.hatayama.UnityCliLoop.Tests.Editor.SetupWizardWindowTests"-> 110 passed