Skip to content

feat: add delete confirmation modal with 5-second undo option#55

Open
nidhii-dev wants to merge 5 commits into
suresh1319:masterfrom
nidhii-dev:feat/delete-confirmation-modal
Open

feat: add delete confirmation modal with 5-second undo option#55
nidhii-dev wants to merge 5 commits into
suresh1319:masterfrom
nidhii-dev:feat/delete-confirmation-modal

Conversation

@nidhii-dev
Copy link
Copy Markdown

Changes

  • Added DeleteConfirmModal component with keyboard support
  • Added 5-second undo toast before permanent deletion
  • Added server-side permission check to block viewers
  • Shows child count warning when deleting folders

Testing

  • ✅ Cancel closes modal
  • ✅ Delete shows undo toast
  • ✅ Undo cancels deletion
  • ✅ Auto-delete after 5 seconds
  • ✅ Viewers cannot delete files

Closes issue #54

Comment thread src/components/DeleteConfirmModal.js
Comment on lines +74 to +77
const timer = setTimeout(() => {
onDeleteNode(nodeId);
setUndoToast(null);
}, 5000);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MAJOR RELIABILITY Pending delete timer is not cleared on FileNode unmount

The setTimeout stored in undoToast.timer is never cancelled if the FileNode unmounts during the 5-second window (e.g. parent re-renders, room closes). When it fires, onDeleteNode(nodeId) executes unintentionally and setUndoToast(null) is called on an unmounted component.

Prompt to fix with AI

Copy this prompt into your AI coding assistant to fix this issue.

In `src/components/FileExplorer.js`, the `FileNode` component creates a `setTimeout` inside `handleDeleteConfirm` (lines 74-77) and stores it in `undoToast.timer`, but never clears it when the component unmounts. Add a `useEffect` cleanup:

1. Add `useEffect` to the import on line 1: `import React, { useState, useRef, useEffect } from 'react';`
2. After the `handleUndo` function (after line 85), add:
```js
useEffect(() => {
    return () => {
        if (undoToast?.timer) clearTimeout(undoToast.timer);
    };
}, [undoToast]);

This ensures the pending deletion timer is cancelled if the component unmounts during the 5-second undo window, preventing unintended file deletion.


</details>

<Pencil size={13} />
</button>
<button title="Delete" onClick={() => onDeleteNode(node.id)}>
<button title="Delete" onClick={() => handleDeleteClick(node.id, node.name, node.type, node.children?.length ?? 0)}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT CORRECTNESS childCount is shallow-only but modal says "all X files inside"

FileExplorer.js passes node.children?.length (direct children only) as childCount, but DeleteConfirmModal.js:26 displays it as "along with all X files inside" — language that implies a deep/recursive count. A folder with 2 subfolders each containing 10 files would show "2 files inside" instead of 22.

Prompt to fix with AI

Copy this prompt into your AI coding assistant to fix this issue.

Either (a) compute a recursive child count before passing it to handleDeleteClick — add a helper like `function countDescendants(node, fs) { return (node.children || []).reduce((n, id) => n + 1 + countDescendants(fs[id] || {}, fs), 0); }` and pass the result instead of `node.children?.length`, OR (b) change the modal copy to say "along with all items directly inside" to match the shallow count that is actually passed.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown
Contributor


Confidence Score: 2/5 - Changes Needed

Not safe to merge — this PR introduces a delete confirmation modal with a 5-second undo timer, but contains two reliability bugs that could cause data loss or undefined behavior before the feature is fit for production. Specifically, DeleteConfirmModal.js installs a global keydown listener while also auto-focusing the confirmRef button, causing onConfirm to fire twice on Enter — creating a leaked setTimeout that the undo mechanism cannot cancel. Additionally, FileExplorer.js never clears the undoToast.timer on FileNode unmount, meaning the delete will execute silently even after the component is gone, with no user-visible undo option remaining. The shallow node.children?.length being passed as childCount is a lesser concern but will mislead users about how many files are actually at risk when deleting nested directories.

Key Findings:

  • In DeleteConfirmModal.js, the combination of confirmRef auto-focus and a global keydown listener on the Delete button causes onConfirm to be invoked twice when the user presses Enter — the first call via the keydown handler and the second via the button's native click event. This creates a second setTimeout timer that the undo toast has no reference to, making it impossible to cancel the deletion.
  • In FileExplorer.js, the setTimeout stored in undoToast.timer is never cleared in a useEffect cleanup or equivalent teardown, so if FileNode unmounts during the 5-second undo window (e.g., a parent re-render or navigation), the deletion fires silently with no way for the user to intervene — a potential data-loss scenario.
  • The childCount prop passed to DeleteConfirmModal uses node.children?.length, which only counts direct children. The modal displays this as 'all X files inside', which is factually incorrect for nested directory trees and could cause user confusion about the scope of deletion.
Files requiring special attention
  • src/components/DeleteConfirmModal.js
  • src/components/FileExplorer.js

@suresh1319
Copy link
Copy Markdown
Owner

@nidhii-dev
Confidence Score: 2/5 - Changes Needed
Not safe to merge — this PR introduces a delete confirmation modal with a 5-second undo timer, but contains two reliability bugs that could cause data loss or undefined behavior before the feature is fit for production. Specifically, DeleteConfirmModal.js installs a global keydown listener while also auto-focusing the confirmRef button, causing onConfirm to fire twice on Enter — creating a leaked setTimeout that the undo mechanism cannot cancel. Additionally, FileExplorer.js never clears the undoToast.timer on FileNode unmount, meaning the delete will execute silently even after the component is gone, with no user-visible undo option remaining. The shallow node.children?.length being passed as childCount is a lesser concern but will mislead users about how many files are actually at risk when deleting nested directories.

Key Findings:

In DeleteConfirmModal.js, the combination of confirmRef auto-focus and a global keydown listener on the Delete button causes onConfirm to be invoked twice when the user presses Enter — the first call via the keydown handler and the second via the button's native click event. This creates a second setTimeout timer that the undo toast has no reference to, making it impossible to cancel the deletion.
In FileExplorer.js, the setTimeout stored in undoToast.timer is never cleared in a useEffect cleanup or equivalent teardown, so if FileNode unmounts during the 5-second undo window (e.g., a parent re-render or navigation), the deletion fires silently with no way for the user to intervene — a potential data-loss scenario.
The childCount prop passed to DeleteConfirmModal uses node.children?.length, which only counts direct children. The modal displays this as 'all X files inside', which is factually incorrect for nested directory trees and could cause user confusion about the scope of deletion.
Files requiring special attention
check them and solve

fix: prevent Enter key from triggering deletion twice

Co-authored-by: entelligence-ai-pr-reviews[bot] <174136889+entelligence-ai-pr-reviews[bot]@users.noreply.github.com>
@entelligence-ai-pr-reviews
Copy link
Copy Markdown
Contributor

entelligence-ai-pr-reviews Bot commented May 18, 2026

EntelligenceAI PR Summary

This PR improves the deletion confirmation message wording and adds timer cleanup logic to prevent memory leaks in the FileNode component.

  • DeleteConfirmModal.js: Pluralizes item count dynamically using childCount (item/items) instead of hardcoded 'files'
  • DeleteConfirmModal.js: Updated phrasing from 'inside' to 'directly inside' for clearer scope indication
  • FileExplorer.js: Added useEffect import to support the new cleanup effect
  • FileExplorer.js: Introduced useEffect cleanup to clear undoToast timer on FileNode unmount, preventing memory leaks
  • FileExplorer.js: Cleanup effect is duplicated twice — flagged as unintentional redundancy requiring follow-up

Confidence Score: 5/5 - Safe to Merge

Safe to merge — this PR makes clean, targeted improvements to DeleteConfirmModal.js and FileExplorer.js with no identified issues. The dynamic pluralization of childCount (item/items) replaces a hardcoded string, the updated 'directly inside' phrasing improves UX clarity, and the useEffect-based timer cleanup in FileExplorer.js correctly addresses potential memory leaks from lingering timeouts. Both changed files were reviewed and no logic, correctness, security, or performance concerns were surfaced.

Key Findings:

  • DeleteConfirmModal.js correctly replaces hardcoded 'files' with dynamic pluralization based on childCount, eliminating a subtle display bug for single-item deletions.
  • The useEffect cleanup in FileExplorer.js properly clears the undo timer on component unmount, which is a genuine memory-leak/stale-state fix rather than cosmetic change.
  • Both files had full review coverage and zero issues were identified across logic, security, performance, and correctness dimensions.
  • 2 previous unresolved comment(s) likely resolved in latest diff (score-only signal; thread status unchanged)
Files requiring special attention
  • FileExplorer.js
  • DeleteConfirmModal.js

Comment thread src/components/DeleteConfirmModal.js Outdated
Co-authored-by: entelligence-ai-pr-reviews[bot] <174136889+entelligence-ai-pr-reviews[bot]@users.noreply.github.com>
@suresh1319
Copy link
Copy Markdown
Owner

EntelligenceAI PR Summary

This PR improves focus detection logic in the DeleteConfirmModal Enter key handler to correctly handle descendant element focus.

  • Updated condition in useEffect from document.activeElement !== confirmRef.current to !confirmRef.current?.contains(document.activeElement)
  • Prevents onConfirm from being triggered when focus is on the confirm button or any of its child elements
  • Uses optional chaining (?.) for safe null/undefined handling on confirmRef.current

Confidence Score: 3/5 - Review Recommended

Likely safe but review recommended — the change in DeleteConfirmModal's Enter key handler from strict reference equality (!== confirmRef.current) to !confirmRef.current?.contains(document.activeElement) is a correct and targeted improvement that properly handles focus within descendant elements. However, two pre-existing unresolved issues in FileExplorer.js remain open and were not introduced by this PR: a MAJOR reliability concern where the pending delete timer is not cleared on FileNode unmount (risking stale callbacks and potential memory leaks), and a NIT around childCount being shallow-only while the modal copy says 'all X files inside', which could mislead users. The core focus-detection fix is sound, but the unresolved timer cleanup issue is substantive enough to warrant a review pass before merging.

Key Findings:

  • The fix from document.activeElement !== confirmRef.current to !confirmRef.current?.contains(document.activeElement) correctly addresses cases where a child element of the confirm button holds focus, and the optional chaining guards against null refs — this is a genuine correctness improvement.
  • A pre-existing unresolved MAJOR issue: the pending delete timer in FileExplorer.js (L63-L89) is not cleared on FileNode unmount, which can cause onConfirm to fire after the component is gone, leading to stale state updates or errors on unmounted components.
  • A pre-existing NIT: childCount at L139-L145 is computed shallowly, but the confirmation modal tells users 'all X files inside', which could display an inaccurate count for nested directories — a correctness concern for user-facing messaging even if minor.

Files requiring special attention

@nidhii-dev kindly fix these issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants