Parse From header according to RFC 2822
#472
Conversation
|
@danteissaias is attempting to deploy a commit to the Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR modifies email handling and UI components. The driver function now extracts sender details using a new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant Driver as Driver Function
participant Parser as parseFrom()
participant EmailLib as email-addresses Library
Driver->>Parser: Pass sender string
Parser->>EmailLib: Parse email header
EmailLib-->>Parser: Return parsed data (or empty)
Parser-->>Driver: Return sender object (or fallback)
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
danteissaias
changed the title
From header according to RFC 2822
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/mail/lib/email-utils.ts (4)
135-138: Consider more descriptive fallback valuesWhile "Unknown" works as a fallback, consider using values that better indicate a parsing failure rather than potentially misleading users into thinking there might be an actual sender named "Unknown".
const FALLBACK_SENDER = { - name: "Unknown", - email: "unknown" + name: "No Sender Name", + email: "no-sender@unknown" }
140-159: Missing TypeScript return type annotationThe function is well-implemented and handles various edge cases correctly. However, adding a return type would improve type safety and make it clearer for consumers what structure to expect.
- export const parseFrom = (fromHeader: string) => { + export const parseFrom = (fromHeader: string): { name: string; email: string } => {Also, the implementation correctly handles parsing failures and different sender types according to RFC 2822. The comments explaining the handling of multiple email addresses are helpful for future maintainers.
151-154: Add defensive check for empty addresses arrayThe code correctly handles the group case by using the first address, but there's a potential edge case if the addresses array is defined but empty.
if (firstSender.type === "group") { - const email = firstSender.addresses[0]?.address || FALLBACK_SENDER.email + const email = firstSender.addresses?.length ? (firstSender.addresses[0]?.address || FALLBACK_SENDER.email) : FALLBACK_SENDER.email return {name, email} }
156-158: Add defensive check for missing address propertyFor completeness, consider adding a fallback for the unlikely case where the address property is undefined.
- const email = firstSender.address + const email = firstSender.address || FALLBACK_SENDER.email
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
apps/mail/app/api/driver/google.ts(3 hunks)apps/mail/components/mail/mail-list.tsx(1 hunks)apps/mail/components/mail/mail.tsx(2 hunks)apps/mail/lib/email-utils.ts(2 hunks)apps/mail/package.json(4 hunks)
🔇 Additional comments (8)
apps/mail/components/mail/mail-list.tsx (1)
64-64: Good spacing adjustment for consistency.The spacing in the object literal has been updated for better code style consistency.
apps/mail/components/mail/mail.tsx (2)
99-99: CSS class reordering looks good.Reordering CSS classes for better readability. This is a stylistic change that doesn't affect functionality.
134-134: CSS class reordering looks good.Similar to the previous change, this is a stylistic improvement that reorders CSS classes without changing their effect.
apps/mail/app/api/driver/google.ts (2)
6-6: Good addition of parseFrom import.Adding import for the new utility function that will handle RFC 2822 email parsing.
141-141: Great improvement in From header parsing.Replacing manual string splitting with a dedicated parsing function that handles RFC 2822 format correctly. This should properly address the bug where From headers containing only an email address were incorrectly displayed.
apps/mail/package.json (2)
74-74: Good addition of email-addresses dependency.Adding the email-addresses package to support proper RFC 2822 compliant parsing of email headers.
17-17: Package dependency adjustments look good.The reordering and addition of various dependencies supports the changes made in the codebase while maintaining compatibility.
Also applies to: 49-49, 55-55, 79-79
apps/mail/lib/email-utils.ts (1)
3-3: Good import renaming strategyRenaming the library's
parseFromto_parseFromclearly distinguishes it from the wrapper function you've created. This prevents namespace conflicts and indicates the internal usage relationship.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/mail/lib/email-utils.ts (2)
140-159: Well-structured RFC 2822 parsing implementationThe implementation of
parseFromis thorough and handles multiple scenarios correctly, including:
- Failed parsing
- Missing sender information
- Group-type senders (as defined in RFC 2822)
- Regular individual senders
The function's comment about multiple email addresses is helpful context that explains your design decision.
Consider adding TypeScript return type annotations for better type safety:
-export const parseFrom = (fromHeader: string) => { +export const parseFrom = (fromHeader: string): { name: string; email: string } => {
152-152: Consider simplifying the conditional expressionThis line handles several nested conditions which could be simplified for better readability.
- const email = firstSender.addresses?.length ? (firstSender.addresses[0]?.address || FALLBACK_SENDER.email) : FALLBACK_SENDER.email; + const firstAddress = firstSender.addresses?.[0]?.address; + const email = firstAddress || FALLBACK_SENDER.email;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/lib/email-utils.ts(2 hunks)
🔇 Additional comments (3)
apps/mail/lib/email-utils.ts (3)
3-3: Good addition of email parsing libraryThe import of
parseFromfrom the "email-addresses" library is a solid choice for standardized email address parsing, aligning well with the PR's objective to handle theFromheader according to RFC 2822.
135-138: Appropriate fallback valuesThe
FALLBACK_SENDERconstant defines reasonable default values for cases where sender information cannot be parsed, which is good defensive programming.
144-146: Great documentation of RFC understandingThe comment explaining that the
Fromheader can technically contain multiple email addresses according to RFC 2822 shows attention to detail and standards compliance, which aligns perfectly with the PR's objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/mail/lib/email-utils.ts (2)
140-160: Consider adding TypeScript type definitions for the return value.The implementation correctly handles various edge cases when parsing the "From" header according to RFC 2822, which should fix the reported UI bug. However, adding an explicit interface or type for the return value would improve type safety and documentation.
+interface SenderInfo { + name: string; + email: string; +} + +const FALLBACK_SENDER: SenderInfo = { const FALLBACK_SENDER = { name: "No Sender Name", email: "no-sender@unknown" } -export const parseFrom = (fromHeader: string) => { +export const parseFrom = (fromHeader: string): SenderInfo => {
151-155: Fix indentation for consistency.The indentation in this block uses 3 spaces instead of 2 spaces used elsewhere in the file.
if (firstSender.type === "group") { - const firstAddress = firstSender.addresses?.[0]?.address; - const email = firstAddress || FALLBACK_SENDER.email; - return { name, email }; + const firstAddress = firstSender.addresses?.[0]?.address; + const email = firstAddress || FALLBACK_SENDER.email; + return { name, email }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/lib/email-utils.ts(2 hunks)
🔇 Additional comments (3)
apps/mail/lib/email-utils.ts (3)
1-1: Good choice importing and renaming the external library function.Using
_parseFromfor the imported function while exporting your ownparseFromimplementation is a clean approach that avoids name conflicts while making your API clear.
135-138: FALLBACK_SENDER provides good defaults for error cases.Creating a constant for fallback values ensures consistency across the codebase when parsing fails. This is a good defensive programming practice.
144-146: Good documentation on RFC 2822 compliance.The comment explaining why you're only processing the first sender is valuable for future maintainers to understand the design decision, especially since it references the relevant RFC standard.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Description
This PR makes the
Fromheader parsing match RFC 2822. It can technically hold multiple email addresses, but since that’s not used in practice, we just grab the first one.This fixes a bug I had where a
Fromheader with just an email address and no name showed up wrong in the UI.Important
When testing this PR, make sure to clear your local Dexie/SWC cache. We don’t revalidate messages after fetching them, so you won’t have the newly parsed sender.
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Testing Done
Describe the tests you've done:
Security Considerations
For changes involving data or authentication:
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit