-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Gmail - increase fetch limit #17167
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
Gmail - increase fetch limit #17167
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update the Gmail integration's message retrieval process from bulk fetching to asynchronous streaming, replacing the use of a removed Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GmailApp
loop For each messageId
Caller->>GmailApp: getAllMessages(messageIds) [async iterator]
GmailApp-->>Caller: yields Message
end
Caller->>Caller: Collect messages, sort, emit events
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
components/gmail/actions/add-label-to-email/add-label-to-email.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/gmail/actions/approve-workflow/approve-workflow.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/gmail/actions/archive-email/archive-email.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (17)
✨ 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 (
|
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
🧹 Nitpick comments (4)
components/gmail/common/constants.mjs (1)
8-17: ClarifyMAX_LIMITintent and verify it matches Gmail API caps
MAX_LIMITis hard-coded to500and used forhistory.list. According to Gmail API docs, the maximum allowed values differ (messages.list: 500,history.list: 1000). Please double-check that 500 is the desired cap for both endpoints; otherwise consider separate constants (e.g.MAX_MESSAGES_LIMIT,MAX_HISTORY_LIMIT) or pull the value from a single source-of-truth config.-const MAX_LIMIT = 500; +// Max page size for users.messages.list (500) – history.list allows 1000. +// Adjust if you need the larger window for history polling. +const MAX_MESSAGES_LIMIT = 500;components/gmail/actions/find-email/find-email.mjs (1)
58-61: Streaming fetch is good, but we still buffer everythingSwitching to
for await…ofremoves the bulk API, 👍.
However, pushing every message intomessagesToEmitmeans memory usage is unchanged and we lose the opportunity to emit each record as soon as it arrives. If the action is expected to handle >100 messages in the future, consider emitting inside the loop and only keeping a counter for the summary.-const messagesToEmit = []; -for await (const message of this.gmail.getAllMessages(messageIds)) { - messagesToEmit.push(message); +const messagesToEmit = []; +for await (const message of this.gmail.getAllMessages(messageIds)) { + /* emit here if downstream permits, e.g. */ + // $.export("message", message); + messagesToEmit.push(message); }components/gmail/sources/common/polling-history.mjs (1)
62-66: Minor:Array.from()is redundant
messagesis already an array after the push loop. Creating a shallow copy viaArray.from()is unnecessary allocation.-const sortedMessages = - Array.from(messages) - .sort((a, b) => (Number(b.historyId) - Number(a.historyId))); +const sortedMessages = messages + .sort((a, b) => Number(b.historyId) - Number(a.historyId));components/gmail/sources/common/polling-messages.mjs (1)
60-63: Consider bounded parallelism for faster cold-startsSequentially awaiting each item of
getAllMessagescan be slow whenmessageIdsis large (e.g. 500). A simple concurrency pool (e.g.p-mapor customPromise.allSettledwith a limit) can cut run-time dramatically while staying within Gmail’s QPS limits.If you prefer to keep it lightweight:
-let messages = []; -for await (const message of this.gmail.getAllMessages(messageIds)) { - messages.push(message); -} +const concurrency = 10; +const messages = []; +for (let i = 0; i < messageIds.length; i += concurrency) { + const batch = messageIds.slice(i, i + concurrency); + const chunk = await Promise.all(batch.map((id) => this.gmail.getMessage({ id }))); + messages.push(...chunk); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/gmail/actions/find-email/find-email.mjs(1 hunks)components/gmail/common/constants.mjs(1 hunks)components/gmail/gmail.app.mjs(0 hunks)components/gmail/sources/common/polling-history.mjs(2 hunks)components/gmail/sources/common/polling-messages.mjs(1 hunks)
💤 Files with no reviewable changes (1)
- components/gmail/gmail.app.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
| maxResults: constants.MAX_LIMIT, | ||
| }; |
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
Pagination still ignored when raising the cap
Raising maxResults to constants.MAX_LIMIT (500) is helpful, but the loop doesn’t handle nextPageToken, so histories beyond the first page are silently skipped. Consider iterating until nextPageToken is empty to ensure completeness.
🤖 Prompt for AI Agents
In components/gmail/sources/common/polling-history.mjs around lines 81 to 82,
the code sets maxResults to constants.MAX_LIMIT but does not handle pagination
with nextPageToken, causing only the first page of histories to be processed.
Modify the code to implement a loop that continues fetching and processing
history pages until nextPageToken is empty, ensuring all history entries are
retrieved completely.
* remove get messages function that uses promise all * increase max limit * bump versions
WHY
Summary by CodeRabbit
New Features
Refactor
Maintenance