Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arndom
Copy link

@arndom arndom commented Jan 5, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced Gmail API interaction with new utility functions for retrieving messages, threads, history, and managing user settings.
    • Added support for more flexible message and thread retrieval with optional parameters.
  • Improvements

    • Streamlined code by abstracting direct Gmail API calls into modular utility functions.
    • Improved message and thread filtering capabilities.
    • Enhanced error handling and logging mechanisms.
  • Refactoring

    • Standardized response handling across various Gmail-related functions.
    • Simplified API interaction methods in multiple components.

Copy link

vercel bot commented Jan 5, 2025

@arndom is attempting to deploy a commit to the Inbox Zero Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented Jan 5, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

coderabbitai bot commented Jan 5, 2025

Walkthrough

The pull request focuses on refactoring Gmail API interactions by centralizing API calls within the apps/web/utils/gmail/ directory. Multiple utility functions have been added or modified across various files to abstract direct Gmail API calls, including getMessages, getThreads, watchUser, unwatchUser, getHistory, and modifyThread. These changes improve code modularity, reduce direct API dependencies, and standardize message and thread retrieval processes across the application.

Changes

File Change Summary
apps/web/utils/gmail/message.ts Added optional labelIds parameter to getMessages function.
apps/web/utils/gmail/thread.ts Updated getThreads to use options object, added modifyThread function.
apps/web/utils/gmail/misc.ts Added utility functions: getForwardingAddresses, getHistory, watchUser, unwatchUser.
apps/web/app/(app)/simple/page.tsx, apps/web/app/api/google/threads/[id]/route.ts, apps/web/app/api/google/threads/basic/route.ts, apps/web/app/api/google/threads/controller.ts, apps/web/app/api/google/watch/controller.ts, apps/web/app/api/google/webhook/process-history.ts, apps/web/app/api/user/assess/route.ts, apps/web/app/api/user/bulk-archive/route.ts, apps/web/app/api/user/group/[groupId]/messages/controller.ts, apps/web/app/api/user/no-reply/route.ts, apps/web/app/api/user/rules/[id]/example/controller.ts, apps/web/app/api/user/stats/recipients/route.ts, apps/web/app/api/user/stats/tinybird/load/load-emails.ts Replaced direct Gmail API calls with new utility functions.

Assessment against linked issues

Objective Addressed Explanation
Move all Gmail API calls to gmail utils folder [#272]
Create corresponding functions in gmail folder [#272]
Replace direct API calls with these functions [#272]

Possibly related PRs

Poem

🐰 In the warren of code, we hop and we leap,
Refactoring Gmail calls, no more secrets to keep.
Utility functions bloom like carrots so bright,
Centralizing our API with algorithmic might.
A rabbit's clean code brings developer's delight! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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.
The gmail.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.
When res.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 from unWatchUser.
Currently, no response is returned, so it’s unclear whether the unwatching completed successfully. Reusing the return value from unWatchUser 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 if maxResults 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 the older_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 for modifyThread.

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 from getMessage calls or an unexpected empty response.

apps/web/app/api/google/threads/controller.ts (1)

67-67: Safeguard against undefined threads.
Accessing gmailThreads.threads is fine, but confirm that gmailThreads itself is not null 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 to getFiltersList 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 calling getMessagesBatch.
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 for getThread.
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

📥 Commits

Reviewing files that changed from the base of the PR and between 009a0d3 and 1e3568f.

📒 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 in watchUser.
The requestBody 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.
Renaming getThread to getGmailThread 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 calling parseMessages.

apps/web/app/api/google/watch/controller.ts (1)

7-7: Well-structured import for watch/unwatch functionality.
Centralizing watch and unwatch logic in misc.ts improves code readability and maintainability.

apps/web/app/api/user/bulk-archive/route.ts (1)

9-9: Utilizing utility functions keeps bulkArchive compact.
Importing getThreads and modifyThread 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 and getMessages is consistent with refactoring efforts.

apps/web/app/api/user/stats/recipients/route.ts (2)

9-9: Refactored import is consistent.

Importing both getMessage and getMessages 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:

  1. The getMessages function has a built-in safety limit of 20 messages per request to prevent rate limiting (429 errors) from Google's API.
  2. 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 and getMessages reflects the shift toward centralizing Gmail API interactions.


42-42: Retaining filtered messages.

Assigning response.messages to filteredMessages is straightforward. Ensure no unintended duplication or missed messages occur if additional filtering logic is reintroduced later.


68-68: Preserving continuity with nextPageToken.

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 employing nextPageToken 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:

  1. The getMessages function accepts and handles pageToken parameter
  2. The response includes nextPageToken which is used for subsequent page requests
  3. The SimplePage component receives and utilizes pageToken from search params
  4. The SimpleList component handles pagination by routing to the next page when nextPageToken is available

The 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 import getGmailThreads 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 using getGmailThreads 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: Validate nextPageToken usage.
Ensure that downstream pagination logic properly handles the nextPageToken when undefined or empty.


59-59: ⚠️ Potential issue

Remove or utilize the second getGmailThreads call.
A second call to getGmailThreads is assigned to r 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: Optional labelIds parameter looks good.
Allowing for label-based filtering can streamline specialized queries. Ensure that the caller always verifies if labelIds is valid or not to avoid unexpected behavior.

apps/web/app/api/user/assess/route.ts (1)

20-21: Abstracting filters and forwarding addresses.
Using getFiltersList and getForwardingAddresses 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 with getMessages centralizes logic. This is beneficial for consistent error handling and debugging.


74-76: Careful with early break conditions in pagination.
Breaking when res.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 the before pagination logic.
Similar rationale applies here. Confirm that the final page is processed correctly when res.messages.length < PAGE_SIZE.


146-149: Maintain consistent naming for query strings.
q: string | undefined is now passed as query to getMessages. 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 both getMessages and getMessage from @/utils/gmail/message, ensuring consistency across the module.


217-220: Confirm error handling for new getMessages usage.
This call to the new getMessages 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.
Returning undefined 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: Importing getHistory 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 for getThreads 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 of getThread 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.

Comment on lines +10 to +28
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;
}
Copy link
Contributor

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.

Comment on lines +10 to +13
const res = await watchUser(gmail, {
labelIds: [INBOX_LABEL_ID],
labelFilterBehavior: "include",
topicName: env.GOOGLE_PUBSUB_TOPIC_NAME,
Copy link
Contributor

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.

Comment on lines +32 to +35
for (const thread of res.threads || []) {
await modifyThread(gmail, thread.id!, {
addLabelIds: [archivedLabel.id],
removeLabelIds: [INBOX_LABEL_ID],
Copy link
Contributor

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.

Comment on lines +69 to +71
const response = await getMessages(gmail, {
maxResults: 50,
q,
query: q,
Copy link
Contributor

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:

  1. The getMessages function in utils/gmail/message.ts accepts a pageToken parameter and returns paginated results from the Gmail API
  2. The codebase already has pagination handling in other places:
    • fetchPaginatedMessages is used for group-related message fetching
    • The /api/google/messages/route.ts endpoint handles nextPageToken

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) => {
Copy link
Contributor

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, {
Copy link
Contributor

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

@elie222
Copy link
Owner

elie222 commented Jan 5, 2025

Thanks @arndom! Could you sign the CLA above, please!

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 issue

Improve error handling and logging in watchEmails function.

  1. The error handling from the previous review is still relevant.

  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1e3568f and 679bee5.

📒 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.

  1. The function currently returns only the first page of results without handling pagination via nextPageToken.

  2. 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 suggestion

Add 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.

Comment on lines +3 to +8
export async function getForwardingAddresses(gmail: gmail_v1.Gmail) {
const res = await gmail.users.settings.forwardingAddresses.list({
userId: "me",
});
return res.data;
}
Copy link
Contributor

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.

Suggested change
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;
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move all Gmail API calls into gmail utils folder
3 participants