inbounds and parsing #3
Conversation
RUP-884 Email Agent MVP
|
WalkthroughMove inbox/message routes under a versioned /v1 router; make inbox email optional with generation fallback; add threads and thread linkage to messages (threadId, externalMessageId, "received" status); refactor SES handling into inbound/outbound handlers; update schemas, controllers, Node SDK, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant SNS as AWS SNS
participant WebhookRoute as /webhooks/ses
participant SESCtrl as SESController
participant InboxCtrl as InboxController
participant ThreadCtrl as ThreadController
participant MsgCtrl as MessageController
participant DB as Database
participant Emit as WebhookEmitter
SNS->>WebhookRoute: POST Notification
WebhookRoute->>SESCtrl: handleDeliveryNotification(notification)
SESCtrl->>SESCtrl: inspect notification & tags
alt inbound (no messageId tag)
SESCtrl->>InboxCtrl: ensure/create inbox (email optional)
SESCtrl->>ThreadCtrl: resolve/create thread (reply refs)
SESCtrl->>MsgCtrl: createMessage(..., status: "received", externalMessageId?)
MsgCtrl->>DB: persist message (with threadId)
SESCtrl->>ThreadCtrl: addMessageToThread(threadId, messageId)
SESCtrl->>Emit: sendWebhookEvent("message.received")
else outbound (has messageId tag)
SESCtrl->>MsgCtrl: getMessageByExternalMessageId/messageId
SESCtrl->>SESCtrl: map SES event => message status/event
SESCtrl->>MsgCtrl: update message.status
SESCtrl->>Emit: sendWebhookEvent(mapped_event)
end
WebhookRoute-->>SNS: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
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)
🧰 Additional context used🧬 Code graph analysis (1)api/routes/v1/inboxes/messages.ts (6)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/models/SES.ts (1)
92-103: Consider making the content field optional.The
contentfield is added as required, but SES notifications don't always include message content (e.g., delivery, bounce, and complaint notifications typically lack content). This could cause runtime errors when processing outbound SES events that don't contain the full message body.Apply this diff to make the field optional:
export type SNSMessage = { notificationType?: NotificationType; eventType?: NotificationType; mail: SNSMessageMail; bounce?: SNSMessageBounce<BounceType>; complaint?: SNSMessageComplaint; delivery?: SNSMessageDelivery; reject?: any; failure?: any; deliveryDelay?: any; - content: string; + content?: string; };
🧹 Nitpick comments (1)
api/tests/InboxController.spec.ts (1)
3-4: Consider adding type definitions instead of using @ts-ignore.Using
@ts-ignoresuppresses type checking for the bun:test import. Consider adding type definitions for bun:test or using@ts-expect-errorwith a comment explaining why types are unavailable.Apply this diff:
-// @ts-ignore -import { expect, beforeAll, afterAll, describe, it } from "bun:test"; +// @ts-expect-error - Bun test types not yet available in @types +import { expect, beforeAll, afterAll, describe, it } from "bun:test";
📜 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 (22)
api/controllers/AuthController.ts(0 hunks)api/controllers/InboxController.ts(1 hunks)api/controllers/MessageController.ts(4 hunks)api/controllers/SESController.ts(3 hunks)api/index.ts(2 hunks)api/models/Message.ts(1 hunks)api/models/SES.ts(1 hunks)api/models/Webhook.ts(1 hunks)api/package.json(2 hunks)api/routes/inboxes.ts(0 hunks)api/routes/organizations.ts(0 hunks)api/routes/organizations/inboxes.ts(0 hunks)api/routes/v1/inboxes/index.ts(1 hunks)api/routes/v1/inboxes/messages.ts(3 hunks)api/routes/v1/index.ts(1 hunks)api/routes/webhooks/ses.ts(0 hunks)api/tests/InboxController.spec.ts(1 hunks)api/tests/MessageController.spec.ts(0 hunks)node-sdk/index.ts(1 hunks)node-sdk/tests/InboxAPI.spec.ts(1 hunks)node-sdk/tests/InboxController.spec.ts(0 hunks)node-sdk/tests/messageAPI.spec.ts(1 hunks)
💤 Files with no reviewable changes (7)
- api/routes/inboxes.ts
- api/routes/organizations/inboxes.ts
- api/routes/organizations.ts
- api/controllers/AuthController.ts
- node-sdk/tests/InboxController.spec.ts
- api/tests/MessageController.spec.ts
- api/routes/webhooks/ses.ts
🧰 Additional context used
🧬 Code graph analysis (6)
api/routes/v1/inboxes/index.ts (4)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/InboxController.ts (3)
getInboxesByOrganizationId(26-30)createInbox(4-24)getInboxByOrganizationIdAndInboxId(32-43)api/middlewares/expressValidatorMiddleware.ts (1)
expressValidatorMiddleware(4-16)api/controllers/WebhookAttemptController.ts (1)
sendWebhookEvent(10-55)
api/controllers/MessageController.ts (1)
api/models/Message.ts (1)
MessageStatus(3-10)
api/tests/InboxController.spec.ts (2)
api/controllers/AuthController.ts (1)
register(21-45)api/controllers/InboxController.ts (1)
createInbox(4-24)
api/controllers/InboxController.ts (1)
api/models/Inbox.ts (1)
Inbox(3-11)
api/routes/v1/inboxes/messages.ts (3)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/InboxController.ts (1)
getInboxByOrganizationIdAndInboxId(32-43)api/controllers/MessageController.ts (1)
createMessage(5-41)
api/controllers/SESController.ts (6)
api/models/SES.ts (1)
SNSMessage(92-103)api/controllers/InboxController.ts (1)
getInboxByEmail(45-47)api/controllers/MessageController.ts (2)
createMessage(5-41)getMessageById(43-45)api/controllers/WebhookAttemptController.ts (1)
sendWebhookEvent(10-55)api/models/Message.ts (1)
MessageStatus(3-10)api/models/Webhook.ts (1)
WebhookEvents(3-13)
🔇 Additional comments (11)
api/package.json (1)
10-11: Dependencies verified as valid and secure.All four packages have been confirmed to exist on npm with no active security vulnerabilities or compatibility issues:
email-reply-parser@2.1.0,mailparser@3.9.0,@types/email-reply-parser@1.4.2, and@types/mailparser@3.4.6are properly specified and ready for use.api/models/Message.ts (1)
3-10: LGTM! The new "received" status is well-positioned.The addition of "received" to the MessageStatus array is correct and semantically positioned between "sent" and "delivered", which aligns well with the message lifecycle for inbound messages.
api/models/Webhook.ts (1)
3-13: LGTM! The new webhook event aligns with the "received" status.The addition of "message.received" to WebhookEvents is consistent with the naming pattern and complements the new "received" status added to MessageStatus.
api/tests/InboxController.spec.ts (1)
17-35: LGTM! The test validates the optional email behavior.The test properly verifies that
createInboxgenerates a random email when none is provided, and all assertions are appropriate for validating the inbox creation flow.api/controllers/MessageController.ts (2)
3-3: LGTM! Type import added for status parameter.The type import is correctly added to support the new optional status parameter.
15-15: LGTM! Status parameter implementation is correct.The optional status parameter is properly typed, threaded through the function signature, and assigned to the message object. The implementation maintains type safety and allows for flexible message creation with or without a status.
Also applies to: 26-26, 38-38
api/controllers/InboxController.ts (1)
13-13: LGTM! Optional email parameter is correctly implemented.The email parameter is now optional, and the fallback to
getNewRandomInboxEmailis properly handled with the logical OR operator. This provides flexibility while maintaining backward compatibility.Also applies to: 21-21
api/routes/v1/index.ts (1)
7-8: The route structure is correct—no conflicts exist.The review comment's concern is based on a false premise. The
messagesRouteris mounted only once at/inboxes/:inboxId/messagesat the top level. The inboxesRouter does not mount it—the relevant line is commented out:// router.use("/:inboxId/messages", messageRouter);The actual route structure is clean and distinct:
/inboxesand its sub-routes (from inboxesRouter)/inboxes/:inboxId/messages(from messagesRouter)No redundancy or conflict occurs. The commented line indicates this alternative was considered but intentionally not used.
Likely an incorrect or invalid review comment.
api/routes/v1/inboxes/index.ts (1)
13-48: Strong org scoping via API key.Casting
req.userto the org doc and driving all fetch/create calls fromorganization._idensures callers can’t enumerate other tenants just by tweaking the path. Nice to see the webhook hooked in right after creation.api/routes/v1/inboxes/messages.ts (1)
29-65: Scoped message creation looks solid.Reusing
req.userto resolve the org keeps every downstream call multi-tenant safe, and the webhook dispatch stays aligned with the message lifecycle.api/controllers/SESController.ts (1)
131-155: Inbound parsing flow looks great.Parsing the raw SES payload and storing a
receivedmessage while firingmessage.receivedkeeps the pipeline symmetrical with outbound events. Nice touch withEmailReplyParserto trim quoted replies.
| import organizationsRouter from "./routes/organizations"; | ||
| import webhooksRouter from "./routes/webhooks"; | ||
| import inboxesRouter from "./routes/inboxes"; | ||
| import v1Router from "./routes/v1/index"; |
There was a problem hiding this comment.
Breaking API change: Document the migration path.
Replacing the /inboxes endpoint with /v1 is a breaking change that will affect existing API consumers. Consider maintaining both endpoints during a transition period or providing clear migration documentation.
Would you like me to generate a migration guide or help implement backward compatibility with a deprecation notice?
🤖 Prompt for AI Agents
In api/index.ts around line 10, the import indicates the router was switched to
/v1 which is a breaking change for existing /inboxes consumers; restore backward
compatibility by mounting the existing v1Router under both routes during a
transition (mount /inboxes to delegate to the same router or a thin proxy that
emits a deprecation response header/body), add a server-side deprecation warning
(e.g., Deprecation and Sunset headers or a JSON notice) for /inboxes, and update
API docs and changelog to include a clear migration path and timeline for
removing /inboxes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/db/mongo/schemas/Thread.ts (1)
16-20: Allow emptymessageson new threads.Right now
messagesis markedrequired: true, and Mongoose treats a required array as invalid if it’s empty. Fresh threads (e.g.createThread()inThreadController.ts) persist with zero messages, so every save trips aValidationError: Thread validation failed: messages: Path "messages" is required. Drop therequiredflag and give the field a default empty array so new threads can be created before the first message arrives.messages: { type: [mongoose.Schema.Types.ObjectId], ref: "Message", - required: true, + default: [], },
♻️ Duplicate comments (1)
api/controllers/SESController.ts (1)
88-88: Remove logging of full notification payload (duplicate concern).This
console.logwas flagged in a previous review. The notification object contains the full SES payload including raw message content and headers, which may include PII. Please remove this log statement.Apply this diff:
- console.log("notification", notification);
🧹 Nitpick comments (5)
api/controllers/MessageController.ts (2)
32-47: Consider validating the status parameter.The
statusparameter is assigned directly without runtime validation. While TypeScript provides compile-time type safety, consider adding runtime validation to ensure the status value is valid, especially if this function might be called from untyped contexts.Apply this diff to add runtime validation:
}) { + if (status && !MessageStatus.includes(status as any)) { + throw new Error(`Invalid status: ${status}`); + } const message = new Message();
53-55: Consider adding pagination to prevent performance issues.
getMessagesByInboxIdreturns all messages for an inbox without pagination. This could become a performance bottleneck as the number of messages grows.api/routes/v1/inboxes/messages.ts (2)
14-29: Add input validation for request body fields.The route accepts
to,subject,text, andhtmlfrom the request body without validation. Consider adding validation to ensure required fields are present and properly formatted (e.g., email format forto).Example validation:
if (!req.body.to || !req.body.subject) { return res.status(400).json({ error: "Missing required fields" }); } if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(req.body.to)) { return res.status(400).json({ error: "Invalid email format" }); }
45-70: Optimize message creation to avoid multiple saves.The message is created (line 45-55), then the
externalMessageIdis updated (lines 67-70), requiring two database writes. Consider refactoring to includeexternalMessageIdin the initialcreateMessagecall once the SES message is sent, or batch the operations differently.Apply this diff to refactor:
- const message = await createMessage({ + const sesMessage = await sendSESMessage({ + messageId: "temp-id", + from: inbox.email, + fromName: inbox.name, + to: req.body.to, + subject: req.body.subject, + text: req.body.text, + html: req.body.html, + }); + + const message = await createMessage({ organizationId: organization._id.toString(), inboxId: req.params.inboxId, threadId: thread.id, fromInboxId: req.params.inboxId, from: inbox.email, to: req.body.to, + externalMessageId: sesMessage.MessageId, subject: req.body.subject, text: req.body.text, html: req.body.html, }); - - const sesMessage = await sendSESMessage({ - messageId: message.id, - from: inbox.email, - fromName: inbox.name, - to: req.body.to, - subject: req.body.subject, - text: req.body.text, - html: req.body.html, - }); - - if (sesMessage.MessageId) { - message.externalMessageId = sesMessage.MessageId; - await message.save(); - }Note: You'll need to handle the messageId tagging differently in
sendSESMessage.node-sdk/index.ts (1)
72-72: Remove or clarify the stray comment.The
//replycomment on line 72 appears to be a leftover TODO or planning note. Either remove it or expand it into proper documentation if it indicates planned functionality.- //reply list: async (inboxId: string) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/controllers/MessageController.ts(1 hunks)api/controllers/SESController.ts(2 hunks)api/controllers/ThreadController.ts(1 hunks)api/db/mongo/schemas/Message.ts(2 hunks)api/db/mongo/schemas/Thread.ts(2 hunks)api/models/Message.ts(2 hunks)api/routes/v1/inboxes/messages.ts(1 hunks)api/tests/MessageController.spec.ts(2 hunks)node-sdk/index.ts(1 hunks)node-sdk/tests/messageAPI.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- node-sdk/tests/messageAPI.spec.ts
🧰 Additional context used
🧬 Code graph analysis (5)
api/controllers/ThreadController.ts (1)
api/models/Thread.ts (1)
Thread(3-10)
api/controllers/MessageController.ts (1)
api/models/Message.ts (2)
MessageStatus(3-10)Message(12-29)
api/tests/MessageController.spec.ts (2)
api/controllers/ThreadController.ts (1)
createThread(4-17)api/controllers/MessageController.ts (1)
createMessage(5-47)
api/routes/v1/inboxes/messages.ts (6)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/InboxController.ts (1)
getInboxByOrganizationIdAndInboxId(32-43)api/controllers/ThreadController.ts (2)
createThread(4-17)addMessageToThread(19-33)api/controllers/MessageController.ts (3)
createMessage(5-47)getMessagesByInboxId(53-55)getMessageById(49-51)api/controllers/SESController.ts (1)
sendSESMessage(36-83)api/controllers/WebhookAttemptController.ts (1)
sendWebhookEvent(10-55)
api/controllers/SESController.ts (6)
api/models/SES.ts (1)
SNSMessage(92-103)api/controllers/InboxController.ts (1)
getInboxByEmail(45-47)api/controllers/MessageController.ts (3)
getMessageByExternalMessageId(57-59)createMessage(5-47)getMessageById(49-51)api/controllers/ThreadController.ts (2)
createThread(4-17)addMessageToThread(19-33)api/models/Message.ts (1)
MessageStatus(3-10)api/models/Webhook.ts (1)
WebhookEvents(3-13)
🔇 Additional comments (7)
api/routes/v1/inboxes/messages.ts (2)
88-105: LGTM!The inbox ownership validation and message retrieval logic are correct. Pagination concerns are addressed at the controller level.
107-120: LGTM!The authorization check correctly validates that the message belongs to the authenticated organization before returning it.
api/controllers/SESController.ts (1)
175-220: LGTM!The outbound SES message handling correctly maps SES event types to message statuses and webhook events, with proper error handling for missing messages.
node-sdk/index.ts (3)
7-10: LGTM!The base URL change combined with explicit
/v1prefixes in endpoint calls maintains the same effective URLs while providing clearer version separation.
12-45: LGTM! Note the breaking change.The restructured inbox API is cleaner and more consistent. The
create, which aligns with the auto-generation feature. Note that this is a breaking change for SDK users who were using the oldcreateInboxmethod.Consider documenting this breaking change in a migration guide or changelog.
46-90: LGTM!The nested message operations follow a consistent pattern with proper authorization headers and clear parameter types.
api/controllers/MessageController.ts (1)
8-8: No issues found—threadId is provided at all call sites.The search found 3 call sites for
createMessage, and all of them passthreadId:
api/tests/MessageController.spec.ts:41withthread.idapi/routes/v1/inboxes/messages.ts:45withthread.idapi/controllers/SESController.ts:147withthreadIdvariableThe breaking change has been properly accommodated across the codebase.
| const message = await createMessage({ | ||
| organizationId: inbox.organizationId.toString(), | ||
| inboxId: inbox.id, | ||
| threadId, | ||
| from: notification.mail.source, | ||
| fromInboxId: fromInboxId?.id, | ||
| to: notification.mail.destination[0], | ||
| toInboxId: inbox.id, | ||
| subject: notification.mail.commonHeaders?.subject, | ||
| text: content.getVisibleText(), | ||
| html: content.getVisibleText(), | ||
| status: "received", | ||
| }); |
There was a problem hiding this comment.
HTML field should contain HTML content, not plain text.
Line 157 sets the html field to content.getVisibleText(), which returns plain text. This should either use the parsed HTML from the email or properly convert the text to HTML.
Consider this fix:
text: content.getVisibleText(),
- html: content.getVisibleText(),
+ html: mail.html || content.getVisibleText(),
status: "received",Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/controllers/SESController.ts around lines 147 to 159, the message html
field is being set to plain text via content.getVisibleText(); change it to use
the parsed HTML from the email (e.g. content.getHtml() / content.getHTML() or
the parser method that returns HTML) and, if no HTML part exists, fall back to a
safe conversion of the visible text (escape HTML entities and replace newlines
with <br/> and wrap in a container) before assigning to html; ensure you use the
HTML-returning method when available and only convert text as a fallback.
| const thread = await createThread({ | ||
| organizationId: organization._id.toString(), | ||
| inboxId: req.params.inboxId, | ||
| }); |
There was a problem hiding this comment.
Creating a new thread for every sent message may be incorrect.
The current implementation creates a new thread for every message sent. This means reply messages won't be associated with the original conversation thread. Consider checking if the message is a reply (e.g., via In-Reply-To or conversation context) and reusing the existing thread instead.
🤖 Prompt for AI Agents
In api/routes/v1/inboxes/messages.ts around lines 40–43, the code
unconditionally creates a new thread for every outgoing message; instead detect
when the incoming message is a reply or belongs to an existing conversation
(check request fields like inReplyTo, parentMessageId, conversationId, or the
In-Reply-To HTTP/email header) and attempt to load the existing thread (e.g.,
findThreadByMessageId/ conversationId/ inReplyTo) and use that threadId when
saving the message; only call createThread when no matching thread is found,
then proceed to attach the message to the resolved thread.
There was a problem hiding this comment.
@marcrupt agree here. this seems to create a new thread for every message.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
api/routes/v1/inboxes/messages.ts (1)
40-43: Creating a new thread for every sent message may be incorrect.The current implementation creates a new thread for every message sent, which means reply messages won't be associated with the original conversation thread. Consider checking if the message is a reply (e.g., via an
inReplyToorthreadIdfield in the request body) and reusing the existing thread instead of always creating a new one.node-sdk/tests/messageAPI.spec.ts (1)
17-17: Critical: Inconsistent inbox identifier persists despite past fix.Despite a previous review comment being marked as addressed in commit f6f05a9, all test methods still use
newInbox._id(lines 17, 34, 40, 53, 59) instead ofnewInbox.id. Sinceinbox.createreturnsid, these calls will passundefined, causing requests to/v1/inboxes/undefined/messagesand all tests to fail.Apply this diff to fix all occurrences:
describe("sendMessage", function () { it("should send a message", async function () { const sendook = new Sendook(process.env.API_KEY, process.env.API_URL); const name = faker.person.fullName(); const newInbox = await sendook.inbox.create({ name }); const message = await sendook.inbox.message.send({ - inboxId: newInbox._id, + inboxId: newInbox.id, to: "marc@rupt.dev", subject: "Test Subject", text: "Test Text", html: "<p>Test HTML</p>", }); describe("listMessages", function () { it("should list messages", async function () { const sendook = new Sendook(process.env.API_KEY, process.env.API_URL); const name = faker.person.fullName(); const newInbox = await sendook.inbox.create({ name }); const newMessage = await sendook.inbox.message.send({ - inboxId: newInbox._id, + inboxId: newInbox.id, to: "marc@rupt.dev", subject: "Test Subject", text: "Test Text", html: "<p>Test HTML</p>", }); - const messages = await sendook.inbox.message.list(newInbox._id); + const messages = await sendook.inbox.message.list(newInbox.id); describe("getMessage", function () { it("should get a message", async function () { const sendook = new Sendook(process.env.API_KEY, process.env.API_URL); const name = faker.person.fullName(); const newInbox = await sendook.inbox.create({ name }); const newMessage = await sendook.inbox.message.send({ - inboxId: newInbox._id, + inboxId: newInbox.id, to: "marc@rupt.dev", subject: "Test Subject", text: "Test Text", html: "<p>Test HTML</p>", }); - const message = await sendook.inbox.message.get(newInbox._id, newMessage._id); + const message = await sendook.inbox.message.get(newInbox.id, newMessage._id);Also applies to: 34-34, 40-40, 53-53, 59-59
🧹 Nitpick comments (5)
api/routes/v1/inboxes/messages.ts (2)
40-84: Consider adding error handling for external operations.Multiple operations (thread creation, message creation, SES sending, webhook dispatch) can fail. Consider wrapping critical operations in try-catch blocks to provide meaningful error responses. Additionally, webhook dispatch failures (lines 77-82) should not block the successful message response.
Example pattern:
try { const thread = await createThread({ ... }); const message = await createMessage({ ... }); const sesMessage = await sendSESMessage({ ... }); if (sesMessage.MessageId) { message.externalMessageId = sesMessage.MessageId; await message.save(); } await addMessageToThread({ ... }); // Fire and forget for webhook sendWebhookEvent({ ... }).catch(err => console.error('Webhook failed:', err) ); return res.json(message); } catch (error) { console.error('Send message failed:', error); return res.status(500).json({ error: 'Failed to send message' }); }
88-105: Consider adding pagination support.This endpoint fetches all messages for an inbox without pagination, which could cause performance issues and large payload sizes for inboxes with many messages. Consider adding query parameters for limit, offset, or cursor-based pagination.
api/routes/v1/inboxes/threads.ts (1)
1-10: Clean up unused imports.Several imports are not used in this file:
createMessage,getMessageById,getMessagesByInboxIdfrom MessageController (line 5)sendWebhookEventfrom WebhookAttemptController (line 6)sendSESMessagefrom SESController (line 7)These appear to be copied from
messages.ts. Remove unused imports or implement the routes that would use them.node-sdk/index.ts (2)
72-72: Address or remove the incomplete comment.The
//replycomment suggests unfinished work. If reply functionality is planned, track it in an issue; otherwise, remove the comment.Do you want me to open an issue to track the reply functionality implementation?
47-59: Make text and html mutually optional to reduce redundancy.Both
textandhtmlare currently required throughout the stack (SDK, controller, schema, and SES sending function). However, the backend code shows redundancy—inhandleInboundSESMessage(SESController.ts, line 160), both are set to the same plain text value. Since email APIs typically accept either format, consider making one optional to avoid forcing callers to provide duplicate content.Changes would be needed in:
node-sdk/index.ts(SDK interface)api/controllers/MessageController.ts(function signature)api/db/mongo/schemas/Message.ts(schema)api/controllers/SESController.ts(sending logic)- Route handlers in
api/routes/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/routes/v1/inboxes/index.ts(1 hunks)api/routes/v1/inboxes/messages.ts(1 hunks)api/routes/v1/inboxes/threads.ts(1 hunks)api/routes/v1/index.ts(1 hunks)node-sdk/index.ts(1 hunks)node-sdk/tests/messageAPI.spec.ts(1 hunks)node-sdk/tests/threadAPI.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/routes/v1/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
api/routes/v1/inboxes/messages.ts (6)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/InboxController.ts (1)
getInboxByOrganizationIdAndInboxId(32-43)api/controllers/ThreadController.ts (2)
createThread(4-17)addMessageToThread(19-33)api/controllers/MessageController.ts (3)
createMessage(5-47)getMessagesByInboxId(53-55)getMessageById(49-51)api/controllers/SESController.ts (1)
sendSESMessage(36-83)api/controllers/WebhookAttemptController.ts (1)
sendWebhookEvent(10-55)
api/routes/v1/inboxes/threads.ts (1)
api/models/Organization.ts (1)
Organization(3-12)
🔇 Additional comments (4)
api/routes/v1/inboxes/messages.ts (1)
108-121: LGTM!The message retrieval endpoint properly validates ownership by comparing
message.organizationIdwith the authenticated organization ID, ensuring proper access control.api/routes/v1/inboxes/index.ts (3)
15-67: Well-structured inbox routes.The route implementations are clean and follow good patterns:
- Proper authentication on all routes
- Input validation on POST with express-validator
- Consistent error handling with 404 responses
- Webhook emission after inbox creation
69-70: LGTM!The sub-router mounts correctly delegate to the messages and threads modules, enabling the nested routes under each inbox.
37-40: No changes required—parameter naming is correct.The
createInboxfunction inapi/controllers/InboxController.tsexplicitly expectsorganization_id(snake_case) as a parameter, which is what line 38 provides. The function internally converts this toorganizationId(camelCase) for the model property. This is a standard pattern where API function parameters use snake_case while internal properties use camelCase. The code is already consistent with the function's contract.
| router.post( | ||
| "/send", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request< | ||
| { organizationId: string; inboxId: string }, | ||
| {}, | ||
| { | ||
| to: string; | ||
| subject: string; | ||
| text: string; | ||
| html: string; | ||
| } | ||
| >, | ||
| res: Response | ||
| ) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add input validation for the request body.
The endpoint lacks validation for required fields (to, subject, text, html). Consider adding validation middleware (e.g., express-validator) to ensure:
tois a valid email addresssubject,text, andhtmlare non-empty strings- Input is properly sanitized to prevent injection attacks
Apply this pattern similar to the POST / route in index.ts:
router.post(
"/send",
body("to").isEmail().normalizeEmail(),
body("subject").isString().notEmpty().trim(),
body("text").isString().notEmpty(),
body("html").isString().notEmpty(),
expressValidatorMiddleware,
passport.authenticate("api_key", { session: false }),
async (req, res) => {
// ... handler logic
}
);🤖 Prompt for AI Agents
In api/routes/v1/inboxes/messages.ts around lines 14 to 29, the POST /send route
currently has no validation of the request body; add express-validator checks
before passport.authenticate to enforce and sanitize inputs: validate
body("to").isEmail().normalizeEmail(),
body("subject").isString().notEmpty().trim(),
body("text").isString().notEmpty(), body("html").isString().notEmpty(), then
include the existing expressValidatorMiddleware to short-circuit and return
errors; keep passport.authenticate and the async handler afterwards and adjust
request typing if necessary to reflect validated fields.
| public inbox = { | ||
| create: async ({ | ||
| name, | ||
| email, | ||
| }, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }: { | ||
| name: string; | ||
| email?: string; | ||
| }) => { | ||
| const response = await axios.post(`${this.apiUrl}/v1/inboxes`, { | ||
| name, | ||
| email, | ||
| }, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| list: async () => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| get: async (inboxId: string) => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes/${inboxId}`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| message: { | ||
| send: async ({ | ||
| inboxId, | ||
| to, | ||
| subject, | ||
| text, | ||
| html, | ||
| }: { | ||
| inboxId: string; | ||
| to: string; | ||
| subject: string; | ||
| text: string; | ||
| html: string; | ||
| }) => { | ||
| const response = await axios.post(`${this.apiUrl}/v1/inboxes/${inboxId}/messages/send`, { | ||
| to, | ||
| subject, | ||
| text, | ||
| html, | ||
| }, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| }); | ||
| return response.data; | ||
| } | ||
| //reply | ||
| list: async (inboxId: string) => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes/${inboxId}/messages`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| get: async (inboxId: string, messageId: string) => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes/${inboxId}/messages/${messageId}`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| }, | ||
| thread: { | ||
| list: async (inboxId: string) => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes/${inboxId}/threads`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| get: async (inboxId: string, threadId: string) => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes/${inboxId}/threads/${threadId}`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract repeated authorization logic to reduce duplication.
The Authorization header construction is repeated in every method (8 times). This violates DRY and makes maintenance harder if the auth mechanism changes.
Consider extracting a private helper method:
+ private async request(method: 'GET' | 'POST', path: string, data?: any) {
+ const config = {
+ headers: {
+ Authorization: `Bearer ${this.apiSecret}`,
+ },
+ };
+ const url = `${this.apiUrl}${path}`;
+ const response = method === 'GET'
+ ? await axios.get(url, config)
+ : await axios.post(url, data, config);
+ return response.data;
+ }
public inbox = {
create: async ({ name, email }: { name: string; email?: string }) => {
- const response = await axios.post(`${this.apiUrl}/v1/inboxes`, {
- name,
- email,
- }, {
- headers: {
- Authorization: `Bearer ${this.apiSecret}`,
- },
- });
- return response.data;
+ return this.request('POST', '/v1/inboxes', { name, email });
},Apply similar changes to all other methods.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
node-sdk/index.ts lines 12 to 108: the Authorization header is duplicated across
every inbox method; add a single private helper (e.g. getAuthHeaders() that
returns { Authorization: `Bearer ${this.apiSecret}` } or create a preconfigured
axios instance on the class with baseURL and auth header) and replace each
inline headers block with a call to that helper (or use the instance) for all
inbox methods (create, list, get, message.*, thread.*) to remove duplication and
centralize auth header construction.
node-sdk/tests/threadAPI.spec.ts
Outdated
| const newInbox = await sendook.inbox.create({ | ||
| name, | ||
| }); | ||
| const threads = await sendook.inbox.thread.list(newInbox.id); |
There was a problem hiding this comment.
Fix inconsistent inbox identifier usage.
Line 16 uses newInbox.id, but lines 29 and 35 use newInbox._id. Based on the past review of messageAPI.spec.ts, inbox.create returns id, not _id. This inconsistency will cause lines 29 and 35 to pass undefined to the API calls, resulting in requests to /v1/inboxes/undefined/messages and /v1/inboxes/undefined/threads.
Apply this diff to use the correct identifier consistently:
- const newMessage = await sendook.inbox.message.send({
- inboxId: newInbox._id,
+ const newMessage = await sendook.inbox.message.send({
+ inboxId: newInbox.id,
- const thread = await sendook.inbox.thread.get(newInbox._id, newMessage.threadId);
+ const thread = await sendook.inbox.thread.get(newInbox.id, newMessage.threadId);Also applies to: 29-29, 35-35
🤖 Prompt for AI Agents
In node-sdk/tests/threadAPI.spec.ts around lines 16, 29, and 35, the test mixes
inbox identifier properties: line 16 correctly uses newInbox.id but lines 29 and
35 use newInbox._id which is undefined; update the references on lines 29 and 35
to use newInbox.id so all API calls consistently pass the returned id (replace
newInbox._id with newInbox.id in those two locations).
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/controllers/ThreadController.ts(1 hunks)api/routes/v1/inboxes/messages.ts(1 hunks)api/routes/v1/inboxes/threads.ts(1 hunks)api/routes/webhooks/ses.ts(0 hunks)node-sdk/tests/InboxAPI.spec.ts(1 hunks)node-sdk/tests/threadAPI.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/routes/webhooks/ses.ts
✅ Files skipped from review due to trivial changes (1)
- node-sdk/tests/threadAPI.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- node-sdk/tests/InboxAPI.spec.ts
- api/routes/v1/inboxes/messages.ts
🧰 Additional context used
🧬 Code graph analysis (2)
api/routes/v1/inboxes/threads.ts (3)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/InboxController.ts (1)
getInboxByOrganizationIdAndInboxId(32-43)api/controllers/ThreadController.ts (2)
getThreadsByInboxId(35-38)getThreadById(40-43)
api/controllers/ThreadController.ts (1)
api/models/Thread.ts (1)
Thread(3-10)
| export async function createThread({ | ||
| organizationId, | ||
| inboxId, | ||
| }: { | ||
| organizationId: string; | ||
| inboxId: string; | ||
| }) { | ||
| const thread = new Thread(); | ||
| thread.organizationId = new mongoose.Types.ObjectId(organizationId); | ||
| thread.inboxId = new mongoose.Types.ObjectId(inboxId); | ||
| thread.messages = new mongoose.Types.Array<mongoose.Types.ObjectId>(); | ||
| await thread.save(); | ||
| return thread; | ||
| } |
There was a problem hiding this comment.
Add error handling and input validation.
This function lacks try-catch error handling and input validation. Invalid ObjectId strings will throw unhandled exceptions that could crash the service or leak error details.
Wrap in try-catch and validate inputs:
export async function createThread({
organizationId,
inboxId,
}: {
organizationId: string;
inboxId: string;
}) {
+ try {
+ // Validate ObjectId format
+ if (!mongoose.Types.ObjectId.isValid(organizationId) || !mongoose.Types.ObjectId.isValid(inboxId)) {
+ throw new Error("Invalid organizationId or inboxId format");
+ }
+
const thread = new Thread();
thread.organizationId = new mongoose.Types.ObjectId(organizationId);
thread.inboxId = new mongoose.Types.ObjectId(inboxId);
thread.messages = new mongoose.Types.Array<mongoose.Types.ObjectId>();
await thread.save();
return thread;
+ } catch (error) {
+ console.error("Error creating thread:", error);
+ throw error;
+ }
}📝 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.
| export async function createThread({ | |
| organizationId, | |
| inboxId, | |
| }: { | |
| organizationId: string; | |
| inboxId: string; | |
| }) { | |
| const thread = new Thread(); | |
| thread.organizationId = new mongoose.Types.ObjectId(organizationId); | |
| thread.inboxId = new mongoose.Types.ObjectId(inboxId); | |
| thread.messages = new mongoose.Types.Array<mongoose.Types.ObjectId>(); | |
| await thread.save(); | |
| return thread; | |
| } | |
| export async function createThread({ | |
| organizationId, | |
| inboxId, | |
| }: { | |
| organizationId: string; | |
| inboxId: string; | |
| }) { | |
| try { | |
| // Validate ObjectId format | |
| if (!mongoose.Types.ObjectId.isValid(organizationId) || !mongoose.Types.ObjectId.isValid(inboxId)) { | |
| throw new Error("Invalid organizationId or inboxId format"); | |
| } | |
| const thread = new Thread(); | |
| thread.organizationId = new mongoose.Types.ObjectId(organizationId); | |
| thread.inboxId = new mongoose.Types.ObjectId(inboxId); | |
| thread.messages = new mongoose.Types.Array<mongoose.Types.ObjectId>(); | |
| await thread.save(); | |
| return thread; | |
| } catch (error) { | |
| console.error("Error creating thread:", error); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In api/controllers/ThreadController.ts around lines 4–17, validate that
organizationId and inboxId are non-empty strings and pass
mongoose.Types.ObjectId.isValid before constructing ObjectId instances; if
validation fails, throw a controlled error (e.g., BadRequest) with a safe
message. Wrap the creation and thread.save() in a try-catch, log a concise error
message (without leaking stack or raw input) and rethrow or return a controlled
error response so callers can handle it; ensure thread.messages is initialized
as a plain array of ObjectId placeholders and only create ObjectId instances
after validation.
| export async function addMessageToThread({ | ||
| threadId, | ||
| messageId, | ||
| }: { | ||
| threadId: string; | ||
| messageId: string; | ||
| }) { | ||
| const thread = await Thread.findById(threadId); | ||
| if (!thread) { | ||
| return null; | ||
| } | ||
| thread.messages.push(new mongoose.Types.ObjectId(messageId)); | ||
| await thread.save(); | ||
| return thread; | ||
| } |
There was a problem hiding this comment.
Add error handling, input validation, and duplicate check.
This function has several issues:
- No try-catch error handling for invalid ObjectIds or database errors
- No validation of input ID formats
- No check for duplicate messages in the thread
- Potential race condition with concurrent addMessageToThread calls
Apply these improvements:
export async function addMessageToThread({
threadId,
messageId,
}: {
threadId: string;
messageId: string;
}) {
+ try {
+ if (!mongoose.Types.ObjectId.isValid(threadId) || !mongoose.Types.ObjectId.isValid(messageId)) {
+ throw new Error("Invalid threadId or messageId format");
+ }
+
const thread = await Thread.findById(threadId);
if (!thread) {
return null;
}
+
+ // Check for duplicate
+ const messageObjectId = new mongoose.Types.ObjectId(messageId);
+ if (thread.messages.some(id => id.equals(messageObjectId))) {
+ return thread; // Already exists
+ }
+
- thread.messages.push(new mongoose.Types.ObjectId(messageId));
+ thread.messages.push(messageObjectId);
await thread.save();
return thread;
+ } catch (error) {
+ console.error("Error adding message to thread:", error);
+ throw error;
+ }
}| export async function getThreadsByInboxId(inboxId: string) { | ||
| const threads = await Thread.find({ inboxId: new mongoose.Types.ObjectId(inboxId) }); | ||
| return threads; | ||
| } |
There was a problem hiding this comment.
Add error handling and consider pagination.
This function lacks error handling and could return large result sets without pagination.
Add error handling:
export async function getThreadsByInboxId(inboxId: string) {
+ try {
+ if (!mongoose.Types.ObjectId.isValid(inboxId)) {
+ throw new Error("Invalid inboxId format");
+ }
+
const threads = await Thread.find({ inboxId: new mongoose.Types.ObjectId(inboxId) });
return threads;
+ } catch (error) {
+ console.error("Error fetching threads:", error);
+ throw error;
+ }
}Consider adding pagination parameters (limit, skip) to handle inboxes with many threads.
🤖 Prompt for AI Agents
In api/controllers/ThreadController.ts around lines 35 to 38, the function has
no error handling and can return unbounded results; wrap the DB call in a
try/catch that validates inboxId and rethrows or returns a controlled error, and
add pagination parameters (e.g., limit and skip or page and size) with sane
defaults and a maximum cap to prevent huge responses; apply
.limit(limit).skip(skip) to the query and optionally return metadata (total
count, page) or throw a 400 for invalid input and a 500 for DB errors.
| export async function getThreadById(threadId: string) { | ||
| const thread = await Thread.findById(threadId).populate("messages"); | ||
| return thread; | ||
| } |
There was a problem hiding this comment.
Add error handling and input validation.
Missing error handling and input validation could cause unhandled exceptions.
export async function getThreadById(threadId: string) {
+ try {
+ if (!mongoose.Types.ObjectId.isValid(threadId)) {
+ throw new Error("Invalid threadId format");
+ }
+
const thread = await Thread.findById(threadId).populate("messages");
return thread;
+ } catch (error) {
+ console.error("Error fetching thread:", error);
+ throw error;
+ }
}📝 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.
| export async function getThreadById(threadId: string) { | |
| const thread = await Thread.findById(threadId).populate("messages"); | |
| return thread; | |
| } | |
| export async function getThreadById(threadId: string) { | |
| try { | |
| if (!mongoose.Types.ObjectId.isValid(threadId)) { | |
| throw new Error("Invalid threadId format"); | |
| } | |
| const thread = await Thread.findById(threadId).populate("messages"); | |
| return thread; | |
| } catch (error) { | |
| console.error("Error fetching thread:", error); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In api/controllers/ThreadController.ts around lines 40 to 43, the function lacks
input validation and error handling; validate the threadId (e.g., ensure it's
present and a valid Mongo ObjectId using mongoose.Types.ObjectId.isValid) before
calling Thread.findById, and wrap the database call in a try/catch to handle and
log DB errors and return a controlled response (null or throw a descriptive
error) instead of letting exceptions bubble up.
| router.get( | ||
| "/", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string; inboxId: string }>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
|
|
||
| const inbox = await getInboxByOrganizationIdAndInboxId({ | ||
| organizationId: organization._id.toString(), | ||
| inboxId: req.params.inboxId!, | ||
| }); | ||
| if (!inbox) { | ||
| return res.status(404).json({ error: "Inbox not found" }); | ||
| } | ||
|
|
||
| const threads = await getThreadsByInboxId(req.params.inboxId); | ||
| return res.json(threads); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Good implementation, but add error handling.
The route is now properly implemented (addressing previous review comments), and correctly validates inbox ownership before returning threads. However, it lacks try-catch error handling.
Add error handling:
router.get(
"/",
passport.authenticate("api_key", { session: false }),
async (
req: Request<{ organizationId: string; inboxId: string }>,
res: Response
) => {
+ try {
const organization = req.user as HydratedDocument<Organization>;
const inbox = await getInboxByOrganizationIdAndInboxId({
organizationId: organization._id.toString(),
inboxId: req.params.inboxId!,
});
if (!inbox) {
return res.status(404).json({ error: "Inbox not found" });
}
const threads = await getThreadsByInboxId(req.params.inboxId);
return res.json(threads);
+ } catch (error) {
+ console.error("Error fetching threads:", error);
+ return res.status(500).json({ error: "Internal server error" });
+ }
}
);📝 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.
| router.get( | |
| "/", | |
| passport.authenticate("api_key", { session: false }), | |
| async ( | |
| req: Request<{ organizationId: string; inboxId: string }>, | |
| res: Response | |
| ) => { | |
| const organization = req.user as HydratedDocument<Organization>; | |
| const inbox = await getInboxByOrganizationIdAndInboxId({ | |
| organizationId: organization._id.toString(), | |
| inboxId: req.params.inboxId!, | |
| }); | |
| if (!inbox) { | |
| return res.status(404).json({ error: "Inbox not found" }); | |
| } | |
| const threads = await getThreadsByInboxId(req.params.inboxId); | |
| return res.json(threads); | |
| } | |
| ); | |
| router.get( | |
| "/", | |
| passport.authenticate("api_key", { session: false }), | |
| async ( | |
| req: Request<{ organizationId: string; inboxId: string }>, | |
| res: Response | |
| ) => { | |
| try { | |
| const organization = req.user as HydratedDocument<Organization>; | |
| const inbox = await getInboxByOrganizationIdAndInboxId({ | |
| organizationId: organization._id.toString(), | |
| inboxId: req.params.inboxId!, | |
| }); | |
| if (!inbox) { | |
| return res.status(404).json({ error: "Inbox not found" }); | |
| } | |
| const threads = await getThreadsByInboxId(req.params.inboxId); | |
| return res.json(threads); | |
| } catch (error) { | |
| console.error("Error fetching threads:", error); | |
| return res.status(500).json({ error: "Internal server error" }); | |
| } | |
| } | |
| ); |
🤖 Prompt for AI Agents
In api/routes/v1/inboxes/threads.ts around lines 14 to 34, the async route
handler lacks try/catch error handling; wrap the body of the handler in a try
block that awaits getInboxByOrganizationIdAndInboxId and getThreadsByInboxId,
add a catch block that logs the caught error (use the project's logger) and
returns res.status(500).json({ error: "Internal server error" }) so unexpected
exceptions don't crash the process and callers receive a clear 500 response.
| router.get( | ||
| "/:threadId", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string; inboxId: string; threadId: string }>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
|
|
||
| const thread = await getThreadById(req.params.threadId); | ||
| if ( | ||
| !thread || | ||
| thread.organizationId.toString() !== organization._id.toString() | ||
| ) { | ||
| return res.status(404).json({ error: "Thread not found" }); | ||
| } | ||
|
|
||
| return res.json(thread); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Type fixed correctly, but missing inbox validation and error handling.
Good fixes from previous review (type parameter now uses threadId instead of messageId, and route is now functional). However:
- The route validates thread ownership but doesn't verify the thread belongs to the inbox specified in the URL path (
req.params.inboxId) - Missing try-catch error handling
Apply this diff:
router.get(
"/:threadId",
passport.authenticate("api_key", { session: false }),
async (
req: Request<{ organizationId: string; inboxId: string; threadId: string }>,
res: Response
) => {
+ try {
const organization = req.user as HydratedDocument<Organization>;
const thread = await getThreadById(req.params.threadId);
if (
!thread ||
- thread.organizationId.toString() !== organization._id.toString()
+ thread.organizationId.toString() !== organization._id.toString() ||
+ thread.inboxId.toString() !== req.params.inboxId
) {
return res.status(404).json({ error: "Thread not found" });
}
return res.json(thread);
+ } catch (error) {
+ console.error("Error fetching thread:", error);
+ return res.status(500).json({ error: "Internal server error" });
+ }
}
);🤖 Prompt for AI Agents
In api/routes/v1/inboxes/threads.ts around lines 36 to 55, the handler currently
checks organization ownership but misses validating that the thread belongs to
the inbox from req.params.inboxId and lacks error handling; wrap the async logic
in a try-catch, call getThreadById inside the try, on error log/return
res.status(500).json({ error: "Internal server error" }) (or include the error
message per project conventions), then after fetching the thread verify thread
&& thread.organizationId.toString() === organization._id.toString() &&
thread.inboxId.toString() === req.params.inboxId, returning 404 if any check
fails, otherwise return the thread.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ahmedmawiri
left a comment
ahmedmawiri
left a comment
There was a problem hiding this comment.
The thread creation appears to be a bug the other check to verify message security is probably a must too.
| const thread = await createThread({ | ||
| organizationId: organization._id.toString(), | ||
| inboxId: req.params.inboxId, | ||
| }); |
There was a problem hiding this comment.
@marcrupt agree here. this seems to create a new thread for every message.
| router.get( | ||
| "/:threadId", | ||
| passport.authenticate("api_key", { session: false }), | ||
| async ( | ||
| req: Request<{ organizationId: string; inboxId: string; threadId: string }>, | ||
| res: Response | ||
| ) => { | ||
| const organization = req.user as HydratedDocument<Organization>; | ||
|
|
||
| const thread = await getThreadById(req.params.threadId); | ||
| if ( | ||
| !thread || | ||
| thread.organizationId.toString() !== organization._id.toString() | ||
| ) { | ||
| return res.status(404).json({ error: "Thread not found" }); | ||
| } | ||
|
|
||
| return res.json(thread); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/routes/v1/inboxes/messages.ts(1 hunks)api/routes/v1/inboxes/threads.ts(1 hunks)node-sdk/index.ts(1 hunks)node-sdk/tests/messageAPI.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- node-sdk/tests/messageAPI.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
api/routes/v1/inboxes/messages.ts (6)
api/models/Organization.ts (1)
Organization(3-12)api/controllers/InboxController.ts (1)
getInboxByOrganizationIdAndInboxId(32-43)api/controllers/ThreadController.ts (2)
createThread(4-17)addMessageToThread(19-33)api/controllers/MessageController.ts (3)
createMessage(5-47)getMessagesByInboxId(53-55)getMessageById(49-51)api/controllers/SESController.ts (1)
sendSESMessage(36-83)api/controllers/WebhookAttemptController.ts (1)
sendWebhookEvent(10-55)
| const message = await getMessageById(req.params.messageId); | ||
| if ( | ||
| !message || | ||
| message.organizationId.toString() !== organization._id.toString() | ||
| ) { | ||
| return res.status(404).json({ error: "Message not found" }); | ||
| } |
There was a problem hiding this comment.
Enforce inbox scoping when fetching a message.
This endpoint only checks the organization, so callers can request /inboxes/{otherInboxId}/messages/{messageId} and still see the message if it belongs to the org. Please add an inboxId equality check to keep resources aligned with the URL.
if (
!message ||
- message.organizationId.toString() !== organization._id.toString()
+ message.organizationId.toString() !== organization._id.toString() ||
+ message.inboxId.toString() !== req.params.inboxId
) {
return res.status(404).json({ error: "Message not found" });
}🤖 Prompt for AI Agents
In api/routes/v1/inboxes/messages.ts around lines 130 to 136, the current check
only verifies organization ownership so a message from another inbox in the same
org can be returned; add an inboxId equality check by comparing message.inboxId
(toString()) with req.params.inboxId (toString()) and return 404 when they do
not match, keeping the existing org check; ensure both IDs are normalized to
strings before comparison.
| const replyToMessage = await getMessageById(req.params.messageId); | ||
| if ( | ||
| !replyToMessage || | ||
| replyToMessage.organizationId.toString() !== organization._id.toString() | ||
| ) { | ||
| return res.status(404).json({ error: "Message not found" }); | ||
| } |
There was a problem hiding this comment.
Also validate inbox ownership before replying.
The reply guard mirrors the GET handler and needs the same inboxId check; otherwise a caller can reply to a message that belongs to another inbox in the org, producing cross-inbox thread corruption.
if (
!replyToMessage ||
- replyToMessage.organizationId.toString() !== organization._id.toString()
+ replyToMessage.organizationId.toString() !== organization._id.toString() ||
+ replyToMessage.inboxId.toString() !== req.params.inboxId
) {
return res.status(404).json({ error: "Message not found" });
}🤖 Prompt for AI Agents
In api/routes/v1/inboxes/messages.ts around lines 163 to 169, the reply guard
checks message existence and org ownership but misses verifying that the message
belongs to the requested inbox; add a check that replyToMessage.inboxId
(stringified) matches req.params.inboxId (or the loaded inbox._id) and return
404 if it does not match so replies cannot target messages from a different
inbox in the same org.
| this.apiUrl = apiUrl || "https://api.sendook.com"; | ||
| } | ||
|
|
||
| async createInbox({ | ||
| name, | ||
| email, | ||
| }: { | ||
| name: string; | ||
| email?: string; | ||
| }) { | ||
| const response = await axios.post(`${this.apiUrl}/inboxes`, { | ||
| public inbox = { | ||
| create: async ({ | ||
| name, | ||
| email, | ||
| }, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }: { | ||
| name: string; | ||
| email?: string; | ||
| }) => { | ||
| const response = await axios.post(`${this.apiUrl}/v1/inboxes`, { | ||
| name, | ||
| email, | ||
| }, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| list: async () => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| get: async (inboxId: string) => { | ||
| const response = await axios.get(`${this.apiUrl}/v1/inboxes/${inboxId}`, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, | ||
| message: { | ||
| send: async ({ | ||
| inboxId, | ||
| to, | ||
| subject, | ||
| text, | ||
| html, | ||
| }: { | ||
| inboxId: string; | ||
| to: string; | ||
| subject: string; | ||
| text: string; | ||
| html: string; | ||
| }) => { | ||
| const response = await axios.post(`${this.apiUrl}/v1/inboxes/${inboxId}/messages/send`, { | ||
| to, | ||
| subject, | ||
| text, | ||
| html, | ||
| }, { | ||
| headers: { | ||
| "Authorization": `Bearer ${this.apiSecret}`, | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, |
There was a problem hiding this comment.
Fix regression when apiUrl already contains /v1.
Previously consumers could pass apiUrl values like https://api-dev.sendook.com/v1. With the new default plus the hardcoded "/v1/..." paths, those same clients now hit /v1/v1/... and get 404s. Please normalize the supplied base URL (or centralize URL building) so that existing custom apiUrl values keep working.
- this.apiUrl = apiUrl || "https://api.sendook.com";
+ this.apiUrl = (apiUrl || "https://api.sendook.com").replace(/\/+$/, "");
+
+ private buildUrl(path: string) {
+ return new URL(path, `${this.apiUrl}/`).toString();
+ }Then call this.buildUrl("/v1/inboxes"), this.buildUrl("/v1/inboxes/${inboxId}"), etc., to avoid duplicating the version.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In node-sdk/index.ts around lines 9 to 71, the client currently concatenates
hardcoded "/v1/..." paths onto the supplied apiUrl which can already include
"/v1", causing duplicated "/v1/v1" and 404s; add a small helper method (e.g.,
this.buildUrl(path: string)) that normalizes the base URL and path by: trimming
any trailing slash from apiUrl, removing a trailing "/v1" if present from the
base, ensuring the path begins with a single "/", and then returning
`${cleanBase}/v1${path}`; replace all direct `${this.apiUrl}/v1/...` usages with
calls to this.buildUrl("/inboxes"), this.buildUrl(`/inboxes/${inboxId}`),
this.buildUrl(`/inboxes/${inboxId}/messages/send`) etc., so custom apiUrl values
that already include "/v1" work correctly and URL construction is centralized.
Summary by CodeRabbit
New Features
API Changes
Tests