fix(gui): make the delete confirmation reachable, and give overlays one stacking scale - #540
Conversation
…ne stacking scale Find Duplicates asked for confirmation twice. The dialog raises its own prompt, and the App callback behind `onDeleteFiles` raised a second one through `setConfirmDialog` whenever "confirm before delete" was on. `ConfirmDialog` is mounted ~300 lines earlier in App.tsx than the finder and both sat at `z-50`, so at an equal z-index DOM order decided it: the finder painted over the question it was waiting on, and its full-viewport backdrop swallowed every click aimed at the buttons underneath. Dragging the finder aside revealed the confirm but did not free it — the panel moves, the overlay does not. The promise never settled, so "Delete Selected" span forever, and the only way to answer was to close the window that had asked. Reported in #537 with all five steps. The dialog now owns the confirmation outright: it takes `confirmBeforeDelete` and skips its own prompt when the setting is off, and the callback only deletes. The stacking half is fixed for the whole app rather than for this one pair. `src/utils/modalLayers.ts` holds the scale, and two things the sweep found go with it: - The app-wide confirm had to clear more than `z-50`. Of the twelve `setConfirmDialog` call sites, the overwrite and delete prompts fire from flows started inside the z-9998..10000 tier (AeroSync, the transfer plan, the trash managers), so a confirm below them would have repeated #537 there. - The lock screens sat at z-100 and z-200, under that same tier. `isAppLocked` gates no rendering in App.tsx — the whole tree stays mounted behind the lock overlay — so a dialog left open when the idle probe fired stayed legible on top of the lock screen. Both now sit above everything. Also: the in-dialog confirm asked `duplicates.confirmDelete`, a key present in none of the 47 locale files, so `t()` printed the key name at the user instead of a question. It now uses `duplicates.deleteConfirm`, which exists. Pins, all four failing on the code they replace: the sweep walks every `fixed inset-0` overlay in the source and refuses one that can cover the confirm or the lock screens; the App mount site must carry no `setConfirmDialog`; the dialog must branch on `confirmBeforeDelete`; and every `t()` key the dialog uses must resolve in en.json, the fallback for all 47 locales. Closes #537 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change centralizes modal z-index values and updates modal overlays to use them. Duplicate-file deletion confirmation now resides in ChangesDuplicate deletion and modal layering
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant App
participant DuplicateFinderDialog
participant ConfirmDialog
participant onDeleteFiles
App->>DuplicateFinderDialog: pass confirmBeforeDelete
DuplicateFinderDialog->>ConfirmDialog: request confirmation when enabled
ConfirmDialog->>DuplicateFinderDialog: call runDelete
DuplicateFinderDialog->>onDeleteFiles: delete selected files
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/DuplicateFinderDialog.tsx (1)
278-301: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReconcile deletion with current state, not the stale render snapshot.
After Line 285 awaits
onDeleteFiles, Lines 287-295 use the capturedgroupsandselectedPaths. During a slow deletion, users can change selections or change the scan mode. A new scan can also replacegroups. The completion then overwrites newer results and clears newer selections.Use functional state updates keyed by the
pathssnapshot. Also disable controls that can change selection or start a scan whileisDeletingis true.Proposed reconciliation
await onDeleteFiles(paths); // Remove deleted files from groups and update state - const updatedGroups: DuplicateGroup[] = []; - for (const group of groups) { - const remaining = group.files.filter(f => !selectedPaths.has(f)); - if (remaining.length > 1) { - updatedGroups.push({ ...group, files: remaining }); - } - } - setGroups(updatedGroups); - setSelectedPaths(new Set()); + const deletedPaths = new Set(paths); + setGroups((currentGroups) => { + const updatedGroups: DuplicateGroup[] = []; + for (const group of currentGroups) { + const remaining = group.files.filter(f => !deletedPaths.has(f)); + if (remaining.length > 1) { + updatedGroups.push({ ...group, files: remaining }); + } + } + return updatedGroups; + }); + setSelectedPaths((currentSelection) => { + const next = new Set(currentSelection); + for (const path of deletedPaths) next.delete(path); + return next; + }); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/DuplicateFinderDialog.tsx` around lines 278 - 301, Update runDelete to reconcile deletion against current state after onDeleteFiles completes: use functional setGroups and setSelectedPaths updates keyed to the local paths snapshot, so newer scans and selections are preserved. Also disable controls that modify selection or start a scan while isDeleting is true, including the relevant selection and scan handlers/rendered controls.
🧹 Nitpick comments (1)
src/utils/modalLayerSweep.test.ts (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
modalZIndexOfinstead of a duplicate parser.
readZduplicates the regex logic already exported asmodalZIndexOffrommodalLayers.ts. Import and reuse it here. This keeps the parsing logic in one place, matching the file's own stated goal of preventing drift between the numeric scale and its string representations.♻️ Proposed refactor to remove the duplicate parser
-import { MODAL_LAYER, MODAL_Z } from './modalLayers'; +import { MODAL_LAYER, MODAL_Z, modalZIndexOf } from './modalLayers';-/** `z-50` / `z-[9999]` → 50 / 9999. Anything else → null. */ -const readZ = (token: string): number | null => { - const arbitrary = token.match(/^z-\[(\d+)\]$/); - if (arbitrary) return Number(arbitrary[1]); - const scale = token.match(/^z-(\d+)$/); - return scale ? Number(scale[1]) : null; -}; -const literal = window.match(/\bz-(?:\[\d+\]|\d+)(?![\w-])/); - const z = literal ? readZ(literal[0]) : null; + const z = literal ? modalZIndexOf(literal[0]) : null;Also applies to: 29-35, 68-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/modalLayerSweep.test.ts` around lines 4 - 5, Update modalLayerSweep.test.ts to import and reuse modalZIndexOf from modalLayers.ts wherever readZ currently parses modal z-index strings, then remove the duplicate regex/parser logic from readZ while preserving the existing test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/DuplicateFinderDialog.tsx`:
- Around line 278-301: Update runDelete to reconcile deletion against current
state after onDeleteFiles completes: use functional setGroups and
setSelectedPaths updates keyed to the local paths snapshot, so newer scans and
selections are preserved. Also disable controls that modify selection or start a
scan while isDeleting is true, including the relevant selection and scan
handlers/rendered controls.
---
Nitpick comments:
In `@src/utils/modalLayerSweep.test.ts`:
- Around line 4-5: Update modalLayerSweep.test.ts to import and reuse
modalZIndexOf from modalLayers.ts wherever readZ currently parses modal z-index
strings, then remove the duplicate regex/parser logic from readZ while
preserving the existing test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 25644c49-7279-4aae-8d8c-5700c4a596dc
📒 Files selected for processing (10)
src/App.tsxsrc/components/AccountLockScreen.tsxsrc/components/Dialogs/index.tsxsrc/components/DuplicateFinderDialog.test.tssrc/components/DuplicateFinderDialog.tsxsrc/components/GuardedCloseConfirm.tsxsrc/components/LockScreen.tsxsrc/utils/modalLayerSweep.test.tssrc/utils/modalLayers.test.tssrc/utils/modalLayers.ts
…he snapshot Review finding on #540. `runDelete` closes over `groups` and `selectedPaths` as they were when the button was pressed, then writes them back after awaiting the delete. Deleting many files takes long enough for something to move underneath it: switching mode re-runs the scan, and the checkboxes stay live throughout. The completion would then write the captured groups over newer scan results and clear ticks the user had made after pressing. Both writes are now functional updates keyed by the set of paths that were actually deleted, so a newer scan survives and only what was deleted is unticked. The three controls that start a scan, the two mode toggles and the fuzzy-cutoff field, are disabled while a delete is in flight, which closes the other half of the race rather than only tidying up after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Valid, and the more interesting half is the one the finding does not spell out. Fixed in 50cc6bd. Both writes are now functional updates keyed by the set of paths actually deleted, so a newer scan survives and only what was deleted gets unticked, rather than the whole selection being cleared. But reconciling correctly only makes the outcome sane; it does not stop the race. The three controls that start a scan (the two mode toggles and the fuzzy-cutoff field) were live throughout a delete, so a scan could complete against a tree the delete had already changed. Those are now disabled while Pinned in |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 3 minutes. |
…erlay's own className Four review findings from #541, all in code this PR introduced. `onDeleteFiles` walks the list, so a reject can arrive with earlier paths already deleted. Which ones is not knowable from the dialog, and keeping the old rows left it offering to delete files that no longer exist. It re-scans, which is the only answer that cannot be wrong. The overlay sweep read the first `z-` token in a five-line window that began two lines before the `fixed inset-0` match, so a class belonging to a neighbouring element could be recorded as the overlay's. It now reads the `className` value that actually contains the match, whether that is a quoted string or a `{...}` expression. Making it strict immediately produced a false positive worth keeping out: `SaveAllMenu` has a comment explaining why its confirm is portalled, and the words `fixed inset-0` inside it were being counted as an overlay with no z-index. Comments are blanked before the scan, newlines preserved so the reported line numbers still point at the right place. The sweep also ran at module scope, so a parse failure aborted the file instead of failing a named test; it is collected lazily inside the tests now, and the "every overlay has a z-index" check is one of them rather than a bare `expect` during evaluation. `readZ` duplicated the exported `modalZIndexOf`, which is the parser the source itself uses, and an eslint-disable had no `any` to suppress. The scale gains the assertions that were missing: the elevated tier ordered internally and against the modal tier, and no two layers sharing a number, since an accidental tie is exactly what #537 was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Fixes #537.
What happened
Find Duplicates asked for confirmation twice. The dialog raises its own prompt, and the App callback behind
onDeleteFilesraised a second one throughsetConfirmDialogwhenever "confirm before delete" was on.ConfirmDialogis mounted ~300 lines earlier inApp.tsxthan the finder, and both sat atz-50— so at an equal z-index DOM order decided it. The finder painted over the very question it was waiting on, and its full-viewport backdrop swallowed every click aimed at the buttons underneath.That accounts for each step in the report: the button "loads up forever" because the promise the callback awaited never settled; dragging the finder aside reveals the confirm but does not free it, because the panel moves and the overlay does not; and closing the finder unmounts the overlay, at which point the confirm becomes clickable and the delete goes through to the Recycle Bin.
The fix
The dialog owns the confirmation outright. It takes
confirmBeforeDeleteand skips its own prompt when the setting is off; the callback only deletes. One question, in the window that asked it.The stacking half is fixed for the whole app rather than for this one pair.
src/utils/modalLayers.tsholds the scale — modal, a modal's own confirm, the elevated dialog tier, the app-wide confirm, the quit guard, the lock screens — and the sweep that came with it found two more of the same defect:z-50. Of the twelvesetConfirmDialogcall sites, the overwrite and delete prompts fire from flows started inside the z-9998..10000 tier (AeroSync, the transfer plan, the trash managers), so a confirm below them would have repeated this bug there.isAppLockedgates no rendering inApp.tsx— the whole tree stays mounted behind the lock overlay — so a dialog left open when the idle probe fired stayed legible on top of the lock screen. Both now sit above everything.One more thing surfaced on the way: the in-dialog confirm asked for
duplicates.confirmDelete, a key present in none of the 47 locale files, sot()printed the key name at the user instead of a question. It now usesduplicates.deleteConfirm, which exists.Pins
Four, each verified failing on the code it replaces:
modalLayerSweep.test.tswalks all 144fixed inset-0overlays in the source and refuses any that can cover the app-wide confirm or the lock screens, and any with no z-index at all.DuplicateFinderDialogmount site inApp.tsxmust carry nosetConfirmDialog.confirmBeforeDeleterather than confirm unconditionally.t()key the dialog uses must resolve inen.json, the fallback all 47 locales fall through to.On the alternative in the report
Hold-to-confirm is a real pattern, but not as a replacement for this one: a press-and-hold has no keyboard equivalent and is hard to complete with a motor impairment. Worth considering later as something a user can opt into, not as the default path to a delete.
Summary by CodeRabbit
New Features
Bug Fixes
Tests