feat: Trash clear function#14455
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an "Empty trash" action: a postfix menu on TrashButton and an inline button on TrashPage that confirm and permanently delete trashed documents, using trashDocs$, permission checks, permanentlyDeletePage, a success toast, i18n keys, and a small button style. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as "Trash Button / Trash Page"
participant Docs as "DocsService (trashDocs$)"
participant Confirm as "Confirmation Modal"
participant Deleter as "permanentlyDeletePage"
participant Toast as "Toast Notification"
User->>UI: open menu or click empty action
UI->>Docs: read current trashed docs
UI->>Confirm: open confirmation dialog
Confirm->>User: display confirmation
User->>Confirm: confirm deletion
Confirm-->>UI: confirmed
UI->>Deleter: call permanentlyDeletePage for each doc
Deleter->>Deleter: perform permanent deletions
UI->>Toast: show success message
Toast-->>User: display notification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 You can disable sequence diagrams in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/core/src/desktop/pages/workspace/trash-page.tsx (1)
28-55:⚠️ Potential issue | 🟠 MajorEmpty trash button is not gated by admin/owner permissions.
The
DocsExplorerbelow (lines 210-214) conditionally hides delete/restore actions for non-admin/non-owner users, but the "Empty Trash" button inTrashHeaderis always visible. A non-privileged user could permanently delete all trashed docs.Consider applying the same permission guard:
🛡️ Proposed fix
In
TrashPage, pass the permission check toTrashHeader:<TrashHeader onEmptyTrash={onEmptyTrash} - disableEmptyTrash={trashDocs.length === 0} + disableEmptyTrash={trashDocs.length === 0 || (!isAdmin && !isOwner)} />
🤖 Fix all issues with AI agents
In `@packages/frontend/core/src/components/root-app-sidebar/trash-button.tsx`:
- Around line 36-55: Add a permission guard and make the confirm title
consistent: inside handleEmptyTrash, before opening the modal check
guardService.can('Doc_Trash', { workspaceId: workspace.id }) (or the same guard
call used in the drop-to-trash handler) and early-return or show a
permission-error if false; update the openConfirmModal title to use
t['com.affine.trashOperation.deletePermanently']() so it matches trash-page.tsx;
keep the existing onConfirm body (workspace.docCollection.removeDoc for each doc
and toast) but only invoke the modal when the guard passes.
- Around line 48-51: The trash-button uses
workspace.docCollection.removeDoc(doc.id) whereas trash-page uses the helper
permanentlyDeletePage from useBlockSuiteMetaHelper; change the onConfirm handler
to call permanentlyDeletePage for each doc in trashDocs (e.g., map or forEach
over trashDocs invoking permanentlyDeletePage(doc.id)) instead of
workspace.docCollection.removeDoc, and remove the now-unneeded WorkspaceService
import if nothing else uses it so both components use the same deletion API.
packages/frontend/core/src/components/root-app-sidebar/trash-button.tsx
Outdated
Show resolved
Hide resolved
packages/frontend/core/src/components/root-app-sidebar/trash-button.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/frontend/core/src/components/root-app-sidebar/trash-button.tsx`:
- Around line 63-68: The onConfirm handler calls permanentlyDeletePage for each
item in trashDocs with no error handling; make onConfirm async and wrap the
deletion logic in a try/catch (or use Promise.allSettled and check results) so
any thrown errors are caught, show a user-friendly error toast (consistent with
the drag-to-trash handler’s UserFriendlyError pattern) on failure, and only call
toast(t['com.affine.toastMessage.permanentlyDeleted']()) when all deletions
succeed; reference the onConfirm handler, trashDocs, permanentlyDeletePage, and
toast when updating the code.
- Around line 40-47: The permission check currently uses
guardService.can('Doc_Trash', doc.id) for items in trashDocs before calling
permanentlyDeletePage(); update the check to use guardService.can('Doc_Delete',
doc.id) instead so permanent deletions require the correct permission, leaving
the rest of the flow (Promise.all over trashDocs, the toast call
t['com.affine.no-permission'](), and the early return) unchanged.
packages/frontend/core/src/components/root-app-sidebar/trash-button.tsx
Outdated
Show resolved
Hide resolved
packages/frontend/core/src/components/root-app-sidebar/trash-button.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/frontend/core/src/desktop/pages/workspace/trash-page.tsx (3)
153-155: Consider using a distinct confirmation message for "Empty Trash".
onEmptyTrashreusesonConfirmPermanentlyDelete, which shows a generic "delete permanently?" dialog. For a bulk "empty entire trash" action, users would benefit from a more explicit message (e.g., mentioning the count of items or that all trashed items will be removed). This reduces the risk of accidental data loss.
199-202: Two different data sources drive "empty" state vs. button disabled state.
disableEmptyTrashis based ontrashDocs.length(fromDocsService), whileisEmpty(line 94-96) is derived fromgroups(fromCollectionRulesService). These could momentarily disagree — e.g., the button could be enabled while the list appears empty, or vice versa. Consider unifying on one source, or at minimum be aware of the transient inconsistency.
112-120: Add error handling tohandleMultiDeleteto prevent partial failures on bulk delete.
permanentlyDeletePagecallsworkspace.docCollection.removeDoc(), which throwsBlockSuiteErrorif a document cannot be found. InhandleMultiDelete, the forEach loop has no error handling, so if any deletion throws, the loop breaks and remaining items skip silently—yet the success toast still fires. This is inconsistent withtrash-button.tsx(lines 64–73), which wraps the same operation in try-catch.Proposed improvement
const handleMultiDelete = useCallback( (ids: string[]) => { - ids.forEach(pageId => { - permanentlyDeletePage(pageId); - }); - toast(t['com.affine.toastMessage.permanentlyDeleted']()); + const failed: string[] = []; + ids.forEach(pageId => { + try { + permanentlyDeletePage(pageId); + } catch { + failed.push(pageId); + } + }); + if (failed.length > 0) { + toast(`Failed to delete ${failed.length} item(s)`); + } else { + toast(t['com.affine.toastMessage.permanentlyDeleted']()); + } }, [permanentlyDeletePage, t] );
…of-fox/AFFiNE into canary-trash-clear-function
|
Screenshot to visualize the changes: Changes in Trash page (see icon on the right side): |
|
quickinfo on mouse over the icon in trash page: |
man-of-fox
changed the title
man-of-fox
left a comment
man-of-fox
left a comment
There was a problem hiding this comment.
everything commented by coderabbitai[bot] is integrated and tested
man-of-fox
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## canary #14455 +/- ##
==========================================
- Coverage 57.43% 56.93% -0.50%
==========================================
Files 2859 2859
Lines 154468 154468
Branches 23330 23199 -131
==========================================
- Hits 88713 87954 -759
- Misses 62771 63603 +832
+ Partials 2984 2911 -73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Added a new “Empty Trash” feature in two places:
Both actions open a confirmation dialog and then permanently delete all items in Trash.
Summary by CodeRabbit
Screenshot to visualize the changes:
Changes on left menu bar trash icon (the 3 dots only shown on mouse over, like in folder view):
On click at "Empty trash" the following dialog is shown:
Changes in Trash page (see icon on the right side):
On click at the icon the same dialog as above is shown:
quickinfo on mouse over the icon in trash page:
