fix(email): honor Reply-To on Reply and Reply-All - #5588
Conversation
Use usable Reply-To addresses when composing Reply/Reply-All so replies reach the intended recipient instead of automated From addresses. Falls back to From when Reply-To is missing or unusable. Interim path reads headers_json; prefers a future first-class reply_to field. Closes macro-inc#5586
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdated recipient conversion to parse Reply-To addresses from first-class data or 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/features/block-email/util/recipientConversion.ts`:
- Around line 121-126: Update recipientConversion.ts lines 121-126 to filter
withReplyTo.reply_to and return the filtered contacts only when non-empty;
otherwise call replyToFromHeadersJson(message.headers_json). In
recipientConversion.test.ts lines 82-93, add coverage for invalid first-class
reply_to entries with a valid Reply-To header and assert the header address is
returned.
- Around line 149-168: In the Reply-All branch around getUsableReplyToContacts,
filter userEmail out of the Reply-To contacts before building both to and the
toEmails exclusion set, while preserving existing deduplication and ordering.
Update apps/web/src/features/block-email/util/recipientConversion.ts lines
149-168 accordingly; in
apps/web/src/features/block-email/util/recipientConversion.test.ts lines
115-132, include USER in replyToHeader and assert it is absent from to.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79c5c5e8-35e1-4649-8326-2d7c48a74bae
📒 Files selected for processing (2)
apps/web/src/features/block-email/util/recipientConversion.test.tsapps/web/src/features/block-email/util/recipientConversion.ts
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Standards
praise: Small seam — only recipientConversion.ts + tests. Conventional commit / branch naming look right; no any.
suggestion (non-blocking, FE-16 / Divergent Change): parseAddressList + headers_json walking grew this conversion module with a second reason-to-change (RFC address-list parsing). Consider extracting header/address parsing to a sibling util when this interim path sticks around.
thought (non-blocking, Speculative Generality): ApiMessageWithReplyTo prefers a future first-class reply_to before headers_json — intentional interim per the lock, still a speculative surface until OpenAPI catches up.
thought (non-blocking): Mild duplication in Reply-All filters (user / To-overlap) next to the existing From path.
Hard documented-standard violations: none.
Spec
Spec sources: #5586 + locked comment (user stories / decisions) + agreed helper seam.
praise: Locked v1 acceptance looks met — Reply / Reply-All / multi-value Reply-To / unusable fallback / draft non-rewrite (createEmailFormState draft branch untouched) / no new UI / List-Post·Sender·outbound left alone / tests at getReplyRecipientsFromParent + getReplyAllRecipients.
note (non-blocking): No unit case exercises the first-class reply_to preference path (factory supports it; not in the locked required list).
Missing/wrong requirements: none material.
Summary: Standards 0 hard / judgement nits (worst: parser living in conversion util). Spec 0 open findings.
Note: COMMENT instead of APPROVE only because GitHub blocks approving your own PR as cameronapak. Spec + Standards look good to merge from this bot's perspective.
Code Reviewer bot, sent on behalf of Cam.
| @@ -103,13 +219,22 @@ export const getReplyRecipientsFromParent = ( | |||
| // If last message was from user, reply === replyAll | |||
| if (replyingTo?.from?.email === userEmail) { | |||
| return getReplyAllRecipients(replyingTo, userEmail); | |||
There was a problem hiding this comment.
praise: Reply delegates into the Reply-All helper when appropriate — keeps one policy path.
Locked stories for Reply / Reply-All / draft non-rewrite stay at this seam with tests alongside. Good match to the grill lock.
Code Reviewer bot, sent on behalf of Cam.
| * Parse a Reply-To / address-list header value into contacts. | ||
| * Supports `email`, `Name <email>`, and comma-separated lists. | ||
| */ | ||
| const parseAddressList = (raw: string): ContactInfo[] => { |
There was a problem hiding this comment.
suggestion (non-blocking): Address-list parsing is a separate reason-to-change from recipient policy.
Fine as the interim headers_json path, but if this sticks, a sibling util (or shared email-header helper) would keep recipientConversion single-purpose (FE-16 / STYLE_GUIDE).
Code Reviewer bot, sent on behalf of Cam.
| }; | ||
|
|
||
| /** ApiMessage may gain a first-class reply_to field; prefer it when present. */ | ||
| type ApiMessageWithReplyTo = ApiMessage & { |
There was a problem hiding this comment.
thought (non-blocking): Preferring future reply_to is the right interim shape.
Just noting there's no test that hits this branch yet — not required by the lock, but cheap insurance when the OpenAPI field lands.
Code Reviewer bot, sent on behalf of Cam.
Fall back to headers_json when first-class reply_to is unusable, exclude the current user from Reply-All Reply-To recipients, extract address-list parsing to a sibling util, and cover the first-class preference path.
|
Addressed the open review feedback in 39ad84d:
|
|
Ready for review from a Macro maintainer |
|
Nice! This looks like it would work, but we'd want to do this parsing server-side in our Rust instead of client-side. Closing this and creating an internal task to implement this fix server-side. |
2 participants
Warning
PR created through AI bot. Take the meat. Spit out the bones. It won't hurt my feelings if this needs changes!
Summary
Reply-Towhen composing Reply and Reply-All so replies go to the intended address instead of an automatedFrom.createEmailFormState).Approach
Interim
headers_jsonpath (not first-class API field yet):ApiMessagealready exposesheaders_jsonwith Gmail-style{ name, value }entries, and backend search already extracts Reply-To viaextract_reply_to.ApiMessagehas no first-classreply_to: ContactInfo[]yet; regenerating OpenAPI would require a Rust openapi binary build, so this PR uses the allowed interim path.reply_tofield when present, then fall back to parsingheaders_json.Main seam:
getReplyRecipientsFromParent/getReplyAllRecipientsinrecipientConversion.ts.Closes #5586
Test plan
cd apps/web && bunx vitest run src/features/block-email/util/recipientConversion.test.ts --project=block-email(7 passed)cd apps/web && bunx vitest run --project=block-email(40 passed)cd apps/web && bun run type-check(clean)