Domains, messages, attachments, tests #5
Conversation
RUP-884 Email Agent MVP
|
WalkthroughAdds domain management with DNS MX resolution and verification, expands message model and APIs to support multi-recipient (to/cc/bcc), labels and embedded attachments, switches SES sending to raw MIME via nodemailer, adds inbox deletion with cascading message removal, and exposes domain APIs in SDK and routes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant DomainsRoute as Domains Router
participant DomainCtrl as DomainController
participant DNSCtrl as DNSController
participant MongoDB
Client->>DomainsRoute: POST /v1/domains/{name}/verify
DomainsRoute->>DomainCtrl: getVerifiedDomainByOrganizationIdAndName(org, name)
DomainCtrl->>MongoDB: findOne(orgId, name)
MongoDB-->>DomainCtrl: domain doc
rect rgba(200,220,255,0.6)
Note over DomainCtrl,DNSCtrl: Resolve MX records
DomainCtrl->>DNSCtrl: getDNSMXRecords(domain.name)
DNSCtrl->>DNSCtrl: dns.promises.resolveMx(domain)
DNSCtrl-->>DomainCtrl: MX records[]
end
rect rgba(220,255,220,0.6)
Note over DomainCtrl,MongoDB: Compare & persist verification
DomainCtrl->>DomainCtrl: compare MX vs domain.records
DomainCtrl->>MongoDB: update(domain.verified, records.status)
MongoDB-->>DomainCtrl: updated domain
end
DomainCtrl-->>Client: 200 OK (verification result)
sequenceDiagram
autonumber
participant Client
participant MsgRoute as Messages Router
participant MsgCtrl as MessageController
participant SESCtrl as SESController
participant MailComp as Nodemailer MailComposer
participant AWSSES
participant MongoDB
Client->>MsgRoute: POST /v1/inboxes/{id}/messages/send (to:[], cc:[], bcc:[], attachments:[])
MsgRoute->>MsgCtrl: createMessage(payload)
MsgCtrl->>MongoDB: save message (to/cc/bcc/labels/attachments)
MongoDB-->>MsgCtrl: saved message
rect rgba(255,240,200,0.6)
Note over MsgCtrl,SESCtrl: Build MIME & send raw
MsgCtrl->>SESCtrl: sendSESMessage({to,cc,bcc,attachments,...})
SESCtrl->>MailComp: build MIME message (attachments)
MailComp-->>SESCtrl: MIME buffer
SESCtrl->>AWSSES: SendRawEmailCommand(RawMessage)
AWSSES-->>SESCtrl: send result
end
SESCtrl-->>MsgCtrl: send result
MsgRoute-->>Client: 200 OK (message created)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/tests/MessageController.spec.ts (1)
56-56: Fix the assertion to match the array-basedtofield.The assertion compares
message.to(now astring[]) withtoEmail(astring). This test will fail because the types don't match.- expect(message.to).toBe(toEmail); + expect(message.to).toEqual([toEmail]);api/routes/v1/inboxes/messages.ts (2)
110-138: Critical: Sanitize query parameter before regex operations.The
queryparameter is passed directly togetMessages, which uses it in MongoDB$regexoperations without sanitization. This creates multiple security and stability risks:
- ReDoS vulnerability: Malicious regex patterns (e.g.,
(a+)+$) can cause exponential backtracking and DoS- Query injection: Unsanitized input in regex can cause unexpected behavior
- Application crashes: Invalid regex syntax will throw runtime errors
Apply this diff to add basic sanitization:
+ // Sanitize query parameter + const sanitizedQuery = req.query?.query + ? req.query.query.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + : undefined; + const messages = await getMessages({ inboxId: req.params.inboxId, - query: req.query?.query, + query: sanitizedQuery, });Alternatively, consider using a proper text search index instead of regex for better performance and security.
21-108: Add input validation middleware and error handling to the /send route.The route currently lacks critical validation:
toarray can be empty or contain invalid email addresses—no validation before sendingccandbccfields accepted without format validation- Attachment structure (content, contentType) not validated
- Route has no try-catch, so SES failures would crash unhandled
Add validation middleware (e.g., express-validator matching the pattern in
api/routes/auth.ts) or inline checks to:
- Require
toarray with at least one valid email- Validate all recipient email formats
- Validate attachment objects have required fields
- Wrap the async handler in try-catch for graceful error responses
Note:
api/controllers/MessageController.tscontains unresolved merge conflict (lines 13–18) that should be resolved first.
🧹 Nitpick comments (9)
api/db/mongo/schemas/Domain.ts (1)
19-29: Consider stronger typing for the records field.The
recordsfield usesmongoose.Schema.Types.Mixed, which provides flexibility but sacrifices type safety. Consider defining a dedicated schema or interface for domain records to enforce structure and enable better validation.Apply this diff to add stronger typing:
+const recordSchema = new mongoose.Schema({ + type: { type: String, required: true }, + name: { type: String, required: true }, + value: { type: String, required: true }, + status: { type: String, enum: ['pending', 'verified', 'failed'], default: 'pending' }, +}, { _id: false }); + const domainSchema = new mongoose.Schema( { // ... other fields records: { - type: [mongoose.Schema.Types.Mixed], + type: [recordSchema], default: [ { type: "MX", name: "@", value: "inbound-smtp.us-east-2.amazonaws.com", status: "pending", }, ], }, }, // ... );node-sdk/tests/domainAPI.spec.ts (2)
10-18: Consider cleanup after test execution.The
createDomaintest does not clean up the created domain, which could lead to test pollution and failures on subsequent runs due to the unique constraint on domain names.Add cleanup logic:
it("should create a domain", async function () { const domainName = faker.internet.domainName(); const sendook = new Sendook(process.env.API_KEY, process.env.API_URL); const domain = await sendook.domain.create({ name: domainName, }); expect(domain).toBeDefined(); expect(domain.name).toBe(domainName); + // Cleanup + await sendook.domain.delete({ domainId: domain.name }); });
24-33: Empty catch block swallows errors silently.The empty catch block on line 33 suppresses all errors during domain cleanup, which could hide legitimate issues during test execution. Consider logging the error or at least adding a comment explaining why it's safe to ignore.
- } catch (error) { } + } catch (error) { + // Ignore error if domain doesn't exist + }api/routes/v1/domains/index.ts (1)
94-94: Removeanytype for better type safety.Line 94 uses
anyfordnsRecord, which bypasses TypeScript's type checking. ThegetDNSMXRecordsfunction returnsMxRecord[]from Node's dns module, which has a known structure.Apply this diff:
+import type { MxRecord } from "dns"; // In the verification logic: - mxRecords.find((dnsRecord: any) => { + mxRecords.find((dnsRecord: MxRecord) => { return ( typeof dnsRecord.exchange === "string" && dnsRecord.exchange.toLowerCase() === domainRecord.value.toLowerCase() ); })api/controllers/SESController.ts (1)
68-86: Replaceanytype for mailOptions.The
mailOptionsvariable is typed asany, which bypasses type checking. Consider using the proper type from nodemailer.+import type { Options as MailOptions } from 'nodemailer/lib/mailer'; - const mailOptions: any = { + const mailOptions: Partial<MailOptions> = {api/controllers/MessageController.ts (2)
85-97: Potential performance issue with multiple regex queries.The
getMessagesfunction performs an$orquery with 7 regex searches across different fields. This could be slow on large message collections without proper indexes, and regex searches don't use indexes efficiently.Consider:
- Adding text indexes to the Message schema for full-text search
- Limiting the fields searched or making query more specific
In
api/db/mongo/schemas/Message.ts, add:messageSchema.index({ subject: 'text', text: 'text', html: 'text', labels: 'text' });Then update the query:
export async function getMessages({ inboxId, query }: { inboxId: string, query?: string }) { + if (!query) { + return await Message.find({ inboxId: new mongoose.Types.ObjectId(inboxId) }); + } + return await Message.find({ inboxId: new mongoose.Types.ObjectId(inboxId), - $or: [ - { subject: { $regex: query, $options: 'i' } }, - { text: { $regex: query, $options: 'i' } }, - { html: { $regex: query, $options: 'i' } }, - { to: { $regex: query, $options: 'i' } }, - { cc: { $regex: query, $options: 'i' } }, - { bcc: { $regex: query, $options: 'i' } }, - { labels: { $regex: query, $options: 'i' } }, - ] + $text: { $search: query } }); }
62-75: Simplify attachment initialization.The attachment initialization (lines 62-75) creates a new
mongoose.Types.Arrayand manually pushes each attachment. This can be simplified by directly assigning the array.- message.attachments = new mongoose.Types.Array<{ - content: string; - name: string; - contentType: string; - }>(); - if (attachments) { - for (const attachment of attachments) { - message.attachments.push({ - content: attachment.content, - name: attachment.name, - contentType: attachment.contentType, - }); - } - } + message.attachments = attachments?.map(att => ({ + content: att.content, + name: att.name ?? '', + contentType: att.contentType ?? 'application/octet-stream', + })) ?? [];api/controllers/DomainController.ts (2)
48-52: Simplify redundant property shorthand.Line 50 uses
name: namewhich can be shortened to justnameusing ES6 object property shorthand.Apply this diff:
return await Domain.findOne({ organizationId: new mongoose.Types.ObjectId(organizationId), - name: name, + name, verified: true, });
62-65: Simplify redundant property shorthand.Line 64 uses
name: namewhich can be shortened to justnameusing ES6 object property shorthand.Apply this diff:
return await Domain.findOneAndDelete({ organizationId: new mongoose.Types.ObjectId(organizationId), - name: name, + name, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
api/controllers/DNSController.ts(1 hunks)api/controllers/DomainController.ts(1 hunks)api/controllers/InboxController.ts(1 hunks)api/controllers/MessageController.ts(4 hunks)api/controllers/SESController.ts(3 hunks)api/db/mongo/schemas/Domain.ts(1 hunks)api/db/mongo/schemas/Message.ts(2 hunks)api/models/Attachment.ts(0 hunks)api/models/Domain.ts(1 hunks)api/models/Message.ts(1 hunks)api/package.json(2 hunks)api/routes/v1/domains/index.ts(1 hunks)api/routes/v1/inboxes/index.ts(3 hunks)api/routes/v1/inboxes/messages.ts(6 hunks)api/routes/v1/index.ts(1 hunks)api/tests/MessageController.spec.ts(2 hunks)node-sdk/index.ts(3 hunks)node-sdk/tests/domainAPI.spec.ts(1 hunks)node-sdk/tests/messageAPI.spec.ts(4 hunks)node-sdk/tests/threadAPI.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- api/models/Attachment.ts
🧰 Additional context used
🧬 Code graph analysis (5)
api/controllers/InboxController.ts (1)
api/models/Inbox.ts (1)
Inbox(3-11)
api/routes/v1/domains/index.ts (4)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/DomainController.ts (4)
getDomainsByOrganizationId(18-26)createDomain(4-16)getDomainByOrganizationIdAndName(28-39)deleteDomainByOrganizationIdAndName(55-66)api/middlewares/expressValidatorMiddleware.ts (1)
expressValidatorMiddleware(4-16)api/controllers/DNSController.ts (1)
getDNSMXRecords(4-15)
api/controllers/DomainController.ts (1)
api/models/Domain.ts (1)
Domain(3-15)
api/controllers/MessageController.ts (1)
api/models/Message.ts (1)
Message(12-36)
api/routes/v1/inboxes/messages.ts (2)
api/controllers/SESController.ts (1)
sendSESMessage(37-106)api/controllers/MessageController.ts (1)
getMessages(85-98)
🪛 Biome (2.1.2)
api/controllers/MessageController.ts
[error] 32-32: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 32-32: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 33-33: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 33-33: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 34-34: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 34-34: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 35-35: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 35-35: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 36-36: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 40-40: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 41-41: expected , but instead found ;
Remove ;
(parse)
[error] 42-42: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 43-43: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
🔇 Additional comments (20)
api/controllers/InboxController.ts (1)
45-56: LGTM!The delete function follows the established pattern of other functions in this controller, correctly converts both IDs to ObjectId, and uses the appropriate
findOneAndDeletemethod.api/package.json (1)
12-12: LGTM!The nodemailer dependencies are correctly added to support enhanced email sending capabilities with attachments.
Also applies to: 33-33
node-sdk/tests/threadAPI.spec.ts (1)
18-18: LGTM!The test data correctly reflects the updated message API that now accepts
toas an array of email addresses.Also applies to: 37-37
api/tests/MessageController.spec.ts (1)
47-47: LGTM!The message creation and SES sending correctly use the array format for the
tofield.Also applies to: 64-64
api/routes/v1/index.ts (1)
3-3: LGTM!The domains router is correctly imported and mounted, following the same pattern as the existing inboxes router.
Also applies to: 8-8
api/db/mongo/schemas/Message.ts (2)
44-55: LGTM!The new optional fields (
cc,bcc,labels) are correctly defined as string arrays, enabling multi-recipient support and message labeling functionality.
68-75: LGTM!Refactoring attachments from referenced documents to embedded objects simplifies the data model and reduces the need for joins. The structure with
content,name, andcontentTypefields is appropriate for inline attachment storage.api/models/Domain.ts (1)
8-12: LGTM!The
recordsfield provides a clear structure for tracking DNS records (e.g., MX records) with their verification status, supporting the domain verification flow.api/db/mongo/schemas/Domain.ts (1)
36-36: LGTM! Unique index prevents duplicate domains.The composite unique index on
(organizationId, name)correctly ensures that each organization can only have one domain with a given name, preventing data integrity issues.node-sdk/tests/domainAPI.spec.ts (1)
21-42: Hardcoded domain may cause test brittleness.The test uses a hardcoded domain
"marcwhitbread.com"which requires actual DNS configuration to pass verification. This makes the test environment-dependent and may fail in CI/CD pipelines or other developers' environments.Consider either:
- Mocking the DNS verification in tests
- Using a test domain that's guaranteed to be configured
- Documenting the DNS requirements for running these tests
Do the DNS records for
marcwhitbread.comexist in all test environments?api/routes/v1/inboxes/index.ts (1)
50-61: LGTM! Domain validation prevents invalid inbox creation.The domain validation flow correctly checks if a verified domain exists before allowing inbox creation, returning a 404 if the domain is not found. This ensures data integrity.
node-sdk/tests/messageAPI.spec.ts (2)
18-18: LGTM! Recipient field updated to array format.The tests correctly updated the
tofield from a string to an array of strings, aligning with the new Message model structure that supports multiple recipients.Also applies to: 35-35, 54-54, 73-73
40-40: List method signature is correct and properly used in test.The verification confirms that the
listmethod signature innode-sdk/index.ts(line 163) correctly accepts an optionalquery?: stringparameter as the second argument. The test usage at line 40 properly passes"Test"as the query filter, which aligns perfectly with the actual API signature.api/routes/v1/domains/index.ts (1)
30-46: LGTM! Domain creation with proper validation.The POST route correctly validates the domain name and creates the domain with proper authentication and organization context.
api/controllers/SESController.ts (1)
64-106: LGTM! Proper implementation of MIME-based email with attachments.The switch from
SendEmailCommandtoSendRawEmailCommandwith MIME message construction is the correct approach for supporting attachments in SES. The implementation properly handles multiple recipients, attachments, and custom headers.api/controllers/MessageController.ts (1)
54-75: Verify cc, bcc, and labels defaults align with model.The code defaults
to,cc,bcc, andlabelsto empty arrays ([]) when not provided. However, the Message interface (api/models/Message.ts) definescc,bcc, andlabelsas required (non-optional) fields. Ensure the model is updated to make these optional, or update the initialization logic.This relates to the comment made on
api/models/Message.tsabout field optionality. The controller assumes optional fields, but the model defines them as required. Confirm that the model changes are applied consistently.node-sdk/index.ts (3)
99-106: LGTM!The inbox deletion method follows the consistent pattern used throughout the SDK.
108-141: LGTM!The enhanced message sending signature correctly supports multiple recipients and additional email fields (cc, bcc, labels). The type definitions properly reflect these as arrays.
163-173: LGTM!The optional query parameter is properly typed and correctly passed via the
paramsobject for GET requests.api/routes/v1/inboxes/messages.ts (1)
194-204: LGTM!The change from a single
tovalue to an array[replyToMessage.from]correctly aligns with the new array-based recipient fields while maintaining the reply-to-sender logic.
| export async function getDNSMXRecords({ | ||
| domain, | ||
| }: { | ||
| domain: string; | ||
| }): Promise<MxRecord[]> { | ||
| try { | ||
| return await resolveMx(domain); | ||
| } catch (error) { | ||
| console.error('Error resolving MX records:', error); | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
Error handling may mask DNS lookup failures.
Returning an empty array on error makes it impossible for callers to distinguish between legitimate scenarios (no MX records configured) and transient failures (network issues, DNS timeouts). This could lead to false negatives in domain verification flows, where a temporary DNS failure is interpreted as "domain has no MX records."
Consider either:
- Rethrowing the error and letting callers handle it appropriately, or
- Returning a result object like
{ success: boolean, records: MxRecord[], error?: Error }
-export async function getDNSMXRecords({
- domain,
-}: {
- domain: string;
-}): Promise<MxRecord[]> {
- try {
- return await resolveMx(domain);
- } catch (error) {
- console.error('Error resolving MX records:', error);
- return [];
- }
-}
+export async function getDNSMXRecords({
+ domain,
+}: {
+ domain: string;
+}): Promise<MxRecord[]> {
+ return await resolveMx(domain);
+}🤖 Prompt for AI Agents
In api/controllers/DNSController.ts around lines 4 to 15, the current catch
swallows DNS lookup errors by returning an empty array which conflates "no MX
records" with transient failures; change the function to return a discriminated
result object (e.g. Promise<{ success: boolean; records: MxRecord[]; error?:
Error }>) instead of MxRecord[]: on success return { success: true, records },
and on failure return { success: false, records: [], error } (or alternatively
rethrow the caught error if callers should handle it); update the function
signature, adjust the catch block to populate the error field rather than
returning [], and update all callers to handle the new result shape.
| export async function createDomain({ | ||
| organizationId, | ||
| name, | ||
| }: { | ||
| organizationId: string; | ||
| name: string; | ||
| }) { | ||
| const domain = new Domain(); | ||
| domain.organizationId = new mongoose.Types.ObjectId(organizationId); | ||
| domain.name = name; | ||
| await domain.save(); | ||
| return domain; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Add input validation and error handling.
The function lacks:
- Validation for empty or invalid
namevalues - Error handling for invalid
organizationId(ObjectId conversion can throw) - Explicit duplicate checking (though the unique index will enforce this at the DB level, returning a more user-friendly error would improve the API experience)
Consider adding validation and try-catch blocks for database operations.
Run the following script to check if validation is handled at the route layer:
🏁 Script executed:
#!/bin/bash
# Check if domain routes include validation middleware or error handling
fd -e ts -e js "domains" api/routes | xargs rg -A 10 "createDomain|validation|validate"Length of output: 3631
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full POST route handler that creates a domain
rg -B 5 -A 20 'router.post.*function|router\.post' api/routes/v1/domains/index.ts | head -80
Length of output: 1227
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for global error handling middleware
rg -n "error|catch|handler|middleware" api/middlewares/ | head -30
Length of output: 266
Add error handling for database operations.
Input validation is already handled at the route layer via express-validator (body("name").isString().notEmpty().trim()). However, the createDomain() function lacks error handling for:
- Database save failures (e.g., connection errors, validation errors from schema)
- Duplicate domain index violations (should return a user-friendly error instead of an unhandled exception)
Wrap the domain.save() operation in a try-catch block to handle these cases gracefully.
🤖 Prompt for AI Agents
In api/controllers/DomainController.ts around lines 4 to 16, the createDomain
function performs domain.save() without any error handling; wrap the save call
in a try-catch, catch Mongoose validation errors and duplicate-key errors
(E11000) specifically and throw/return a clear, user-friendly error (e.g.,
"Domain already exists" for duplicate index) while logging the original error
for diagnostics, and rethrow or convert other DB errors into a generic
persistence error so callers can respond appropriately.
| attachments: (attachments ?? []).map(att => ({ | ||
| filename: att.name, | ||
| content: att.content, | ||
| contentType: att.contentType, | ||
| encoding: "base64", | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add attachment size validation.
The code maps attachments without validating their size. Since attachments are base64-encoded and stored in MongoDB (16MB document limit), large attachments could cause failures. Add validation to prevent oversized attachments.
Add validation before the MailComposer:
+ // Validate total attachment size (MongoDB 16MB limit, leave buffer for other fields)
+ const MAX_ATTACHMENT_SIZE = 10 * 1024 * 1024; // 10MB
+ const totalAttachmentSize = (attachments ?? []).reduce((sum, att) => {
+ // Base64 encoded size is ~4/3 of original
+ const decodedSize = (att.content.length * 3) / 4;
+ return sum + decodedSize;
+ }, 0);
+
+ if (totalAttachmentSize > MAX_ATTACHMENT_SIZE) {
+ throw new Error(`Total attachment size exceeds ${MAX_ATTACHMENT_SIZE} bytes`);
+ }
const mailOptions: any = {
// ... rest of the codeCommittable suggestion skipped: line range outside the PR's diff.
| type: [String], | ||
| required: true, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify consistency between schema and TypeScript interface.
The schema marks to as required: true, but based on the context from the AI summary, the TypeScript interface shows to?: string[] (optional). This inconsistency between the Mongoose schema and the TypeScript type definition could lead to validation mismatches.
Run this script to check the Message interface definition:
🏁 Script executed:
#!/bin/bash
# Description: Check the Message interface to verify if 'to' is optional or required
# Search for the Message interface definition
rg -n -A 20 "^export (default )?interface Message" api/models/Message.tsLength of output: 585
Fix schema-interface inconsistencies for to, cc, bcc, and labels fields.
The TypeScript interface shows these fields with different optionality than the schema:
to: Optional in interface (to?: string[]) but markedrequired: truein schemacc,bcc,labels: Required in interface but marked optional in schema
These mismatches will cause validation failures. Update the schema to match the interface definitions:
- Either make
torequired in the interface, or removerequired: truefrom schema - Mark
cc,bcc,labelsas required in schema (or optional in interface for consistency)
🤖 Prompt for AI Agents
In api/db/mongo/schemas/Message.ts around lines 41-43, the schema's required
flags don't match the TypeScript interface: `to` is required in the schema but
optional in the interface, while `cc`, `bcc`, and `labels` are optional in the
schema but required in the interface. Fix by making the schema align with the
interface—remove `required: true` from the `to` field, and add `required: true`
to `cc`, `bcc`, and `labels` (alternatively, if you prefer the schema as
source-of-truth, update the interface instead; pick one consistent approach).
api/routes/v1/domains/index.ts
Outdated
| if (mxRecordFound) { | ||
| mxRecordFound.status = "verified"; | ||
| } | ||
| domain.verified = mxRecordFound ? true : false; | ||
| await domain.save(); |
There was a problem hiding this comment.
🧩 Analysis chain
Potential Mongoose change detection issue with nested array mutation.
Directly mutating a property within an array element (mxRecordFound.status = "verified") may not trigger Mongoose's change tracking, especially if records is defined as Mixed type. This could cause the change to not be persisted.
Run the following script to check how the records field is defined in the schema:
If records uses Mixed type, consider using markModified:
if (mxRecordFound) {
mxRecordFound.status = "verified";
+ domain.markModified('records');
}🏁 Script executed:
#!/bin/bash
# Check the Domain schema definition for records field
ast-grep --pattern 'records: {
$$$
}'Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the Domain schema file
fd -e ts -e js | xargs grep -l "records.*:" | grep -i schema
Length of output: 30
🏁 Script executed:
#!/bin/bash
# Look for Domain model/schema definition with broader pattern
rg -n "records\s*:" --type ts --type js -B5 -A5 | head -100Length of output: 1575
🏁 Script executed:
#!/bin/bash
# Check the context in the specific file at lines 104-108
sed -n '95,115p' api/routes/v1/domains/index.tsLength of output: 438
---
🏁 Script executed:
```shell
#!/bin/bash
# Check broader context to see where mxRecordFound comes from
sed -n '85,110p' api/routes/v1/domains/index.ts
Length of output: 709
Records field uses Mixed type - change tracking will not work without markModified().
The records field in the Domain schema is defined as [mongoose.Schema.Types.Mixed], which means Mongoose does not automatically track mutations to nested properties. The direct assignment mxRecordFound.status = "verified" on line 105 will not be persisted when domain.save() is called.
Add domain.markModified('records') after the mutation:
if (mxRecordFound) {
mxRecordFound.status = "verified";
+ domain.markModified('records');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (mxRecordFound) { | |
| mxRecordFound.status = "verified"; | |
| } | |
| domain.verified = mxRecordFound ? true : false; | |
| await domain.save(); | |
| if (mxRecordFound) { | |
| mxRecordFound.status = "verified"; | |
| domain.markModified('records'); | |
| } | |
| domain.verified = mxRecordFound ? true : false; | |
| await domain.save(); |
🤖 Prompt for AI Agents
In api/routes/v1/domains/index.ts around lines 104 to 108, the code mutates a
nested object on the Domain.records array (mxRecordFound.status = "verified")
but records is defined as Mongoose.Mixed so change tracking won’t detect the
mutation; after mutating the nested record set domain.verified as before, call
domain.markModified('records') before await domain.save() so Mongoose persists
the change.
| router.delete( | ||
| "/:inboxId", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string; inboxId: string }, {}, {}>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
| const deletedInbox = await deleteInboxByOrganizationIdAndInboxId({ | ||
| organizationId: organization._id.toString(), | ||
| inboxId: req.params.inboxId, | ||
| }); | ||
| if (!deletedInbox) { | ||
| return res.status(404).json({ error: "Inbox not found" }); | ||
| } | ||
| await deleteMessagesByInboxId(req.params.inboxId); | ||
| await sendWebhookEvent({ | ||
| organizationId: organization._id.toString(), | ||
| event: "inbox.deleted", | ||
| inboxId: deletedInbox._id.toString(), | ||
| payload: deletedInbox, | ||
| }); | ||
| return res.json(deletedInbox); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify thread cleanup on inbox deletion.
The DELETE endpoint deletes the inbox and associated messages, but doesn't explicitly handle thread cleanup. Orphaned threads may remain in the database if they're not cascade-deleted.
Run the following script to check if threads are automatically cleaned up:
🏁 Script executed:
#!/bin/bash
# Search for thread cleanup logic or cascade deletion
rg -nC5 'deleteMessagesByInboxId|Thread.*delete|cascade' --type=ts
# Check if Thread model has any cleanup hooks
ast-grep --pattern 'threadSchema.pre($_, $_)'Length of output: 2219
I need to examine the Message and Thread model definitions to verify the relationship and cleanup behavior.
🏁 Script executed:
#!/bin/bash
# Find model files
echo "=== Finding Message model ==="
fd -e ts -path '*models*' -name '*[Mm]essage*' | head -20
echo "=== Finding Thread model ==="
fd -e ts -path '*models*' -name '*[Tt]hread*' | head -20
# Look for message and thread schema definitions
echo "=== Searching for Message schema ==="
rg -nA15 'const messageSchema|export.*messageSchema' --type=ts
echo "=== Searching for Thread schema ==="
rg -nA15 'const threadSchema|export.*threadSchema' --type=tsLength of output: 2530
🏁 Script executed:
#!/bin/bash
# Look for any thread cleanup or deletion logic
rg -n 'deleteThread|Thread.*deleteMany|thread.*delete|orphan' --type=tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find Thread model definition
echo "=== Searching for Thread model/schema ==="
find . -type f -name "*.ts" | xargs grep -l "threadSchema\|Thread.*model\|export.*Thread" | head -20
echo "=== Looking at Thread schema structure ==="
rg -nA20 'const threadSchema|ThreadSchema.*new' --type=ts
echo "=== Looking for any message or thread cleanup handlers ==="
rg -n 'post.*delete|pre.*delete|findByIdAndDelete' --type=ts | grep -i 'thread\|message'Length of output: 1463
🏁 Script executed:
#!/bin/bash
# Check ThreadController for deletion functions
echo "=== ThreadController.ts - checking for delete functions ==="
cat api/controllers/ThreadController.ts | head -50
# Look for any other cleanup logic during inbox/message deletion
echo "=== Checking InboxController for cleanup logic ==="
rg -nA30 'deleteInboxByOrganizationIdAndInboxId' --type=ts
# Check if there are any cascade delete middleware hooks anywhere
echo "=== Searching for mongoose middleware hooks ==="
rg -n 'schema\.pre|schema\.post' --type=ts
# Search explicitly for thread deletion
echo "=== Any explicit thread cleanup ==="
rg -n 'deleteMany.*[Tt]hread|Thread\.delete|removeThread' --type=tsLength of output: 6717
Add thread cleanup to the inbox deletion flow.
The DELETE endpoint deletes the inbox and messages but omits thread cleanup. Threads remain orphaned in the database with dangling message references. The codebase lacks any deleteThread() or deleteThreadsByInboxId() function—you need to add thread deletion to the cleanup chain.
Add a deleteThreadsByInboxId() function to ThreadController.ts and call it after deleteMessagesByInboxId() in the route, similar to how messages are cleaned up.
🤖 Prompt for AI Agents
In api/routes/v1/inboxes/index.ts around lines 100 to 124, the DELETE inbox flow
removes the inbox and messages but doesn't remove threads, leaving orphaned
thread records; add thread cleanup by implementing a new export
deleteThreadsByInboxId(inboxId: string) in controllers/ThreadController.ts that
deletes all threads with the given inboxId (and any associated thread-message
links if applicable), export it, import it into this inbox route file, and call
await deleteThreadsByInboxId(req.params.inboxId) immediately after await
deleteMessagesByInboxId(req.params.inboxId) so threads are removed as part of
the inbox deletion flow.
| public domain = { | ||
| create: async ({ | ||
| name, | ||
| }: { | ||
| name: string; | ||
| }) => { | ||
| const response = await axios.post(`${this.apiUrl}/v1/domains`, { | ||
| name, | ||
| }, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| get: async ({ | ||
| domainId, | ||
| }: { | ||
| domainId: string; | ||
| }) => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/domains/${domainId}`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| verify: async ({ | ||
| domainId, | ||
| }: { | ||
| domainId: string; | ||
| }) => { | ||
| const response = await axios.post(`${this.apiUrl}/v1/domains/${domainId}/verify`, {}, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| delete: async ({ | ||
| domainId, | ||
| }: { | ||
| domainId: string; | ||
| }) => { | ||
| const response = await axios.delete(`${this.apiUrl}/v1/domains/${domainId}`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify API endpoint consistency between SDK and controller.
The SDK methods domain.get, domain.verify, and domain.delete use domainId in the URL path (e.g., /v1/domains/${domainId}), but the controller functions only accept organizationId and name parameters.
Ensure that the routes layer (api/routes/v1/domains/index.ts) properly handles the conversion from domainId to the required parameters, or that a getDomainById controller function exists.
Run the following script to verify the routes implementation:
🏁 Script executed:
#!/bin/bash
# Check domain routes implementation for endpoint parameter handling
cat api/routes/v1/domains/index.ts
# Also check if there's a getDomainById function
rg -n "getDomainById|function.*domainId" api/controllers/Length of output: 3892
Critical API mismatch confirmed between SDK and backend routes.
The verification reveals a significant issue: the SDK methods pass domainId as a URL parameter, but the routes layer treats this as a domain name and passes it to controller functions that expect (organizationId, name).
Specifically, in the GET /:domainId route:
const domain = await getDomainByOrganizationIdAndName({
organizationId: organization._id.toString(),
name: req.params.domainId, // Treating domainId as name
});This means the SDK parameter should be the domain name, not an actual domain ID. Either:
- Rename the SDK parameter from
domainIdtodomainNamefor clarity, or - Implement backend logic to look up domains by actual ID and update the routes accordingly
The original review comment's concern is valid and requires resolution.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api/controllers/MessageController.ts (1)
13-15: Past merge conflict has been resolved.The merge conflict markers previously flagged in this area have been resolved. The parameters cc, bcc, and labels are now properly integrated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/controllers/MessageController.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/controllers/MessageController.ts (1)
api/models/Message.ts (1)
Message(12-36)
🔇 Additional comments (2)
api/controllers/MessageController.ts (2)
51-54: LGTM: Clean default handling for array fields.The use of nullish coalescing (
?? []) provides sensible defaults for optional array fields.
101-107: LGTM: Delete operations are well-implemented.Both delete functions use appropriate Mongoose methods and correctly handle ObjectId conversions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/routes/v1/inboxes/index.ts (1)
107-120: Orphaned threads left on inbox deletionYou delete the inbox and its messages but never clear the associated thread documents, so every delete leaves orphaned threads pointing at a now-nonexistent inbox. Nothing elsewhere in the codebase cascades that cleanup. Please add thread deletion alongside message cleanup.
@@ -import { deleteMessagesByInboxId } from "../../../controllers/MessageController"; +import { deleteMessagesByInboxId } from "../../../controllers/MessageController"; +import { deleteThreadsByInboxId } from "../../../controllers/ThreadController"; @@ - await deleteMessagesByInboxId(req.params.inboxId); + await deleteMessagesByInboxId(req.params.inboxId); + await deleteThreadsByInboxId(req.params.inboxId);…and implement/export
deleteThreadsByInboxIdinThreadControllerso the route stays in sync.
🧹 Nitpick comments (1)
api/routes/v1/domains/index.ts (1)
15-15: Remove unused import.The
getDNSMXRecordsimport is no longer needed after the DNS verification logic was moved to theverifyDomainDNScontroller function (used on line 86).Apply this diff to remove the unused import:
-import { getDNSMXRecords } from "../../../controllers/DNSController";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/controllers/DomainController.ts(1 hunks)api/controllers/MessageController.ts(3 hunks)api/routes/v1/domains/index.ts(1 hunks)api/routes/v1/inboxes/index.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/controllers/MessageController.ts
- api/controllers/DomainController.ts
🧰 Additional context used
🧬 Code graph analysis (2)
api/routes/v1/domains/index.ts (3)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/DomainController.ts (5)
getDomainsByOrganizationId(20-28)createDomain(6-18)getDomainByOrganizationIdAndName(30-41)verifyDomainDNS(70-98)deleteDomainByOrganizationIdAndName(57-68)api/middlewares/expressValidatorMiddleware.ts (1)
expressValidatorMiddleware(4-16)
api/routes/v1/inboxes/index.ts (3)
api/controllers/DomainController.ts (1)
getVerifiedDomainByOrganizationIdAndName(43-55)api/controllers/InboxController.ts (2)
createInbox(4-24)deleteInboxByOrganizationIdAndInboxId(45-56)api/controllers/MessageController.ts (1)
deleteMessagesByInboxId(120-124)
🔇 Additional comments (6)
api/routes/v1/domains/index.ts (6)
17-17: LGTM!Router setup with
mergeParams: trueis correct for accessing parent route parameters likeorganizationId.
19-29: LGTM!The list domains endpoint correctly authenticates, scopes results to the authenticated organization, and returns the domain list.
50-68: LGTM!The get domain endpoint correctly authenticates, scopes the query to the organization, and handles the not-found case with a proper 404 response.
70-90: LGTM! Previous refactoring concern has been addressed.The DNS verification logic has been successfully extracted to the
verifyDomainDNScontroller function, achieving clean separation of concerns. The route handler now focuses solely on HTTP-level responsibilities.
92-110: LGTM!The delete domain endpoint correctly authenticates, scopes the operation to the organization, and handles the not-found case appropriately.
112-112: LGTM!Standard router export for integration with the v1 routes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
api/routes/v1/domains/index.ts (3)
15-15: Remove unused import.The
getDNSMXRecordsimport is no longer used in this file after the DNS verification logic was extracted to the controller layer. TheverifyDomainDNSfunction now handles DNS lookups internally.Apply this diff to remove the unused import:
-import { getDNSMXRecords } from "../../../controllers/DNSController";
51-51: Return 201 Created for resource creation.The endpoint creates a new domain resource but returns 200 OK. REST best practices recommend returning 201 Created when a resource is successfully created.
Apply this diff:
- return res.json(domain); + return res.status(201).json(domain);
55-73: Consider renaming parameter for clarity.The parameter is named
domainIdbut it actually represents the domain name (used ingetDomainByOrganizationIdAndName). This naming inconsistency could confuse API consumers who might expect an actual ID value.Consider renaming to
domainNamefor clarity:router.get( - "/:domainId", + "/:domainName", passport.authenticate("api_key", { session: false }), async ( - req: Request<{ organizationId: string; domainId: string }, {}, {}>, + req: Request<{ organizationId: string; domainName: string }, {}, {}>, res: Response ) => { const organization = req.user as HydratedDocument<Organization>; const domain = await getDomainByOrganizationIdAndName({ organizationId: organization._id.toString(), - name: req.params.domainId, + name: req.params.domainName, });Note: This change would need to be applied consistently to the verify and delete endpoints as well.
| router.get( | ||
| "/", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async (req: Request<{ organizationId: string }, {}, {}>, res: Response) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
| const domains = await getDomainsByOrganizationId({ | ||
| organizationId: organization._id.toString(), | ||
| }); | ||
| return res.json(domains); | ||
| } | ||
| ); | ||
|
|
||
| router.post( | ||
| "/", | ||
| body("name") | ||
| .isString() | ||
| .trim() | ||
| .notEmpty() | ||
| .matches(/^(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}$/) | ||
| .withMessage("Invalid domain name format"), | ||
| expressValidatorMiddleware, | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string }, {}, { name: string }>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
| const domain = await createDomain({ | ||
| organizationId: organization._id.toString(), | ||
| name: req.body.name, | ||
| }); | ||
|
|
||
| return res.json(domain); | ||
| } | ||
| ); | ||
|
|
||
| router.get( | ||
| "/:domainId", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string; domainId: string }, {}, {}>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
| const domain = await getDomainByOrganizationIdAndName({ | ||
| organizationId: organization._id.toString(), | ||
| name: req.params.domainId, | ||
| }); | ||
|
|
||
| if (!domain) { | ||
| return res.status(404).json({ error: "Domain not found" }); | ||
| } | ||
| return res.json(domain); | ||
| } | ||
| ); | ||
|
|
||
| router.post( | ||
| "/:domainId/verify", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string; domainId: string }, {}, {}>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
| const domain = await getDomainByOrganizationIdAndName({ | ||
| organizationId: organization._id.toString(), | ||
| name: req.params.domainId, | ||
| }); | ||
| if (!domain) { | ||
| return res.status(404).json({ error: "Domain not found" }); | ||
| } | ||
|
|
||
| const { domain: verifiedDomain } = await verifyDomainDNS({ domain }); | ||
|
|
||
| return res.json(verifiedDomain); | ||
| } | ||
| ); | ||
|
|
||
| router.delete( | ||
| "/:domainId", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string; domainId: string }, {}, {}>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
| const deletedDomain = await deleteDomainByOrganizationIdAndName({ | ||
| organizationId: organization._id.toString(), | ||
| name: req.params.domainId, | ||
| }); | ||
| if (!deletedDomain) { | ||
| return res.status(404).json({ error: "Domain not found" }); | ||
| } | ||
|
|
||
| return res.json(deletedDomain); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Add error handling to async route handlers.
None of the async route handlers have try-catch blocks. If any controller method throws an error, it will result in an unhandled promise rejection, potentially crashing the server or leaving requests hanging. Express does not automatically catch errors in async functions unless you're using Express 5+ or a wrapper middleware.
Consider using a wrapper function like express-async-handler or add explicit error handling:
Option 1: Use express-async-handler
Install and import:
import asyncHandler from 'express-async-handler';Then wrap each handler:
router.get(
"/",
passport.authenticate("api_key", { session: false }),
asyncHandler(async (req: Request<{ organizationId: string }, {}, {}>, res: Response) => {
// ... existing code
})
);Option 2: Add explicit try-catch to each handler
router.get(
"/",
passport.authenticate("api_key", { session: false }),
async (req: Request<{ organizationId: string }, {}, {}>, res: Response) => {
try {
const organization = req.user as HydratedDocument<Organization>;
const domains = await getDomainsByOrganizationId({
organizationId: organization._id.toString(),
});
return res.json(domains);
} catch (error) {
// Log error and return appropriate response
console.error('Error fetching domains:', error);
return res.status(500).json({ error: 'Internal server error' });
}
}
);Apply similar patterns to all async route handlers.
🤖 Prompt for AI Agents
In api/routes/v1/domains/index.ts around lines 19 to 115, the async Express
route handlers lack error handling which can lead to unhandled promise
rejections; wrap each async handler with a safe error wrapper (e.g., import and
use express-async-handler's asyncHandler around each async function) or add a
try-catch inside each handler that logs the error and returns a 500 response;
ensure you import asyncHandler if you choose that approach and apply it to all
GET/POST/DELETE handlers in this file so any thrown errors are forwarded to
Express error middleware.
Summary by CodeRabbit
New Features
Enhancements
Tests
Chores