Repository navigation
Inbound Stuff + Email send - #196
Conversation
WalkthroughAdds Email.send for composing and transmitting replies (with attachments) requiring an auth token, adds EmailApi.send client method with OpenTelemetry tracing for POST /email/send, constrains VectorSearchParams generic, and updates tests to access Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Email as Email.send()
participant Resolver as fromDataType
participant Composer as MailComposer
participant Transport as context.email.send
Caller->>Email: send(req, context, to[], email, from?)
alt missing auth token
Email-->>Caller: throw Error (missing email-auth-token)
else token present
Note right of Email: normalize recipients, resolve sender
loop attachments
Email->>Resolver: convert attachment data
Resolver-->>Email: nodemailer attachment
end
Email->>Composer: compose + compile message
Composer-->>Email: compiled message
Email->>Transport: send(agentId, compiledMessage, authToken, messageId)
Transport-->>Email: ack / error
Email-->>Caller: return messageId / throw
end
sequenceDiagram
autonumber
participant Service
participant EmailApi as EmailApi.send()
participant HTTP as POST /email/send
Note right of EmailApi: start OpenTelemetry span, set attrs (agentId, messageId)
Service->>EmailApi: send(agentId, email, authToken, messageId)
EmailApi->>HTTP: POST /email/send (child context, auth)
HTTP-->>EmailApi: 200 OK / non-200
alt 200
EmailApi-->>Service: resolve
else non-200
EmailApi-->>Service: throw Error (status + body)
end
Note right of EmailApi: end span
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/io/email.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(962-982)AgentContext(839-957)src/server/util.ts (1)
fromDataType(188-269)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/io/email.ts (1)
462-536: Consider extracting shared email sending logic.The
sendandsendReplymethods contain significant code duplication (attachment processing, mail compilation, and sending). Consider extracting the common logic into a private helper method to improve maintainability and reduce the risk of inconsistencies.For example, create a helper method like this:
private async sendEmail( context: AgentContext, authToken: string, mailOptions: { from: { name?: string; address: string }; to: string | Address; subject: string; text: string; html?: string; attachments: Attachment[]; inReplyTo?: string; references?: string; } ): Promise<string> { return new Promise<string>((resolve, reject) => { const mail = new MailComposer({ ...mailOptions, date: new Date(), }); const newemail = mail.compile(); newemail.build(async (err, message) => { if (err) { reject(err); } else { try { await context.email.sendReply( context.agent.id, message.toString(), authToken, newemail.messageId() ); resolve(newemail.messageId()); } catch (ex) { reject(ex); } } }); }); }Then both
sendandsendReplycan use this helper with their specific options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/io/email.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(962-982)AgentContext(839-957)src/server/util.ts (1)
fromDataType(188-269)
🔇 Additional comments (1)
src/io/email.ts (1)
497-500: LGTM! Recipient validation properly implemented.The recipient list is now normalized and validated before composing the mail, ensuring at least one non-empty recipient exists. This correctly addresses the concern from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/io/email.ts (1)
462-541: Consider extracting shared email-sending logic.The
sendandsendReplymethods share ~70% of their implementation (auth validation, attachment processing, email building, and error handling). This duplication increases maintenance burden and the risk of inconsistencies.Consider extracting common logic into a private helper method:
private async buildAndSendEmail( authToken: string, context: AgentContext, mailOptions: { from: { name?: string; address: string }; to: string | { name?: string; address?: string }; subject: string; text: string; html?: string; attachments: Attachment[]; inReplyTo?: string; references?: string; } ): Promise<string> { const mail = new MailComposer(mailOptions); const newemail = mail.compile(); return new Promise<string>((resolve, reject) => { newemail.build(async (err, message) => { if (err) { reject(err); } else { try { await context.email.sendReply( context.agent.id, message.toString(), authToken, newemail.messageId() ); resolve(newemail.messageId()); } catch (ex) { reject(ex); } } }); }); }Then both
sendandsendReplycan call this helper after their specific setup logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/io/email.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(962-982)AgentContext(839-957)src/server/util.ts (1)
fromDataType(188-269)
🔇 Additional comments (1)
src/io/email.ts (1)
497-505: LGTM! Validation improvements address past concerns.The recipient and from address validations correctly handle edge cases:
- Recipients are normalized and validated for at least one non-empty entry
- From address is validated to ensure a non-empty value exists
- Both throw clear, descriptive errors
These changes successfully address the issues raised in previous review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/io/email.ts (2)
479-539: Simplify by removing the IIFE wrapper.The IIFE pattern
return (async () => { ... })()adds unnecessary nesting. Since the method is alreadyasync, you can perform the async work directly in the method body and only wrap the callback-basednewemail.build()in a Promise.Apply this diff to simplify the structure:
- return (async () => { - let attachments: Attachment[] = []; - if (reply.attachments) { - attachments = await Promise.all( - reply.attachments.map(async (attachment) => { - const resp = await fromDataType(attachment.data); - return { - filename: attachment.filename, - content: await resp.data.buffer(), - contentType: resp.data.contentType, - contentDisposition: - attachment.contentDisposition ?? ('attachment' as const), - }; - }) - ); - } + let attachments: Attachment[] = []; + if (reply.attachments) { + attachments = await Promise.all( + reply.attachments.map(async (attachment) => { + const resp = await fromDataType(attachment.data); + return { + filename: attachment.filename, + content: await resp.data.buffer(), + contentType: resp.data.contentType, + contentDisposition: + attachment.contentDisposition ?? ('attachment' as const), + }; + }) + ); + } - const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); - if (normalizedTo.length === 0) { - throw new Error('at least one recipient email is required'); - } + const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); + if (normalizedTo.length === 0) { + throw new Error('at least one recipient email is required'); + } - const fromAddress = from?.email ?? this.toEmail(); - if (!fromAddress) { - throw new Error('a valid from email address is required'); - } + const fromAddress = from?.email ?? this.toEmail(); + if (!fromAddress) { + throw new Error('a valid from email address is required'); + } - const mail = new MailComposer({ - date: new Date(), - from: { - name: from?.name ?? context.agent.name, - address: fromAddress, - }, - to: normalizedTo.join(', '), - subject: reply.subject ?? '', - text: reply.text, - html: reply.html, - attachments, - }); - const newemail = mail.compile(); + const mail = new MailComposer({ + date: new Date(), + from: { + name: from?.name ?? context.agent.name, + address: fromAddress, + }, + to: normalizedTo.join(', '), + subject: reply.subject ?? '', + text: reply.text, + html: reply.html, + attachments, + }); + const newemail = mail.compile(); - return new Promise<string>((resolve, reject) => { - newemail.build(async (err, message) => { - if (err) { - reject(err); - } else { - try { - await context.email.sendReply( - context.agent.id, - message.toString(), - authToken, - newemail.messageId() - ); - resolve(newemail.messageId()); - } catch (ex) { - reject(ex); - } - } - }); + return new Promise<string>((resolve, reject) => { + newemail.build(async (err, message) => { + if (err) { + reject(err); + } else { + try { + await context.email.sendReply( + context.agent.id, + message.toString(), + authToken, + newemail.messageId() + ); + resolve(newemail.messageId()); + } catch (ex) { + reject(ex); + } + } }); - })(); + });
462-540: Consider refactoring sendReply to match this pattern.The new
sendmethod avoids the async Promise executor anti-pattern by moving async work outside the Promise constructor, while the existingsendReplymethod (lines 545-617) still uses the anti-pattern with a biome-ignore comment. For consistency and maintainability, consider refactoringsendReplyto follow the same pattern assend.Do you want me to generate the refactored
sendReplyimplementation or open a new issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/io/email.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(962-982)AgentContext(839-957)src/server/util.ts (1)
fromDataType(188-269)
🔇 Additional comments (3)
src/io/email.ts (3)
501-504: Verify the from address fallback logic.The fallback
from?.email ?? this.toEmail()uses the recipient of the incoming email (represented bythis) as the sender address for the new email. This seems semantically unusual sincesendcreates a new email to arbitrary recipients, not a reply to the incoming email.Confirm whether using
this.toEmail()as the default sender is the intended behavior for this use case, or if a different default (e.g., a dedicated agent email address fromcontext) would be more appropriate.
480-499: LGTM! Validation and attachment processing are correct.The recipient validation (lines 496-499) and attachment processing (lines 480-494) properly implement the previous review feedback. The normalization and filtering ensure at least one valid recipient exists before composing the email.
520-538: LGTM! Promise wrapper correctly handles the callback-based API.The Promise wrapper properly converts the callback-based
newemail.build()API to a Promise, with appropriate error handling for both the callback error and async send failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/io/email.ts (1)
473-551: Previous feedback fully addressed; implementation is solid.All critical validations are in place (auth token, recipients, from address), error handling is proper, and the code follows existing patterns in the codebase.
The IIFE wrapper at lines 490-550 adds unnecessary nesting since the outer function is already async. You can simplify by moving the async work directly into the function body:
): Promise<string> { const authToken = req.metadata?.['email-auth-token'] as string; if (!authToken) { throw new Error( 'email authorization token is required but not found in metadata' ); } - return (async () => { - let attachments: Attachment[] = []; - if (reply.attachments) { - attachments = await Promise.all( - reply.attachments.map(async (attachment) => { - const resp = await fromDataType(attachment.data); - return { - filename: attachment.filename, - content: await resp.data.buffer(), - contentType: resp.data.contentType, - contentDisposition: - attachment.contentDisposition ?? ('attachment' as const), - }; - }) - ); - } + let attachments: Attachment[] = []; + if (reply.attachments) { + attachments = await Promise.all( + reply.attachments.map(async (attachment) => { + const resp = await fromDataType(attachment.data); + return { + filename: attachment.filename, + content: await resp.data.buffer(), + contentType: resp.data.contentType, + contentDisposition: + attachment.contentDisposition ?? ('attachment' as const), + }; + }) + ); + } - const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); - if (normalizedTo.length === 0) { - throw new Error('at least one recipient email is required'); - } + const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); + if (normalizedTo.length === 0) { + throw new Error('at least one recipient email is required'); + } - const fromAddress = from?.email ?? this.toEmail(); - if (!fromAddress) { - throw new Error('a valid from email address is required'); - } + const fromAddress = from?.email ?? this.toEmail(); + if (!fromAddress) { + throw new Error('a valid from email address is required'); + } - const mail = new MailComposer({ - date: new Date(), - from: { - name: from?.name ?? context.agent.name, - address: fromAddress, - }, - to: normalizedTo.join(', '), - subject: reply.subject ?? '', - text: reply.text, - html: reply.html, - attachments, - }); - const newemail = mail.compile(); + const mail = new MailComposer({ + date: new Date(), + from: { + name: from?.name ?? context.agent.name, + address: fromAddress, + }, + to: normalizedTo.join(', '), + subject: reply.subject ?? '', + text: reply.text, + html: reply.html, + attachments, + }); + const newemail = mail.compile(); - return new Promise<string>((resolve, reject) => { - newemail.build(async (err, message) => { - if (err) { - reject(err); - } else { - try { - await context.email.sendReply( - context.agent.id, - message.toString(), - authToken, - newemail.messageId() - ); - resolve(newemail.messageId()); - } catch (ex) { - reject(ex); - } - } - }); + return new Promise<string>((resolve, reject) => { + newemail.build(async (err, message) => { + if (err) { + reject(err); + } else { + try { + await context.email.sendReply( + context.agent.id, + message.toString(), + authToken, + newemail.messageId() + ); + resolve(newemail.messageId()); + } catch (ex) { + reject(ex); + } + } }); - })(); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/io/email.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(962-982)AgentContext(839-957)src/server/util.ts (1)
fromDataType(188-269)
🔇 Additional comments (1)
src/io/email.ts (1)
462-472: LGTM! Documentation is clear and complete.The JSDoc properly documents the method's purpose, parameters, return value, and potential exceptions. This addresses previous feedback requesting documentation.
Requires inbound implementation on catalyst.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apis/email.ts(1 hunks)src/io/email.ts(1 hunks)src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.tssrc/apis/email.tssrc/types.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/email.ts
src/apis/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
src/apis/**/*.ts: Do not hardcode generated prompt content (e.g., copyWriter) in source files; load it dynamically
Avoid overly complex TypeScript generics for generated content; prefer simple, maintainable types
Maintain type safety for dynamically loaded content by generating and referencing TypeScript definitions, with proper annotations for require() results
Do not use relative imports to generated artifacts; resolve via absolute node_modules paths or the package entry
Generated content is loaded at runtime, not build time; avoid static imports of generated modules
Prefer bracket notation for accessing slug-named properties with hyphens (e.g., prompts['slug-name'])
Avoid relative require('./generated/_index.js'); resolve absolute paths from process.cwd() into node_modules for generated assets
Files:
src/apis/email.ts
🧬 Code graph analysis (3)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(979-999)AgentContext(856-974)src/server/util.ts (1)
fromDataType(188-269)
src/apis/email.ts (2)
src/router/router.ts (2)
getTracer(60-65)recordException(107-127)src/apis/api.ts (1)
POST(234-251)
src/types.ts (2)
src/router/data.ts (1)
src/io/email.ts (1)
messageId(275-277)
🔇 Additional comments (3)
src/types.ts (1)
480-481: LGTM—Appropriate generic constraint.Constraining
VectorSearchParams<T>toT extends JsonObjectensures type safety for metadata filtering while maintaining backward compatibility with the default type parameter.src/apis/email.ts (1)
7-57: LGTM—Proper OpenTelemetry integration.The implementation correctly creates a child span, sets appropriate attributes, handles success/error cases with proper span status codes, and ensures the span is ended in the finally block.
src/io/email.ts (1)
462-551: LGTM—Well-structured implementation.The method correctly validates inputs (auth token, recipients, from address), processes attachments asynchronously, composes the email with MailComposer, and delivers via
context.email.send. The async IIFE pattern wrapping the Promise constructor is an acceptable approach for handling the callback-basednewemail.buildAPI while keeping async operations clean.
Huijiro
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/io/email.ts (1)
565-637: Async Promise executor anti-pattern remains unresolved.Despite the past review comment being marked as addressed,
sendReplystill usesnew Promise(async (resolve, reject) => ...)(line 581). The same refactoring pattern applied to thesendmethod (moving async work outside the Promise constructor) should be applied here.Apply the same refactoring as suggested in past comments: move attachment processing, MailComposer construction, and mail.compile() outside the Promise constructor, then wrap only the callback-based
newemail.build()in a non-async Promise.Example structure:
// Process attachments first let attachments: Attachment[] = []; if (reply.attachments) { attachments = await Promise.all(...); } // Build mail const mail = new MailComposer({...}); const newemail = mail.compile(); // Wrap only the callback return new Promise<string>((resolve, reject) => { newemail.build(async (err, message) => { // ... existing callback logic }); });
🧹 Nitpick comments (1)
src/io/email.ts (1)
499-559: Simplify: remove unnecessary IIFE wrapper.The IIFE
(async () => { ... })()adds unnecessary nesting. Sincesendis already an async method, you can perform the async operations directly in the method body and return the Promise at the end.Apply this diff to simplify:
- return (async () => { - let attachments: Attachment[] = []; - if (reply.attachments) { - attachments = await Promise.all( - reply.attachments.map(async (attachment) => { - const resp = await fromDataType(attachment.data); - return { - filename: attachment.filename, - content: await resp.data.buffer(), - contentType: resp.data.contentType, - contentDisposition: - attachment.contentDisposition ?? ('attachment' as const), - }; - }) - ); - } - - const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); - if (normalizedTo.length === 0) { - throw new Error('at least one recipient email is required'); - } - - const fromAddress = from?.email ?? this.toEmail; - if (!fromAddress) { - throw new Error('a valid from email address is required'); - } - - const mail = new MailComposer({ - date: new Date(), - from: { - name: from?.name ?? context.agent.name, - address: fromAddress, - }, - to: normalizedTo.join(', '), - subject: reply.subject ?? '', - text: reply.text, - html: reply.html, - attachments, - }); - const newemail = mail.compile(); - - return new Promise<string>((resolve, reject) => { - newemail.build(async (err, message) => { - if (err) { - reject(err); - } else { - try { - await context.email.send( - context.agent.id, - message.toString(), - authToken, - newemail.messageId() - ); - resolve(newemail.messageId()); - } catch (ex) { - reject(ex); - } - } - }); - }); - })(); + let attachments: Attachment[] = []; + if (reply.attachments) { + attachments = await Promise.all( + reply.attachments.map(async (attachment) => { + const resp = await fromDataType(attachment.data); + return { + filename: attachment.filename, + content: await resp.data.buffer(), + contentType: resp.data.contentType, + contentDisposition: + attachment.contentDisposition ?? ('attachment' as const), + }; + }) + ); + } + + const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); + if (normalizedTo.length === 0) { + throw new Error('at least one recipient email is required'); + } + + const fromAddress = from?.email ?? this.toEmail; + if (!fromAddress) { + throw new Error('a valid from email address is required'); + } + + const mail = new MailComposer({ + date: new Date(), + from: { + name: from?.name ?? context.agent.name, + address: fromAddress, + }, + to: normalizedTo.join(', '), + subject: reply.subject ?? '', + text: reply.text, + html: reply.html, + attachments, + }); + const newemail = mail.compile(); + + return new Promise<string>((resolve, reject) => { + newemail.build(async (err, message) => { + if (err) { + reject(err); + } else { + try { + await context.email.send( + context.agent.id, + message.toString(), + authToken, + newemail.messageId() + ); + resolve(newemail.messageId()); + } catch (ex) { + reject(ex); + } + } + }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/io/email.ts(6 hunks)src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(972-992)AgentContext(849-967)src/server/util.ts (1)
fromDataType(188-269)
🪛 GitHub Actions: Run Tests
src/io/email.ts
[error] 643-647: Email parsing test failed. The pipeline reported an error related to the subject 'Test Email with Attachment' in email.ts (trace references lines 643-647).
🔇 Additional comments (1)
src/io/email.ts (1)
263-272: LGTM!The updates to
toString()correctly use the new getter-based API, and the new_rawandpayloadgetters provide appropriate access to the internal parsed mail structure.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/io/email.test.ts (1)
142-144: Update test to use getter syntax.The test still attempts to call
attachments()as a method, but it's now a getter property. This will cause the test to fail.Apply this diff to fix the test:
- expect(() => email.attachments()).toThrow( + expect(() => email.attachments).toThrow( 'Invalid attachment headers: missing filename' );
♻️ Duplicate comments (1)
src/io/email.ts (1)
579-635: Refactor remains pending from previous reviews.The async Promise executor anti-pattern identified in previous reviews is still present. While the new
sendmethod demonstrates a better approach (moving async work outside the Promise constructor),sendReplyshould be similarly refactored.
🧹 Nitpick comments (2)
src/io/email.ts (2)
498-558: Remove unnecessary IIFE wrapper.The method wraps its entire body in an immediately-invoked async function expression
(async () => { ... })(), which is redundant since the method itself is already declared asasync. This adds cognitive overhead without providing any benefit.Apply this diff to simplify the code:
- return (async () => { - let attachments: Attachment[] = []; + let attachments: Attachment[] = []; - if (reply.attachments) { + if (reply.attachments) { - attachments = await Promise.all( + attachments = await Promise.all( - reply.attachments.map(async (attachment) => { + reply.attachments.map(async (attachment) => { - const resp = await fromDataType(attachment.data); + const resp = await fromDataType(attachment.data); - return { + return { - filename: attachment.filename, + filename: attachment.filename, - content: await resp.data.buffer(), + content: await resp.data.buffer(), - contentType: resp.data.contentType, + contentType: resp.data.contentType, - contentDisposition: + contentDisposition: - attachment.contentDisposition ?? ('attachment' as const), + attachment.contentDisposition ?? ('attachment' as const), - }; + }; - }) + }) - ); + ); - } + } - const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); + const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); - if (normalizedTo.length === 0) { + if (normalizedTo.length === 0) { - throw new Error('at least one recipient email is required'); + throw new Error('at least one recipient email is required'); - } + } - const fromAddress = from?.email ?? this.toEmail; + const fromAddress = from?.email ?? this.toEmail; - if (!fromAddress) { + if (!fromAddress) { - throw new Error('a valid from email address is required'); + throw new Error('a valid from email address is required'); - } + } - const mail = new MailComposer({ + const mail = new MailComposer({ - date: new Date(), + date: new Date(), - from: { + from: { - name: from?.name ?? context.agent.name, + name: from?.name ?? context.agent.name, - address: fromAddress, + address: fromAddress, - }, + }, - to: normalizedTo.join(', '), + to: normalizedTo.join(', '), - subject: reply.subject ?? '', + subject: reply.subject ?? '', - text: reply.text, + text: reply.text, - html: reply.html, + html: reply.html, - attachments, + attachments, - }); + }); - const newemail = mail.compile(); + const newemail = mail.compile(); - return new Promise<string>((resolve, reject) => { + return new Promise<string>((resolve, reject) => { - newemail.build(async (err, message) => { + newemail.build(async (err, message) => { - if (err) { + if (err) { - reject(err); + reject(err); - } else { + } else { - try { + try { - await context.email.send( + await context.email.send( - context.agent.id, + context.agent.id, - message.toString(), + message.toString(), - authToken, + authToken, - newemail.messageId() + newemail.messageId() - ); + ); - resolve(newemail.messageId()); + resolve(newemail.messageId()); - } catch (ex) { + } catch (ex) { - reject(ex); + reject(ex); - } + } - } + } - }); + }); - }); - })(); + });
493-495: Break long error message to comply with line length limit.The error message exceeds the 80-character line limit specified in the coding guidelines.
As per coding guidelines.
Apply this diff:
throw new Error( - 'email authorization token is required but not found in metadata' + 'email authorization token is required but not found in ' + + 'metadata' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/io/email.ts(5 hunks)test/io/email.test.ts(15 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.tstest/io/email.test.ts
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/io/email.test.ts
🧬 Code graph analysis (2)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(972-992)AgentContext(849-967)src/server/util.ts (1)
fromDataType(188-269)
test/io/email.test.ts (1)
src/io/email.ts (1)
attachments(382-454)
🔇 Additional comments (2)
src/io/email.ts (2)
261-263: LGTM!The
toStringmethod correctly uses the new getter-based API.
265-271: LGTM!The
_rawandpayloadgetters provide appropriate access to the underlying parsed message for advanced use cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/io/email.test.ts (1)
59-60: Remove redundant.toBeDefined()check and avoid double getter access.The
attachmentsgetter always returns an array (empty or populated), neverundefinedornull. The.toBeDefined()check is redundant, and accessing the getter twice causes the validation logic to run twice unnecessarily.Apply this diff to simplify:
- expect(email.attachments).toBeDefined(); const attachments = email.attachments;Also applies to: 85-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/io/email.test.ts(16 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/io/email.test.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
test/io/email.test.ts
🧬 Code graph analysis (1)
test/io/email.test.ts (1)
src/io/email.ts (1)
attachments(382-454)
🔇 Additional comments (1)
test/io/email.test.ts (1)
30-30: LGTM! Clean migration to property-based attachments access.All tests correctly updated to treat
attachmentsas a property getter instead of a method call. Error handling is properly tested withexpect(() => email.attachments).toThrow()on line 142, and all assertions remain intact.Also applies to: 115-115, 142-144, 167-167, 193-193, 218-218, 243-243, 268-268, 293-293, 318-318, 359-359, 396-396, 432-432, 461-461
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/io/email.ts (1)
507-515: Consider validating email address formats.The code validates presence of recipients and from address but doesn't validate email format. Invalid email addresses will cause runtime errors from the downstream email service. Early validation with regex or a validation library would provide clearer error messages to users.
Example validation:
const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); if (normalizedTo.length === 0) { throw new Error('at least one recipient email is required'); } // Validate email format (basic regex) const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; for (const addr of normalizedTo) { if (!emailRegex.test(addr)) { throw new Error(`invalid recipient email address: ${addr}`); } } const fromAddress = from?.email ?? this.toEmail(); if (!fromAddress) { throw new Error('a valid from email address is required'); } if (!emailRegex.test(fromAddress)) { throw new Error(`invalid from email address: ${fromAddress}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/io/email.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(972-992)AgentContext(849-967)src/server/util.ts (1)
fromDataType(188-269)
🔇 Additional comments (3)
src/io/email.ts (3)
462-482: LGTM! Well-documented public API.The JSDoc is comprehensive and the method signature is clear. The method properly declares all parameters with appropriate types and returns a Promise with the message ID.
483-488: LGTM! Auth token validation is appropriate.The early validation of the auth token with a clear error message follows best practices.
490-551: Implementation correctly addresses the async Promise executor anti-pattern.The code uses an IIFE to perform async work (attachment processing, validation, MailComposer setup) before wrapping only the callback-based
newemail.buildAPI in a non-async Promise constructor. This properly avoids the async Promise executor anti-pattern while maintaining correct error propagation.The Promise correctly handles both success and error cases from the callback, and any errors in the outer async work will naturally propagate as a rejected promise.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/io/email.test.ts (2)
59-61: Inconsistent pattern: remove redundant check or apply consistently.This test adds
expect(email.attachments).toBeDefined()before calling the method, but other tests that also expect filtered attachments (lines 193, 218, 243, 268, 293, etc.) callattachments()directly without this check. With strict TypeScript, the type system ensures the method exists at compile time, making the runtime check redundant.Either remove the check here to match the rest of the test suite:
- expect(email.attachments).toBeDefined(); const attachments = email.attachments();Or apply it consistently to all tests expecting 0 attachments.
85-87: Inconsistent pattern: remove redundant check or apply consistently.Same issue as lines 59-61. This defensive check is inconsistent with other tests in the suite and redundant with TypeScript's compile-time type checking.
Apply this diff:
- expect(email.attachments).toBeDefined(); const attachments = email.attachments();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/io/email.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/io/email.test.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
test/io/email.test.ts
🧬 Code graph analysis (1)
test/io/email.test.ts (1)
src/router/data.ts (1)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/io/email.ts (1)
473-551: Implementation is correct with proper validation; consider simplifying the async pattern.The implementation correctly addresses all previous review feedback:
- ✅ Auth token validation (lines 483-488)
- ✅ Recipient validation with normalization (lines 507-510)
- ✅ From address validation (lines 512-515)
- ✅ Async work moved outside Promise constructor (lines 491-529)
- ✅ Proper error propagation
The IIFE wrapper at lines 490-550 (
return (async () => { ... })()) adds an unnecessary layer since the method is alreadyasync. You can simplify by removing the IIFE and having the async operations directly in the method body.Apply this diff to simplify the async pattern:
async send( req: AgentRequest, context: AgentContext, to: string[], email: EmailReply, from?: { name?: string; email?: string; } ): Promise<string> { const authToken = req.metadata?.['email-auth-token'] as string; if (!authToken) { throw new Error( 'email authorization token is required but not found in metadata' ); } - return (async () => { - let attachments: Attachment[] = []; - if (email.attachments) { - attachments = await Promise.all( - email.attachments.map(async (attachment) => { - const resp = await fromDataType(attachment.data); - return { - filename: attachment.filename, - content: await resp.data.buffer(), - contentType: resp.data.contentType, - contentDisposition: - attachment.contentDisposition ?? ('attachment' as const), - }; - }) - ); - } - - const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); - if (normalizedTo.length === 0) { - throw new Error('at least one recipient email is required'); - } - - const fromAddress = from?.email ?? this.toEmail(); - if (!fromAddress) { - throw new Error('a valid from email address is required'); - } - - const mail = new MailComposer({ - date: new Date(), - from: { - name: from?.name ?? context.agent.name, - address: fromAddress, - }, - to: normalizedTo.join(', '), - subject: email.subject ?? '', - text: email.text, - html: email.html, - attachments, - }); - const newemail = mail.compile(); - - return new Promise<string>((resolve, reject) => { - newemail.build(async (err, message) => { - if (err) { - reject(err); - } else { - try { - await context.email.send( - context.agent.id, - message.toString(), - authToken, - newemail.messageId() - ); - resolve(newemail.messageId()); - } catch (ex) { - reject(ex); - } - } - }); - }); - })(); + let attachments: Attachment[] = []; + if (email.attachments) { + attachments = await Promise.all( + email.attachments.map(async (attachment) => { + const resp = await fromDataType(attachment.data); + return { + filename: attachment.filename, + content: await resp.data.buffer(), + contentType: resp.data.contentType, + contentDisposition: + attachment.contentDisposition ?? ('attachment' as const), + }; + }) + ); + } + + const normalizedTo = to.map((addr) => addr.trim()).filter(Boolean); + if (normalizedTo.length === 0) { + throw new Error('at least one recipient email is required'); + } + + const fromAddress = from?.email ?? this.toEmail(); + if (!fromAddress) { + throw new Error('a valid from email address is required'); + } + + const mail = new MailComposer({ + date: new Date(), + from: { + name: from?.name ?? context.agent.name, + address: fromAddress, + }, + to: normalizedTo.join(', '), + subject: email.subject ?? '', + text: email.text, + html: email.html, + attachments, + }); + const newemail = mail.compile(); + + return new Promise<string>((resolve, reject) => { + newemail.build(async (err, message) => { + if (err) { + reject(err); + } else { + try { + await context.email.send( + context.agent.id, + message.toString(), + authToken, + newemail.messageId() + ); + resolve(newemail.messageId()); + } catch (ex) { + reject(ex); + } + } + }); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apis/email.ts(1 hunks)src/io/email.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apis/email.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.ts
🧬 Code graph analysis (1)
src/io/email.ts (2)
src/types.ts (2)
AgentRequest(972-992)AgentContext(849-967)src/server/util.ts (1)
fromDataType(188-269)
🔇 Additional comments (1)
src/io/email.ts (1)
462-472: LGTM! Comprehensive documentation.The JSDoc is complete and clearly describes the method's purpose, parameters, return value, and error conditions. This addresses the previous review feedback.
2 participants
Summary by CodeRabbit
New Features
API
Public API
Types
Tests