[Fix: 488] - Mark thread as read to update email status to read #499
Conversation
|
@Lokendra-sinh is attempting to deploy a commit to the Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces a new function, Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (3)
apps/mail/app/api/driver/google.ts (3)
180-209: Improved thread handling to properly mark emails as read.The new implementation correctly addresses the issue where email threads weren't being marked as read by retrieving all messages in a thread and removing the "UNREAD" label from each message. The added error handling is also a good improvement.
There's an indentation inconsistency - the new code uses tabs while the rest of the file uses spaces. Consider standardizing the indentation style:
markAsRead: async (id: string[]) => { - try { - - if (id.length > 0) { - const threadResponse = await gmail.users.threads.get({ - userId: 'me', - id: id[0], - }); - - const thread = threadResponse.data; - - const messageIds = - thread.messages - ?.map((message) => message.id) - .filter((id): id is string => id !== null && id !== undefined) || []; - - - if (messageIds.length > 0) { - await gmail.users.messages.batchModify({ - userId: 'me', - requestBody: { - ids: messageIds, - removeLabelIds: ['UNREAD'], - }, - }); - } - } - } catch (error) { - console.error('Error marking messages as read:', error); - throw error - } + try { + if (id.length > 0) { + const threadResponse = await gmail.users.threads.get({ + userId: 'me', + id: id[0], + }); + + const thread = threadResponse.data; + + const messageIds = + thread.messages + ?.map((message) => message.id) + .filter((id): id is string => id !== null && id !== undefined) || []; + + if (messageIds.length > 0) { + await gmail.users.messages.batchModify({ + userId: 'me', + requestBody: { + ids: messageIds, + removeLabelIds: ['UNREAD'], + }, + }); + } + } + } catch (error) { + console.error('Error marking messages as read:', error); + throw error; + } },
207-208: Add missing semicolon for consistency.There's a missing semicolon at the end of the
throw errorstatement. This should be fixed to maintain code style consistency.- throw error + throw error;
211-219: Consider updating markAsUnread function to mirror markAsRead implementation.For consistency, the
markAsUnreadfunction should also use a try-catch block and possibly use the same thread-based approach to ensure all messages in a thread are marked as unread.markAsUnread: async (id: string[]) => { - await gmail.users.messages.batchModify({ - userId: "me", - requestBody: { - ids: id, - addLabelIds: ["UNREAD"], - }, - }); + try { + if (id.length > 0) { + const threadResponse = await gmail.users.threads.get({ + userId: 'me', + id: id[0], + }); + + const thread = threadResponse.data; + + const messageIds = + thread.messages + ?.map((message) => message.id) + .filter((id): id is string => id !== null && id !== undefined) || []; + + if (messageIds.length > 0) { + await gmail.users.messages.batchModify({ + userId: "me", + requestBody: { + ids: messageIds, + addLabelIds: ["UNREAD"], + }, + }); + } + } + } catch (error) { + console.error('Error marking messages as unread:', error); + throw error; + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/app/api/driver/google.ts(1 hunks)
🔇 Additional comments (1)
apps/mail/app/api/driver/google.ts (1)
190-193: Well-implemented null safety with TypeScript type predicate.Good use of the TypeScript type predicate
(id): id is string => id !== null && id !== undefinedto filter out null or undefined values while maintaining type safety.
…or multiple threads
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/mail/app/api/driver/google.ts (2)
102-104: Consider revising early return conditionThe current check might allow unnecessary processing when both arrays are non-empty but one has elements and the other is empty.
- if(addLabelIds.length === 0 || removeLabelIds.length === 0){ + if(addLabelIds.length === 0 && removeLabelIds.length === 0){ return }
245-257: Fix error message in markAsUnreadThe error message incorrectly states "Error marking messages as read" instead of "unread".
- console.error('Error marking messages as read:', error); + console.error('Error marking messages as unread:', error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/mail/app/api/driver/google.ts(3 hunks)apps/mail/components/mail/mail-list.tsx(1 hunks)apps/mail/components/mail/mail.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/mail/components/mail/mail.tsx
- apps/mail/components/mail/mail-list.tsx
🔇 Additional comments (4)
apps/mail/app/api/driver/google.ts (4)
38-42: Good optimization: Auth client defined at module levelMoving the OAuth2 client outside the driver function allows it to be reused across different function calls, avoiding unnecessary recreation of the authorization object.
87-91: Well-defined TypeScript interfaceGood use of TypeScript interface to define the structure of props for the new function, making the code more maintainable and providing clear type definitions.
93-137: Great implementation of batch processingThis new function effectively implements the batch processing capability suggested in the previous review. I appreciate the thorough input validation, proper error handling, and efficient use of Promise.all for concurrent execution.
The implementation correctly:
- Retrieves all message IDs from multiple threads
- Filters out null/undefined values
- Uses the batchModify endpoint for better performance
- Includes comprehensive error handling
232-244: Good refactoring of markAsReadThe method has been well refactored to use the new batch processing function with proper error handling.
|
Fixes #488 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/mail/app/api/driver/google.ts (2)
93-137: Excellent implementation of batch processing for multiple threads.The function efficiently processes multiple email threads by:
- Retrieving message IDs from each thread
- Flattening the results into a single array
- Using a batch modify operation to update all messages at once
Good job including proper validation and error handling.
Consider clarifying the early return condition on line 102-104. It currently returns if either array is empty, which works for the current use cases but might be confusing:
- if(addLabelIds.length === 0 || removeLabelIds.length === 0){ + if(addLabelIds.length === 0 && removeLabelIds.length === 0){ return }This change would only return if there are no label operations to perform.
254-255: Fix error message in markAsUnread.The error message incorrectly refers to marking messages as "read" instead of "unread".
-console.error('Error marking messages as read:', error); +console.error('Error marking messages as unread:', error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/app/api/driver/google.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/app/api/driver/google.ts (1)
apps/mail/lib/auth.ts (1) (1)
auth(155-158)
🔇 Additional comments (3)
apps/mail/app/api/driver/google.ts (3)
87-91: LGTM! Well-structured interface.Good practice defining a clear TypeScript interface for the function parameters. This improves code readability and type safety.
233-244: Great refactoring of markAsRead to use batch processing.The implementation now efficiently handles multiple threads and includes proper error handling.
38-42:❓ Verification inconclusive
Good optimization by moving auth outside the driver function.
Making
autha module-level constant improves performance by creating a single reusable OAuth2 client instance instead of recreating it on each driver initialization.
🏁 Script executed:
#!/bin/bash # Check for any potential issues with the shared auth approach grep -A 5 -B 5 "auth.setCredentials" apps/mail/app/api/driver/google.tsLength of output: 853
OAuth2 Client Optimization Approved – Verify Concurrency Implications
Great work moving the OAuth2 client instantiation to the module level. This approach avoids recreating the client on every driver initialization, which is a smart performance enhancement. The grep output confirms that calls to
auth.setCredentialsoccur in both the configuration initialization and thegetUserInfomethod.
- Confirmed Usage:
- The instance is shared across calls, with credentials being set in both the
if (config.auth)block and ingetUserInfo.- Verification Note:
- Although this shared instance is efficient, please double-check that simultaneous or overlapping invocations do not lead to race conditions or unintended overwrites of credentials in a concurrent execution environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/mail/app/api/driver/google.ts (2)
246-257: Error message needs correction in markAsUnread method.The error message on line 254 incorrectly states "Error marking messages as read" but this is the
markAsUnreadmethod.- console.error('Error marking messages as read:', error); + console.error('Error marking messages as unread:', error);
119-131: Consider adding a success log message.For better debugging and observability, consider adding a success log message after the batch operation completes.
await gmail.users.messages.batchModify({ userId: "me", requestBody: { ids: allMessagesIds, removeLabelIds: removeLabelIds, addLabelIds: addLabelIds } }) + console.log(`Successfully processed ${allMessagesIds.length} messages from ${threadIds.length} threads`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/app/api/driver/google.ts(3 hunks)
🔇 Additional comments (5)
apps/mail/app/api/driver/google.ts (5)
38-42: Good refactoring: Moving OAuth2 client outside the driver function.Moving the auth instance to module scope improves reusability and allows it to be shared between functions, which is necessary for the new
processMultipleThreadsfunction.
87-91: LGTM: Well-structured interface definition.The
ProcessMultipleThreadsPropsinterface is clear and well-typed, providing good documentation for the function parameters.
93-137: Great implementation of batch thread processing.This implementation correctly addresses the issue where email threads were not being marked as read. The function:
- Takes multiple thread IDs for batch processing
- Properly retrieves all messages from each thread
- Uses batchModify to efficiently update all messages at once
- Includes robust error handling and input validation
This approach is scalable and will work well with the future "select all" feature mentioned in the PR description.
232-244: Good refactoring of markAsRead method.The refactoring improves code organization by leveraging the new
processMultipleThreadsfunction and adds proper error handling.
98-104:✅ Verification successful
Verify that empty array validation works as expected.
The function returns early if both
addLabelIdsandremoveLabelIdsare empty, but it throws an error ifthreadIdsis empty. This logic looks correct, but it's worth verifying that this matches the expected behavior in all cases.
🏁 Script executed:
#!/bin/bash # Check how the function handles empty arrays in other parts of the codebase # Check if threadIds are validated elsewhere rg -A 5 -B 5 "threadIds.*length.*0" --iglob "*.ts" # Check for similar validation patterns rg -A 5 -B 5 "(addLabelIds|removeLabelIds).*length.*0" --iglob "*.ts"Length of output: 1479
Empty array validation logic verified
The script output confirms that the validation checks in
apps/mail/app/api/driver/google.tswork as expected. The code correctly throws an error whenthreadIdsis empty and returns early when bothaddLabelIdsandremoveLabelIdsare empty. No additional adjustments are needed.
There was a problem hiding this comment.
revert this, doesnt need to be touched.
There was a problem hiding this comment.
revert this, doesnt need to be touched.
|
Done in #502 |
Description
Threads weren't marked as read previously even after reading all the messages under the email. This was happening because we weren't marking all the messages under the email as read. I have added logic to remove "UNREAD" labels from all the messages to mark the email as read.
Type of Change
Areas Affected
Testing Done
Describe the tests you've done:
Security Considerations
None
Checklist
Additional Notes
I couldn't see an option to mark emails as read in batches so currently I'm just picking up the first email and its messages and marking them as read. Once we have "select all" and "mark as read" feature up, we can extend the logic to get messages for all the email IDs and mark as read.
Screenshots/Recordings
screen-capture.47.webm
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
New Features
Refactor
Style
itemContentprop in theMailListcomponent for better readability without changing functionality.idproperties of thecategoriesarray for consistency.