-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor all Gmail API calls #289
base: main
Are you sure you want to change the base?
Conversation
@arndom is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request focuses on refactoring Gmail API interactions by centralizing API calls within the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (16)
apps/web/utils/gmail/misc.ts (2)
3-8
: Consider adding error handling for API failures.
Thegmail.users.settings.forwardingAddresses.list()
call may fail under various conditions (e.g. invalid credentials, permissions, or network errors). Handling thrown errors or rejected promises in this function can improve resilience.export async function getForwardingAddresses(gmail: gmail_v1.Gmail) { try { const res = await gmail.users.settings.forwardingAddresses.list({ userId: "me", }); return res.data; } catch (error) { + // Log error or handle it appropriately + console.error("Failed to get forwarding addresses:", error); throw error; } }
47-49
: Return an acknowledgment or updated watch status after unwatching.
Currently,unWatchUser
is called but no result is returned. Providing a success value or updated watch status can help confirm that unwatching has completed successfully.export async function unWatchUser(gmail: gmail_v1.Gmail) { await gmail.users.stop({ userId: "me" }); + return { success: true }; }
apps/web/app/api/google/watch/controller.ts (2)
16-24
: Log a more descriptive error message.
Whenres.expiration
is absent, the console statement logs a generic message. Improving the log message (e.g., “Inbox watch request failed!”) can help diagnose issues faster.
29-29
: Return a confirmation fromunWatchUser
.
Currently, no response is returned, so it’s unclear whether the unwatching completed successfully. Reusing the return value fromunWatchUser
can improve clarity.apps/web/app/api/user/bulk-archive/route.ts (3)
16-19
: Consider logging or handling partial list scenarios.
getThreads
may return partial results ifmaxResults
is reached. Consider retrieving remaining threads or informing the user if there are more threads to archive beyond the limit.
22-22
: Log messages can specify the query context.
Including theolder_than
query in the log can help clarify the scope of what’s being archived.
43-43
: Return more details on archived threads.
Providing the IDs or the summary of archived threads can be useful for auditing or UI feedback.apps/web/utils/gmail/thread.ts (1)
87-100
: Add error handling or return status formodifyThread
.
modifyThread
is a newly introduced function that can throw exceptions if the Gmail API call fails. It might be beneficial to return the updated thread or at least handle/propagate errors for better observability.apps/web/app/api/user/rules/[id]/example/controller.ts (1)
75-75
: Handle potential errors in message fetching.
getMessage
might fail or return partial data. Adding a try/catch block or a fallback helps avoid runtime errors and ensures partial success does not break the flow.apps/web/app/api/user/stats/recipients/route.ts (1)
36-36
: Consider partial failures or empty results.When mapping over
res.messages
, be mindful of handling errors fromgetMessage
calls or an unexpected empty response.apps/web/app/api/google/threads/controller.ts (1)
67-67
: Safeguard against undefined threads.
AccessinggmailThreads.threads
is fine, but confirm thatgmailThreads
itself is notnull
or an error response. A simple defensive check can prevent runtime errors in edge cases.- const threadIds = gmailThreads.threads?.map((t) => t.id).filter(isDefined) || []; + const threadIds = gmailThreads?.threads?.map((t) => t.id).filter(isDefined) || [];apps/web/app/api/user/assess/route.ts (2)
104-104
: Optimize filter listing error handling.
Wrapping calls togetFiltersList
with a try/catch might help gracefully handle partial failures.async function getFiltersCount(gmail: gmail_v1.Gmail) { + try { const res = await getFiltersList({ gmail }); const filters = res.data.filter || []; return filters.length; + } catch (error) { + console.error("Error fetching filters:", error); + return 0; + } }
111-112
: Surface or log forwarding address errors more clearly.
Currently, the code logs a general message. If possible, incorporate an error code or message to facilitate easier debugging.apps/web/app/api/user/stats/tinybird/load/load-emails.ts (1)
154-154
: Validate message IDs before callinggetMessagesBatch
.
Consider short-circuiting if no message IDs exist to avoid unnecessary calls.- const messages = await getMessagesBatch( - res.messages?.map((m) => m.id!) || [], - accessToken, - ); + const messageIds = res.messages?.map((m) => m.id!).filter(isDefined) ?? []; + if (messageIds.length === 0) return res; + const messages = await getMessagesBatch(messageIds, accessToken);apps/web/app/api/user/no-reply/route.ts (1)
22-22
: Consider robust error handling forgetThread
.
If the Gmail API fails or the thread is missing, ensure it’s caught to avoid unhandled promise rejections.+ try { const thread = await getThread(message.threadId!, options.gmail); + } catch (err) { + // Fallback or logging here + return; + }apps/web/utils/actions/cold-email.ts (1)
66-67
: Consider using Gmail's RFC2822 query syntax for more precise sender matching.The current query
from:${sender}
might match partial email addresses. Consider using the more precise RFC2822 syntax:- q: `from:${sender}`, + q: `from:(${sender}) exact:true`,This ensures exact matching of the sender's email address and prevents potential false positives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/web/app/(app)/simple/page.tsx
(3 hunks)apps/web/app/api/google/threads/[id]/route.ts
(2 hunks)apps/web/app/api/google/threads/basic/route.ts
(1 hunks)apps/web/app/api/google/threads/controller.ts
(3 hunks)apps/web/app/api/google/watch/controller.ts
(1 hunks)apps/web/app/api/google/webhook/process-history.ts
(2 hunks)apps/web/app/api/user/assess/route.ts
(2 hunks)apps/web/app/api/user/bulk-archive/route.ts
(2 hunks)apps/web/app/api/user/group/[groupId]/messages/controller.ts
(3 hunks)apps/web/app/api/user/no-reply/route.ts
(2 hunks)apps/web/app/api/user/rules/[id]/example/controller.ts
(2 hunks)apps/web/app/api/user/stats/recipients/route.ts
(2 hunks)apps/web/app/api/user/stats/tinybird/load/load-emails.ts
(4 hunks)apps/web/utils/actions/cold-email.ts
(1 hunks)apps/web/utils/gmail/message.ts
(1 hunks)apps/web/utils/gmail/misc.ts
(1 hunks)apps/web/utils/gmail/thread.ts
(2 hunks)
🔇 Additional comments (32)
apps/web/utils/gmail/misc.ts (2)
1-2
: Module import structure follows best practices.
No issues found with the import statement.
30-45
: Validate or sanitize the request body inwatchUser
.
TherequestBody
parameter is passed directly to the API. It would be safer to validate or sanitize these fields to avoid issues like unexpected or invalid values.apps/web/app/api/google/threads/[id]/route.ts (2)
5-5
: Modular import usage is good.
RenaminggetThread
togetGmailThread
clarifies the function’s origin and prevents naming collisions.
18-18
: Confirm thread existence before parsing messages.
getGmailThread
might return undefined or a malformed thread if the ID is invalid. Consider verifying that the response is valid before callingparseMessages
.apps/web/app/api/google/watch/controller.ts (1)
7-7
: Well-structured import for watch/unwatch functionality.
Centralizing watch and unwatch logic inmisc.ts
improves code readability and maintainability.apps/web/app/api/user/bulk-archive/route.ts (1)
9-9
: Utilizing utility functions keepsbulkArchive
compact.
ImportinggetThreads
andmodifyThread
improves readability and reuse across the codebase.apps/web/utils/gmail/thread.ts (2)
11-16
: Use typed object parameters consistently for better readability.The updated approach using an object parameter for
getThreads
improves clarity by consolidating optional fields into a single parameter. Ensure that all calling sites have been updated accordingly.
20-21
: Confirm the default of 100 threads.Defaulting
maxResults
to 100 is convenient, but verify it's appropriate for all use cases. Consider whether performance or rate-limiting constraints necessitate a configurable upper bound.✅ Verification successful
Default of 100 threads is appropriate with proper handling in place
The verification reveals that the default of 100 threads is well-considered:
- The controller layer (
getThreads
in controller.ts) actually enforces a lower default limit of 50 threads, providing an additional safety layer- The batch processing system has a hard limit of 100 items (
BATCH_LIMIT = 100
in batch.ts), making the default align well with system constraints- Message-related operations have stricter limits (20) with explicit rate limiting protection
- The codebase shows careful consideration of performance and rate limiting across different operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for all calls to `getThreads` to verify explicit usage or reliance on default rg -A 3 "getThreads\(.*\)"Length of output: 881
Script:
#!/bin/bash # Let's check the implementation of getThreads in the controller to understand the usage ast-grep --pattern 'export async function getThreads($_) { $$$ }' # Also check for any rate limiting or performance related configurations rg -i "rate|limit|max.*result" apps/web/utils/gmail/Length of output: 8289
apps/web/app/api/user/rules/[id]/example/controller.ts (1)
3-3
: Import statement looks good.The consolidated import of
getMessage
andgetMessages
is consistent with refactoring efforts.apps/web/app/api/user/stats/recipients/route.ts (2)
9-9
: Refactored import is consistent.Importing both
getMessage
andgetMessages
aligns this file with the new centralized Gmail utility functions.
29-30
: Confirm the “in:sent” query usage.Review if limiting to 50 messages under the “in:sent” query is sufficient for all scenarios. Large mailboxes may require paging through results.
✅ Verification successful
The maxResults limit of 50 is appropriate for the recipients stats endpoint
The current implementation is well-designed with appropriate limits:
- The
getMessages
function has a built-in safety limit of 20 messages per request to prevent rate limiting (429 errors) from Google's API.- The recipients stats endpoint uses
maxResults: 50
, which is reasonable because:
- It's used for generating statistics about email recipients
- The data is being sent to Tinybird for analytics (
getRecipientsTinybird
)- The actual implementation shows this is not used directly but rather through Tinybird, which likely has its own data aggregation
The code has proper pagination support through
pageToken
when needed, but for this specific stats endpoint, limiting to 50 messages is sufficient as it's meant for generating representative statistics rather than processing the entire mailbox.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references of getRecipients to see if there is ever a need for more than 50 messages rg -A 3 "getRecipients\(.*\)"Length of output: 705
Script:
#!/bin/bash # Let's check the implementation of getMessages to understand the limit ast-grep --pattern 'function getMessages($$$)' # Also check if there are any references to pagination or maxResults rg -A 3 "maxResults|pageToken"Length of output: 20832
apps/web/app/(app)/simple/page.tsx (4)
14-14
: Aligned imports for consolidated Gmail utilities.The import of
getMessage
andgetMessages
reflects the shift toward centralizing Gmail API interactions.
42-42
: Retaining filtered messages.Assigning
response.messages
tofilteredMessages
is straightforward. Ensure no unintended duplication or missed messages occur if additional filtering logic is reintroduced later.
68-68
: Preserving continuity withnextPageToken
.Passing
response.nextPageToken
to the UI is a neat approach. When full pagination is introduced, the user can easily move through results with minimal changes.
31-34
: Check for repeated queries or missing pagination logic.Using
getMessages
for a single page of results may suffice for a limited scope, but consider employingnextPageToken
to handle larger mail volumes.✅ Verification successful
Let me check the implementation of
getMessages
and the query parameters to verify the pagination concern.
Pagination is properly implemented and handled
The codebase shows comprehensive pagination implementation:
- The
getMessages
function accepts and handlespageToken
parameter- The response includes
nextPageToken
which is used for subsequent page requests- The
SimplePage
component receives and utilizespageToken
from search params- The
SimpleList
component handles pagination by routing to the next page whennextPageToken
is availableThe current implementation with
maxResults: 5
is intentional for this simple view, and the pagination mechanism is correctly in place to handle larger volumes when needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching usage for `SimplePage` to verify if there's a scenario needing multiple pages rg -A 3 "SimplePage"Length of output: 314
Script:
#!/bin/bash # Check implementation of getMessages and its usage of pagination parameters ast-grep --pattern 'function getMessages($_) { $$$ }' # Also check for any nextPageToken handling rg -A 3 "nextPageToken|pageToken"Length of output: 26272
apps/web/app/api/google/threads/controller.ts (4)
17-20
: Confirm consistent usage of imports.
The newly added importgetGmailThreads
is consistent with the refactor strategy. Ensure that all references to Gmail API calls are indeed routed through these utility functions.
52-58
: Validate the second retrieval approach before finalizing.
Here,gmailThreads
is fetched usinggetGmailThreads
with the specified options. Make sure this is the only call required for listing threads; otherwise, consider explaining why two calls are performed (see line 59).
112-112
: ValidatenextPageToken
usage.
Ensure that downstream pagination logic properly handles thenextPageToken
whenundefined
or empty.
59-59
:⚠️ Potential issueRemove or utilize the second
getGmailThreads
call.
A second call togetGmailThreads
is assigned tor
but never used. This may introduce unnecessary overhead and confusion.- const r = await getGmailThreads(gmail, { - labelIds: getLabelIds(query.type), - maxResults: query.limit || 50, - q: getQuery(), - pageToken: query.nextPageToken || undefined, - });Likely invalid or redundant comment.
apps/web/utils/gmail/message.ts (1)
129-134
: OptionallabelIds
parameter looks good.
Allowing for label-based filtering can streamline specialized queries. Ensure that the caller always verifies iflabelIds
is valid or not to avoid unexpected behavior.apps/web/app/api/user/assess/route.ts (1)
20-21
: Abstracting filters and forwarding addresses.
UsinggetFiltersList
andgetForwardingAddresses
ensures consistent logic across the codebase. Good move for code organization.apps/web/app/api/user/stats/tinybird/load/load-emails.ts (4)
5-5
: Unified message retrieval approach.
Replacing direct calls withgetMessages
centralizes logic. This is beneficial for consistent error handling and debugging.
74-76
: Careful with earlybreak
conditions in pagination.
Breaking whenres.messages.length < PAGE_SIZE
may skip fetching incomplete data for the last page if Gmail changes paging strategies or hits partial responses. Confirm that this behavior is desired.
112-114
: Re-check thebefore
pagination logic.
Similar rationale applies here. Confirm that the final page is processed correctly whenres.messages.length < PAGE_SIZE
.
146-149
: Maintain consistent naming for query strings.
q: string | undefined
is now passed asquery
togetMessages
. Good job aligning with the new function signature.apps/web/app/api/user/group/[groupId]/messages/controller.ts (3)
5-5
: Consolidated import statement looks good.
It’s good to see a centralized import for bothgetMessages
andgetMessage
from@/utils/gmail/message
, ensuring consistency across the module.
217-220
: Confirm error handling for newgetMessages
usage.
This call to the newgetMessages
wrapper is consistent with the rest of the refactor. However, consider verifying if additional error-handling or retries are required when Gmail API responses fail or rate-limit.
[refactor_suggestion_good_to_have, verify]#!/bin/bash # Description: Check for try-catch blocks around all `getMessages` calls in the codebase ast-grep --pattern $'try { $$$ } catch($_$) { $$$ }' | rg 'getMessages'
238-238
: Good fallback for next page token.
Returningundefined
instead of a falsy value or empty string helps ensure clarity in the paginated responses.apps/web/app/api/google/webhook/process-history.ts (1)
29-29
: ImportinggetHistory
improves modularity.
This aligns with the refactoring goal of centralizing Gmail API calls.apps/web/app/api/google/threads/basic/route.ts (1)
20-24
: New parameter structure forgetThreads
is well-formed.
Switching to an options object for query parameters improves readability and consistency.apps/web/app/api/user/no-reply/route.ts (1)
8-8
: Import ofgetThread
is consistent.
This import is aligned with the refactoring pattern to reduce direct API calls by delegating them to a unified utility.apps/web/utils/actions/cold-email.ts (1)
65-68
: LGTM! Changes align well with the Gmail API refactoring.The updated
getThreads
call follows the new centralized pattern for Gmail API interactions while maintaining proper error handling.
export async function getHistory( | ||
gmail: gmail_v1.Gmail, | ||
options: { | ||
startHistoryId?: string; | ||
labelId?: string; | ||
historyTypes?: string[]; | ||
maxResults?: number; | ||
}, | ||
) { | ||
const res = await gmail.users.history.list({ | ||
userId: "me", | ||
startHistoryId: options.startHistoryId, | ||
labelId: options.labelId, | ||
historyTypes: options.historyTypes, | ||
maxResults: options.maxResults, | ||
}); | ||
|
||
return res; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Implement pagination for large email history retrieval.
The gmail.users.history.list()
method can return partial results if maxResults
is reached or if more history is available, signaled by a nextPageToken
. Currently, the function returns only the first page of results.
export async function getHistory(
gmail: gmail_v1.Gmail,
options: {
startHistoryId?: string;
labelId?: string;
historyTypes?: string[];
maxResults?: number;
},
) {
const res = await gmail.users.history.list({
userId: "me",
startHistoryId: options.startHistoryId,
labelId: options.labelId,
historyTypes: options.historyTypes,
maxResults: options.maxResults,
});
- return res;
+ // Consider retrieving subsequent pages using nextPageToken if present
+ return res;
}
Committable suggestion skipped: line range outside the PR's diff.
const res = await watchUser(gmail, { | ||
labelIds: [INBOX_LABEL_ID], | ||
labelFilterBehavior: "include", | ||
topicName: env.GOOGLE_PUBSUB_TOPIC_NAME, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Handle potential exceptions thrown by watchUser
.
If the API call fails due to invalid credentials or permissions, the error may go unhandled until caught in a higher-level block. Consider adding try/catch or passing the error upward in a structured way.
for (const thread of res.threads || []) { | ||
await modifyThread(gmail, thread.id!, { | ||
addLabelIds: [archivedLabel.id], | ||
removeLabelIds: [INBOX_LABEL_ID], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider parallel archiving in small batches.
Archiving threads one-by-one with a sleep(40)
call is straightforward but can be slow for large numbers of threads. Batching multiple modifications before sleeping can improve throughput.
const response = await getMessages(gmail, { | ||
maxResults: 50, | ||
q, | ||
query: q, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
The review comment is valid - pagination support should be implemented
Based on the code analysis:
- The
getMessages
function inutils/gmail/message.ts
accepts apageToken
parameter and returns paginated results from the Gmail API - The codebase already has pagination handling in other places:
fetchPaginatedMessages
is used for group-related message fetching- The
/api/google/messages/route.ts
endpoint handlesnextPageToken
However, in the current implementation at apps/web/app/api/user/rules/[id]/example/controller.ts
, the pagination support is not utilized when fetching static example messages, potentially missing relevant messages beyond the first 50 results.
🔗 Analysis chain
Consider handling paginated results.
While fetching 50 messages might suffice for now, getMessages
may support pagination. Consider handling the returned nextPageToken
to fetch subsequent pages when necessary.
Let me search for more context about the pagination handling in the codebase.
Let me search for more specific information about the getMessages implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching usage for `fetchStaticExampleMessages` to confirm it does not require subsequent pages
rg -A 4 "fetchStaticExampleMessages"
Length of output: 901
Script:
#!/bin/bash
# Search for fetchPaginatedMessages implementation and usage
rg -A 10 "fetchPaginatedMessages"
# Search for getMessages implementation to understand pagination support
ast-grep --pattern 'async function getMessages'
Length of output: 3520
Script:
#!/bin/bash
# Search for getMessages implementation and usage
rg -A 15 "getMessages = async|function getMessages"
# Search for files that might contain Gmail API related code
fd -e ts gmail
Length of output: 3111
}); | ||
|
||
const messages = await Promise.all( | ||
(response.data.messages || []).map(async (message) => { | ||
(response.messages || []).map(async (message) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Limit concurrency in Promise.all loop.
When processing large numbers of messages, Promise.all
can cause excessive concurrent requests. Consider introducing concurrency limits (e.g., with libraries like p-limit
) or batching to prevent potential performance issues and rate limits.
- const messages = await Promise.all(
- (response.messages || []).map(async (message) => {
- ...
- }),
- );
+ import pLimit from "p-limit";
+ const limit = pLimit(5); // limit concurrency to 5
+ const messages = await Promise.all(
+ (response.messages || []).map((msg) => limit(async () => {
+ ...
+ })),
+ );
Committable suggestion skipped: line range outside the PR's diff.
@@ -153,8 +154,7 @@ export async function processHistoryForUser( | |||
email, | |||
}); | |||
|
|||
const history = await gmail.users.history.list({ | |||
userId: "me", | |||
const history = await getHistory(gmail, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Pagination not implemented in getHistory function
The current implementation of getHistory
doesn't handle pagination for large result sets. It makes a single request to Gmail API and returns the response directly without checking for or processing nextPageToken
. This could lead to incomplete history retrieval for heavily backlogged accounts.
Key issues:
- No handling of
nextPageToken
from Gmail API response - Single request limited to
maxResults: 500
in the calling code - No aggregation of results across multiple pages
🔗 Analysis chain
Check long history retrieval edge cases.
Wrapping getHistory
within a try-catch is helpful. For heavily backlogged accounts, ensure we handle large volumes gracefully (e.g., by paginating multiple times).
Let me check the implementation of getHistory
to see if it handles pagination.
Let me try a different approach to check the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if multiple pages of history are handled consistently
rg -A 10 'getHistory\('
Length of output: 1539
Script:
#!/bin/bash
# Check the full implementation of getHistory function
ast-grep --pattern 'export async function getHistory($_) {
$$$
}'
Length of output: 69
Script:
#!/bin/bash
# Check the full implementation of getHistory function using ripgrep
rg -U "export async function getHistory[\s\S]*?\n}" --multiline
Length of output: 1040
Thanks @arndom! Could you sign the CLA above, please! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/app/api/google/watch/controller.ts (1)
9-24
:⚠️ Potential issueImprove error handling and logging in watchEmails function.
The error handling from the previous review is still relevant.
Logging the entire response object could expose sensitive information.
- console.error("Error watching inbox", res); + console.error("Error watching inbox: Failed to get expiration date");
🧹 Nitpick comments (2)
apps/web/utils/gmail/misc.ts (1)
30-45
: Add error handling and JSDoc documentation.The function would benefit from error handling and documentation explaining the purpose and options.
+/** + * Sets up a watch on the user's Gmail inbox + * @param gmail - Gmail API client + * @param requestBody - Watch configuration + * @param requestBody.labelFilterAction - Action to take on messages with matching labels + * @param requestBody.labelFilterBehavior - How to interpret label filters + * @param requestBody.labelIds - Gmail labels to watch + * @param requestBody.topicName - Google Cloud Pub/Sub topic to publish notifications + * @returns Watch response data + */ export async function watchUser( gmail: gmail_v1.Gmail, requestBody: { labelFilterAction?: string | null; labelFilterBehavior?: string | null; labelIds?: string[] | null; topicName?: string | null; }, ) { + try { const res = await gmail.users.watch({ userId: "me", requestBody, }); return res.data; + } catch (error) { + console.error("Failed to set up Gmail watch:", error); + throw error; + } }apps/web/app/api/google/watch/controller.ts (1)
28-30
: Remove unnecessary logging and add error propagation.The console.log statement adds noise and the error should be handled by the caller.
async function unwatch(gmail: gmail_v1.Gmail) { - console.log("Unwatching emails"); await unwatchUser(gmail); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/google/watch/controller.ts
(1 hunks)apps/web/utils/gmail/misc.ts
(1 hunks)
🔇 Additional comments (2)
apps/web/utils/gmail/misc.ts (2)
10-28
: Implement pagination for history retrieval and improve type safety.
The function currently returns only the first page of results without handling pagination via nextPageToken.
The historyTypes parameter should be typed more strictly using a union type of valid history types.
+type HistoryType = 'messageAdded' | 'messageDeleted' | 'labelAdded' | 'labelRemoved'; + export async function getHistory( gmail: gmail_v1.Gmail, options: { startHistoryId?: string; labelId?: string; - historyTypes?: string[]; + historyTypes?: HistoryType[]; maxResults?: number; }, ) {
47-49
: 🛠️ Refactor suggestionAdd error handling and rename function for consistency.
The function should handle potential API errors and follow the naming convention mentioned in previous reviews.
-export async function unwatchUser(gmail: gmail_v1.Gmail) { +export async function unWatchUser(gmail: gmail_v1.Gmail) { + try { await gmail.users.stop({ userId: "me" }); + } catch (error) { + console.error("Failed to stop Gmail watch:", error); + throw error; + } }Likely invalid or redundant comment.
export async function getForwardingAddresses(gmail: gmail_v1.Gmail) { | ||
const res = await gmail.users.settings.forwardingAddresses.list({ | ||
userId: "me", | ||
}); | ||
return res.data; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for Gmail API call.
The function should handle potential API errors to prevent them from propagating up the call stack unexpectedly.
export async function getForwardingAddresses(gmail: gmail_v1.Gmail) {
+ try {
const res = await gmail.users.settings.forwardingAddresses.list({
userId: "me",
});
return res.data;
+ } catch (error) {
+ console.error("Failed to fetch forwarding addresses:", error);
+ throw error;
+ }
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export async function getForwardingAddresses(gmail: gmail_v1.Gmail) { | |
const res = await gmail.users.settings.forwardingAddresses.list({ | |
userId: "me", | |
}); | |
return res.data; | |
} | |
export async function getForwardingAddresses(gmail: gmail_v1.Gmail) { | |
try { | |
const res = await gmail.users.settings.forwardingAddresses.list({ | |
userId: "me", | |
}); | |
return res.data; | |
} catch (error) { | |
console.error("Failed to fetch forwarding addresses:", error); | |
throw error; | |
} | |
} |
apps/web/utils/gmail/
Summary by CodeRabbit
Release Notes
New Features
Improvements
Refactoring