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.
| { | ||
| "name": "@pipedream/gmail", | ||
| "version": "1.3.2", | ||
| "version": "1.3.3", |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Pipeline failure indicates incomplete version bumps.
The pipeline is failing because 19 dependencies need their versions increased, but only this package version was bumped. This is blocking the PR from being merged.
Please identify and update all dependent component versions that reference this package or the send-email action. Run the following script to find dependencies:
🏁 Script executed:
#!/bin/bash
# Description: Find all components that depend on @pipedream/gmail or reference the send-email action
echo "=== Finding package dependencies ==="
fd -e json -x grep -l '"@pipedream/gmail"' {} \;
echo -e "\n=== Finding send-email action references ==="
rg -l "gmail-send-email" --type-add 'components:*.mjs' -t componentsLength of output: 165
Please run the following to list all package.json files and their versions:
🏁 Script executed:
#!/bin/bash
# List every package.json and its current version
fd -g 'package.json' -x sh -c 'echo "{}: version $(jq -r ".version" {})"'Length of output: 157842
Please run:
🏁 Script executed:
#!/bin/bash
# Locate the pipeline error source
rg -n "needs to be increased" -C3Length of output: 1161
Critical: Outdated dependencies in components/gmail/package.json are blocking CI
Pipeline reports 19 dependencies need their versions increased. Run npm outdated in components/gmail, bump each dependency to the latest allowed patch/minor release, update package.json (and lockfile), then rerun CI.
🤖 Prompt for AI Agents
In components/gmail/package.json around line 3 the dependency versions are
outdated and blocking CI; run npm outdated inside components/gmail, update
package.json by bumping each listed dependency to the latest allowed patch/minor
release (not major), run npm install to update the lockfile (package-lock.json
or yarn.lock), run the test/build scripts locally to verify no regressions,
commit the updated package.json and lockfile, and push to trigger CI again.
1 participant
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" <[email protected]>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