Filter for labels in Gmail trigger #15637
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request updates the Gmail integration across several components. The package version in Changes
Sequence Diagram(s)sequenceDiagram
participant GmailComponent as Gmail Component
participant PollingModule as Polling-History Module
participant GmailAPI as Gmail API
GmailComponent->>PollingModule: Call getHistoryId()
PollingModule->>GmailAPI: Request history for each label in this.labels
GmailAPI-->>PollingModule: Return history responses per label
PollingModule->>PollingModule: Iterate responses to calculate maxHistoryId
PollingModule-->>GmailComponent: Return updated maxHistoryId
sequenceDiagram
participant NewEmail as New Email Module
participant GmailAPI as Gmail API
NewEmail->>GmailAPI: Call getHistoryResponses(startHistoryId) for each label
GmailAPI-->>NewEmail: Return array of history responses
NewEmail->>NewEmail: Filter messages using labels and excludeLabels
NewEmail->>NewEmail: Determine latest historyId from responses
Possibly related PRs
Suggested labels
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/sources/common/polling-history.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs ✨ Finishing Touches
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 (
|
![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 (4)
components/gmail/sources/new-labeled-email/new-labeled-email.mjs (1)
34-37: Consider adding label information to the summary.While the ID generation change is good, removing the label information from the summary reduces visibility into which labels triggered the event.
- summary: `A new message with ID: ${message.id} was labeled"`, + summary: `A new message with ID: ${message.id} was labeled with: ${message.labelIds.join(', ')}"`,components/gmail/sources/common/polling-history.mjs (2)
29-31: Consider adding null check for labelIds.The optional chaining on labels is good, but consider adding a null check for labelIds to prevent undefined behavior.
- if (this.labels?.length) { + if (this.labels?.length && this.labelIds) { params.labelIds = this.labelIds; }
52-79: Potential performance optimization for label processing.The current implementation processes labels sequentially. Consider using Promise.all for parallel processing when appropriate.
- for (let i = 0; i < length; i++) { - if (this.labels) { - opts.labelId = this.labels[i]; - } - - const { - history, historyId, - } = await this.gmail.listHistory(opts); + const historyPromises = Array.from({ length }, async (_, i) => { + if (!this.labels) return null; + return this.gmail.listHistory({ + ...opts, + labelId: this.labels[i], + }); + }); + + const results = await Promise.all(historyPromises); + for (const { history, historyId } of results.filter(Boolean)) {components/gmail/sources/new-email-received/new-email-received.mjs (1)
414-427: Consider simplifying the filterHistory method.The nested if conditions and repeated checks could be simplified for better readability.
- let filteredHistory = history.filter((item) => item.messagesAdded?.length); - if (this.labels) { - filteredHistory = filteredHistory.filter((item) => - item.messagesAdded[0].message.labelIds && - item.messagesAdded[0].message.labelIds.some((i) => this.labels.includes(i))); - } - if (this.excludeLabels) { - filteredHistory = filteredHistory.filter((item) => - item.messagesAdded[0].message.labelIds && - !(item.messagesAdded[0].message.labelIds.some((i) => this.excludeLabels.includes(i)))); - } + return history.filter((item) => { + const labelIds = item.messagesAdded?.[0]?.message?.labelIds; + if (!item.messagesAdded?.length || !labelIds) return false; + + const hasRequiredLabel = !this.labels || labelIds.some(i => this.labels.includes(i)); + const hasExcludedLabel = this.excludeLabels && labelIds.some(i => this.excludeLabels.includes(i)); + + return hasRequiredLabel && !hasExcludedLabel; + });🧰 Tools
🪛 Biome (1.9.4)
[error] 418-419: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/gmail/package.json(1 hunks)components/gmail/sources/common/polling-history.mjs(3 hunks)components/gmail/sources/new-email-received/new-email-received.mjs(9 hunks)components/gmail/sources/new-labeled-email/new-labeled-email.mjs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/gmail/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
components/gmail/sources/new-email-received/new-email-received.mjs
[error] 418-419: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/gmail/sources/new-labeled-email/new-labeled-email.mjs
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 43-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
components/gmail/sources/new-labeled-email/new-labeled-email.mjs (3)
11-11: Version bump looks appropriate.The version increment from 0.0.6 to 0.0.7 is appropriate for the feature enhancement of multiple label support.
16-23: LGTM: Labels property correctly configured for multiple selection.The conversion from single label to multiple labels is well implemented with appropriate type and label changes.
41-45:❓ Verification inconclusive
Verify the label filtering logic.
The multiple label filtering logic looks correct but should be tested with edge cases:
- Messages with multiple matching labels
- Empty labels array
- Non-existent labels
🏁 Script executed:
#!/bin/bash # Search for existing test cases covering label filtering rg -A 5 "test.*label.*filter|filterHistory.*test"Length of output: 51
Review Verification: Label Filtering Logic Requires Test Coverage
The label filtering logic in
components/gmail/sources/new-labeled-email/new-labeled-email.mjs(lines 41–45) appears to be implemented correctly. However, a search for test cases that cover the following edge cases:
- Messages with multiple matching labels,
- An empty labels array,
- Non-existent labels
did not yield any results. This absence suggests that automated validation for these scenarios may be lacking.
Please ensure that tests are added (or manually verified) to confirm that the filtering behaves correctly in these edge cases.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 43-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/gmail/sources/common/polling-history.mjs (1)
103-105: Good addition of excludeLabels filtering.The implementation of excludeLabels is clean and efficient, using Array.some for optimal performance.
components/gmail/sources/new-email-received/new-email-received.mjs (1)
75-98: Well-structured properties for label handling.The implementation of labels and excludeLabels properties is clean and well-documented. Good default value for labels.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/gmail/sources/common/polling-history.mjs (2)
52-79: Consider using array methods for cleaner implementation.While the current implementation is correct, it could be more concise and maintainable using array methods.
Here's a suggested refactor:
- const length = this.labels?.length > 0 - ? this.labels.length - : 1; - let maxHistoryId = 0; - - for (let i = 0; i < length; i++) { - if (this.labels) { - opts.labelId = this.labels[i]; - } - - const { - history, historyId, - } = await this.gmail.listHistory(opts); - - if (!history) { - continue; - } - - maxHistoryId = Math.max(maxHistoryId, historyId); - const responseArray = this.filterHistory(history); - - for (const item of responseArray) { - await this.emitFullMessage(item.messages[0].id); - } - } - if (maxHistoryId > 0) { - this._setLastHistoryId(maxHistoryId); + const labelIds = this.labels?.length ? this.labels : [null]; + let maxHistoryId = 0; + + await Promise.all(labelIds.map(async (labelId) => { + const currentOpts = labelId ? { ...opts, labelId } : opts; + const { history, historyId } = await this.gmail.listHistory(currentOpts); + + if (history) { + maxHistoryId = Math.max(maxHistoryId, historyId); + const responseArray = this.filterHistory(history); + await Promise.all(responseArray.map( + item => this.emitFullMessage(item.messages[0].id) + )); + } + })); + + if (maxHistoryId > 0) { + this._setLastHistoryId(maxHistoryId);Benefits:
- More declarative and easier to understand
- Parallel processing of labels using
Promise.all- Cleaner handling of the no-labels case
103-105: Add validation for excludeLabels array.While the exclusion logic is well implemented, consider adding validation to ensure
excludeLabelsis a valid array when present.Here's a suggested improvement:
- if (this.excludeLabels && message.labelIds.some((i) => this.excludeLabels.includes(i))) { + if (Array.isArray(this.excludeLabels) && + message.labelIds?.some((i) => this.excludeLabels.includes(i))) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
components/gmail/sources/common/polling-history.mjs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
components/gmail/sources/common/polling-history.mjs (2)
29-30: LGTM! Safe handling of multiple labels.The change from single label to array with optional chaining provides safe and flexible label handling.
85-86: LGTM! Consistent handling of multiple labels.The implementation maintains consistency with other methods in handling multiple labels.
Resolves #15608
Summary by CodeRabbit