add confirmation dialog before deleting folders #955
Conversation
github-actions
bot
added
enhancement
📝 WalkthroughWalkthroughAdds a confirmation dialog component for folder deletion and integrates it into the folder management UI. Users now see a warning dialog before deletion is executed, replacing immediate deletion with a two-step confirmation flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (4)
frontend/src/components/Dialog/DeleteFolderDialog.tsx (3)
27-30: Consider handling async deletion with loading states.The
handleConfirmfunction callsonConfirm()and immediately closes the dialog. If the deletion operation is asynchronous, users won't see loading feedback or errors within the dialog context. Consider:
- Making
onConfirmreturn a Promise- Showing a loading state while the operation completes
- Handling errors before closing the dialog
This would provide better user feedback during slow operations or failures.
💡 Example async handling pattern
+ const [isDeleting, setIsDeleting] = useState(false); + - const handleConfirm = () => { + const handleConfirm = async () => { + setIsDeleting(true); + try { + await onConfirm(); - onConfirm(); setIsOpen(false); + } catch (error) { + // Handle error (show message, etc.) + } finally { + setIsDeleting(false); + } };And update the button:
<Button variant="destructive" onClick={handleConfirm} + disabled={isDeleting} className="cursor-pointer" > - Delete from Library + {isDeleting ? 'Deleting...' : 'Delete from Library'} </Button>
42-67: Consider accessibility of complex content in DialogDescription.The
DialogDescriptioncomponent contains complex structured content including styled divs, lists, and multiple sections. For optimal accessibility,DialogDescriptiontypically maps toaria-describedbyand works best with simpler text content.Consider moving the complex warning content outside of
DialogDescriptionbut still withinDialogContent, keeping only a brief description text inDialogDescription.💡 Example structure
<DialogHeader> <div className="flex items-center gap-3"> <div className="flex h-10 w-10 items-center justify-center rounded-full bg-red-100 dark:bg-red-900/20"> <AlertTriangle className="h-5 w-5 text-red-600 dark:text-red-400" /> </div> <DialogTitle className="text-xl">Delete Folder?</DialogTitle> </div> <DialogDescription className="pt-4 text-left"> - <div className="space-y-3"> - <p className="font-medium text-foreground"> - You are about to remove this folder from your library: - </p> - ... - </div> + You are about to remove this folder from your library. This action will remove the folder and all associated data from PictoPy, but your files on disk will remain safe. </DialogDescription> </DialogHeader> + <div className="space-y-3 px-6"> + {/* Move detailed content here */} + </div>
71-84: Minor: Redundant cursor-pointer classes on buttons.The
className="cursor-pointer"on both buttons is redundant since buttons havecursor: pointerby default in the UI library.<Button variant="outline" onClick={() => setIsOpen(false)} - className="cursor-pointer" > Cancel </Button> <Button variant="destructive" onClick={handleConfirm} - className="cursor-pointer" > Delete from Library </Button>frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
44-49: Consider awaiting the async delete operation.The
handleConfirmDeletefunction callsdeleteFolderand immediately clears the state. IfdeleteFolderis asynchronous (suggested by thedeleteFolderPendingstate), consider awaiting it to:
- Keep the dialog open during the operation
- Handle errors gracefully before clearing state
- Provide user feedback on success/failure
💡 Example async handling
- const handleConfirmDelete = () => { + const handleConfirmDelete = async () => { if (folderToDelete) { - deleteFolder(folderToDelete.folder_id); + try { + await deleteFolder(folderToDelete.folder_id); + } catch (error) { + // Error handling might be done in the hook itself + console.error('Failed to delete folder:', error); + } setFolderToDelete(null); } };Note: This also requires updating the
onConfirmprop type inDeleteFolderDialogto accept async functions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Dialog/DeleteFolderDialog.tsxfrontend/src/pages/SettingsPage/components/FolderManagementCard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Dialog/DeleteFolderDialog.tsx (1)
frontend/src/components/ui/dialog.tsx (6)
Dialog(133-133)DialogContent(135-135)DialogHeader(138-138)DialogTitle(141-141)DialogDescription(136-136)DialogFooter(137-137)
🔇 Additional comments (2)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (2)
90-90: Excellent implementation of the confirmation flow.The change from directly calling
deleteFolderto opening the confirmation dialog viahandleDeleteClickproperly implements the safety mechanism requested in issue #939. Users now get an explicit warning and confirmation step before deletion occurs.
154-160: LGTM: Dialog integration is clean and complete.The
DeleteFolderDialogcomponent is properly integrated with all required props. The defensive use of optional chaining (folderToDelete?.folder_path || '') for the folder path is good practice, even though the dialog should only be open when a folder is selected.
1 participant
Fixes: #939
Adds a warning and confirmation dialog before folder deletion to prevent accidental data loss.
Does not affect files on disk.
Before
delete1.mp4
After
delete2.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.