fix(core): add permanent delete functionality to trash page #13053
Conversation
|
Warning Rate limit exceeded@yifeiyin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (19)
WalkthroughThe changes introduce a new permanent deletion confirmation flow for single and multiple items in the trash page. This includes updating the UI to show confirmation modals with localized text for permanent deletion, modifying the trash page logic to handle bulk permanent deletions, and adding new translation keys and localized strings for multiple languages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TrashPage
participant ConfirmModal
participant DocsExplorer
participant BlockSuiteMetaHelper
User->>DocsExplorer: Select one or more items
User->>DocsExplorer: Click "Delete Permanently"
DocsExplorer->>TrashPage: Trigger onDelete handler with selected IDs
TrashPage->>ConfirmModal: Show confirmation modal (single/multiple)
ConfirmModal-->>TrashPage: User confirms deletion
TrashPage->>BlockSuiteMetaHelper: Call permanentlyDeletePage for each ID
BlockSuiteMetaHelper-->>TrashPage: Deletion complete
TrashPage->>User: Show success toast
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/frontend/i18n/src/resources/ko.json (1)
818-821: Terminology & counting style diverge from existing stringsElsewhere in the file the UI consistently refers to items as “페이지” and formats counts without the “개의” classifier (e.g.
{{ number }} 페이지 …).
For consistency and to avoid user-visible mismatch, consider:- "com.affine.deletePermanently.confirmModal.title.multiple": "{{ number }}개의 문서를 영구적으로 삭제하시겠습니까?", - "com.affine.deletePermanently.confirmModal.description.multiple": "{{ number }}개의 문서가 영구적으로 삭제됩니다. 이 작업은 되돌릴 수 없습니다.", + "com.affine.deletePermanently.confirmModal.title.multiple": "{{ number }} 문서를 영구적으로 삭제하시겠습니까?", + "com.affine.deletePermanently.confirmModal.description.multiple": "{{ number }} 문서가 영구적으로 삭제됩니다. 이 작업은 되돌릴 수 없습니다.",(Optional) also decide whether “문서” or “페이지” should be the canonical noun and adjust all four newly added strings accordingly.
packages/frontend/i18n/src/resources/ar.json (1)
697-700: Ensure consistency with existing modal-text placeholdersOther confirmation dialogs (e.g.
moveToTrash.confirmModal.description) inject the page title via{{title}}, which gives clearer context to the user.
Here the singular “permanent delete” flow drops that placeholder and instead shows a generic “سيتم حذف مستند بشكل دائم”. Consider aligning with the existing pattern:-"com.affine.deletePermanently.confirmModal.description": "سيتم حذف مستند بشكل دائم. لا يمكن التراجع عن هذا الإجراء.", +"com.affine.deletePermanently.confirmModal.description": "{{title}} سيتم حذفه بشكل دائم. لا يمكن التراجع عن هذا الإجراء.",This keeps wording consistent across modals and makes the affected document explicit.
(Plural string already matches the earlier trash-move wording, so no change is required there.)packages/frontend/i18n/src/resources/sv-SE.json (1)
697-700: Align phrasing with existing “Permanent radera” terminologyElsewhere in this file (e.g.
com.affine.trashOperation.deletePermanently) the phrase “Permanent radera” is used.
The new entries flip the word order to “Radera … permanent”, which introduces a minor inconsistency and makes text-search harder.-"Radera dokument permanent?" +"Permanent radera dokument?" -"{{ number }} dokument kommer att raderas permanent. Denna åtgärd kan inte ångras." +"{{ number }} dokument kommer att permanent raderas. Denna åtgärd kan inte ångras."Apply the same adjustment to all four new keys so the wording is uniform across the locale.
packages/frontend/i18n/src/resources/de.json (1)
697-700: Keep key-group ordering & placeholder style consistent with neighbouring trash modal stringsIn the preceding “moveToTrash” block the order is description ➜ title (singular & plural).
Here you switched to title ➜ description. Although purely cosmetic, staying consistent makes future merges and automated diff reviews easier.While touching this, consider mirroring the existing placeholder pattern:
- "com.affine.deletePermanently.confirmModal.description": "Ein Dokument wird dauerhaft gelöscht. Diese Aktion kann nicht rückgängig gemacht werden.", + "com.affine.deletePermanently.confirmModal.description": "{{title}} wird dauerhaft gelöscht. Diese Aktion kann nicht rückgängig gemacht werden.",and swap the lines to match the earlier block:
- "com.affine.deletePermanently.confirmModal.title": "Seite dauerhaft löschen?", - "com.affine.deletePermanently.confirmModal.description": "{{title}} wird dauerhaft gelöscht. Diese Aktion kann nicht rückgängig gemacht werden.", + "com.affine.deletePermanently.confirmModal.description": "{{title}} wird dauerhaft gelöscht. Diese Aktion kann nicht rückgängig gemacht werden.", + "com.affine.deletePermanently.confirmModal.title": "Seite dauerhaft löschen?",Same applies to the “multiple” variants.
No functional impact, just tidying up.packages/frontend/i18n/src/resources/zh-Hant.json (1)
697-700: Align wording & placeholders with existing ‘move-to-trash’ strings
- Existing trash-flow keys use
頁面for single items (confirmModal.titleat L695) and inject the actual page title via{{title}}in the description. Here, the new permanent-delete keys switch to文件and hard-code “一篇文件”.- This breaks visual consistency for end-users and loses the specific page title that the user is about to erase.
Consider mirroring the established pattern:
-"com.affine.deletePermanently.confirmModal.title": "確認永久刪除文件嗎？", -"com.affine.deletePermanently.confirmModal.description": "一篇文件將被永久刪除。此操作無法撤銷。", +"com.affine.deletePermanently.confirmModal.title": "確認永久刪除頁面？", +"com.affine.deletePermanently.confirmModal.description": "{{title}} 將被永久刪除。此操作無法撤銷。",(Plural keys are fine as-is; they already follow the
{{ number }}pattern.)This keeps terminology unified (“頁面”), preserves contextual information (
{{title}}), and matches the UX of the existing delete-to-trash flow.packages/frontend/i18n/src/resources/zh-Hans.json (1)
697-700: 保持与现有确认文案的一致性现有“移动到回收站”确认弹窗使用了：
"title": "{{title}} 将被移动到回收站"而永久删除弹窗的单条描述改为：
"description": "一篇文档将被永久删除。此操作无法撤销。"
- 「一篇文档」缺少具体指向，UI 上可能已有文档标题可用，建议保持一致使用
{{title}}占位符，或统一成「该文档」。- 单数标题目前用“确认永久删除文档吗？”，而回收站用“确定要删除文档？”。若无特殊需求，建议统一动词（“确认”/“确定”）与问号样式，减少体验割裂。
示例 diff（若决定采用标题占位符和统一动词）：
-"com.affine.deletePermanently.confirmModal.title": "确认永久删除文档吗？", -"com.affine.deletePermanently.confirmModal.description": "一篇文档将被永久删除。此操作无法撤销。", +"com.affine.deletePermanently.confirmModal.title": "确定要永久删除文档？", +"com.affine.deletePermanently.confirmModal.description": "{{title}} 将被永久删除。此操作无法撤销。",仅供参考，确保与英文/其他语言资源及前端实现匹配后再修改。
packages/frontend/i18n/src/resources/uk.json (1)
697-700: Incorrect pluralisation & verb agreement for “multiple-delete” stringsWhen
{{ number }}> 1 the noun must be plural (“документів”) and the verb should match (“будуть”). Current text remains singular, mirroring the same issue found in the existing moveToTrash keys.
Consider the quick fix below:-"com.affine.deletePermanently.confirmModal.title.multiple": "Видалити {{ number }} документ назавжди?", -"com.affine.deletePermanently.confirmModal.description.multiple": "{{ number }} документ буде видалено назавжди. Цю дію не можна скасувати.", +"com.affine.deletePermanently.confirmModal.title.multiple": "Видалити {{ number }} документів назавжди?", +"com.affine.deletePermanently.confirmModal.description.multiple": "{{ number }} документів будуть видалені назавжди. Цю дію не можна скасувати.",(Feel free to refine further if you plan to add proper ICU plural rules later.)
packages/frontend/i18n/src/resources/ja.json (1)
697-700: Unify the noun used with the numeric placeholder for consistencyExisting “move-to-trash” strings mix 「件のドキュメント」 (title) and 「個のドキュメント」 (description).
The newly-added permanent-delete strings use 「件のドキュメント」 in both places.
Prefer picking one noun repo-wide (either 「件」 or 「個」) and applying it uniformly to avoid subtle wording differences in otherwise identical confirmation dialogs.No functional breakage, but worth tidying up before translations proliferate further.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/frontend/core/src/desktop/pages/workspace/trash-page.tsx(4 hunks)packages/frontend/i18n/src/i18n.gen.ts(1 hunks)packages/frontend/i18n/src/resources/ar.json(1 hunks)packages/frontend/i18n/src/resources/de.json(1 hunks)packages/frontend/i18n/src/resources/el-GR.json(1 hunks)packages/frontend/i18n/src/resources/en.json(1 hunks)packages/frontend/i18n/src/resources/es.json(1 hunks)packages/frontend/i18n/src/resources/fa.json(1 hunks)packages/frontend/i18n/src/resources/fr.json(1 hunks)packages/frontend/i18n/src/resources/it-IT.json(1 hunks)packages/frontend/i18n/src/resources/ja.json(1 hunks)packages/frontend/i18n/src/resources/ko.json(1 hunks)packages/frontend/i18n/src/resources/pl.json(1 hunks)packages/frontend/i18n/src/resources/pt-BR.json(1 hunks)packages/frontend/i18n/src/resources/ru.json(1 hunks)packages/frontend/i18n/src/resources/sv-SE.json(1 hunks)packages/frontend/i18n/src/resources/uk.json(1 hunks)packages/frontend/i18n/src/resources/zh-Hans.json(1 hunks)packages/frontend/i18n/src/resources/zh-Hant.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/frontend/i18n/src/i18n.gen.ts (1)
undefined
<retrieved_learning>
Learnt from: EYHN
PR: #12320
File: packages/frontend/core/src/desktop/dialogs/collection-editor/rules-mode.tsx:366-366
Timestamp: 2025-05-22T02:37:07.462Z
Learning: In React components, avoid redundant translation calls. When using services like docDisplayMetaService.title$ that already provide localized strings, don't wrap the result in another translation call like i18n.t() as it's unnecessary and may break interpolation if the string contains special markers.
</retrieved_learning>
🔇 Additional comments (13)
packages/frontend/i18n/src/resources/es.json (1)
697-700: Switch the word order so it matches the existing “Eliminar permanentemente” terminologyElsewhere in this file the wording pattern is “Eliminar permanentemente …” (e.g.
com.affine.trashOperation.deletePermanently).
For consistency—and a slightly more natural-sounding Spanish—swap the adverb in the two title keys:-"com.affine.deletePermanently.confirmModal.title": "¿Eliminar documento permanentemente?", +"com.affine.deletePermanently.confirmModal.title": "¿Eliminar permanentemente el documento?", … -"com.affine.deletePermanently.confirmModal.title.multiple": "¿Eliminar {{ number }} documentos permanentemente?", +"com.affine.deletePermanently.confirmModal.title.multiple": "¿Eliminar permanentemente {{ number }} documentos?",No functional impact, just keeps the UX copy coherent across the app.
[ suggest_nitpick ]packages/frontend/i18n/src/resources/en.json (1)
702-705: Confirm placeholder & key propagation across all localesNew keys look fine in English, but please ensure:
- The
{{ number }}placeholder name/spacing matches the code that interpolates it (most usages in the repo seem to rely onnumberwithout spaces or oncount).- Equivalent keys are added to every non-English locale to avoid runtime fallback to IDs.
No code changes needed here—just asking for a quick cross-check before merge.
packages/frontend/i18n/src/resources/it-IT.json (1)
697-700: Localization entries look correct and follow existing conventions• Key naming mirrors the pattern used for the trash-move strings (lines 693-696).
• Placeholder spacing{{ number }}matches earlier usage.
• Italian wording and punctuation are natural and consistent.No changes required.
packages/frontend/i18n/src/resources/pl.json (1)
697-700: New translation entries look good and consistent
The keys follow the existing naming/placeholder conventions ({{ number }}) used for the trash-modal strings, and the Polish phrasing is clear. No issues spotted.packages/frontend/i18n/src/resources/pt-BR.json (1)
697-700: New permanent-delete strings look consistentKeys follow the existing naming scheme (
deletePermanently.confirmModal.*), the singular/plural messages mirror the earliermoveToTrashcounterparts, and the{{ number }}placeholder matches the established placeholder style in this file. No issues spotted. 👍packages/frontend/i18n/src/resources/fr.json (1)
697-700: New permanent-delete strings look correct and consistentKeys follow existing naming conventions (
deletePermanently.confirmModal.*), placeholders ({{ number }}) mirror the trash-modal counterparts, and the French phrasing is idiomatic.
Nothing else to flag.packages/frontend/i18n/src/resources/el-GR.json (1)
697-700: Strings look consistent – no issues found• Keys follow existing naming conventions.
• Greek phrasing, punctuation (;as question mark) and placeholders ({{ number }}) match the style of neighbouring entries.
• No missing commas or malformed JSON detected.packages/frontend/i18n/src/resources/ru.json (1)
697-700: Keys & placeholders look consistent – no issues found.The new confirmation-modal strings align with existing patterns (
{{ number }}placeholder matches earlier “moveToTrash” keys) and use clear Russian wording. No action required.packages/frontend/i18n/src/resources/fa.json (1)
697-700: Persian punctuation / wording – use the Persian question mark and keep wording consistent with existing stringsCurrent strings end with the western “?” but elsewhere in this file the Persian “؟” is used (e.g. key
com.affine.moveToTrash.confirmModal.title).
Also keep wording parallel to the existing “آیا سند را حذف کنید؟” pattern.-"com.affine.deletePermanently.confirmModal.title": "آیا سند را به طور دائم حذف کنید?", +"com.affine.deletePermanently.confirmModal.title": "آیا سند را به طور دائم حذف کنید؟", -"com.affine.deletePermanently.confirmModal.title.multiple": "آیا {{ number }} سند را به طور دائم حذف کنید?", +"com.affine.deletePermanently.confirmModal.title.multiple": "آیا {{ number }} سند را به طور دائم حذف کنید؟",Optional: consider replacing «قابل بازگشت نیست» with «قابل بازگردانی نیست» for fluency (used elsewhere), but that’s stylistic.
[ suggest_nitpick ]packages/frontend/core/src/desktop/pages/workspace/trash-page.tsx (3)
1-1: LGTM: Import statement is correct.The addition of
useConfirmModalimport is appropriate for the confirmation modal functionality being added.
46-47: LGTM: Hook usage is properly implemented.The destructuring of
openConfirmModalandpermanentlyDeletePagefollows the existing pattern and is correctly positioned within the component.
190-192: LGTM: Permission-gated deletion handler is correctly implemented.The
onDeleteprop correctly passes the deletion handler only for users with admin or owner permissions, maintaining consistency with the existingonRestoreprop pattern.packages/frontend/i18n/src/i18n.gen.ts (1)
2812-2831: LGTM! Well-structured translation key additions.The new translation keys for permanent deletion confirmation modals are properly structured and follow the existing naming conventions. The TypeScript signatures are correct, with appropriate parameter types for interpolation in the multiple document variants that match the existing patterns in the codebase.
- The button was not working (resolves toeverything#12869) - Introduce a confirmation modal for permanently deleting documents from the trash - Update the TrashPage component to handle multi-document deletion - Added necessary translations
yifeiyin
force-pushed
the
multi-permanent-delete
branch
from
6ca1741 to
55cca0d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## canary #13053 +/- ##
==========================================
- Coverage 57.23% 56.74% -0.50%
==========================================
Files 2702 2702
Lines 131363 131363
Branches 20601 20512 -89
==========================================
- Hits 75190 74542 -648
- Misses 54512 54611 +99
- Partials 1661 2210 +549
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
Also, the icons were not consistent (delete vs permanently delete), updated in a separate PR: #13051
Summary by CodeRabbit
New Features
Localization