Skip to content

feat: add skill uninstall confirmation#2177

Merged
arnestrickmann merged 2 commits into
mainfrom
jan/eng-1286-add-uninstall-confirmation-modal-to-skill-flow
May 21, 2026
Merged

feat: add skill uninstall confirmation#2177
arnestrickmann merged 2 commits into
mainfrom
jan/eng-1286-add-uninstall-confirmation-modal-to-skill-flow

Conversation

@janburzinski
Copy link
Copy Markdown
Collaborator

image

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 21, 2026

Greptile Summary

This PR adds an uninstall confirmation step for skills by introducing a confirmActionModal interstitial before the actual uninstall RPC is called. The onUninstall prop type on SkillDetailModal changes from Promise<boolean> to void, and loading-state management is removed from the modal's uninstall path.

  • handleUninstallRequest in SkillsView resolves the skill display name from the catalog, immediately closes the detail modal via closeDetail(), then opens ConfirmActionDialog — the actual uninstall(skillId) call only runs in the onSuccess callback.
  • SkillDetailModal.handleUninstall is simplified to a one-liner that delegates directly to onUninstall; isProcessing is now only toggled during install, and the dialog's close-prevention guard still correctly blocks ESC/backdrop-close during install.

Confidence Score: 4/5

Safe to merge; the core uninstall flow is correct and error handling is intact.

The confirmation dialog wires up correctly and errors are handled by the existing mutation callbacks. The only notable issue is that the detail modal is always closed before the confirmation appears — cancelling the dialog leaves the user at the skills list rather than returning to the detail view they were reading.

src/renderer/features/skills/components/SkillsView.tsx — specifically the order of closeDetail() and showConfirm() in handleUninstallRequest.

Important Files Changed

Filename Overview
src/renderer/features/skills/components/SkillDetailModal.tsx Simplified handleUninstall to be synchronous; delegates loading state and close logic to the new confirmation flow in SkillsView. isProcessing now only guards install operations.
src/renderer/features/skills/components/SkillsView.tsx Adds handleUninstallRequest that closes the detail modal then shows a confirmActionModal before calling uninstall. Detail modal is always dismissed on Uninstall click, even if the user cancels confirmation.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant SM as SkillDetailModal
    participant SV as SkillsView
    participant CD as ConfirmActionDialog
    participant API as uninstall RPC

    U->>SM: Click "Uninstall"
    SM->>SV: handleUninstallRequest(skillId)
    SV->>SV: closeDetail() — modal closes immediately
    SV->>CD: "showConfirm({ title, description, onSuccess })"
    CD-->>U: Show confirmation dialog

    alt User confirms
        U->>CD: Click "Uninstall"
        CD->>SV: onSuccess()
        SV->>API: uninstall(skillId)
        API-->>SV: success toast + cache invalidation
    else User cancels
        U->>CD: Click "Cancel"
        CD-->>U: Dialog dismissed (user is back at skills list)
    end
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/renderer/features/skills/components/SkillsView.tsx:40-46
**Detail modal discarded even on confirmation cancel**

`closeDetail()` is called unconditionally before the confirm dialog is shown, so if the user opens a skill's detail view, clicks Uninstall, and then cancels the confirmation, they end up back at the empty skills list instead of the detail modal they were reading. Previously, the detail modal stayed open until the uninstall actually succeeded. If this is intentional (avoiding two overlapping modals), it's worth adding a comment; otherwise, deferring `closeDetail()` to the `onSuccess` callback would preserve the previous "cancel = stay in detail" behaviour.

Reviews (1): Last reviewed commit: "Add skill uninstall confirmation" | Re-trigger Greptile

Comment thread src/renderer/features/skills/components/SkillsView.tsx Outdated
@arnestrickmann arnestrickmann merged commit dc4fb7b into main May 21, 2026
1 check passed
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.

3 participants