Skip to content

fix: Improve skills setup panel empty state and refresh scoping#1983

Merged
hatayama merged 11 commits into
v3-betafrom
feature/skills-panel-refresh-scoping-and-empty-state
Jul 24, 2026
Merged

fix: Improve skills setup panel empty state and refresh scoping#1983
hatayama merged 11 commits into
v3-betafrom
feature/skills-panel-refresh-scoping-and-empty-state

Conversation

@hatayama

@hatayama hatayama commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

  • Drop Cursor and Gemini skill targets from Settings, detection, and the Go CLI (Gemini CLI ended; Common covers Antigravity's default .agents location).
  • The Install to specific target foldout follows the resolved skill-state default and reapplies that default when a reload changes it (for example, it opens while targets are missing and closes once they become installed).
  • When no agent skill folders are detected, Settings/Setup Wizard hide the bulk Install Skills button and show short muted guidance instead of a permanently disabled control.
  • The skills reload control (↻) now sits on the Skills / Step 2 heading row so it is easy to find without opening Install to specific target.
  • Refreshing CLI status no longer briefly forces the skills panel into Checking....

User Impact

  • Projects without .claude / .codex / similar folders no longer look stuck on a disabled Install Skills button.
  • Reloading skill detection is visible at the section heading in both Settings and Setup Wizard.
  • Pressing the CLI ↻ control only updates CLI status; the skills list stays interactive.

Changes

  • Shared skills panel empty-state label + visibility rules in SkillsSetupPanelView.
  • Move refresh-skills-state-button out of the Target row into each window's heading row; pass the button into SkillsSetupPanelView.
  • Split CliSetupData.IsSkillStateChecking from IsChecking so CLI refresh and skill-state refresh drive separate UI paths.

Verification

  • uloop compile → ErrorCount 0 / WarningCount 0
  • Filtered EditMode tests (CliSetupSectionTests|SkillsSetupPanelViewTests|SetupWizardWindowTests|StaticFacadeStateGuardTests|SkillsTargetSelectionResolverTests) → 178 passed
  • Manual screenshots (Settings + Setup Wizard):
    • ↻ is on the heading row (right side) and gone from the Target row
    • Heading spacing looks intact
    • Empty-state (0 targets) covered by unit tests in this project (folders present, so not reproduced live)

Screenshots

Local captures used for visual check (not committed):

  • Settings: /tmp/skills-panel-screenshots/Unity CLI Loop_20260724_223955_947.png
  • Setup Wizard: /tmp/skills-panel-screenshots/Unity CLI Loop Setup_20260724_224035_989.png

Review in cubic

hatayama and others added 3 commits July 24, 2026 22:25
…s are detected

When a project has no agent skill folders, the disabled Install Skills button
looked stuck. Hide it and show muted guidance so users know to use
Install to specific target instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
The refresh control lived next to the Target dropdown inside the foldout,
so it was easy to miss. Put it on the Skills / Step 2 heading so reloading
skill state is discoverable without expanding Install to specific target.

Co-authored-by: Cursor <cursoragent@cursor.com>
CLI refresh reused IsChecking for the skills panel, so pressing the CLI
reload control briefly showed Checking... on unrelated skills UI. Split
skill-state checking into IsSkillStateChecking so CLI refresh no longer
drives the shared skills panel.

Co-authored-by: Cursor <cursoragent@cursor.com>
@hatayama

Copy link
Copy Markdown
Owner Author

Visual check screenshots (Settings / Setup Wizard). Refresh control is on the heading row in both windows.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The skills UI separates skill-state checking from CLI checking, adds refresh controls and an empty-target message, preserves foldout behavior across updates, and removes cursor and Gemini skill targets while renaming the agents target to Common.

Changes

Skills state and panel behavior

Layer / File(s) Summary
Separate skills checking state
Packages/src/Editor/Presentation/UnityCliLoopSettingsSkillsPresenter.cs, Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs, Packages/src/Editor/Presentation/UnityCliLoopSettingsWindowViewData.cs, Packages/src/Editor/Presentation/UIToolkit/Components/CliSetupSection.cs
Skill scan availability now produces a dedicated IsSkillStateChecking value, separate from CLI checking, and drives the skills-panel checking UI.
Skills panel and refresh controls
Packages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs, Packages/src/Editor/Presentation/Shared/SkillsSetupPanel.uxml, Packages/src/Editor/Presentation/Shared/SkillsSetupPanel.uss, Packages/src/Editor/Presentation/Setup/*, Packages/src/Editor/Presentation/UIToolkit/UnityCliLoopSettingsWindow.*
The panel manages no-target guidance, bulk-install visibility, and foldout defaults; refresh buttons are wired into the setup wizard and Skills subsection headers.
Supported targets and validation
Packages/src/Editor/Infrastructure/SkillSetup/SkillTargetDetector.cs, Packages/src/Editor/Presentation/SkillsTargetSelectionResolver.cs, Packages/src/Editor/Presentation/UnityCliLoopSettingsWindowState.cs, cli/dispatcher/internal/dispatcher/skills.*, Assets/Tests/Editor/*
Cursor and Gemini targets are removed, Agents is renamed Common, and tests cover target mappings, state policy, empty targets, checking independence, and foldout behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SkillsPresenter as UnityCliLoopSettingsSkillsPresenter
  participant SetupPresenter as UnityCliLoopSettingsCliSetupPresenter
  participant CliSection as CliSetupSection
  participant SkillsView as SkillsSetupPanelView
  SkillsPresenter->>SetupPresenter: provide scan result and installable targets
  SetupPresenter->>CliSection: provide CLI and skill checking states
  CliSection->>SkillsView: update skills status
  SkillsView->>SkillsView: show checking, bulk install, or no-target message
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.55% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: skills setup empty-state handling and refresh scoping.
Description check ✅ Passed The description matches the changeset and explains the UI, refresh, and state-splitting updates.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/skills-panel-refresh-scoping-and-empty-state

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Title margins inside the heading row were included in flex centering, so
the label sat above the refresh control. Move those margins to the row
and clear them on the nested title so both windows align the text and
button on the same vertical center.

Co-authored-by: Cursor <cursoragent@cursor.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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/Setup/SetupWizardWindow.uss (1)

79-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Move title spacing to the new flex row.

.setup-step__title still has margin-bottom: 10px, but it is now a child of an align-items: center row. The refresh button is therefore centered against the label’s extra bottom margin instead of the visible title. Set the row’s bottom margin to 10px and reset the nested title margin to 0.

🤖 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.uss` around lines 79
- 89, Update the .setup-step__title-row styles to add a 10px bottom margin, and
reset .setup-step__title margin-bottom to 0 so the flex row controls spacing
without affecting title/button alignment.
🤖 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/Shared/SkillsSetupPanelView.cs`:
- Around line 154-157: Update the _skillsNoTargetsMessage visibility condition
in CliSetupSection.UpdateSkillsSubsection to also require canManageSkills, while
preserving the existing !isCheckingSkills and empty installableTargets checks.

---

Outside diff comments:
In `@Packages/src/Editor/Presentation/Setup/SetupWizardWindow.uss`:
- Around line 79-89: Update the .setup-step__title-row styles to add a 10px
bottom margin, and reset .setup-step__title margin-bottom to 0 so the flex row
controls spacing without affecting title/button alignment.
🪄 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: e8a8359a-e6bc-40cf-b09f-be27d1386c79

📥 Commits

Reviewing files that changed from the base of the PR and between eeb7aaa and 511da34.

📒 Files selected for processing (13)
  • Assets/Tests/Editor/CliSetupSectionTests.cs
  • Assets/Tests/Editor/SkillsTargetSelectionResolverTests.cs
  • Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs
  • Packages/src/Editor/Presentation/Setup/SetupWizardWindow.uss
  • Packages/src/Editor/Presentation/Setup/SetupWizardWindow.uxml
  • Packages/src/Editor/Presentation/Shared/SkillsSetupPanel.uss
  • Packages/src/Editor/Presentation/Shared/SkillsSetupPanel.uxml
  • Packages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs
  • Packages/src/Editor/Presentation/UIToolkit/Components/CliSetupSection.cs
  • Packages/src/Editor/Presentation/UIToolkit/UnityCliLoopSettingsWindow.uss
  • Packages/src/Editor/Presentation/UIToolkit/UnityCliLoopSettingsWindow.uxml
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsWindowViewData.cs

Comment thread Packages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs
@hatayama

Copy link
Copy Markdown
Owner Author

CodeRabbit follow-up

  1. Gate empty-state on canManageSkills — rejected (see reply on the inline comment). Visibility stays tied to detection; enablement stays tied to CLI/manageability.
  2. Move title spacing to the flex row — already fixed in 68c0f391 (fix: Center the skills heading label and refresh button vertically). Nested title margins are cleared; spacing lives on the heading row.
  3. Docstring coverage / generate unit tests — out of scope for this PR; no change.

hatayama and others added 7 commits July 24, 2026 22:55
…re missing

Keep Install to specific target collapsed when every detected target is
already installed or only outdated, and expand it when something still
needs install. Skip expansion while states are still Checking so the
resolved update can decide.

Co-authored-by: Cursor <cursoragent@cursor.com>
…window

Before the first skill-target scan result arrives, Settings briefly treated
an empty target list as resolved. That opened Install to specific target
via the empty-count rule and flashed the no-targets message. Keep skill
UI in checking until a real scan result is available.

Co-authored-by: Cursor <cursoragent@cursor.com>
…ill state changes

Open-only foldout updates left Install to specific target expanded after a
reload turned missing targets into installed ones. Apply the foldout default
only when that default changes so resolved installs can collapse it again
while still preserving deliberate user toggles.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Gemini CLI ended in 2026-06 when it moved to Antigravity CLI, and Cursor is no longer needed. Antigravity's default skill location is covered by Common (.agents), so the dropdown keeps only Claude, Codex, and Common. Existing .agent/--antigravity detection and Go flags are left unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/UnityCliLoopSettingsSkillsPresenter.cs (1)

153-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not mark an unscanned target list as resolved.

This branch clears _installableTargets without calling a target detector, then sets _hasSkillTargetScanResult = true. Downstream, that can make the empty list appear resolved and show “no agent skill folders detected” even when folders exist. Keep visibility tied to an actual folder scan: scan independently of CLI installation, or leave the flag unset until a real result is available.

🤖 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/UnityCliLoopSettingsSkillsPresenter.cs`
around lines 153 - 157, The CLI-not-installed branch in the
UnityCliLoopSettingsSkillsPresenter flow must not mark target scanning as
complete when no detector has run. Update the logic around _installableTargets
and _hasSkillTargetScanResult to perform the folder scan independently of CLI
installation, or leave _hasSkillTargetScanResult unset until an actual scan
result exists; preserve the empty-list assignment only if it represents a real
scan.
🤖 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.

Outside diff comments:
In `@Packages/src/Editor/Presentation/UnityCliLoopSettingsSkillsPresenter.cs`:
- Around line 153-157: The CLI-not-installed branch in the
UnityCliLoopSettingsSkillsPresenter flow must not mark target scanning as
complete when no detector has run. Update the logic around _installableTargets
and _hasSkillTargetScanResult to perform the folder scan independently of CLI
installation, or leave _hasSkillTargetScanResult unset until an actual scan
result exists; preserve the empty-list assignment only if it represents a real
scan.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ef21b89c-2f2e-49ca-9350-e06b22701581

📥 Commits

Reviewing files that changed from the base of the PR and between 68c0f39 and edc0b40.

📒 Files selected for processing (15)
  • Assets/Tests/Editor/CliSetupSectionTests.cs
  • Assets/Tests/Editor/SkillsSetupPanelViewTests.cs
  • Assets/Tests/Editor/SkillsTargetSelectionResolverTests.cs
  • Assets/Tests/Editor/UnityCliLoopSettingsWindowRefreshPolicyTests.cs
  • Packages/src/Editor/Infrastructure/SkillSetup/SkillTargetDetector.cs
  • Packages/src/Editor/Presentation/Setup/SetupWizardWindow.uss
  • Packages/src/Editor/Presentation/Shared/SkillsSetupPanelView.cs
  • Packages/src/Editor/Presentation/SkillsTargetSelectionResolver.cs
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsSkillsPresenter.cs
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsWindowEventHandler.cs
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsWindowState.cs
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsWindowViewData.cs
  • cli/dispatcher/internal/dispatcher/skills.go
  • cli/dispatcher/internal/dispatcher/skills_test.go
💤 Files with no reviewable changes (1)
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsWindowViewData.cs
🚧 Files skipped from review as they are similar to previous changes (3)
  • Packages/src/Editor/Presentation/Setup/SetupWizardWindow.uss
  • Packages/src/Editor/Presentation/UnityCliLoopSettingsCliSetupPresenter.cs
  • Assets/Tests/Editor/CliSetupSectionTests.cs

@hatayama
hatayama merged commit caeb079 into v3-beta Jul 24, 2026
13 checks passed
@hatayama
hatayama deleted the feature/skills-panel-refresh-scoping-and-empty-state branch July 24, 2026 15:01
@github-actions github-actions Bot mentioned this pull request Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant