feat: Add confirmation dialogs for delete actions in ACL, Alerts, SSL and Users components #32
Conversation
…, and Users components (#31)
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds confirmation dialogs for delete actions across multiple components to improve user experience and prevent accidental deletions. The implementation includes enhancements to the existing ConfirmDialog component and its integration into ACL, Alerts, SSL, and Users management interfaces.
Key changes:
- Enhanced ConfirmDialog component with loading states, variant support, and React node descriptions
- Replaced browser confirm() dialogs with custom confirmation dialogs in all delete operations
- Added destructive styling and loading indicators for better UX
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/components/ui/confirm-dialog.tsx | Enhanced with loading spinner, variant prop, and React node description support |
| apps/web/src/components/pages/Users.tsx | Replaced confirm() with ConfirmDialog for user deletion with detailed warning message |
| apps/web/src/components/pages/SSLTable.tsx | Added confirmation dialog for SSL certificate deletion with domain name context |
| apps/web/src/components/pages/Alerts.tsx | Implemented confirmation dialogs for both notification channels and alert rules deletion |
| apps/web/src/components/pages/ACL.tsx | Added confirmation dialog for ACL rule deletion with access control warnings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const [confirmDialog, setConfirmDialog] = useState<{ | ||
| open: boolean; | ||
| title: string; | ||
| description: string; | ||
| onConfirm: () => void; | ||
| certificateId: string; | ||
| }>({ | ||
| open: false, | ||
| title: '', | ||
| description: '', | ||
| onConfirm: () => {}, | ||
| certificateId: '', | ||
| }); |
There was a problem hiding this comment.
The confirmDialog state structure is overly complex and duplicates functionality already provided by the ConfirmDialog component. Consider simplifying to only track the certificate being deleted, similar to the pattern used in other components.
| const handleDelete = (id: string, domainName: string) => { | ||
| setConfirmDialog({ | ||
| open: true, | ||
| title: 'Delete SSL Certificate', | ||
| description: `Are you sure you want to delete the SSL certificate for ${domainName}? This action cannot be undone.`, | ||
| certificateId: id, | ||
| onConfirm: async () => { | ||
| try { | ||
| await deleteMutation.mutateAsync(id); | ||
| toast.success('Certificate deleted successfully'); | ||
| setConfirmDialog(prev => ({ ...prev, open: false })); | ||
| } catch (error: any) { | ||
| toast.error(error.response?.data?.message || 'Failed to delete certificate'); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Async function is defined inline within state initialization. This creates a new function on every state update and makes error handling inconsistent with other components. Extract this to a separate method like the other components do.
| const handleDelete = (id: string, domainName: string) => { | |
| setConfirmDialog({ | |
| open: true, | |
| title: 'Delete SSL Certificate', | |
| description: `Are you sure you want to delete the SSL certificate for ${domainName}? This action cannot be undone.`, | |
| certificateId: id, | |
| onConfirm: async () => { | |
| try { | |
| await deleteMutation.mutateAsync(id); | |
| toast.success('Certificate deleted successfully'); | |
| setConfirmDialog(prev => ({ ...prev, open: false })); | |
| } catch (error: any) { | |
| toast.error(error.response?.data?.message || 'Failed to delete certificate'); | |
| } | |
| }, | |
| const handleConfirmDelete = async (id: string) => { | |
| try { | |
| await deleteMutation.mutateAsync(id); | |
| toast.success('Certificate deleted successfully'); | |
| setConfirmDialog(prev => ({ ...prev, open: false })); | |
| } catch (error: any) { | |
| toast.error(error.response?.data?.message || 'Failed to delete certificate'); | |
| } | |
| }; | |
| const handleDelete = (id: string, domainName: string) => { | |
| setConfirmDialog({ | |
| open: true, | |
| title: 'Delete SSL Certificate', | |
| description: `Are you sure you want to delete the SSL certificate for ${domainName}? This action cannot be undone.`, | |
| certificateId: id, | |
| onConfirm: () => handleConfirmDelete(id), |
No description provided.