Gmail: fixing address parsing in replyAll option #18740
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds robust RFC 5322 address parsing via nodemailer’s addressparser in the Gmail app, updates email composition to use parsed arrays for to/cc/bcc (including reply-all flows), bumps the action metadata version for send-email, and increments the package version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Composer as Email Composer
participant App as Gmail App
participant Parser as nodemailer addressparser
participant Gmail as Gmail API
User->>Composer: Compose email (to/cc/bcc, replyAll)
Composer->>App: getOptionsToSendEmail(input)
App->>Parser: parseEmailAddresses(to/cc/bcc)
Parser-->>App: Arrays of normalized addresses
App->>Gmail: Send with parsed to/cc/bcc
Gmail-->>App: Message ID / status
App-->>Composer: Options / result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/gmail/gmail.app.mjs (1)
329-336: Add null safety checks for from/to headers.The code uses
.find()to locate headers, which returnsundefinedif not found. Accessing.valueon undefined would throw a runtime error. While emails typically have From and To headers, defensive coding is essential here.Apply this diff to add null safety:
if (props.replyAll) { const from = repliedMessage.payload.headers.find(({ name }) => name.toLowerCase() === "from"); const to = repliedMessage.payload.headers.find(({ name }) => name.toLowerCase() === "to"); const cc = repliedMessage.payload.headers.find(({ name }) => name.toLowerCase() === "cc"); const bcc = repliedMessage.payload.headers.find(({ name }) => name.toLowerCase() === "bcc"); - opts.to = [ - ...this.parseEmailAddresses(from.value), - ...this.parseEmailAddresses(to.value), - ]; + + const fromAddresses = from ? this.parseEmailAddresses(from.value) : []; + const toAddresses = to ? this.parseEmailAddresses(to.value) : []; + opts.to = [...fromAddresses, ...toAddresses];
🧹 Nitpick comments (2)
components/gmail/package.json (1)
23-23: Consider updating nodemailer to latest 6.x for parsing improvements.The current constraint
^6.7.8allows upgrading to 6.10.0, which includes address parsing fixes and security improvements mentioned in the learnings.Based on learnings: nodemailer 6.10.0 (released 2025-01-23) includes address parsing fixes that may further improve the reliability of the parseEmailAddresses implementation.
components/gmail/gmail.app.mjs (1)
277-297: Well-implemented address parsing utility.The method correctly handles edge cases with input validation and uses nodemailer's addressparser for RFC 5322 compliance. This solves the comma-in-display-name issue mentioned in the PR.
For additional robustness, consider wrapping the addressparser call in a try-catch block:
// Use nodemailer's addressparser to properly handle RFC 5322 format - const parsed = addressparser(addressString.trim()); + try { + const parsed = addressparser(addressString.trim()); + // Reconstruct addresses in standard format + return parsed.map((addr) => { + if (addr.name) { + return `${addr.name} <${addr.address}>`; + } + return addr.address; + }); + } catch (error) { + console.warn('Failed to parse email addresses:', error); + return []; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/gmail/actions/send-email/send-email.mjs(1 hunks)components/gmail/gmail.app.mjs(4 hunks)components/gmail/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/gmail/gmail.app.mjs (1)
components/gmail/actions/send-email/send-email.mjs (1)
opts(123-123)
🪛 GitHub Actions: Components Checks
components/gmail/package.json
[error] 1-1: Version of 19 dependencies needs to be increased. Detected in diffs: send-email action version 0.1.16 -> 0.1.17 and package.json dependency version 1.3.2 -> 1.3.3.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pnpm publish
🔇 Additional comments (3)
components/gmail/actions/send-email/send-email.mjs (1)
9-9: LGTM!Version bump aligns with the PR objectives. Since this action is the sole consumer of the replyAll property and the underlying logic was fixed in gmail.app.mjs, this version increment is appropriate.
components/gmail/gmail.app.mjs (2)
9-9: LGTM!Importing addressparser from nodemailer's internal modules is the correct approach for RFC 5322 address parsing.
352-357: LGTM!The cc and bcc parsing includes proper null checks before accessing
.value, preventing potential runtime errors. The implementation correctly applies the new parsing logic.
GTFalcao
moved this from Ready for PR Review
to Ready for QA
in Component (Source and Action) Backlog
vunguyenhung
moved this from Ready for QA
to Ready for Release
in Component (Source and Action) Backlog
|
Hi everyone, all test cases are passed! Ready for release! |
|
/approve |
github-project-automation
bot
moved this from Ready for Release
to Done
in Component (Source and Action) Backlog
3 participants
When using the
replyAlloption in the Gmail "Send Email" action, it would split the "from" and "to" values by comma in an attempt to retrieve multiple comma-separated emails. This caused a problem when the email itself included a comma in the name (e.g."Falcao, Guilherme" <guilherme@nowhere.com>Using addressparser from nodemailer ensures proper parsing of these fields
Note: I only bumped the version of the
send-emailcomponent, because it is the only component that uses thereplyAllprop, and the logic was only changed in the block that checks for this prop.Summary by CodeRabbit
Bug Fixes
Chores