Skip to content

fix: added confirmation modal for folder deletion-955 #1233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aditii23 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: added confirmation modal for folder deletion-955 #1233

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 43 additions & 3 deletions frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React from 'react';
import React, { useState } from 'react';
import { Folder, Trash2, Check } from 'lucide-react';

import { Switch } from '@/components/ui/switch';
import { Button } from '@/components/ui/button';
import { Progress } from '@/components/ui/progress';
import { useSelector } from 'react-redux';
import { RootState } from '@/app/store';
import { RootState } from '@/store/store';
// import { RootState } from '@/app/store';
Comment on lines +8 to +9
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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).

import FolderPicker from '@/components/FolderPicker/FolderPicker';

import { useFolderOperations } from '@/hooks/useFolderOperations';
Expand All @@ -16,6 +17,10 @@ import SettingsCard from './SettingsCard';
* Component for managing folder operations in settings
*/
const FolderManagementCard: React.FC = () => {
const [isModalOpen, setIsModalOpen] = useState(false);
const [selectedFolder, setSelectedFolder] = useState<string | null>(null);
// const [selectedFolder, setSelectedFolder] = useState<any>(null);
// const taggingStatus = useSelector((state: any) => state.folders.taggingStatus || {});
const {
folders,
toggleAITagging,
Expand Down Expand Up @@ -68,7 +73,11 @@ const FolderManagementCard: React.FC = () => {
</div>

<Button
onClick={() => deleteFolder(folder.folder_id)}
// onClick={() => deleteFolder(folder.folder_id)}
onClick={() => {
setSelectedFolder(folder.folder_id);
setIsModalOpen(true);
}}
Comment on lines 75 to +80
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

variant="outline"
size="sm"
className="h-8 w-8 cursor-pointer text-gray-500 hover:border-red-300 hover:text-red-600 dark:text-gray-400 dark:hover:text-red-400"
Expand Down Expand Up @@ -131,6 +140,37 @@ const FolderManagementCard: React.FC = () => {
<div className="border-border mt-6 border-t pt-6">
<FolderPicker />
</div>

{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>
)}
Comment on lines +144 to +173
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

</SettingsCard>
);
};
Expand Down
Loading