fix: replace window.confirm with React dialogs in trash operations #274
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughDialog state was moved from a removed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Sidebar
participant TrashDialog
participant ConfirmModal
participant useTrashOperations
Note over Sidebar,TrashDialog: Open Recycle Bin
User->>Sidebar: click "Recycle Bin"
Sidebar->>TrashDialog: open (showTrashDialog = true)
Note over TrashDialog: User triggers action
User->>TrashDialog: click "Delete from disk"
TrashDialog->>ConfirmModal: show DeleteConfirmDialog
User->>ConfirmModal: confirm
ConfirmModal->>useTrashOperations: handleDeleteProjectFromDisk(projectId)
useTrashOperations-->>ConfirmModal: success / error
ConfirmModal-->>TrashDialog: close confirm
TrashDialog-->>Sidebar: optionally update UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/ui/src/components/layout/sidebar/dialogs/trash-dialog.tsx (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
Comment |
illia1f
changed the title
Summary of ChangesHello @illia1f, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Recycle Bin functionality by replacing native browser confirmation prompts with custom React dialogs. This change not only resolves a critical accessibility issue related to focus management but also enhances the overall user experience with a more integrated and visually consistent interface. Additionally, the underlying state management for trash operations has been optimized, and comprehensive error handling has been introduced to make the feature more robust. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively replaces the native window.confirm dialogs with React-based components for trash operations, which resolves the focus trap issue. The refactoring of state management for the trash dialog is well-executed, reducing prop drilling and improving component encapsulation. The addition of error handling for restore and empty trash operations is a great improvement for robustness. I have one suggestion to improve the conditional rendering of the delete confirmation dialog to prevent passing malformed props.
![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 (1)
apps/ui/src/components/layout/sidebar/dialogs/trash-dialog.tsx (1)
52-70: Consider consolidating confirmation state management.The confirmation handlers follow a consistent pattern, which is good. However, the defensive null check in
onConfirmDeleteFromDisk(lines 57-60) is redundant since the dialog'sopenprop is controlled bydeleteFromDiskProject !== null(line 155). The dialog cannot be confirmed if the project is null.🔎 Simplified version without redundant null check
const onConfirmDeleteFromDisk = () => { - if (deleteFromDiskProject) { - handleDeleteProjectFromDisk(deleteFromDiskProject); - setDeleteFromDiskProject(null); - } + handleDeleteProjectFromDisk(deleteFromDiskProject!); + setDeleteFromDiskProject(null); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/ui/src/components/layout/sidebar.tsxapps/ui/src/components/layout/sidebar/dialogs/trash-dialog.tsxapps/ui/src/components/layout/sidebar/hooks/index.tsapps/ui/src/components/layout/sidebar/hooks/use-trash-dialog.tsapps/ui/src/components/layout/sidebar/hooks/use-trash-operations.ts
💤 Files with no reviewable changes (2)
- apps/ui/src/components/layout/sidebar/hooks/use-trash-dialog.ts
- apps/ui/src/components/layout/sidebar/hooks/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ui/src/components/layout/sidebar/dialogs/trash-dialog.tsx (2)
apps/ui/src/lib/electron.ts (1)
TrashedProject(2802-2805)apps/ui/src/components/ui/confirm-dialog.tsx (1)
ConfirmDialog(34-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (6)
apps/ui/src/components/layout/sidebar/hooks/use-trash-operations.ts (2)
21-31: LGTM! Error handling properly added.The try-catch block correctly wraps the restore operation and provides user feedback via toast notifications. The error handling pattern is consistent with the other operations in this file.
66-79: TheemptyTrash()function is synchronous. Its implementation in the store simply callsset({ trashedProjects: [] }), which updates state synchronously with no async operations or unhandled promises. TheisEmptyingTrashstate is correctly reset in the finally block.apps/ui/src/components/layout/sidebar/dialogs/trash-dialog.tsx (2)
39-50: Good defensive cleanup of confirmation state.The cleanup logic in
handleOpenChangeproperly resets both confirmation dialog states when the main Trash dialog closes, preventing stale state from persisting across dialog re-opens.
154-176: Add explicit focus management to nested confirmation dialogs.The implementation correctly uses Radix Dialog components to keep nested dialogs within React/Radix control. However, each dialog instance activates its own independent modal focus trap by default, which can cause focus management issues when multiple dialogs are simultaneously active. When the user confirms a nested dialog action and it closes, focus may not properly return to the main TrashDialog.
Add one of these fixes:
- Use
modal={false}on nested dialog's DialogPrimitive.Root (trade-off: disables default scroll lock)- Implement explicit focus restoration with
openAutoFocusandcloseAutoFocusprops on DialogContent- Or add custom focus trap coordination in TrashDialog's state handlers
Test the three scenarios from the original review to confirm focus behavior is correct.
apps/ui/src/components/layout/sidebar.tsx (2)
31-31: Clean refactoring of dialog state management.The separation of concerns is well-executed:
useTrashOperationsfocuses purely on trash operations without UI state- Dialog visibility is managed locally in the component
- Props are cleanly passed to both the hook and the TrashDialog component
This refactoring aligns well with React best practices and makes the code more maintainable.
Also applies to: 71-72, 137-148
313-323: TrashDialog integration looks correct.The dialog receives all necessary props and the state management changes don't affect the data flow. The
trashedProjectsdata continues to come directly from the store, while the operations come from theuseTrashOperationshook.
|
3 participants
Description
Fixes #270
Test plan
2, Click "Delete from disk" and verify confirmation dialog appears
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.