fix: added confirmation modal for folder deletion-955#1233
fix: added confirmation modal for folder deletion-955#1233aditii23 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
WalkthroughAdds a confirmation modal to the folder deletion flow in the FolderManagementCard component. The delete action is now split into two steps: clicking delete opens a confirmation dialog, and the actual deletion only proceeds after explicit user confirmation. Includes an import path update for RootState. Changes
Sequence DiagramsequenceDiagram
actor User
participant FolderManagementCard
participant Modal
User->>FolderManagementCard: Click Delete Button
FolderManagementCard->>Modal: Open Modal (set selectedFolder)
Modal-->>User: Display Confirmation Dialog
alt User Confirms
User->>Modal: Click Delete Button
Modal->>FolderManagementCard: Trigger deleteFolder(selectedFolder)
FolderManagementCard->>FolderManagementCard: Execute deleteFolder
FolderManagementCard->>Modal: Close Modal
Modal-->>User: Folder Deleted
else User Cancels
User->>Modal: Click Cancel Button
Modal->>FolderManagementCard: Close Modal
FolderManagementCard-->>User: Modal Closed
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Flake8 can be used to improve the quality of Python code reviews.Flake8 is a Python linter that wraps PyFlakes, pycodestyle and Ned Batchelder's McCabe script. To configure Flake8, add a '.flake8' or 'setup.cfg' file to your project root. See Flake8 Documentation for more details. |
|
|
1 similar 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: 3
🧹 Nitpick comments (1)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
9-9: Remove stale commented-out code blocks.The commented imports/state/handler lines are redundant and reduce readability.
As per coding guidelines, "Point out redundant obvious comments that do not add clarity to the code".
Also applies to: 22-23, 76-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx` at line 9, Remove the stale commented-out code in the FolderManagementCard component: delete the commented import line for RootState, the commented state/handler lines around the component (the ones at lines 22-23), and the commented block at line 76 to improve readability; search for commented references inside FolderManagementCard.tsx (e.g., the unused "// import { RootState } from '@/app/store';" and other commented handlers/state declarations) and remove them so only active, meaningful code remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx`:
- Around line 75-80: The confirmation modal currently only opens with
setSelectedFolder(folder.folder_id) and generic text; modify the click handler
and modal state so the modal receives the selected folder's path: either
setSelectedFolder to the full folder object (instead of just folder.folder_id)
or keep the id but look up the folder from the folders array inside the modal
render; then update the modal text (the JSX that renders the confirmation
message) to interpolate and display the folder.path (or folder.fullPath) so the
dialog shows the specific affected folder before calling
deleteFolder(selectedFolder or selectedFolder.folder_id). Ensure you update any
usages of selectedFolder elsewhere to handle the object vs id change and that
deleteFolder is passed the correct identifier.
- Around line 144-173: Add automated tests for the delete-confirmation modal in
FolderManagementCard: write unit/RTL tests that render the FolderManagementCard
component, verify that clicking the delete trigger opens the modal (isModalOpen
state), that clicking the "Cancel" button (calls setIsModalOpen(false)) closes
the modal without invoking deleteFolder, and that clicking the "Delete" button
calls deleteFolder with the currently selectedFolder and then closes the modal;
include assertions for modal visibility, that deleteFolder is called/not called
appropriately, and mock/stub selectedFolder and deleteFolder to avoid side
effects.
- Around line 8-9: The import of RootState in FolderManagementCard.tsx is using
the wrong module; update the import statement so RootState is imported from
'@/app/store' instead of '@/store/store' to match where the store type is
actually exported (replace the import for RootState accordingly in the
FolderManagementCard component).
---
Nitpick comments:
In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx`:
- Line 9: Remove the stale commented-out code in the FolderManagementCard
component: delete the commented import line for RootState, the commented
state/handler lines around the component (the ones at lines 22-23), and the
commented block at line 76 to improve readability; search for commented
references inside FolderManagementCard.tsx (e.g., the unused "// import {
RootState } from '@/app/store';" and other commented handlers/state
declarations) and remove them so only active, meaningful code remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddb5967a-6068-4c2d-b9d0-cc1395d44f22
📒 Files selected for processing (1)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx
| import { RootState } from '@/store/store'; | ||
| // import { RootState } from '@/app/store'; |
There was a problem hiding this comment.
Fix the RootState import path to prevent a build break.
Line 8 imports RootState from @/store/store, but the store type is exported from frontend/src/app/store.ts. This path mismatch will fail type-check/compile.
Proposed fix
-import { RootState } from '@/store/store';
-// import { RootState } from '@/app/store';
+import { RootState } from '@/app/store';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { RootState } from '@/store/store'; | |
| // import { RootState } from '@/app/store'; | |
| import { RootState } from '@/app/store'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx` around
lines 8 - 9, The import of RootState in FolderManagementCard.tsx is using the
wrong module; update the import statement so RootState is imported from
'@/app/store' instead of '@/store/store' to match where the store type is
actually exported (replace the import for RootState accordingly in the
FolderManagementCard component).
| <Button | ||
| onClick={() => deleteFolder(folder.folder_id)} | ||
| // onClick={() => deleteFolder(folder.folder_id)} | ||
| onClick={() => { | ||
| setSelectedFolder(folder.folder_id); | ||
| setIsModalOpen(true); | ||
| }} |
There was a problem hiding this comment.
Include the specific folder path in the confirmation dialog.
The modal currently shows generic text only (Line 151-153). Issue #955 asks to show affected folder details (e.g., folder path) before destructive confirmation.
Proposed fix
-const [selectedFolder, setSelectedFolder] = useState<string | null>(null);
+const [selectedFolder, setSelectedFolder] = useState<{
+ folder_id: string;
+ folder_path: string;
+} | null>(null);
- onClick={() => {
- setSelectedFolder(folder.folder_id);
- setIsModalOpen(true);
- }}
+ onClick={() => {
+ setSelectedFolder({
+ folder_id: folder.folder_id,
+ folder_path: folder.folder_path,
+ });
+ setIsModalOpen(true);
+ }}
- if (selectedFolder) deleteFolder(selectedFolder);
+ if (selectedFolder) deleteFolder(selectedFolder.folder_id);
- Are you sure? This will remove the folder from the library but not
- from your computer.
+ Are you sure you want to remove this folder from the library?
+ <span className="mt-2 block font-mono text-xs break-all">
+ {selectedFolder?.folder_path}
+ </span>
+ This does not delete files from disk.As per coding guidelines, "Confirm that the code meets the project's requirements and objectives".
Also applies to: 144-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx` around
lines 75 - 80, The confirmation modal currently only opens with
setSelectedFolder(folder.folder_id) and generic text; modify the click handler
and modal state so the modal receives the selected folder's path: either
setSelectedFolder to the full folder object (instead of just folder.folder_id)
or keep the id but look up the folder from the folders array inside the modal
render; then update the modal text (the JSX that renders the confirmation
message) to interpolate and display the folder.path (or folder.fullPath) so the
dialog shows the specific affected folder before calling
deleteFolder(selectedFolder or selectedFolder.folder_id). Ensure you update any
usages of selectedFolder elsewhere to handle the object vs id change and that
deleteFolder is passed the correct identifier.
| {isModalOpen && ( | ||
| <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/60 backdrop-blur-sm"> | ||
| <div className="w-full max-w-sm rounded-xl border border-gray-200 bg-white p-6 shadow-2xl dark:border-gray-800 dark:bg-gray-900"> | ||
| <h3 className="mb-2 text-lg font-semibold text-gray-900 dark:text-white"> | ||
| Delete Folder? | ||
| </h3> | ||
| <p className="mb-6 text-sm text-gray-500 dark:text-gray-400"> | ||
| Are you sure? This will remove the folder from the library but not | ||
| from your computer. | ||
| </p> | ||
| <div className="flex justify-end gap-3"> | ||
| <button | ||
| onClick={() => setIsModalOpen(false)} | ||
| className="rounded-md px-4 py-2 text-sm font-medium text-gray-700 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800" | ||
| > | ||
| Cancel | ||
| </button> | ||
| <button | ||
| onClick={() => { | ||
| if (selectedFolder) deleteFolder(selectedFolder); | ||
| setIsModalOpen(false); | ||
| }} | ||
| className="rounded-md bg-red-600 px-4 py-2 text-sm font-medium text-white hover:bg-red-700" | ||
| > | ||
| Delete | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Add automated tests for the new delete-confirmation flow.
This PR introduces critical destructive-action behavior, but no tests are included for: opening modal, cancel behavior, and confirm-triggered deletion for selected folder.
As per coding guidelines, "Ensure that test code is automated, comprehensive, and follows testing best practices" and "Verify that all critical functionality is covered by tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx` around
lines 144 - 173, Add automated tests for the delete-confirmation modal in
FolderManagementCard: write unit/RTL tests that render the FolderManagementCard
component, verify that clicking the delete trigger opens the modal (isModalOpen
state), that clicking the "Cancel" button (calls setIsModalOpen(false)) closes
the modal without invoking deleteFolder, and that clicking the "Delete" button
calls deleteFolder with the currently selectedFolder and then closes the modal;
include assertions for modal visibility, that deleteFolder is called/not called
appropriately, and mock/stub selectedFolder and deleteFolder to avoid side
effects.
1 participant
Addressed Issues:
Fixes #955
Summary of Changes:
Implemented a Confirmation Modal in the FolderManagementCard component to prevent accidental folder deletions.
Added local state management using useState to control the modal's visibility and store the ID of the folder selected for deletion.
Enhanced the user experience (UX) by ensuring users are prompted with a clear warning before any destructive action is taken.
Styled the modal using Tailwind CSS to maintain consistency with the PictoPy design system, ensuring full compatibility with both Light and Dark modes.
AI Usage Disclosure:
[x] This PR contains AI-generated code. I have read the AI Usage Policy and this PR complies with this policy. I have tested the code locally and I am responsible for it.
I have used the following AI models and tools: Gemini
Note on AI Usage: I primarily developed the logic and UI myself. I only utilized the AI tool to assist in debugging a specific TypeScript type-mismatch issue and to ensure the modal state logic correctly handles the edge case where a folder ID might be null during the initial render. All generated suggestions were manually reviewed and tested locally.
Checklist
[x] My PR addresses a single issue, fixes a single bug or makes a single improvement.
[x] My code follows the project's code style and conventions.
[x] My changes generate no new warnings or errors.
[x] I have read the Contribution Guidelines.
[x] I have filled this PR template completely and carefully.
Summary by CodeRabbit