Update new-email.mjs #15001
Update new-email.mjs #15001
Conversation
Allow for all folders in Outlook to be checked for new emails
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@mchlbckr is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request primarily focuses on enhancing the Microsoft Outlook new email source component. The main changes involve adding a Changes
Sequence DiagramsequenceDiagram
participant User
participant Trigger
participant MicrosoftGraph
participant EmailFolders
User->>Trigger: Configure folderIds
Trigger->>MicrosoftGraph: List available folders
MicrosoftGraph-->>Trigger: Return folder list
Trigger->>EmailFolders: Retrieve messages from specified folders
EmailFolders-->>Trigger: Return new email messages
Trigger->>User: Emit new email events
Possibly related PRs
Suggested Labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
![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
🔭 Outside diff range comments (1)
components/microsoft_outlook/sources/new-email/new-email.mjs (1)
Line range hint
45-54: Add error handling and implement deduplication strategyThe current implementation needs several improvements to align with Pipedream guidelines:
- Add error handling for getMessage API calls
- Implement deduplication strategy
- Enhance event summary with more details
Consider this implementation:
async run(event) { await this.run({ event, emitFn: async ({ resourceId } = {}) => { - const item = await this.microsoftOutlook.getMessage({ - messageId: resourceId, - }); - this.emitEvent(item); + try { + const item = await this.microsoftOutlook.getMessage({ + messageId: resourceId, + }); + + // Implement deduplication + const dedupKey = `${item.id}-${item.lastModifiedDateTime}`; + if (this.db.get(dedupKey)) { + return; + } + this.db.set(dedupKey, true); + + this.emitEvent(item); + } catch (error) { + console.error('Error fetching message:', error); + throw error; + } }, }); },Also, enhance the event summary in generateMeta:
generateMeta(item) { return { id: item.id, - summary: `New email (ID:${item.id})`, + summary: `New email from ${item.sender?.emailAddress?.address} - Subject: ${item.subject}`, ts: Date.parse(item.createdDateTime), }; },
🧹 Nitpick comments (1)
components/microsoft_outlook/sources/new-email/new-email.mjs (1)
Line range hint
1-54: Add JSDoc documentation and configuration optionsTo fully comply with Pipedream guidelines, please add:
- JSDoc documentation for methods
- Optional configuration props for:
- Folder filtering
- Message age threshold
- Update frequency
Example implementation:
props: { folders: { type: "string[]", label: "Folders to Monitor", description: "Leave empty to monitor all folders", optional: true, }, maxMessageAge: { type: "integer", label: "Maximum Message Age (hours)", description: "Only process messages newer than this", default: 24, optional: true, }, },
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
components/microsoft_outlook/sources/new-email/new-email.mjs (1)
Line range hint
6-6: Consider bumping the version number.The change in message fetching behavior is significant enough to warrant a version bump. Since this is a behavioral change that expands functionality without breaking existing behavior, consider incrementing the minor version (e.g., from 0.0.9 to 0.1.0).
- version: "0.0.9", + version: "0.1.0",
🧹 Nitpick comments (1)
components/microsoft_outlook/sources/new-email/new-email.mjs (1)
Line range hint
4-4: Update component description to reflect the enhanced functionality.The current description "Emit new event when an email received" should be updated to clarify that it monitors all folders, not just the inbox.
- description: "Emit new event when an email received", + description: "Emit new event when an email is received in any folder",Also applies to: 7-7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/microsoft_outlook/sources/new-email/new-email.mjs(1 hunks)
🔇 Additional comments (2)
components/microsoft_outlook/sources/new-email/new-email.mjs (2)
16-16: Verify the impact of querying all messages.
The change from /me/mailfolders('inbox')/messages to /me/messages will now fetch emails from all folders instead of just the inbox. While this aligns with the PR objective, please consider:
- Performance: Fetching from all folders might increase response time and API usage
- Rate Limiting: Verify if this broader scope might hit Microsoft Graph API rate limits sooner
- Duplicate Prevention: Ensure the webhook system properly handles messages that might be moved between folders
Let's verify the Microsoft Graph API documentation and rate limits:
✅ Verification successful
The change to query all messages is properly implemented with safeguards
The codebase analysis reveals that the implementation includes important safeguards for querying all messages:
- Pagination is implemented using
$topparameter to limit the number of messages per request - Messages are ordered by creation date (
$orderby: "createdDateTime desc"), ensuring newest messages are processed first - The change aligns with the existing app-level implementation which already supports the
/me/messagesendpoint
The broader scope of querying all folders should not cause performance or rate limiting issues due to these built-in controls. The webhook system will naturally handle message movements between folders as it operates on message IDs rather than folder locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related API endpoint usage and rate limiting information
# in the codebase
# Check for any existing rate limiting handling
rg -A 5 "rateLimitExceeded|429|TooManyRequests" ./components/microsoft_outlook/
# Look for related documentation or comments about message fetching
rg -l "\/me\/messages|\/mailfolders|Microsoft Graph API" ./components/microsoft_outlook/
Length of output: 287
Script:
#!/bin/bash
# Search for rate limiting handling with proper escaping
rg -A 5 "rateLimitExceeded|429|TooManyRequests" ./components/microsoft_outlook/
# Look for API endpoint usage with proper escaping
rg -A 5 "/me/messages|/me/mailfolders" ./components/microsoft_outlook/
# Check component documentation and readme
fd -e md . ./components/microsoft_outlook/ -x cat {}
# Look for error handling in the component
rg -A 5 "catch|error" ./components/microsoft_outlook/sources/new-email/
Length of output: 3529
Script:
#!/bin/bash
# Check for error handling and rate limiting in the app file
rg -A 10 "async _makeRequest|error handling|rate limit" ./components/microsoft_outlook/microsoft_outlook.app.mjs
# Look for pagination or batch processing
rg -A 5 "nextLink|page|batch|\$top|\$skip" ./components/microsoft_outlook/sources/new-email/
# Check if there are any filters applied to message retrieval
rg -A 5 "\$filter|\$select|\$orderby" ./components/microsoft_outlook/sources/new-email/
# Look for webhook subscription configuration
ast-grep --pattern 'createHook|createSubscription'
Length of output: 1373
Line range hint 28-34: Verify message listing parameters for the sample events.
The getSampleEvents method should be consistent with the new behavior. Verify if additional parameters are needed to properly represent messages from all folders.
✅ Verification successful
The getSampleEvents implementation is consistent with the source's behavior
The implementation of getSampleEvents is correctly aligned with the source's functionality:
- It uses the same message listing endpoint as the webhook subscription (
/me/messages) - The parameters (
$topand$orderby) are appropriate for retrieving recent messages - No folder-specific filtering is needed as the source monitors all incoming messages across folders through the webhook subscription
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any folder-specific filtering in message listing
# Search for message listing parameters across the codebase
rg -A 5 "listMessages|\/messages\?" ./components/microsoft_outlook/
Length of output: 174
Script:
#!/bin/bash
# Search for message listing parameters across the codebase
rg -A 5 "listMessages|/messages\?" ./components/microsoft_outlook/
Length of output: 1112
Script:
#!/bin/bash
# Search for getSampleEvents implementations
ast-grep --pattern 'async getSampleEvents() {
$$$
}'
Length of output: 56
Script:
#!/bin/bash
# Search for message listing related code
rg -A 5 "\$top|\$orderby|listMessages|/messages" ./components/microsoft_outlook/
Length of output: 3481
Script:
#!/bin/bash
# Search for getSampleEvents method with a more flexible pattern
ast-grep --pattern 'getSampleEvents() {
$$$
}'
Length of output: 50
Script:
#!/bin/bash
# Search for any folder-related message filtering
rg -A 5 "folder|mailFolder" ./components/microsoft_outlook/
Length of output: 2179
Script:
#!/bin/bash
# Search for the entire new-email.mjs file content
cat ./components/microsoft_outlook/sources/new-email/new-email.mjs
Length of output: 1380
|
Hi @mchlbckr , thanks for your contribution! The version number needs to be incremented both in the component being changed, and the package.json of the app. Also, perhaps this behavior should be controlled by an optional boolean prop - defaulting to false (keeping the existing behavior), thus giving users the option to enable if desired. What do you think? |
|
@GTFalcao Thanks for reviewing! Yes, I think either a boolean option would be better or an option to specify all folders that should be screened. Should I make a proposal for this? |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/microsoft_outlook/sources/new-email/new-email.mjs(3 hunks)
🧰 Additional context used
🪛 eslint
components/microsoft_outlook/sources/new-email/new-email.mjs
[error] 37-37: A linebreak is required after '['.
(array-bracket-newline)
[error] 37-37: A linebreak is required before ']'.
(array-bracket-newline)
[error] 54-54: Expected a line break after this opening brace.
(object-curly-newline)
[error] 54-54: Expected a line break before this closing brace.
(object-curly-newline)
[error] 55-56: Missing trailing comma.
(comma-dangle)
[error] 69-69: A linebreak is required after '['.
(array-bracket-newline)
[error] 69-69: A linebreak is required before ']'.
(array-bracket-newline)
🔇 Additional comments (1)
components/microsoft_outlook/sources/new-email/new-email.mjs (1)
8-9: Consider version bump for breaking change
Changing from inbox-only to all-folders monitoring is a breaking change that could affect existing implementations. Consider bumping the major version number instead of a minor version bump.
|
Hey @GTFalcao @pipedream-component-development Do you have any update on this issue? Thanks! |
|
Hey @mchlbckr thanks for updating this, I saw your commit after we talked about the optional prop - looks good to me! I'm moving this forward to the QA stage |
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/microsoft_outlook/sources/new-email/new-email.mjs (3)
34-36: Remove debugging console.log statementRemove the console.log statement that was likely used for debugging purposes.
const events = await this.getSampleEvents({ pageSize: 25, -}); console.log(events); +});
86-93: Optimize folder filtering logicConsider caching the
sentItemFolderIdcheck result and using early returns for better readability:isRelevant(item) { - if (this.folderIds?.length) { - return true; - } - // if no folderIds are specified, filter out items in Sent Items - const sentItemFolderId = this.db.get("sentItemFolderId"); - return item.parentFolderId !== sentItemFolderId; + // If specific folders are selected, all items are relevant + if (this.folderIds?.length) return true; + + // Cache sentItemFolderId for the instance + this._sentItemFolderId = this._sentItemFolderId || this.db.get("sentItemFolderId"); + + // Filter out items in Sent Items folder + return item.parentFolderId !== this._sentItemFolderId; }
11-28: Add monitoring controls to prevent API throttlingConsider adding additional properties to control the monitoring behavior:
- Rate limiting controls
- Maximum folders to monitor
- Message age threshold
- Batch size controls
This will help prevent API throttling and manage resource usage, especially important now that the component can monitor multiple folders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/microsoft_outlook/sources/new-email/new-email.mjs(3 hunks)
🔇 Additional comments (5)
components/microsoft_outlook/sources/new-email/new-email.mjs (5)
8-9: Consider major version bump for breaking changeThe change from inbox-only to all-folders monitoring is a breaking change that could affect existing implementations. Consider bumping the major version number instead of a minor version bump.
13-27: Add validation for folder IDsWhile the property is well-documented, consider adding input validation to ensure folder IDs match the expected format and exist in the user's Outlook account.
46-48: Add error handling for subscription creationThe activate hook should include error handling for subscription creation failures, especially important now that it's monitoring all messages.
66-85: Improve result handling in getSampleEventsThe current implementation has several potential issues:
- Results from different folders aren't sorted by creation time
- No deduplication of messages that might appear in multiple folders
- Potential memory issues when aggregating results from many folders
113-134: Improve performance and reliability of the run methodThe current implementation processes folders sequentially without rate limiting or retry logic for transient failures.
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
components/microsoft_outlook/sources/new-email/new-email.mjs (3)
2-2:⚠️ Potential issueConsider a major version bump for breaking changes
The change from inbox-only to all-folders monitoring is a breaking change that could affect existing implementations. Consider bumping the major version number instead of a minor version increment.
Also applies to: 9-10
48-51: 🛠️ Refactor suggestionAdd error handling for subscription creation
The subscription creation in the
activatehook should include error handling to prevent silent failures.
69-88: 🛠️ Refactor suggestionImprove result handling in getSampleEvents
Current implementation has several potential issues:
- Results from different folders aren't sorted by creation time
- No deduplication of messages that might appear in multiple folders
- Potential memory issues when aggregating results from many folders
🧹 Nitpick comments (1)
components/microsoft_outlook/sources/new-email/new-email.mjs (1)
89-97: Add logging for filtered messagesConsider adding debug logging when messages are filtered out. This would help users understand why certain messages aren't being processed.
isRelevant(item) { if (this.folderIds?.length) { - return this.folderIds.includes(item.parentFolderId); + const isRelevant = this.folderIds.includes(item.parentFolderId); + if (!isRelevant) { + console.debug(`Skipping message ${item.id} - folder ${item.parentFolderId} not in monitored folders`); + } + return isRelevant; } // if no folderIds are specified, filter out items in Sent Items & Drafts const sentItemFolderId = this.db.get("sentItemFolderId"); const draftsFolderId = this.db.get("draftsFolderId"); - return item.parentFolderId !== sentItemFolderId && item.parentFolderId !== draftsFolderId; + const isRelevant = item.parentFolderId !== sentItemFolderId && item.parentFolderId !== draftsFolderId; + if (!isRelevant) { + console.debug(`Skipping message ${item.id} - in excluded folder ${item.parentFolderId}`); + } + return isRelevant; }
📜 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 (2)
components/microsoft_outlook/package.json(2 hunks)components/microsoft_outlook/sources/new-email/new-email.mjs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/microsoft_outlook/package.json
⏰ 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/microsoft_outlook/sources/new-email/new-email.mjs (2)
15-29: Add validation and controls for folder monitoringThe
folderIdsproperty implementation could benefit from:
- Input validation for folder IDs
- Rate limiting controls
- Pagination settings for large folder structures
118-139: Enhance performance and reliability of the run methodConsider improving the implementation with:
- Concurrent processing with rate limiting
- Consistent error handling across all API calls
- Retry logic for transient failures
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/microsoft_outlook/sources/new-email/new-email.mjs (2)
20-28: Add error handling and caching for folder optionsThe async options method could fail silently and doesn't implement caching. Consider:
- Adding error handling
- Implementing caching to prevent repeated API calls
async options() { + try { + // Cache folder list for 5 minutes + const cacheKey = "folderOptions"; + const cached = this.db.get(cacheKey); + if (cached && Date.now() - cached.timestamp < 300000) { + return cached.options; + } + const { value: folders } = await this.listFolders(); - return folders?.map(({ + const options = folders?.map(({ id: value, displayName: label, }) => ({ value, label, })) || []; + + this.db.set(cacheKey, { + timestamp: Date.now(), + options, + }); + + return options; + } catch (error) { + console.error("Failed to fetch folder options:", error); + return []; + } }
89-97: Add error handling for folder ID checksThe
isRelevantmethod should handle cases where folder IDs might be undefined or invalid.isRelevant(item) { + if (!item?.parentFolderId) { + console.warn("Received item without parentFolderId:", item); + return false; + } + if (this.folderIds?.length) { return this.folderIds.includes(item.parentFolderId); } // if no folderIds are specified, filter out items in Sent Items & Drafts const sentItemFolderId = this.db.get("sentItemFolderId"); const draftsFolderId = this.db.get("draftsFolderId"); + + if (!sentItemFolderId || !draftsFolderId) { + console.warn("Missing system folder IDs, skipping filter"); + return true; + } return item.parentFolderId !== sentItemFolderId && item.parentFolderId !== draftsFolderId; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/microsoft_outlook/sources/new-email/new-email.mjs(2 hunks)
🔇 Additional comments (4)
components/microsoft_outlook/sources/new-email/new-email.mjs (4)
9-11: Consider major version bump for breaking changesThe change from inbox-only to all-folders monitoring is a breaking change that could affect existing implementations. Consider bumping the major version number instead of a minor version increment.
69-88: Improve result handling in getSampleEventsThe current implementation has several potential issues:
- Results from different folders aren't sorted by creation time
- No deduplication of messages that might appear in multiple folders
- Potential memory issues when aggregating results from many folders
116-139: Improve performance and reliability of the run methodThe current implementation processes folders sequentially without rate limiting.
Line range hint
1-142: Add comprehensive testing and documentationWhile the implementation is solid, please consider:
- Adding unit tests for the new folder selection functionality
- Documenting rate limits and performance implications
- Adding examples in the component's documentation
Run this script to check for existing tests and documentation:
|
Hi everyone, all test cases are passed! Ready for release! Test report |
|
I think this still does not work. At least I just tested it and there is an error:
|
Hi @mchlbckr , I created a New PR that should fix the error. |
Allow for all folders in Outlook to be checked for new emails
WHY
Currently, the node only checks for new emails in the inbox folder. If by certain rules, an email is directly moved into a separate folder, it is ignored by the trigger.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Version Updates
Technical Updates