Open
Conversation
…bug-in-mailtrap-gem Map Reply-To into structured field and strip Reply-To header from payload
…bug-in-mailtrap-gem-itmsm1 test: cover folded line-break reply-to parsing error
📝 WalkthroughWalkthroughThis change adds reply-to header support to the Mailtrap mail library by adding 'replyto' to the SPECIAL_HEADERS constant and implementing extraction of reply-to values from messages. The reply-to field is now mapped to a structured object in API requests, consistent with existing from/to/cc/bcc handling, along with comprehensive test coverage for various formats and edge cases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
spec/mailtrap/mail_spec.rb (1)
186-197: Duplicate test contexts for invalidreply-to.The test at lines 186-197 and lines 212-224 both test the same scenario: an invalid
'Reply-To': 'invalid email@example.com'raisingMailtrap::Errorwith the same message. Consider consolidating these into a single context block with the folded line break case nested within it.♻️ Suggested consolidation
Remove lines 186-197 and keep only the block at lines 212-241, which already covers both the basic invalid case and the folded line break edge case.
- context "when 'reply-to' is invalid" do - before do - message.header['Reply-To'] = 'invalid email@example.com' - end - - it 'raises an error' do - expect { mail }.to raise_error( - Mailtrap::Error, - "failed to parse 'Reply-To': 'invalid email@example.com'" - ) - end - end - %i[from to cc bcc].each do |header|Also applies to: 212-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/mail_spec.rb` around lines 186 - 197, There are duplicate RSpec contexts named "when 'reply-to' is invalid" testing the same invalid Reply-To value; remove the earlier duplicate context (the one that sets message.header['Reply-To'] = 'invalid email@example.com' and its expect) and keep the later context that already covers both the basic invalid case and the folded line-break edge case (the nested case that manipulates message.header['Reply-To'] and expects Mailtrap::Error with the failure message); alternatively, consolidate by merging the simple invalid-value example into the later context as a nested example so only one context named "when 'reply-to' is invalid" remains and both assertions (simple invalid and folded line break) live under it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/mailtrap/mail_spec.rb`:
- Around line 186-197: There are duplicate RSpec contexts named "when 'reply-to'
is invalid" testing the same invalid Reply-To value; remove the earlier
duplicate context (the one that sets message.header['Reply-To'] = 'invalid
email@example.com' and its expect) and keep the later context that already
covers both the basic invalid case and the folded line-break edge case (the
nested case that manipulates message.header['Reply-To'] and expects
Mailtrap::Error with the failure message); alternatively, consolidate by merging
the simple invalid-value example into the later context as a nested example so
only one context named "when 'reply-to' is invalid" remains and both assertions
(simple invalid and folded line break) live under it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 274dddc9-cd03-4644-9a18-84c358f9ebd6
📒 Files selected for processing (3)
lib/mailtrap/mail.rbspec/fixtures/vcr_cassettes/Mailtrap_ActionMailer_DeliveryMethod/_deliver_/converts_the_message_and_sends_via_API.ymlspec/mailtrap/mail_spec.rb
Contributor
|
@ryush00 I see the problem. Will have to test the changes. Thanks. |
i7an
approved these changes
Mar 9, 2026
| body: | ||
| encoding: UTF-8 | ||
| string: '{"from":{"email":"mailtrap@mailtrap.io","name":"Mailtrap Test"},"to":[{"email":"to_1@railsware.com","name":"To 1"},{"email":"to_2@railsware.com"}],"cc":[{"email":"cc_1@railsware.com"},{"email":"cc_2@railsware.com","name":"Cc 2"}],"bcc":[{"email":"bcc_1@railsware.com"},{"email":"bcc_2@railsware.com"}],"subject":"You are awesome!","text":"Some text","html":"<div>HTML part</div>","attachments":[{"content":"VGhpcyBpcyBhIHRleHQgZmlsZQo=","type":"text/plain","filename":"file.txt","disposition":"attachment","content_id":"txt_content_id@test.mail"},{"content":"iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAABiElEQVRIid3VSW5UMRCA4U8d0gIx7AjhAghBTgFICAkxRFHukDDDIRiuwI7xPNAMISQRV4CABFmkWbie2nrt9muaFZTkxavhr7Jfucz/Ln2s4hU28B0/sBO6VczPCl/GNoYd6wsuZ3ELWKuBe3iSAQa4g7M4jEM4jRt4g5+4lMEHEbc+KUED/xVOvY5iThXgg/gek+UMfq4CbstU8L7RmU/c3qxwUkc0TnN/CT9Ycn4djrenhB/H24j5iMXQX8TTUsCncD6T6daUtzyp8hNSV30uJfgWAUfje70AqMFF7FC6kGOy20rQPoKTBd1ii3EsbF9LCUpH1K62q1uWwr5RSvAyjHdb+rzqSZU38iB8npWMTZu+M96mzU5qfT6H98FYKTn0sRUONwv2hQqcNK+G2FSZsNeNRsX5CqwtF7CHfVzpcn6cJbmlfqsPSJXvRczDaarpZUmaf3JP6pAjsZZw3+jM9/FIffKOyTXpRnY9OJu4+ifgXOaljnghtedurA94HraZn8x/Q34DYaON8Fk9Z1IAAAAASUVORK5CYII=","type":"image/png","filename":"file.png","disposition":"inline","content_id":"png_content_id@test.mail"}],"headers":{"Reply-To":"reply-to@railsware.com","X-Special-Domain-Specific-Header":"SecretValue","One-more-custom-header":"CustomValue"},"category":"Module Test"}' | ||
| string: '{"from":{"email":"mailtrap@mailtrap.io","name":"Mailtrap Test"},"to":[{"email":"to_1@railsware.com","name":"To 1"},{"email":"to_2@railsware.com"}],"reply_to":{"email":"reply-to@railsware.com"},"cc":[{"email":"cc_1@railsware.com"},{"email":"cc_2@railsware.com","name":"Cc 2"}],"bcc":[{"email":"bcc_1@railsware.com"},{"email":"bcc_2@railsware.com"}],"subject":"You are awesome!","text":"Some text","html":"<div>HTML part</div>","attachments":[{"content":"VGhpcyBpcyBhIHRleHQgZmlsZQo=","type":"text/plain","filename":"file.txt","disposition":"attachment","content_id":"txt_content_id@test.mail"},{"content":"iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAABiElEQVRIid3VSW5UMRCA4U8d0gIx7AjhAghBTgFICAkxRFHukDDDIRiuwI7xPNAMISQRV4CABFmkWbie2nrt9muaFZTkxavhr7Jfucz/Ln2s4hU28B0/sBO6VczPCl/GNoYd6wsuZ3ELWKuBe3iSAQa4g7M4jEM4jRt4g5+4lMEHEbc+KUED/xVOvY5iThXgg/gek+UMfq4CbstU8L7RmU/c3qxwUkc0TnN/CT9Ycn4djrenhB/H24j5iMXQX8TTUsCncD6T6daUtzyp8hNSV30uJfgWAUfje70AqMFF7FC6kGOy20rQPoKTBd1ii3EsbF9LCUpH1K62q1uWwr5RSvAyjHdb+rzqSZU38iB8npWMTZu+M96mzU5qfT6H98FYKTn0sRUONwv2hQqcNK+G2FSZsNeNRsX5CqwtF7CHfVzpcn6cJbmlfqsPSJXvRczDaarpZUmaf3JP6pAjsZZw3+jM9/FIffKOyTXpRnY9OJu4+ifgXOaljnghtedurA94HraZn8x/Q34DYaON8Fk9Z1IAAAAASUVORK5CYII=","type":"image/png","filename":"file.png","disposition":"inline","content_id":"png_content_id@test.mail"}],"headers":{"X-Special-Domain-Specific-Header":"SecretValue","One-more-custom-header":"CustomValue"},"category":"Module Test"}' |
Contributor
There was a problem hiding this comment.
reply-to address was moved from headers to the body
| to: prepare_addresses(message['to']), | ||
| cc: prepare_addresses(message['cc']), | ||
| bcc: prepare_addresses(message['bcc']), | ||
| reply_to: prepare_addresses(message['reply-to']).first, |
Contributor
There was a problem hiding this comment.
Reply-to in the raw email.
Before:
>=?UTF-8?q?"=EB=A7=88=EC=9D=B8=EB=A6=AC=EC=8A=A4=ED=8A=B8"_<support@mailtr?==?UTF-8?q?ap.io>?=
After:
=?utf-8?q?=EB=A7=88=EC=9D=B8=EB=A6=AC=EC=8A=A4=ED=8A=B8?=<support@mailtrap.io>
i7an
reviewed
Mar 9, 2026
| its(:template_variables) { is_expected.to eq('first_name' => 'John') } | ||
| end | ||
|
|
||
| context "when 'reply-to' is invalid" do |
Contributor
There was a problem hiding this comment.
the tests should be refactored. I can do that once merged.
- this should be merged with the next block.
i7an
reviewed
Mar 9, 2026
| end | ||
|
|
||
| context 'when address contains a folded line break' do | ||
| let(:invalid_reply_to) do |
Contributor
There was a problem hiding this comment.
could you please explain this case?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Some users sending to Yahoo reported bounces like:
In our ActionMailer conversion path,
Reply-Tocould remain inheadersinstead of being represented as structuredreply_to.This risks inconsistent header serialization/parsing for edge cases (including folded/encoded header forms).
What changed
1) Map
Reply-Tointo structured payload fieldMailtrap::Mail.from_messagenow parsesmessage['reply-to']and sets:reply_to: prepare_addresses(message['reply-to']).firstReply-Tohandling with other structured address fields at payload build time.2) Remove
Reply-Tofrom custom headersreplytoto the headers-removal list (SPECIAL_HEADERS→HEADERS_TO_REMOVEpath),so
Reply-Todoes not remain duplicated inheaders.3) Test coverage improvements
Updated
spec/mailtrap/mail_spec.rbto verify:reply_tomapping for:Reply-Toheader removal (including case variants:Reply-To,REPLY-TO,reply-to)Reply-Toerror behaviorReply-Toform (newline folding) raises parse error4) Integration fixture update
reply_toheaders.Reply-ToduplicationWhy this is safe
from/to/cc/bcc/subject/text/html/attachmentsis unchanged.reply_totype remains compatible with currentBasepayload shape (single object).How to test
Notes for reviewers
Summary by CodeRabbit
New Features
Tests