feat: Unify skills setup UI into a shared panel for settings and setup wizard#1982
Merged
Conversation
Extract the common skills status panel helpers and UI assets so Settings and Setup Wizard can share one CloneTree-based panel instead of divergent implementations. Co-authored-by: Cursor <cursoragent@cursor.com>
Wire Step 2 to SkillsSetupPanelView, drop the first-install-only UI mode, and delete SetupWizardSkillsStepPresenter now that both surfaces share one status panel plus specific-target foldout. Co-authored-by: Cursor <cursoragent@cursor.com>
Host SkillsSetupPanelView under Configuration, add bulk install handling, and thread installable target snapshots through CliSetupData so Settings matches the Setup Wizard skills UI. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe skills setup UI is centralized in a shared ChangesSkills setup panel refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SettingsWindow
participant SkillsPresenter
participant CliSetupSection
participant SkillsSetupPanelView
SettingsWindow->>SkillsPresenter: handle install-all callback
SkillsPresenter->>SkillsPresenter: install skill files with cancellation token
SkillsPresenter->>CliSetupSection: refresh installable target state
CliSetupSection->>SkillsSetupPanelView: render status and action controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs (1)
261-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated shared-panel loading logic.
This
LoadLayoutblock for loading/cloning theSkillsSetupPanelUXML/USS is nearly identical toUnityCliLoopSettingsWindowUI.LoadLayout. Consider extracting a small shared helper (e.g., a staticSkillsSetupPanelLoader.LoadInto(rootVisualElement, placeholderName)) to avoid maintaining two copies of this bootstrapping logic.🤖 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 261 - 290, The SkillsSetupPanel loading and cloning logic is duplicated in LoadLayout and UnityCliLoopSettingsWindowUI.LoadLayout. Extract it into a shared helper such as SkillsSetupPanelLoader.LoadInto, passing the target root element and placeholder name, then replace both implementations with calls to that helper while preserving the existing asset paths, assertions, cloning, and stylesheet registration.Packages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs (1)
40-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated assert+throw pattern for 11 fields.
Every queried element repeats the same
Debug.Assert(...)+?? throw new ArgumentNullException(...)pair. Extracting a small generic helper would remove ~35 lines of repetition and make it trivial to add new elements later.♻️ Suggested helper
+ private static T RequireElement<T>(T element, string name) where T : VisualElement + { + Debug.Assert(element != null, $"{name} must not be null"); + return element ?? throw new System.ArgumentNullException(name); + } + internal SkillsSetupPanelView(VisualElement panelRoot) { Debug.Assert(panelRoot != null, "panelRoot must not be null"); VisualElement root = panelRoot ?? throw new System.ArgumentNullException(nameof(panelRoot)); - _skillTargetStatusList = root.Q<VisualElement>("skill-target-status-list"); - ... - Debug.Assert(_skillTargetStatusList != null, "skill-target-status-list must not be null"); - ... - _ = _skillTargetStatusList ?? throw new System.ArgumentNullException(nameof(_skillTargetStatusList)); + _skillTargetStatusList = RequireElement(root.Q<VisualElement>("skill-target-status-list"), "skill-target-status-list"); + // ... repeat for each field🤖 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/Shared/SkillsSetupPanelView.cs` around lines 40 - 95, Refactor the SkillsSetupPanelView constructor to replace the repeated Debug.Assert and null-coalescing ArgumentNullException checks for the queried UI fields with a small reusable generic validation helper. Apply the helper to each Q result while preserving the existing assertion messages and fail-fast null behavior, and keep the panel initialization and event wiring 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.
Inline comments:
In
`@Packages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.cs`:
- Around line 64-77: Keep the group-skills toggle disabled throughout the Setup
Wizard by passing isEnabled: false in the relevant UpdateGroupSkillsToggle
calls, including InitializeGroupSkillsToggle and ShowChecking; preserve the
existing forced flat-install preference behavior.
- Around line 188-203: Update HandleInstallSkillsAsync to run
DetectDisplayedSkillTargets off the Unity Editor UI thread, using the existing
Task.Run pattern from the asynchronous skill-target refresh path. Await the
detection result before filtering or building installable targets, while
preserving the current installation flow and cancellation token handling.
In `@Packages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs`:
- Around line 97-106: Disable _skillsTargetField and _groupSkillsToggle in
SkillsSetupPanelView.ShowChecking() alongside the existing controls. In
Packages/src/Editor/Presentation/UIToolkit/Components/CliSetupSection.cs lines
147-157, update the UpdateGroupSkillsToggle isEnabled argument to include
!data.IsChecking so the group toggle remains disabled during checks.
- Around line 197-202: Remove the unconditional ViewDataBinder.SetVisible call
from SkillsSetupPanelView.UpdateGroupSkillsToggle, preserving the toggle value
and enabled-state updates. Keep row visibility controlled by construction or an
explicit visibility path so repeated calls from CliSetupSection.Update do not
hide supported grouping.
---
Nitpick comments:
In `@Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs`:
- Around line 261-290: The SkillsSetupPanel loading and cloning logic is
duplicated in LoadLayout and UnityCliLoopSettingsWindowUI.LoadLayout. Extract it
into a shared helper such as SkillsSetupPanelLoader.LoadInto, passing the target
root element and placeholder name, then replace both implementations with calls
to that helper while preserving the existing asset paths, assertions, cloning,
and stylesheet registration.
In `@Packages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs`:
- Around line 40-95: Refactor the SkillsSetupPanelView constructor to replace
the repeated Debug.Assert and null-coalescing ArgumentNullException checks for
the queried UI fields with a small reusable generic validation helper. Apply the
helper to each Q result while preserving the existing assertion messages and
fail-fast null behavior, and keep the panel initialization and event wiring
unchanged.
🪄 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 Plus
Run ID: 75b72f8c-9d50-46a7-bfa3-7d1a58ed24ef
⛔ Files ignored due to path filters (5)
Assets/Tests/Editor/SkillsSetupPanelViewTests.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/Shared.metais excluded by none and included by nonePackages/src/Editor/Presentation/Shared/SkillsSetupPanel.uss.metais excluded by none and included by nonePackages/src/Editor/Presentation/Shared/SkillsSetupPanel.uxml.metais excluded by none and included by nonePackages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs.metais excluded by none and included by none
📒 Files selected for processing (21)
Assets/Tests/Editor/CliSetupSectionTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/SkillsSetupPanelViewTests.csAssets/Tests/Editor/SkillsTargetSelectionResolverTests.csAssets/Tests/Editor/StaticFacadeStateGuardTests.csPackages/src/Editor/Presentation/Setup/SetupWizardSkillsStepPresenter.csPackages/src/Editor/Presentation/Setup/SetupWizardSkillsWorkflowController.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.ussPackages/src/Editor/Presentation/Setup/SetupWizardWindow.uxmlPackages/src/Editor/Presentation/Setup/SetupWizardWorkflowController.csPackages/src/Editor/Presentation/Shared/SkillsSetupPanel.ussPackages/src/Editor/Presentation/Shared/SkillsSetupPanel.uxmlPackages/src/Editor/Presentation/Shared/SkillsSetupPanelView.csPackages/src/Editor/Presentation/UIToolkit/Components/CliSetupSection.csPackages/src/Editor/Presentation/UIToolkit/UnityCliLoopSettingsWindow.uxmlPackages/src/Editor/Presentation/UIToolkit/UnityCliLoopSettingsWindowUI.csPackages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsSkillsPresenter.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindowViewData.cs
💤 Files with no reviewable changes (2)
- Packages/src/Editor/Presentation/Setup/SetupWizardSkillsStepPresenter.cs
- Packages/src/Editor/Presentation/Setup/SetupWizardWindow.uss
Run bulk target detection off the UI thread for wizard and settings install paths, latch installing state before the first await, and disable target/ group controls while ShowChecking is active. Co-authored-by: Cursor <cursoragent@cursor.com>
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
SkillsSetupPanelView+ UXML/USS).User Impact
Settings and Setup Wizard now show the same Skills installation status and actions, so agents and humans can see which targets are installed and install/update all or one target without switching mental models between windows.
Before / After
Before
After
Screenshots (After)
.uloop/outputs/Screenshots/Unity CLI Loop Setup_20260724_204818_279.png.uloop/outputs/Screenshots/Unity CLI Loop Setup_20260724_204849_190.png.uloop/outputs/Screenshots/Unity CLI Loop_20260724_205208_153.png.uloop/outputs/Screenshots/Unity CLI Loop_20260724_205250_478.pngImage files will also be attached in a follow-up PR comment.
Test plan
uloop compile— 0 errors / 0 warningsSkillsSetupPanelViewTests/SetupWizardWindowTests/CliSetupSectionTests/StaticFacadeStateGuardTests— passNativeCliInstallerTests.GetInstallCommand_RemoteBootstrapsEmitProgressLinesInStageOrder,WatchExpressionCompilerTests.CompileAsync_ValidExpressionReturnsCompiledEvaluator)