Confirm integration disconnects#1895
Conversation
Greptile SummaryThis PR adds a confirmation modal before disconnecting any enabled integration, reusing the existing
Confidence Score: 4/5Safe to merge — the confirmation flow is correctly wired using an established modal pattern, and the only concern is that async disconnect errors are discarded without user feedback. The implementation is straightforward and consistent with how confirmActionModal is used elsewhere in the codebase. The void onDisconnect() in the onSuccess callback discards any Promise rejection from the underlying mutateAsync call — if a disconnect fails at the network level, the modal has already closed and nothing is surfaced to the user. This was similarly silent before the PR, but the void keyword now makes the discard explicit. Only IntegrationsCard.tsx changed; the onSuccess async-error path in confirmDisconnect is the one spot worth a second look.
|
| Filename | Overview |
|---|---|
| src/renderer/features/settings/components/IntegrationsCard.tsx | Adds a confirmDisconnect helper and wires all six integrations through confirmActionModal before disconnecting; logic is correct but async disconnect errors are swallowed silently via void onDisconnect(). |
Sequence Diagram
sequenceDiagram
actor User
participant IC as IntegrationsCard
participant CAD as ConfirmActionDialog
participant MP as ModalProvider (wrapArgs)
participant DP as disconnect* fn
User->>IC: Click disconnect button
IC->>MP: showConfirmDisconnect({ title, description, confirmLabel, onSuccess })
MP->>CAD: open modal
User->>CAD: Click Disconnect (or Cmd+Enter)
CAD->>MP: onSuccess()
MP->>MP: modalStore.closeModal('completed')
MP->>IC: args.onSuccess()
IC->>DP: void onDisconnect()
Note over IC,DP: Promise returned, but not awaited
DP-->>IC: resolve / reject (silently swallowed)
User->>CAD: Click Cancel
CAD->>MP: onClose()
MP->>MP: modalStore.closeModal('dismissed')
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/settings/components/IntegrationsCard.tsx:89-91
Async disconnect errors are silently swallowed here. `void onDisconnect()` discards the returned `Promise`, so if `disconnectMutation.mutateAsync()` (inside each disconnect function) rejects — e.g. a network failure — the modal has already closed and the user receives no feedback. The integration card will remain in the connected state, but no error is surfaced. Consider awaiting the promise or propagating the error to a toast.
```suggestion
onSuccess: async () => {
try {
await onDisconnect();
} catch {
// TODO: surface disconnect errors (e.g. via toast)
}
},
```
Reviews (1): Last reviewed commit: "Confirm integration disconnects" | Re-trigger Greptile
| onSuccess: () => { | ||
| void onDisconnect(); | ||
| }, |
There was a problem hiding this comment.
Async disconnect errors are silently swallowed here.
void onDisconnect() discards the returned Promise, so if disconnectMutation.mutateAsync() (inside each disconnect function) rejects — e.g. a network failure — the modal has already closed and the user receives no feedback. The integration card will remain in the connected state, but no error is surfaced. Consider awaiting the promise or propagating the error to a toast.
| onSuccess: () => { | |
| void onDisconnect(); | |
| }, | |
| onSuccess: async () => { | |
| try { | |
| await onDisconnect(); | |
| } catch { | |
| // TODO: surface disconnect errors (e.g. via toast) | |
| } | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/settings/components/IntegrationsCard.tsx
Line: 89-91
Comment:
Async disconnect errors are silently swallowed here. `void onDisconnect()` discards the returned `Promise`, so if `disconnectMutation.mutateAsync()` (inside each disconnect function) rejects — e.g. a network failure — the modal has already closed and the user receives no feedback. The integration card will remain in the connected state, but no error is surfaced. Consider awaiting the promise or propagating the error to a toast.
```suggestion
onSuccess: async () => {
try {
await onDisconnect();
} catch {
// TODO: surface disconnect errors (e.g. via toast)
}
},
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
Validation
Note
Low Risk
Low risk UI behavior change that gates existing disconnect actions behind a confirmation modal; primary risk is minor UX regression if modal wiring blocks disconnect flows.
Overview
Adds a confirmation step before disconnecting any enabled integration in
IntegrationsCardby routing allonDisconnecthandlers through the existingconfirmActionModal.The modal copy is tailored per integration (e.g., calling out deletion of saved API keys/credentials), while preserving the existing CLI-managed GitHub disconnect disable/tooltip behavior.
Reviewed by Cursor Bugbot for commit 4f8cd30. Bugbot is set up for automated code reviews on this repo. Configure here.