feat(redesign): skills and MCP action controls in settings#942
feat(redesign): skills and MCP action controls in settings#942viper151 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA reusable ChangesShared UI menu and MCP add-server flow
Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/skills/view/ProviderSkills.tsx (2)
492-504: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCancel button stays enabled while a submission is in flight.
isSubmittingdisables the Install button (Line 510) but not Cancel, so a user can hide the panel and clearqueuedFileswhilehandleUploadInstallis still awaitingaddSkills. The async call still completes in the background and can flipsaveStatus/refresh skills after the panel is already dismissed, which is harmless functionally but confusing.🤖 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 `@src/components/skills/view/ProviderSkills.tsx` around lines 492 - 504, The Cancel button in ProviderSkills should also be disabled while a submission is in flight so the panel state cannot be reset during an active install. Update the Button in the cancel handler block to respect isSubmitting, alongside the existing Install button logic in handleUploadInstall/addSkills flow, so users cannot clear queuedFiles or close the panel before the async submit finishes.
491-504: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent state cleanup between Cancel and the "Add Skill" toggle.
Cancel (Line 497-501) clears
queuedFiles/submitErrorand closes the panel, but clicking the "Add Skill" button again to close it (Line 564, togglingisAddPanelOpenoff) leavesqueuedFilesuntouched. Reopening the panel later then shows previously queued files that the user may have expected to be discarded. Consider clearingqueuedFileswhenever the panel is closed (toggle or Cancel) for consistent behavior.Also applies to: 557-569
🤖 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 `@src/components/skills/view/ProviderSkills.tsx` around lines 491 - 504, The panel close behavior in ProviderSkills is inconsistent: the Cancel handler clears queuedFiles and submitError, but the Add Skill toggle path only flips isAddPanelOpen and leaves queuedFiles behind. Update the close logic used by both the Cancel button and the Add Skill toggle so closing the panel always resets queuedFiles (and submitError if appropriate), keeping reopen behavior consistent. Use the existing state setters in ProviderSkills and the add-panel toggle handler to centralize the cleanup.src/shared/view/ui/ActionMenu.tsx (1)
48-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMenu opens without shifting focus into it; closing doesn't restore focus to the trigger.
role="menu"sets accessibility-tree expectations (arrow-key roving, focus moving into the menu) that aren't met here — focus stays on the trigger when opened, and isn't returned to it on close (Escape/selection/outside-click). Items remain reachable via Tab, so this isn't a hard blocker, but it's a quick fix that meaningfully improves screen-reader/keyboard UX.♿ Suggested fix: shift focus on open, restore on close
+ const firstItemRef = React.useRef<HTMLButtonElement | null>(null); + const triggerRef = React.useRef<HTMLButtonElement | null>(null); + React.useEffect(() => { if (!isOpen) { + triggerRef.current?.focus(); return; } + firstItemRef.current?.focus(); const closeOnOutsideClick = (event: MouseEvent) => {🤖 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 `@src/shared/view/ui/ActionMenu.tsx` around lines 48 - 105, The ActionMenu opens and closes without managing focus, so the trigger keeps focus instead of moving into the menu and focus is not restored after close. Update ActionMenu in the component using isOpen, rootRef, menuId, and runItem so that opening the menu programmatically focuses the first menu item or the menu container, and closing via Escape, outside click, or item selection returns focus to the trigger button. Keep the existing role="menu" behavior aligned with the focus handling so keyboard and screen-reader interaction matches the menu semantics.
🤖 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 `@src/components/skills/view/ProviderSkills.tsx`:
- Around line 357-363: The success banner in ProviderSkills is using persistent
hook state from saveStatus, which can become stale after the add panel is
reopened and cancelled. Update the banner logic in ProviderSkills to rely on a
local “just installed” flag (or otherwise reset saveStatus) so it only shows
after a real successful install. Clear that flag when the Add Skill panel is
opened, cancelled, or when the provider changes, and make sure the success
banner condition no longer depends directly on the hook-level saveStatus alone.
---
Nitpick comments:
In `@src/components/skills/view/ProviderSkills.tsx`:
- Around line 492-504: The Cancel button in ProviderSkills should also be
disabled while a submission is in flight so the panel state cannot be reset
during an active install. Update the Button in the cancel handler block to
respect isSubmitting, alongside the existing Install button logic in
handleUploadInstall/addSkills flow, so users cannot clear queuedFiles or close
the panel before the async submit finishes.
- Around line 491-504: The panel close behavior in ProviderSkills is
inconsistent: the Cancel handler clears queuedFiles and submitError, but the Add
Skill toggle path only flips isAddPanelOpen and leaves queuedFiles behind.
Update the close logic used by both the Cancel button and the Add Skill toggle
so closing the panel always resets queuedFiles (and submitError if appropriate),
keeping reopen behavior consistent. Use the existing state setters in
ProviderSkills and the add-panel toggle handler to centralize the cleanup.
In `@src/shared/view/ui/ActionMenu.tsx`:
- Around line 48-105: The ActionMenu opens and closes without managing focus, so
the trigger keeps focus instead of moving into the menu and focus is not
restored after close. Update ActionMenu in the component using isOpen, rootRef,
menuId, and runItem so that opening the menu programmatically focuses the first
menu item or the menu container, and closing via Escape, outside click, or item
selection returns focus to the trigger button. Keep the existing role="menu"
behavior aligned with the focus handling so keyboard and screen-reader
interaction matches the menu semantics.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5cd890f3-d8c4-4fbe-9158-8abd44034990
📒 Files selected for processing (4)
src/components/mcp/view/McpServers.tsxsrc/components/skills/view/ProviderSkills.tsxsrc/shared/view/ui/ActionMenu.tsxsrc/shared/view/ui/index.ts
Gate the skills install success banner behind a local just-installed flag so it no longer re-appears stale after reopening and cancelling the add dialog, and disable the cancel/close controls while an install is in flight. Add keyboard focus management to ActionMenu: focus the first item on open and restore focus to the trigger on Escape or item selection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation