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.
|
Hi, @290Priyansh, I am interested in contributing to this issue, can you please assign it me |
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Make this Dialog box reusable/modular. And use Redux states to trigger it and get confirmation. Refer frontend/src/components/Dialog/InfoDialog.tsx
|
@rahulharpal1603 I’d like to work on it if it’s still available. Please let me know if I can proceed. |
|
Hi @290Priyansh, thank you for bringing this up earlier, and sorry for the late response. I agree with adding a confirmation step here since folder deletion is a destructive action and it’s easy to misclick. One concern though is around the current behavior of PictoPy where selecting a parent folder lists all its nested folders as well. Say for example I mistakenly add a huge directory with 100s of sub-directories, in that case, a users might often want to delete multiple folders. If we show a confirmation dialog for every single deletion, it could introduce a lot of friction and slow down bulk cleanup workflows. Also, since PictoPy already shows a dialog after a folder is deleted, this change would effectively introduce an additional prompt in the flow, which may feel a bit repetitive from a user experience standpoint. One possible direction worth exploring could be enabling bulk deletion (for example, via a multi-select option in the Folder Management component), followed by a single confirmation dialog for the entire action. This would help maintain safety while keeping the experience efficient for larger cleanups. |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
5 participants
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.