feat: Settings now shows selectable tool details - #1408
Merged
Conversation
Surface skill descriptions directly in Tool Settings so users can inspect each tool without relying on the stale CLI reference link. - Load source skill descriptions into the tool catalog - Show a per-tool ? button only when a description exists - Toggle the description popup for the selected tool
Change the per-tool help display from an inline block to an overlay dialog so descriptions do not shift the Tool Settings list layout. - Mount the description overlay at the settings window root - Style the dialog with a dim backdrop - Cover the overlay placement behavior with EditMode tests
Match the Unity MCP tools list pattern by replacing the question-mark dialog with a Show Details button that expands the selected tool description below its row. - Add an inline details row under the selected tool - Toggle button text between Show Details and Hide Details - Update Tool Settings tests for details row placement
Use a read-only multiline text field for expanded tool descriptions so users can select and copy the Show Details content without editing it.
Render the Tool Settings note as a read-only text field so users can select the simplified guidance text without editing it.
Show protocol-focused setup text when the installed and required CLI release versions match but the IPC protocol generation does not. Update the CLI action tests to match the current required protocol version.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSkill descriptions from SKILL.md are parsed into the tool catalog, carried into tool settings UI data, and shown in expandable tool details. CLI setup status and button labels now choose protocol-aware wording in compatibility cases. ChangesSkill Metadata and Setup UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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.
🧹 Nitpick comments (2)
Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs (1)
910-928: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated protocol-label helpers across two presentation classes.
GetCliReplacementButtonTextandShouldShowProtocolCompatibilityTexthere are byte-identical to the copies inCliSetupSection.cs(Lines 256-274). The two will need to stay in lockstep; consider extracting them into a shared helper (e.g. alongsideCliVersionComparer/CliConstants) so the protocol-vs-version wording rule lives in one place.🤖 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 910 - 928, The protocol-label logic is duplicated between SetupWizardWindow.GetCliReplacementButtonText/ShouldShowProtocolCompatibilityText and the copy in CliSetupSection, so it can drift out of sync. Extract this wording decision into one shared helper near CliVersionComparer/CliConstants, then update both callers to use it so the protocol-vs-version button text is defined in a single place.Assets/Tests/Editor/ToolSkillSynchronizerTests.cs (1)
1139-1147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fixture-backed test for the
name-prefix fallback.This asserts current built-in SKILL.md copy, but the new production branch in
ResolveToolNameForSkillSourceis the fallback whentoolNameis omitted. A temporary skill fixture withdescription:and notoolName:would pin that branch directly and avoid copy-only metadata edits breaking the parser test.🤖 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/ToolSkillSynchronizerTests.cs` around lines 1139 - 1147, The existing test in GetToolDescriptionsByToolName only verifies current built-in SKILL.md text, but it does not cover the new fallback path in ResolveToolNameForSkillSource when toolName is omitted. Update ToolSkillSynchronizerTests to add a fixture-backed case using a temporary skill with description and no toolName so the mapping logic is exercised directly. Keep the current assertions for description lookup, but base the new test on the fallback branch rather than copy-specific metadata so parser behavior is pinned even if built-in skill text changes.
🤖 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 `@Assets/Tests/Editor/ToolSkillSynchronizerTests.cs`:
- Around line 1139-1147: The existing test in GetToolDescriptionsByToolName only
verifies current built-in SKILL.md text, but it does not cover the new fallback
path in ResolveToolNameForSkillSource when toolName is omitted. Update
ToolSkillSynchronizerTests to add a fixture-backed case using a temporary skill
with description and no toolName so the mapping logic is exercised directly.
Keep the current assertions for description lookup, but base the new test on the
fallback branch rather than copy-specific metadata so parser behavior is pinned
even if built-in skill text changes.
In `@Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs`:
- Around line 910-928: The protocol-label logic is duplicated between
SetupWizardWindow.GetCliReplacementButtonText/ShouldShowProtocolCompatibilityText
and the copy in CliSetupSection, so it can drift out of sync. Extract this
wording decision into one shared helper near CliVersionComparer/CliConstants,
then update both callers to use it so the protocol-vs-version button text is
defined in a single place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e8e449e-3140-4d6e-99eb-04003bdaae16
⛔ Files ignored due to path filters (1)
Packages/src/Editor/Infrastructure/SkillSetup/SkillInstallLayoutToolSkillDescriptionProvider.cs.metais excluded by none and included by none
📒 Files selected for processing (18)
Assets/Tests/Editor/CliSetupSectionTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/ToolSettingsSectionTests.csAssets/Tests/Editor/ToolSettingsUseCaseTests.csAssets/Tests/Editor/ToolSkillSynchronizerTests.csAssets/Tests/Editor/UnityCliLoopSettingsWindowCliActionTests.csPackages/src/Editor/Application/UseCases/ToolSettingsUseCase.csPackages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.csPackages/src/Editor/Infrastructure/SkillSetup/SkillInstallLayout.csPackages/src/Editor/Infrastructure/SkillSetup/SkillInstallLayoutInternalToolNameProvider.csPackages/src/Editor/Infrastructure/SkillSetup/SkillInstallLayoutToolSkillDescriptionProvider.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/UIToolkit/Components/CliSetupSection.csPackages/src/Editor/Presentation/UIToolkit/Components/ToolSettingsSection.csPackages/src/Editor/Presentation/UIToolkit/UnityCliLoopSettingsWindow.ussPackages/src/Editor/Presentation/UIToolkit/UnityCliLoopSettingsWindow.uxmlPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindowViewData.cs
💤 Files with no reviewable changes (1)
- Packages/src/Editor/Infrastructure/SkillSetup/SkillInstallLayoutInternalToolNameProvider.cs
There was a problem hiding this comment.
2 issues found across 19 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Fail fast when Tool Settings misses its required info container and share protocol mismatch label formatting between Settings and Setup Wizard to avoid drift.
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