Conversation
|
""" WalkthroughThis change introduces optimistic deletion for draft emails in a mail application. It adds a context menu for drafts with a delete option, updates hooks and state management to support optimistic deletion, extends backend interfaces and APIs to handle draft deletions, and updates localization for the new action. Both single and bulk draft deletions are supported. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DraftContextMenu
participant useOptimisticActions
participant BackendAPI
participant MailManager
User->>DraftContextMenu: Right-click draft, select "Delete"
DraftContextMenu->>useOptimisticActions: optimisticDeleteDrafts([draftId])
useOptimisticActions->>useOptimisticThreadState: Set shouldHide, isRemoving = true
useOptimisticActions->>BackendAPI: Call bulkDeleteDraft([draftId])
BackendAPI->>MailManager: deleteDraft(draftId)
MailManager-->>BackendAPI: Deletion result
BackendAPI-->>useOptimisticActions: Success/Failure response
useOptimisticActions->>useOptimisticThreadState: Finalize optimistic state
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 2
🔭 Outside diff range comments (2)
apps/mail/hooks/use-optimistic-actions.ts (2)
367-402:⚠️ Potential issueFix the syntax error in
optimisticToggleImportantfunction.The function is missing proper closure and the dependency array is misplaced. This is not a
useCallbackhook but a regular function.function optimisticToggleImportant(threadIds: string[], isImportant: boolean) { if (!threadIds.length) return; const optimisticId = addOptimisticAction({ type: 'IMPORTANT', threadIds, important: isImportant, }); createPendingAction({ type: 'IMPORTANT', threadIds, params: { important: isImportant }, optimisticId, execute: async () => { await toggleImportant({ ids: threadIds }); if (mail.bulkSelected.length > 0) { setMail({ ...mail, bulkSelected: [] }); } }, undo: () => { removeOptimisticAction(optimisticId); }, toastMessage: isImportant ? 'Marked as important' : 'Unmarked as important', }); - }, - [ - addOptimisticAction, - createPendingAction, - mail, - removeOptimisticAction, - setMail, - toggleImportant, - ], - ); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 393-402: Expected a statement but instead found ',
[
addOptimisticAction,
createPendingAction,
mail,
removeOptimisticAction,
setMail,
toggleImportant,
],
)'.Expected a statement here.
(parse)
467-487:⚠️ Potential issueFix the
undoLastActionimplementation.The function has several issues that need to be addressed:
- It checks
lastActionIdRef.currentwhich is an unused ref- Missing closing bracket and dependency array for
useCallbackconst undoLastAction = useCallback(() => { - if (!lastActionIdRef.current) return; + if (!optimisticActionsManager.lastActionId) return; const lastAction = optimisticActionsManager.pendingActions.get( optimisticActionsManager.lastActionId, ); if (!lastAction) return; lastAction.undo(); optimisticActionsManager.pendingActions.delete(optimisticActionsManager.lastActionId); optimisticActionsManager.pendingActionsByType .get(lastAction.type) ?.delete(optimisticActionsManager.lastActionId); if (lastAction.toastId) { toast.dismiss(lastAction.toastId); } optimisticActionsManager.lastActionId = null; - } + }, []);
🧹 Nitpick comments (6)
apps/mail/components/mail/mail-list.tsx (1)
534-582: Consider simplifying the complex conditional rendering structure.The Draft component refactor correctly integrates optimistic deletion state, but the nested conditional logic is quite complex and impacts readability.
Consider this cleaner structure:
return ( - draft ? ( - <AnimatePresence mode="sync"> - {!optimisticState.shouldHide && (<DraftContextMenu draftId={message.id}> + draft && !optimisticState.shouldHide ? ( + <DraftContextMenu draftId={message.id}> <div className="select-none py-1" onClick={handleMailClick}> {/* draft content */} </div> - </DraftContextMenu> )} - </AnimatePresence> - ): null + </DraftContextMenu> + ) : nullAlso, consider whether
AnimatePresenceis necessary here since you're only conditionally rendering based onshouldHiderather than animating transitions.apps/server/src/trpc/routes/mail.ts (1)
321-339: Well-implemented bulk deletion with proper error handling.The mutation correctly uses
Promise.allSettledfor concurrent operations and provides appropriate error handling. The implementation follows established patterns in the codebase.Consider enhancing error details to include which specific draft IDs failed:
const results = await Promise.allSettled(ids.map((id) => driver.deleteDraft(id))); - const errors = results.filter((result) => result.status === 'rejected'); + const failures = results + .map((result, index) => ({ result, draftId: ids[index] })) + .filter(({ result }) => result.status === 'rejected'); if (errors.length > 0) { - return { success: false, error: 'Failed to delete some drafts', details: errors }; + return { + success: false, + error: 'Failed to delete some drafts', + failedIds: failures.map(f => f.draftId), + details: failures.map(f => f.result) + }; }apps/mail/components/context/draft-context.tsx (1)
1-11: Clean up unused imports.Several context menu components are imported but not used in this file.
Remove the unused imports:
import { ContextMenu, ContextMenuContent, ContextMenuItem, - ContextMenuSeparator, - ContextMenuShortcut, - ContextMenuSub, - ContextMenuSubContent, - ContextMenuSubTrigger, ContextMenuTrigger, } from '../ui/context-menu';apps/mail/hooks/use-optimistic-actions.ts (3)
423-423: Remove debug console.log statement.Debug logging should be removed before merging to production.
- console.log("DELETING DRAFT FROM OPTIMISTIC", draftIds )
95-95: Remove all debug console.log statements.Multiple debug logging statements should be removed throughout the file.
- console.log('here Generated pending action ID:', pendingActionId);- console.log('here Creating new Set for action type:', type);- console.log( - 'here', - 'Added pending action to type:', - type, - 'Current size:', - optimisticActionsManager.pendingActionsByType.get(type)?.size, - );- console.log('here', { - pendingActionsByTypeRef: optimisticActionsManager.pendingActionsByType.get(type)?.size, - pendingActionsRef: optimisticActionsManager.pendingActions.size, - typeActions: typeActions?.size, - });Also applies to: 98-98, 102-108, 129-133
452-465: Consider optimizing the dependency array.The
optimisticDeleteDraftscallback has many dependencies which could cause frequent re-creations. Consider whether all these dependencies are necessary, particularlyYou might want to use
+ const mailRef = useRef(mail); + mailRef.current = mail; const optimisticDeleteDrafts = useCallback( (draftIds: string[]) => { // ... existing code ... - if (mail.bulkSelected.length > 0) { - setMail({ ...mail, bulkSelected: [] }); + if (mailRef.current.bulkSelected.length > 0) { + setMail({ ...mailRef.current, bulkSelected: [] }); }And remove
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/mail/components/context/draft-context.tsx(1 hunks)apps/mail/components/mail/mail-list.tsx(2 hunks)apps/mail/components/mail/optimistic-thread-state.tsx(1 hunks)apps/mail/hooks/use-optimistic-actions.ts(5 hunks)apps/mail/locales/en.json(1 hunks)apps/mail/store/optimistic-updates.ts(1 hunks)apps/server/src/lib/driver/google.ts(1 hunks)apps/server/src/lib/driver/types.ts(1 hunks)apps/server/src/trpc/routes/mail.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/mail/components/mail/mail-list.tsx (3)
apps/mail/components/mail/optimistic-thread-state.tsx (1)
useOptimisticThreadState(7-80)apps/mail/components/context/draft-context.tsx (1)
DraftContextMenu(24-58)apps/mail/lib/utils.ts (1)
cn(51-51)
apps/server/src/trpc/routes/mail.ts (1)
apps/server/src/trpc/trpc.ts (1)
activeDriverProcedure(44-78)
apps/mail/hooks/use-optimistic-actions.ts (1)
apps/mail/lib/optimistic-actions-manager.ts (1)
PendingAction(1-10)
🪛 Biome (1.9.4)
apps/mail/hooks/use-optimistic-actions.ts
[error] 16-16: Shouldn't redeclare 'PendingAction'. Consider to delete it or rename it.
'PendingAction' is defined here:
(lint/suspicious/noRedeclare)
[error] 393-402: Expected a statement but instead found ',
[
addOptimisticAction,
createPendingAction,
mail,
removeOptimisticAction,
setMail,
toggleImportant,
],
)'.
Expected a statement here.
(parse)
🔇 Additional comments (6)
apps/server/src/lib/driver/types.ts (1)
52-52: Clean interface extension for draft deletion.The method signature is well-designed and appropriately positioned among other draft-related methods in the interface.
apps/mail/locales/en.json (1)
462-462: Appropriate localization for draft deletion.The label is clear and properly placed in the shortcuts actions section.
apps/mail/store/optimistic-updates.ts (1)
9-10: Well-structured optimistic action type for deletion.The new DELETE action follows the established pattern and correctly uses threadIds array for consistency with the UI layer.
apps/server/src/lib/driver/google.ts (1)
589-602: Solid implementation following established patterns.The deleteDraft method correctly implements the interface using the established error handling pattern and appropriate Gmail API calls. The implementation is consistent with other methods in the class.
apps/mail/components/mail/optimistic-thread-state.tsx (1)
66-69: LGTM! Clean implementation following existing patterns.The DELETE case correctly sets both
isRemovingandshouldHideflags, which is consistent with the MOVE action pattern and appropriate for deletion operations.apps/mail/components/context/draft-context.tsx (1)
24-58: Well-designed context menu component with proper state management.The component correctly handles both single and bulk draft deletion scenarios, and appropriately clears bulk selection state after deletion. The logic is clean and follows React best practices.
ahmetskilinc
left a comment
ahmetskilinc
left a comment
There was a problem hiding this comment.
Hey great pr. I left 1 comment.
Thank you @ahmetskilinc, fixed the requested changes. Please let me know if any update needed |
Merge activity
|
|
@MrgSub ready to merge! |
MrgSub
left a comment
MrgSub
left a comment
There was a problem hiding this comment.
Please add a flag to enable/disable this feature, i.e isDeleteDraftEnabled somewhere in the env
|
@MrgSub Added the flag in env and tested. Working fine as expected. |
|
@MrgSub please take a look |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/mail/components/mail/mail-list.tsx (1)
41-41: Verify if the motion import is needed.Based on the past review comments discussing animation removal and the fact that
AnimatePresencewas removed, thismotionimport appears to be unused in the current code. Consider removing it if it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/mail-list.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/mail/components/mail/mail-list.tsx
[error] 646-646: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '!optimisticState.shouldHide && (<DraftContextMenu draftId={message.id'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 690-690: expected ) but instead found <
Remove <
(parse)
[error] 691-691: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
🔇 Additional comments (3)
apps/mail/components/mail/mail-list.tsx (3)
31-31: LGTM!The import of
DraftContextMenuis correctly added and aligns with the draft deletion functionality implementation.
604-604: LGTM!The integration of
useOptimisticThreadStatehook is correctly implemented and necessary for the optimistic deletion functionality.
600-693: Approve the optimistic deletion integration approach.The overall approach to implement optimistic deletion is well-designed:
- Integration with
useOptimisticThreadStatefor state management- Conditional rendering based on
shouldHidestate- Wrapping with
DraftContextMenufor delete functionalityOnce the syntax errors are resolved, this implementation will effectively support the draft deletion feature.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| createPendingAction({ | ||
| type: 'DELETE', | ||
| threadIds: draftIds, | ||
| params: { currentFoler: 'draft', isDraft: true }, |
There was a problem hiding this comment.
There's a typo in the parameter name: currentFoler should be currentFolder.
| params: { currentFoler: 'draft', isDraft: true }, | |
| params: { currentFolder: 'draft', isDraft: true }, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
This PR has merge conflicts and has been open for more than 3 days. It will be automatically closed. Please resolve the conflicts and reopen the PR if you'd like to continue working on it. |
3 participants
Aded delete draft functionality
This PR fix/complete https://feedback.0.email/p/unable-to-delete-draft-emails-in-zero-client
Screen.Recording.2025-06-08.at.4.41.04.AM.mov
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Configuration