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. |
WalkthroughThe pull request modifies the Microsoft Outlook email source component, specifically updating the exported object in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (4)
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 (
|
|
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, }, },
| @@ -13,7 +13,7 @@ export default { | |||
| async activate() { | |||
| await this.activate({ | |||
| changeType: "created", | |||
| resource: "/me/mailfolders('inbox')/messages", | |||
| resource: "/me/messages", | |||
There was a problem hiding this comment.
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 from 0.0.9.
Additionally, monitoring all folders instead of just inbox could significantly increase the number of events and API calls. Consider adding optional props to:
- Allow folder filtering
- Set rate limits
- Configure message age thresholds
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.
| changeType: "created", | ||
| resource: "/me/mailfolders('inbox')/messages", | ||
| resource: "/me/messages", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for subscription creation
The activate hook should include error handling for subscription creation failures, especially important now that it's monitoring all messages.
async activate() {
+ try {
await this.activate({
changeType: "created",
resource: "/me/messages",
});
+ } catch (error) {
+ console.error("Failed to create subscription:", error);
+ throw new Error(`Failed to create message subscription: ${error.message}`);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| const folders = this.folderIds?.length | ||
| ? this.folderIds.map((id) => `/me/mailFolders/${id}/messages`) | ||
| : ["/me/messages"]; | ||
|
|
||
| const results = []; | ||
| for (const folder of folders) { | ||
| const messages = await this.microsoftOutlook.listMessages({ | ||
| resource: folder, | ||
| params: { | ||
| $top: pageSize, | ||
| $orderby: "createdDateTime desc", | ||
| }, | ||
| }); | ||
| results.push(...messages); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve result handling in getSampleEvents
The 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
const results = [];
for (const folder of folders) {
const messages = await this.microsoftOutlook.listMessages({
resource: folder,
params: {
$top: pageSize,
$orderby: "createdDateTime desc",
},
});
- results.push(...messages);
+ results.push(...messages.map(msg => ({...msg, sourceFolder: folder})));
}
+// Sort all results by creation time
+results.sort((a, b) =>
+ new Date(b.createdDateTime) - new Date(a.createdDateTime)
+);
+// Deduplicate by message ID
+const uniqueResults = Array.from(
+ new Map(results.map(item => [item.id, item])).values()
+);
+return uniqueResults.slice(0, pageSize);📝 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.
| const folders = this.folderIds?.length | |
| ? this.folderIds.map((id) => `/me/mailFolders/${id}/messages`) | |
| : ["/me/messages"]; | |
| const results = []; | |
| for (const folder of folders) { | |
| const messages = await this.microsoftOutlook.listMessages({ | |
| resource: folder, | |
| params: { | |
| $top: pageSize, | |
| $orderby: "createdDateTime desc", | |
| }, | |
| }); | |
| results.push(...messages); | |
| } | |
| const folders = this.folderIds?.length | |
| ? this.folderIds.map((id) => `/me/mailFolders/${id}/messages`) | |
| : ["/me/messages"]; | |
| const results = []; | |
| for (const folder of folders) { | |
| const messages = await this.microsoftOutlook.listMessages({ | |
| resource: folder, | |
| params: { | |
| $top: pageSize, | |
| $orderby: "createdDateTime desc", | |
| }, | |
| }); | |
| results.push(...messages.map(msg => ({...msg, sourceFolder: folder}))); | |
| } | |
| // Sort all results by creation time | |
| results.sort((a, b) => | |
| new Date(b.createdDateTime) - new Date(a.createdDateTime) | |
| ); | |
| // Deduplicate by message ID | |
| const uniqueResults = Array.from( | |
| new Map(results.map(item => [item.id, item])).values() | |
| ); | |
| return uniqueResults.slice(0, pageSize); |
🧰 Tools
🪛 eslint
[error] 37-37: A linebreak is required after '['.
(array-bracket-newline)
[error] 37-37: A linebreak is required before ']'.
(array-bracket-newline)
| folderIds: { | ||
| type: "string[]", | ||
| label: "Folder IDs to Monitor", | ||
| description: "Specify the folder IDs or names in Outlook that you want to monitor for new emails. Leave empty to monitor all folders.", | ||
| optional: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and controls for folder monitoring
While the folderIds property is well-documented, consider adding:
- Input validation to ensure folder IDs match expected format
- Rate limiting controls to prevent API throttling
- Pagination settings for large folder structures
folderIds: {
type: "string[]",
label: "Folder IDs to Monitor",
description: "Specify the folder IDs or names in Outlook that you want to monitor for new emails. Leave empty to monitor all folders.",
optional: true,
+ validate: (ids) => {
+ if (!ids?.length) return true;
+ return ids.every(id => /^[A-Za-z0-9-_]+$/.test(id));
+ },
},
+maxItemsPerRun: {
+ type: "integer",
+ label: "Max Items Per Run",
+ description: "Maximum number of items to process per execution",
+ optional: true,
+ default: 100,
+},Committable suggestion skipped: line range outside the PR's diff.
| const folders = this.folderIds?.length | ||
| ? this.folderIds.map((id) => `/me/mailFolders/${id}/messages`) | ||
| : ["/me/messages"]; | ||
|
|
||
| for (const folder of folders) { | ||
| await this.run({ | ||
| event, | ||
| emitFn: async ({ resourceId } = {}) => { | ||
| const item = await this.microsoftOutlook.getMessage({ | ||
| resource: folder, | ||
| messageId: resourceId, | ||
| }); | ||
| this.emitEvent(item); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve performance and reliability of the run method
The current implementation processes folders sequentially without error handling or rate limiting. Consider:
- Adding concurrent processing with rate limiting
- Implementing per-folder error handling
- Adding retry logic for transient failures
+const CONCURRENCY_LIMIT = 3;
+const RETRY_ATTEMPTS = 3;
+
async run(event) {
const folders = this.folderIds?.length
? this.folderIds.map((id) => `/me/mailFolders/${id}/messages`)
: ["/me/messages"];
- for (const folder of folders) {
+ const processBatch = async (folderBatch) => {
+ await Promise.all(folderBatch.map(async (folder) => {
+ for (let attempt = 1; attempt <= RETRY_ATTEMPTS; attempt++) {
+ try {
+ await this.run({
+ event,
+ emitFn: async ({ resourceId } = {}) => {
+ const item = await this.microsoftOutlook.getMessage({
+ resource: folder,
+ messageId: resourceId,
+ });
+ this.emitEvent(item);
+ },
+ });
+ break;
+ } catch (error) {
+ if (attempt === RETRY_ATTEMPTS) {
+ console.error(`Failed to process folder ${folder}:`, error);
+ }
+ await new Promise(resolve => setTimeout(resolve, 1000 * attempt));
+ }
+ }
+ }));
+ };
+
+ // Process folders in batches
+ for (let i = 0; i < folders.length; i += CONCURRENCY_LIMIT) {
+ const batch = folders.slice(i, i + CONCURRENCY_LIMIT);
+ await processBatch(batch);
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 69-69: A linebreak is required after '['.
(array-bracket-newline)
[error] 69-69: A linebreak is required before ']'.
(array-bracket-newline)
|
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 |
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