APP MVP + all of v4 changes (Domains, messages, attachments, tests) #6
Conversation
RUP-884 Email Agent MVP
|
WalkthroughAdds domain management with MX DNS verification, multi-recipient messaging and embedded attachments, SES raw MIME sending, Redis-backed rate limiting, schema changes (Message, Domain), API key CRUD with deletion, inbox/message deletion endpoints, a Nuxt frontend, Node SDK + OpenAPI updates, and route/auth changes that attach organization to requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant V1Router as v1 Router
participant OrgMiddleware as Org Middleware
participant DomainController
participant DNSController
participant Database
participant DNS as DNS Resolver
Client->>V1Router: POST /v1/domains { name }
V1Router->>OrgMiddleware: authenticate + attach organization
OrgMiddleware->>DomainController: createDomain({ organizationId, name })
DomainController->>Database: insert Domain
Database-->>DomainController: domain created
DomainController-->>Client: 201 { domain }
Client->>V1Router: POST /v1/domains/:domainId/verify
V1Router->>DomainController: verifyDomainDNS({ domain })
DomainController->>DNSController: getDNSMXRecords(domain.name)
DNSController->>DNS: resolveMx(domain.name)
DNS-->>DNSController: MX records
DNSController-->>DomainController: MX records array
DomainController->>Database: update domain.records and verified
DomainController-->>Client: { verified, domain }
sequenceDiagram
participant Client
participant InboxRoute as Inbox Route
participant RateLimiter as Redis RateLimiter
participant MessageController
participant SESController
participant MailComposer
participant AWSSES as AWS SES
participant Database
Client->>InboxRoute: POST /v1/inboxes/:id/send { to[], cc[], bcc[], attachments[] }
InboxRoute->>RateLimiter: check allowance
RateLimiter-->>InboxRoute: allowed
InboxRoute->>MessageController: createMessage(payload)
MessageController->>Database: persist message (multi-recipient, attachments)
Database-->>MessageController: saved
InboxRoute->>SESController: sendSESMessage({ to[], cc[], bcc[], attachments[] })
SESController->>MailComposer: build MIME with attachments
MailComposer-->>SESController: MIME bytes
SESController->>AWSSES: SendRawEmailCommand(RawMessage)
AWSSES-->>SESController: sent
SESController-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Areas needing extra attention:
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)
Comment |
marcrupt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
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)
41-59: Fix the assertion to match the new array-basedtofield.The test now passes
toas an array[toEmail](line 47), but line 56 still assertsmessage.toagainst the stringtoEmail. Sincetois nowstring[], this assertion will fail.Apply this diff to fix the assertion:
expect(message).toBeDefined(); expect(message.organizationId.toString()).toBe(organization.id); expect(message.inboxId.toString()).toBe(inbox.id); expect(message.from).toBe(inbox.email); - expect(message.to).toBe(toEmail); + expect(message.to).toEqual([toEmail]); expect(message.subject).toBe(subject); expect(message.text).toBe(text); expect(message.html).toBe(html);node-sdk/index.ts (2)
114-132: Expose the new domain parameter ininbox.create.The API now accepts an optional
domainname when creating an inbox, but the SDK still drops it, so clients can’t assign domains through the SDK. Please thread thedomainfield through.Apply this diff to include the parameter:
- create: async ({ - name, - email, - }: { - name: string; - email?: string; - }) => { - const response = await axios.post(`${this.apiUrl}/v1/inboxes`, { - name, - email, - }, { + create: async ({ + name, + email, + domain, + }: { + name: string; + email?: string; + domain?: string; + }) => { + const response = await axios.post(`${this.apiUrl}/v1/inboxes`, { + name, + email, + domain, + }, {
157-189: Support attachments inmessage.send.Server-side now handles attachments, but the SDK neither accepts nor forwards them, so attachments can’t be sent through the SDK. Please add an
attachmentsparameter and include it in the POST body.Apply this diff to wire the field through:
- send: async ({ - inboxId, - to, - cc, - bcc, - labels, - subject, - text, - html, - }: { - inboxId: string; - to: string[]; - cc?: string[]; - bcc?: string[]; - labels?: string[]; - subject: string; - text: string; - html: string; - }) => { + send: async ({ + inboxId, + to, + cc, + bcc, + labels, + subject, + text, + html, + attachments, + }: { + inboxId: string; + to: string[]; + cc?: string[]; + bcc?: string[]; + labels?: string[]; + subject: string; + text: string; + html: string; + attachments?: { + content: string; + name?: string; + contentType?: string; + }[]; + }) => { const response = await axios.post(`${this.apiUrl}/v1/inboxes/${inboxId}/messages/send`, { to, cc, bcc, labels, subject, text, html, + attachments, }, {
🧹 Nitpick comments (8)
app/README.md (1)
1-75: Consider customizing the README for project-specific details.The current README is the default Nuxt starter template. Consider adding project-specific information such as:
- API URL configuration
- Environment variables required
- Project structure overview
- Links to related API documentation
app/layouts/default.vue (1)
16-26: Verify whether disabled navigation items should be removed.Most navigation items are marked as disabled with the
is-disabledclass. If these features aren't implemented yet, consider either removing them from the UI entirely or adding a clearer indicator (e.g., "Coming Soon" badge) to set user expectations.app/app/pages/api-keys.vue (1)
173-203: Consider more specific error handling in loadKeys.The error handling catches all errors and displays a generic message. For better UX, consider handling different HTTP status codes separately (e.g., 401 for unauthorized, 403 for forbidden, 404 for organization not found).
Example:
if (!response.ok) { if (response.status === 401) { errorMessage.value = 'Session expired. Please log in again.'; } else if (response.status === 403) { errorMessage.value = 'You do not have permission to view API keys.'; } else { errorMessage.value = `Unable to fetch API keys: ${response.status}`; } return; }app/app/assets/css/tailwind.css (1)
1-1: Consider privacy implications of Google Fonts import.The direct import from
fonts.googleapis.comcauses browsers to make requests to Google servers, which may have GDPR/privacy implications depending on your jurisdiction and user base.Consider:
- Self-hosting the Inter font files to avoid external requests
- Adding appropriate privacy disclosures if keeping the Google Fonts CDN
- Using
font-display: swap(already present) is good for performanceIf GDPR compliance is required, self-hosting is the safest approach.
api/controllers/DNSController.ts (1)
4-15: Consider distinguishing between DNS errors and missing records.The current error handling returns an empty array for both DNS resolution failures and domains with no MX records. This makes it difficult for callers (e.g.,
verifyDomainDNS) to distinguish between verification failure due to misconfiguration versus a DNS error.Consider one of these approaches:
Option 1: Re-throw DNS errors
export async function getDNSMXRecords({ domain, }: { domain: string; }): Promise<MxRecord[]> { try { return await resolveMx(domain); } catch (error) { - console.error('Error resolving MX records:', error); - return []; + console.error('Error resolving MX records:', error); + throw error; } }Option 2: Return a result object
export async function getDNSMXRecords({ domain, }: { domain: string; }): Promise<{ records: MxRecord[]; error?: Error }> { try { const records = await resolveMx(domain); return { records }; } catch (error) { console.error('Error resolving MX records:', error); return { records: [], error: error as Error }; } }app/app/pages/signup.vue (1)
50-61: Consider adding password strength validation.While the current implementation uses
type="password"and therequiredattribute, consider adding client-side password strength validation to improve security and user experience by guiding users to create stronger passwords.app/app/pages/inboxes.vue (1)
55-123: Label the dialogs for assistive techBoth modal overlays use
role="dialog"but lack an accessible name, so screen readers announce an unlabeled window. Tie the dialog container to its<h2>viaaria-labelledby(and optionally the body copy witharia-describedby) to meet ARIA dialog guidance. (developer.mozilla.org)Apply this diff:
- <div v-if="showDialog" class="dialog-backdrop" role="dialog" aria-modal="true"> - <div class="dialog-card"> - <header class="dialog-header"> - <h2>Create inbox</h2> + <div + v-if="showDialog" + class="dialog-backdrop" + role="dialog" + aria-modal="true" + aria-labelledby="create-inbox-title" + aria-describedby="create-inbox-desc" + > + <div class="dialog-card"> + <header class="dialog-header"> + <h2 id="create-inbox-title">Create inbox</h2> ... - <p> + <p id="create-inbox-desc"> ... - <div v-if="showDeleteDialog" class="dialog-backdrop" role="dialog" aria-modal="true"> - <div class="dialog-card"> - <header class="dialog-header"> - <h2>Delete inbox</h2> + <div + v-if="showDeleteDialog" + class="dialog-backdrop" + role="dialog" + aria-modal="true" + aria-labelledby="delete-inbox-title" + aria-describedby="delete-inbox-desc" + > + <div class="dialog-card"> + <header class="dialog-header"> + <h2 id="delete-inbox-title">Delete inbox</h2> ... - <p> + <p id="delete-inbox-desc">app/app/pages/domains.vue (1)
62-125: Provide accessible names for the domain dialogsFor parity with ARIA dialog requirements, hook each modal’s
role="dialog"container up to its heading so assistive tech announces “Add domain” / “Delete domain” when focus enters. Add matchingaria-labelledby(andaria-describedbyfor the explanatory text). (developer.mozilla.org)Apply this diff:
- <div v-if="showDialog" class="dialog-backdrop" role="dialog" aria-modal="true"> - <div class="dialog-card"> - <header class="dialog-header"> - <h2>Add domain</h2> + <div + v-if="showDialog" + class="dialog-backdrop" + role="dialog" + aria-modal="true" + aria-labelledby="add-domain-title" + aria-describedby="add-domain-desc" + > + <div class="dialog-card"> + <header class="dialog-header"> + <h2 id="add-domain-title">Add domain</h2> ... - <p> + <p id="add-domain-desc"> ... - <div v-if="showDeleteDialog" class="dialog-backdrop" role="dialog" aria-modal="true"> - <div class="dialog-card"> - <header class="dialog-header"> - <h2>Delete domain</h2> + <div + v-if="showDeleteDialog" + class="dialog-backdrop" + role="dialog" + aria-modal="true" + aria-labelledby="delete-domain-title" + aria-describedby="delete-domain-desc" + > + <div class="dialog-card"> + <header class="dialog-header"> + <h2 id="delete-domain-title">Delete domain</h2> ... - <p> + <p id="delete-domain-desc">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.DS_Storeis excluded by!**/.DS_Storeapi/bun.lockis excluded by!**/*.lockapp/public/favicon.icois excluded by!**/*.icoapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (50)
api/controllers/ApiKeyController.ts(2 hunks)api/controllers/DNSController.ts(1 hunks)api/controllers/DomainController.ts(1 hunks)api/controllers/InboxController.ts(1 hunks)api/controllers/MessageController.ts(3 hunks)api/controllers/PassportController.ts(1 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/auth.ts(1 hunks)api/routes/organizations.ts(1 hunks)api/routes/organizations/apiKeys.ts(0 hunks)api/routes/v1/api_keys/index.ts(1 hunks)api/routes/v1/domains/index.ts(1 hunks)api/routes/v1/inboxes/index.ts(4 hunks)api/routes/v1/inboxes/messages.ts(8 hunks)api/routes/v1/inboxes/threads.ts(2 hunks)api/routes/v1/index.ts(1 hunks)api/tests/MessageController.spec.ts(2 hunks)api/types/environment.d.ts(1 hunks)app/.gitignore(1 hunks)app/README.md(1 hunks)app/app/app.vue(1 hunks)app/app/assets/css/tailwind.css(1 hunks)app/app/components/dashboard/DashboardShell.vue(1 hunks)app/app/composables/useRequireAuth.ts(1 hunks)app/app/pages/api-keys.vue(1 hunks)app/app/pages/domains.vue(1 hunks)app/app/pages/inboxes.vue(1 hunks)app/app/pages/index.vue(1 hunks)app/app/pages/login.vue(1 hunks)app/app/pages/messages.vue(1 hunks)app/app/pages/signup.vue(1 hunks)app/eslint.config.mjs(1 hunks)app/layouts/auth.vue(1 hunks)app/layouts/default.vue(1 hunks)app/nuxt.config.ts(1 hunks)app/package.json(1 hunks)app/postcss.config.js(1 hunks)app/public/robots.txt(1 hunks)app/tailwind.config.ts(1 hunks)app/tsconfig.json(1 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 (2)
- api/routes/organizations/apiKeys.ts
- api/models/Attachment.ts
🧰 Additional context used
🧬 Code graph analysis (9)
api/types/environment.d.ts (1)
api/models/Organization.ts (1)
Organization(3-12)
api/controllers/DomainController.ts (2)
api/models/Domain.ts (1)
Domain(3-15)api/controllers/DNSController.ts (1)
getDNSMXRecords(4-15)
app/app/composables/useRequireAuth.ts (1)
api/models/Organization.ts (1)
Organization(3-12)
api/controllers/InboxController.ts (1)
api/models/Inbox.ts (1)
Inbox(3-11)
api/controllers/ApiKeyController.ts (1)
api/models/ApiKey.ts (1)
ApiKey(3-11)
api/routes/organizations.ts (1)
api/controllers/OrganizationController.ts (1)
getOrganizationById(43-45)
api/routes/v1/inboxes/index.ts (5)
api/controllers/InboxController.ts (3)
createInbox(4-24)getInboxByOrganizationIdAndInboxId(32-43)deleteInboxByOrganizationIdAndInboxId(45-56)api/middlewares/expressValidatorMiddleware.ts (1)
expressValidatorMiddleware(4-16)api/controllers/DomainController.ts (1)
getVerifiedDomainByOrganizationIdAndName(43-55)api/controllers/WebhookAttemptController.ts (1)
sendWebhookEvent(10-55)api/controllers/MessageController.ts (1)
deleteMessagesByInboxId(120-124)
api/controllers/MessageController.ts (1)
api/models/Message.ts (2)
MessageStatus(3-10)Message(12-36)
api/routes/v1/index.ts (1)
api/models/Organization.ts (1)
Organization(3-12)
🪛 ast-grep (0.39.9)
app/app/composables/useRequireAuth.ts
[warning] 28-28: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((?:^|; )${name}=([^;]*))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (42)
app/public/robots.txt (1)
1-2: LGTM! Appropriate robots.txt for MVP.The permissive policy (allow-all crawling) is well-suited for a new MVP app that wants search engine visibility. As the application grows and introduces protected endpoints, you can refine this file to disallow paths like
/admin,/api, etc.app/app/app.vue (1)
1-5: LGTM!Standard Nuxt application shell that correctly uses
NuxtLayoutandNuxtPagefor nested routing.app/tsconfig.json (1)
1-18: LGTM!Standard Nuxt TypeScript configuration using project references to delegate to generated config files.
app/.gitignore (1)
1-24: LGTM!Comprehensive gitignore rules that properly exclude build artifacts, dependencies, logs, and sensitive environment files while preserving
.env.example.app/postcss.config.js (1)
1-7: LGTM!Standard PostCSS configuration with Tailwind CSS and Autoprefixer using default settings.
api/routes/auth.ts (1)
76-76: LGTM!The
last_namevalidation is correctly added and consistent with the existingfirst_namefield validation pattern.api/package.json (1)
12-12: No action required—nodemailer 7.0.10 is current and secure.Verification confirms that nodemailer 7.0.10 is the latest version and contains no known vulnerabilities. All identified security advisories affect earlier versions and have been patched.
api/controllers/InboxController.ts (1)
45-56: No action required — cascade deletion is properly implemented.The code correctly handles related messages when an inbox is deleted. The route handler explicitly calls
deleteMessagesByInboxId()after the inbox deletion succeeds (line 102 inapi/routes/v1/inboxes/index.ts), ensuring messages are cleaned up rather than orphaned.api/controllers/ApiKeyController.ts (1)
28-36: LGTM! Signature change improves readability.The refactor to object destructuring makes the function signature more maintainable and clearer at call sites.
api/routes/v1/inboxes/threads.ts (2)
19-20: LGTM! Organization scoping is correct.The change to use
req.organization._id.toString()aligns with the centralized authentication pattern established in the v1 router.
40-40: LGTM! Thread ownership check is correct.The organization ID comparison properly validates that the thread belongs to the requesting organization.
app/eslint.config.mjs (1)
1-6: LGTM! Standard Nuxt ESLint configuration.This follows the recommended Nuxt ESLint setup pattern by extending the generated configuration. The structure allows for easy addition of custom rules in the future.
app/app/pages/api-keys.vue (3)
46-46: Verify API key display security implications.API keys are displayed in plaintext in the UI (
{{ key.key ?? '••••••••••••••••' }}). While this may be intentional for user convenience, ensure this aligns with your security requirements. Consider:
- Only showing keys immediately after creation (one-time display)
- Requiring re-authentication to view existing keys
- Showing only a prefix/suffix of the key with masked middle characters
If plaintext display is acceptable for your use case, this is fine. Otherwise, consider implementing one of the above patterns.
247-251: LGTM! Proper cleanup of copy timeout.The
onBeforeUnmounthook correctly clears the copy timeout to prevent memory leaks and potential race conditions.
334-342: LGTM! Reactive data loading with proper dependencies.The watcher correctly reloads API keys when the organization ID or token changes, with proper immediate execution on mount.
node-sdk/tests/threadAPI.spec.ts (2)
18-18: LGTM! Correctly updated to array format.The change from a single recipient string to an array aligns with the multi-recipient support introduced in this PR.
37-37: LGTM! Consistent with multi-recipient API changes.app/app/assets/css/tailwind.css (1)
3-17: LGTM! Clean Tailwind and base styles setup.The Tailwind directives, dark color scheme, and font configuration with proper fallbacks are well-structured.
app/package.json (1)
12-25: Remove speculative stability concerns; version pinning strategy is subjective best practice.Nuxt 4.2.1 (released Nov 6, 2025) is a patch on the current stable major line and is intended for production use. No known unpatched vulnerabilities affect Nuxt 4.2.1 or Vue 3.5.24.
The mixed version pinning (exact vs. caret ranges) is a valid stylistic choice. For consistency, consider standardizing on either exact pinning or caret ranges; exact pinning offers more predictable builds, but caret ranges allow security patch updates. Both approaches are common in production projects.
Likely an incorrect or invalid review comment.
api/controllers/PassportController.ts (1)
19-22: LGTM! Consistent serialization pattern.Using
toJSON()for the user payload aligns with the existing pattern used for organization serialization (line 41), ensuring consistent data shapes across authentication strategies.app/layouts/auth.vue (1)
1-88: LGTM! Well-structured authentication layout.The component provides a clean, modern authentication shell with proper:
- Responsive design for different viewports
- Visual hierarchy with z-indexed elements
- Consistent styling with blur effects and gradients
- Reusable slot pattern for nested content
api/models/Domain.ts (1)
8-12: LGTM! Clear structure for DNS record tracking.The
recordsfield provides a well-defined structure for storing DNS verification data with appropriate string types fortype,value, andstatusproperties.app/app/pages/messages.vue (1)
1-99: LGTM! Well-structured messages page.The component follows Vue best practices with:
- Proper authentication enforcement via
useRequireAuth- Clear component naming via
defineOptions- Responsive layout with appropriate breakpoints
- Clear placeholder messaging for future functionality
- Consistent styling patterns
app/tailwind.config.ts (1)
1-40: LGTM! Comprehensive Tailwind configuration.The configuration is properly:
- Typed with
Configand usessatisfiesfor type safety- Scoped with appropriate content globs covering all Vue components
- Extended with custom theme values (sendook colors, glass shadow, Inter font)
- Structured for maintainability
api/routes/v1/index.ts (1)
11-21: API key middleware and context wiring look solid.Auth gating the v1 router and projecting
req.organizationgives the downstream routers what they expect. Nice cleanup.app/nuxt.config.ts (1)
1-32: Nuxt config defaults look good.The API URL fallback and module wiring align with the new app’s needs.
node-sdk/tests/domainAPI.spec.ts (1)
8-43: Nice coverage for the domain client.Exercising create/get/delete/verify in one flow gives us confidence the SDK wiring matches the API.
api/models/Message.ts (1)
28-32: LGTM: Attachments structure is well-defined.The inline attachment object structure with
content,name, andcontentTypefields is appropriate for storing attachment metadata and aligns with the schema definition.app/app/composables/useRequireAuth.ts (3)
25-31: Static analysis false positive on RegExp construction.The static analysis tool flagged line 29 as a potential ReDoS vulnerability. However, the
nameparameter is only ever called with the static string'sendook_token'(line 99), so this is not a security risk.
53-93: LGTM: Well-implemented single-flight pattern with proper error handling.The debouncing via
fetchPromiseprevents duplicate concurrent user fetches, and the error handling appropriately clears the session and redirects on authentication failures.
95-122: LGTM: Clean composable API with reactive state.The composable properly initializes authentication state on mount and exposes a clean API with token, user, organizationId, loading state, and a refreshUser method.
node-sdk/tests/messageAPI.spec.ts (2)
18-18: LGTM: Test updates align with multi-recipient API changes.The updates to pass
toas an array of strings consistently across all test cases align with the API changes for multi-recipient support.Also applies to: 35-35, 54-54, 73-73
40-40: No changes required—SDK implementation correctly accepts the second parameter.The
inbox.message.list()method in node-sdk/index.ts is properly implemented with signaturelist: async (inboxId: string, query?: string), accepting the second parameter and passing it as a query parameter in the axios request. The test usage at line 40 is consistent with this implementation.api/db/mongo/schemas/Message.ts (2)
40-55: Schema fields correctly defined for multi-recipient support.The schema properly defines
toas a required array, andcc,bcc,labelsas optional arrays. However, note that the corresponding model interface (api/models/Message.ts) has inconsistencies that need to be fixed—see my comments on that file.
68-75: LGTM: Attachments schema matches the model structure.The embedded object structure for attachments with
content,name, andcontentTypefields correctly aligns with the model definition.api/routes/v1/domains/index.ts (3)
25-45: LGTM: Proper domain validation and organization scoping.The domain name regex validation at line 31 appropriately validates domain format, and the route correctly scopes domain creation to the requesting organization.
47-63: LGTM: Consistent organization scoping and error handling.All domain retrieval, verification, and deletion endpoints properly scope operations to the requesting organization and return 404 for missing domains.
Also applies to: 65-83, 85-101
15-22: Verify req.organization middleware is properly configured.This route relies on
req.organizationbeing set by authentication middleware. Ensure the parent v1 router properly attaches this middleware before mounting the domains routes (same verification as api_keys).app/app/pages/signup.vue (2)
97-139: LGTM: Robust form submission with proper error handling.The signup handler appropriately validates the response, stores the token securely, and handles errors gracefully with user-friendly messages.
9-35: LGTM: Proper name field implementation with accessibility.The name fields use appropriate autocomplete attributes (
given-name,family-name) and responsive grid layout for improved user experience.api/db/mongo/schemas/Domain.ts (1)
36-36: LGTM: Compound unique index ensures domain uniqueness per organization.The compound index on
organizationIdandnameappropriately prevents duplicate domains within the same organization while allowing the same domain name across different organizations.api/routes/v1/api_keys/index.ts (1)
9-18: Middleware is properly configured—no action needed.Verification confirms that
req.organizationis set by authentication middleware inapi/routes/v1/index.ts(line 14:req.organization = req.user as HydratedDocument<Organization>;) before the api_keys routes are mounted (line 19:router.use("/api_keys", apiKeyRouter);). The middleware is correctly positioned to execute before route handlers.
api/controllers/ApiKeyController.ts
Outdated
| export async function deleteApiKey(apiKeyId: string) { | ||
| return await ApiKey.findByIdAndDelete(apiKeyId); | ||
| } |
There was a problem hiding this comment.
Critical: Missing organization ownership check in deleteApiKey.
The deleteApiKey function uses findByIdAndDelete without verifying that the API key belongs to the requesting organization. This allows an authenticated user to delete API keys from other organizations if they know the key ID.
Apply this diff to add organization ownership verification:
-export async function deleteApiKey(apiKeyId: string) {
- return await ApiKey.findByIdAndDelete(apiKeyId);
+export async function deleteApiKey({
+ apiKeyId,
+ organizationId,
+}: {
+ apiKeyId: string;
+ organizationId: string;
+}) {
+ return await ApiKey.findOneAndDelete({ _id: apiKeyId, organizationId });
}Then update the route handler in api/routes/v1/api_keys/index.ts to pass the organizationId:
await deleteApiKey({
apiKeyId: req.params.apiKeyId,
organizationId: req.organization._id.toString()
});🤖 Prompt for AI Agents
In api/controllers/ApiKeyController.ts around lines 46 to 48, the deleteApiKey
function currently calls findByIdAndDelete without checking organization
ownership; change the function signature to accept an object ({ apiKeyId:
string, organizationId: string }) and replace findByIdAndDelete with a scoped
findOneAndDelete query using {_id: apiKeyId, organization: organizationId} so
only keys belonging to that organization are removed; also update any callers
(per review, update api/routes/v1/api_keys/index.ts) to pass
req.organization._id.toString() as organizationId when invoking deleteApiKey.
api/controllers/SESController.ts
Outdated
| // Create the MIME message using nodemailer's MailComposer | ||
| const mailOptions: any = { | ||
| from: fromName ? `"${fromName}" <${from}>` : from, | ||
| to: (to ?? []).join(", "), | ||
| cc: (cc ?? []).join(", "), | ||
| bcc: (bcc ?? []).join(", "), | ||
| subject, | ||
| text, | ||
| html, | ||
| attachments: (attachments ?? []).map(att => ({ | ||
| filename: att.name, | ||
| content: att.content, | ||
| contentType: att.contentType, | ||
| encoding: "base64", | ||
| })), | ||
| headers: { | ||
| "X-SES-CONFIGURATION-SET": "sendook-config-set", | ||
| "X-SES-MESSAGE-TAGS": `message=${messageId}`, | ||
| }, | ||
| }; | ||
|
|
||
| const composer = new MailComposer(mailOptions); | ||
| const mimeMessage = await new Promise<Buffer>((resolve, reject) => { | ||
| composer.compile().build((err, message) => { | ||
| if (err) return reject(err); | ||
| resolve(message); | ||
| }); | ||
| }); | ||
|
|
||
| const command = new SendRawEmailCommand({ | ||
| RawMessage: { Data: mimeMessage }, | ||
| Tags: [ | ||
| { | ||
| Name: "message", | ||
| Value: messageId, | ||
| }, | ||
| ], | ||
| ConfigurationSetName: "sendook-config-set", | ||
| }); | ||
| return await ses.send(command); | ||
| } |
There was a problem hiding this comment.
Fix BCC delivery in the raw SES payload
MailComposer strips the Bcc header by default. Because we rely on that header to tell SES who the BCC recipients are (and we don’t pass Destinations), those contacts never receive the message. We need to keep the BCC info when building the MIME blob and/or pass the combined recipients explicitly. (nodemailer.com)
Please update the composer usage to preserve BCC and send the recipient list with the command.
Apply this diff:
- const composer = new MailComposer(mailOptions);
- const mimeMessage = await new Promise<Buffer>((resolve, reject) => {
- composer.compile().build((err, message) => {
+ const composer = new MailComposer(mailOptions);
+ const compiled = composer.compile();
+ compiled.keepBcc = true;
+
+ const mimeMessage = await new Promise<Buffer>((resolve, reject) => {
+ compiled.build((err, message) => {
if (err) return reject(err);
resolve(message);
});
});
+ const destinations = [...(to ?? []), ...(cc ?? []), ...(bcc ?? [])].filter(Boolean);
+
const command = new SendRawEmailCommand({
RawMessage: { Data: mimeMessage },
+ Destinations: destinations.length ? destinations : undefined,
Tags: [
{
Name: "message",
Value: messageId,
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/controllers/SESController.ts around lines 67 to 106, MailComposer strips
the Bcc header and you also don’t pass explicit recipients to SES, so BCC
recipients never get the message; fix by building a combined recipients list
(to, cc, bcc flattened and deduplicated), pass that list as the MailComposer
envelope.to (so the MIME blob contains explicit envelope recipients) and also
include that combined list as the SendRawEmailCommand's Destinations array;
ensure you still set the from/envelope.from correctly and keep the existing
headers/attachments mapping.
| records: { | ||
| type: [mongoose.Schema.Types.Mixed], | ||
| default: [ | ||
| { | ||
| type: "MX", | ||
| name: "@", | ||
| value: "inbound-smtp.us-east-2.amazonaws.com", | ||
| status: "pending", | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the hardcoded AWS SES endpoint region.
The default MX record points to inbound-smtp.us-east-2.amazonaws.com (line 25). Ensure this region (us-east-2) matches your AWS SES configuration and that it's appropriate to hardcode this value rather than making it configurable via environment variables.
🏁 Script executed:
#!/bin/bash
# Description: Check if AWS region configuration exists elsewhere in the codebase
# Search for AWS region references
rg -nP 'us-east-2|AWS.*region|SES.*region' --type ts
# Search for environment variable usage for AWS configuration
rg -nP 'process\.env\.AWS' --type tsLength of output: 464
Hardcoded AWS SES region prevents deployment flexibility across regions.
The us-east-2 region is hardcoded in 3 locations (SESController.ts lines 19 and 152; Domain.ts line 25), while AWS credentials are already environment-configurable (SESController.ts lines 21-22). Extract the region into an environment variable (e.g., AWS_REGION or SES_REGION) to maintain consistency and enable multi-region deployments.
🤖 Prompt for AI Agents
In api/db/mongo/schemas/Domain.ts around lines 19 to 29, the SES inbound host
"inbound-smtp.us-east-2.amazonaws.com" is hardcoded; extract the AWS region into
an environment variable (e.g., SES_REGION or AWS_REGION) and replace the literal
with a constructed host using that variable (e.g.,
`inbound-smtp.${region}.amazonaws.com`). Read the region from process.env with a
sensible default/fallback, validate it if appropriate, and update the default
record accordingly; also ensure the same env variable is used consistently in
the other referenced locations (SESController.ts lines ~19 and ~152) so region
configuration is centralized and deployment-region-agnostic.
| to?: string[]; | ||
| cc: string[]; | ||
| bcc: string[]; | ||
| labels: string[]; |
There was a problem hiding this comment.
Fix inconsistencies between model interface and schema.
The required/optional modifiers on these fields don't match the database schema (api/db/mongo/schemas/Message.ts):
- Line 21:
to?: string[]is marked optional here, but the schema hasrequired: true(line 42). - Lines 22-24:
cc,bcc,labelsare marked as required (no?), but the schema hasrequired: false(lines 46, 50, 54).
Apply this diff to align the model with the schema:
- to?: string[];
- cc: string[];
- bcc: string[];
- labels: string[];
+ to: string[];
+ cc?: string[];
+ bcc?: string[];
+ labels?: string[];📝 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.
| to?: string[]; | |
| cc: string[]; | |
| bcc: string[]; | |
| labels: string[]; | |
| to: string[]; | |
| cc?: string[]; | |
| bcc?: string[]; | |
| labels?: string[]; |
🤖 Prompt for AI Agents
In api/models/Message.ts around lines 21 to 24, the TypeScript interface
modifiers don't match the Mongo schema: change `to?: string[]` to required (`to:
string[]`) to match schema's required: true, and change `cc`, `bcc`, and
`labels` to optional (`cc?: string[]`, `bcc?: string[]`, `labels?: string[]`) to
match schema's required: false; update any related type references if necessary
to accept the optional fields.
| router.use("/:organizationId", async (req, res, next) => { | ||
| const organization = await getOrganizationById(req.params.organizationId!); | ||
| if (!organization) { | ||
| return res.status(404).json({ error: "Organization not found" }); | ||
| } | ||
| req.organization = organization; | ||
| next(); | ||
| }); |
There was a problem hiding this comment.
Handle errors from getOrganizationById.
getOrganizationById can reject (DB/network issues). Because this middleware is async, an unhandled rejection skips Express’s error pipeline in v4 and can crash the process. Please wrap the lookup in try/catch and forward failures to next(err).
Apply this diff:
-router.use("/:organizationId", async (req, res, next) => {
- const organization = await getOrganizationById(req.params.organizationId!);
- if (!organization) {
- return res.status(404).json({ error: "Organization not found" });
- }
- req.organization = organization;
- next();
-});
+router.use("/:organizationId", async (req, res, next) => {
+ try {
+ const organization = await getOrganizationById(req.params.organizationId!);
+ if (!organization) {
+ return res.status(404).json({ error: "Organization not found" });
+ }
+ req.organization = organization;
+ return next();
+ } catch (error) {
+ return next(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.use("/:organizationId", async (req, res, next) => { | |
| const organization = await getOrganizationById(req.params.organizationId!); | |
| if (!organization) { | |
| return res.status(404).json({ error: "Organization not found" }); | |
| } | |
| req.organization = organization; | |
| next(); | |
| }); | |
| router.use("/:organizationId", async (req, res, next) => { | |
| try { | |
| const organization = await getOrganizationById(req.params.organizationId!); | |
| if (!organization) { | |
| return res.status(404).json({ error: "Organization not found" }); | |
| } | |
| req.organization = organization; | |
| return next(); | |
| } catch (error) { | |
| return next(error); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In api/routes/organizations.ts around lines 17 to 24, the middleware calls the
async getOrganizationById without catching rejections; wrap the await call in a
try/catch, on success keep the existing not-found check and assignment to
req.organization and call next(), and on error call next(err) to forward
DB/network errors into Express error handling so the process won’t crash.
| const replyToMessage = await getMessageById(req.params.messageId); | ||
| if ( | ||
| !replyToMessage || | ||
| replyToMessage.organizationId.toString() !== organization._id.toString() | ||
| replyToMessage.organizationId.toString() !== req.organization._id.toString() | ||
| ) { | ||
| return res.status(404).json({ error: "Message not found" }); | ||
| } | ||
|
|
||
| const message = await createMessage({ | ||
| organizationId: organization._id.toString(), | ||
| organizationId: req.organization._id.toString(), | ||
| inboxId: req.params.inboxId, | ||
| threadId: replyToMessage.threadId.toString(), | ||
| fromInboxId: req.params.inboxId, | ||
| from: inbox.email, | ||
| to: replyToMessage.from, | ||
| to: [replyToMessage.from], | ||
| subject: `Re: ${replyToMessage.subject}`, |
There was a problem hiding this comment.
Guard replies to the owning inbox only.
We only verify the organization match before replying, so a client can call POST /inboxes/{otherInboxId}/messages/{messageId}/reply and we’ll happily create a reply under the wrong inbox while reusing the original thread. That mixes records across inboxes and leaks the other inbox’s conversation context. Please ensure replyToMessage.inboxId matches req.params.inboxId (return 404 otherwise) before creating the reply.
Apply this diff to enforce the guard:
const replyToMessage = await getMessageById(req.params.messageId);
if (
!replyToMessage ||
replyToMessage.organizationId.toString() !== req.organization._id.toString()
) {
return res.status(404).json({ error: "Message not found" });
}
+ if (replyToMessage.inboxId.toString() !== req.params.inboxId) {
+ return res.status(404).json({ error: "Message not found" });
+ }📝 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.
| const replyToMessage = await getMessageById(req.params.messageId); | |
| if ( | |
| !replyToMessage || | |
| replyToMessage.organizationId.toString() !== organization._id.toString() | |
| replyToMessage.organizationId.toString() !== req.organization._id.toString() | |
| ) { | |
| return res.status(404).json({ error: "Message not found" }); | |
| } | |
| const message = await createMessage({ | |
| organizationId: organization._id.toString(), | |
| organizationId: req.organization._id.toString(), | |
| inboxId: req.params.inboxId, | |
| threadId: replyToMessage.threadId.toString(), | |
| fromInboxId: req.params.inboxId, | |
| from: inbox.email, | |
| to: replyToMessage.from, | |
| to: [replyToMessage.from], | |
| subject: `Re: ${replyToMessage.subject}`, | |
| const replyToMessage = await getMessageById(req.params.messageId); | |
| if ( | |
| !replyToMessage || | |
| replyToMessage.organizationId.toString() !== req.organization._id.toString() | |
| ) { | |
| return res.status(404).json({ error: "Message not found" }); | |
| } | |
| if (replyToMessage.inboxId.toString() !== req.params.inboxId) { | |
| return res.status(404).json({ error: "Message not found" }); | |
| } | |
| const message = await createMessage({ | |
| organizationId: req.organization._id.toString(), | |
| inboxId: req.params.inboxId, | |
| threadId: replyToMessage.threadId.toString(), | |
| fromInboxId: req.params.inboxId, | |
| from: inbox.email, | |
| to: [replyToMessage.from], | |
| subject: `Re: ${replyToMessage.subject}`, |
🤖 Prompt for AI Agents
In api/routes/v1/inboxes/messages.ts around lines 172 to 187, the code only
checks organization ownership before creating a reply which allows creating a
reply under a different inbox; add a guard to verify replyToMessage.inboxId
matches req.params.inboxId (use string comparison, e.g.
replyToMessage.inboxId.toString() !== req.params.inboxId) and return
res.status(404).json({ error: "Message not found" }) if it doesn’t match, before
calling createMessage.
| interface Request { | ||
| organization: HydratedDocument<Organization>; | ||
| } |
There was a problem hiding this comment.
Missing import for Organization type.
The Request interface references HydratedDocument<Organization>, but the Organization type is not imported. This will cause a TypeScript compilation error.
Apply this diff to add the missing import:
import type { HydratedDocument } from "mongoose";
import type User from "../models/User";
+import type Organization from "../models/Organization";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/types/environment.d.ts around lines 18 to 20, the Request interface uses
HydratedDocument<Organization> but the Organization type is not imported; add an
import for Organization (from its module, e.g., models/organization or the
correct path in this codebase) at the top of the file and ensure
HydratedDocument is imported from mongoose if not already present so the Request
interface compiles correctly.
| <div class="search-field"> | ||
| <svg class="icon" viewBox="0 0 24 24" aria-hidden="true"> | ||
| <path | ||
| d="M4 6.5A2.5 2.5 0 0 1 6.5 4h11A2.5 2.5 0 0 1 20 6.5v11a2.5 2.5 0 0 1-2.5 2.5h-11A2.5 2.5 0 0 1 4 17.5v-11Z" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| stroke-width="1.5" | ||
| /> | ||
| <path | ||
| d="M5 7.5 11.45 12a1 1 0 0 0 1.1 0L19 7.5" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| stroke-width="1.5" | ||
| stroke-linecap="round" | ||
| stroke-linejoin="round" | ||
| /> | ||
| </svg> | ||
| <input type="search" placeholder="Search messages"> | ||
| </div> | ||
| <select> | ||
| <option>Last 15 days</option> | ||
| <option>Last 30 days</option> | ||
| <option>Last 90 days</option> | ||
| </select> | ||
| <select> | ||
| <option>All statuses</option> | ||
| <option>Delivered</option> | ||
| <option>Queued</option> | ||
| <option>Bounced</option> | ||
| </select> | ||
| <select> | ||
| <option>All API keys</option> | ||
| <option>Production</option> | ||
| <option>Staging</option> | ||
| </select> |
There was a problem hiding this comment.
Add accessible labels for the search and filter controls.
The search field and all three <select> elements have no associated label or ARIA name. Placeholders and visible context aren’t announced by screen readers, so these controls surface as “edit text” / “combo box” with no description, which blocks non-visual users from applying filters. Please add explicit labels or ARIA attributes.
Apply this diff so each control exposes an accessible name:
- <input type="search" placeholder="Search messages">
+ <input
+ type="search"
+ placeholder="Search messages"
+ aria-label="Search messages"
+ >
- <select>
+ <select aria-label="Date range">
<option>Last 15 days</option>
<option>Last 30 days</option>
<option>Last 90 days</option>
</select>
- <select>
+ <select aria-label="Delivery status">
<option>All statuses</option>
<option>Delivered</option>
<option>Queued</option>
<option>Bounced</option>
</select>
- <select>
+ <select aria-label="API key scope">
<option>All API keys</option>
<option>Production</option>
<option>Staging</option>
</select>📝 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.
| <div class="search-field"> | |
| <svg class="icon" viewBox="0 0 24 24" aria-hidden="true"> | |
| <path | |
| d="M4 6.5A2.5 2.5 0 0 1 6.5 4h11A2.5 2.5 0 0 1 20 6.5v11a2.5 2.5 0 0 1-2.5 2.5h-11A2.5 2.5 0 0 1 4 17.5v-11Z" | |
| fill="none" | |
| stroke="currentColor" | |
| stroke-width="1.5" | |
| /> | |
| <path | |
| d="M5 7.5 11.45 12a1 1 0 0 0 1.1 0L19 7.5" | |
| fill="none" | |
| stroke="currentColor" | |
| stroke-width="1.5" | |
| stroke-linecap="round" | |
| stroke-linejoin="round" | |
| /> | |
| </svg> | |
| <input type="search" placeholder="Search messages"> | |
| </div> | |
| <select> | |
| <option>Last 15 days</option> | |
| <option>Last 30 days</option> | |
| <option>Last 90 days</option> | |
| </select> | |
| <select> | |
| <option>All statuses</option> | |
| <option>Delivered</option> | |
| <option>Queued</option> | |
| <option>Bounced</option> | |
| </select> | |
| <select> | |
| <option>All API keys</option> | |
| <option>Production</option> | |
| <option>Staging</option> | |
| </select> | |
| <div class="search-field"> | |
| <svg class="icon" viewBox="0 0 24 24" aria-hidden="true"> | |
| <path | |
| d="M4 6.5A2.5 2.5 0 0 1 6.5 4h11A2.5 2.5 0 0 1 20 6.5v11a2.5 2.5 0 0 1-2.5 2.5h-11A2.5 2.5 0 0 1 4 17.5v-11Z" | |
| fill="none" | |
| stroke="currentColor" | |
| stroke-width="1.5" | |
| /> | |
| <path | |
| d="M5 7.5 11.45 12a1 1 0 0 0 1.1 0L19 7.5" | |
| fill="none" | |
| stroke="currentColor" | |
| stroke-width="1.5" | |
| stroke-linecap="round" | |
| stroke-linejoin="round" | |
| /> | |
| </svg> | |
| <input | |
| type="search" | |
| placeholder="Search messages" | |
| aria-label="Search messages" | |
| > | |
| </div> | |
| <select aria-label="Date range"> | |
| <option>Last 15 days</option> | |
| <option>Last 30 days</option> | |
| <option>Last 90 days</option> | |
| </select> | |
| <select aria-label="Delivery status"> | |
| <option>All statuses</option> | |
| <option>Delivered</option> | |
| <option>Queued</option> | |
| <option>Bounced</option> | |
| </select> | |
| <select aria-label="API key scope"> | |
| <option>All API keys</option> | |
| <option>Production</option> | |
| <option>Staging</option> | |
| </select> |
🤖 Prompt for AI Agents
In app/app/pages/index.vue around lines 21 to 55, the search input and three
select elements lack accessible names; add explicit associations by giving each
control a unique id and either a visible <label for="..."> (using a
visually-hidden CSS class if you don't want it shown) or an appropriate
aria-label/aria-labelledby attribute. For example, assign ids like
search-messages, date-range, message-status, api-key-env and add labels or
aria-labels such as "Search messages", "Date range", "Message status", and "API
key environment" so screen readers announce each control; keep existing
placeholders/options unchanged.
| const tokenCookie = useCookie('sendook_token', { | ||
| maxAge: 60 * 60 * 24 * 7, | ||
| sameSite: 'lax' | ||
| }); | ||
| tokenCookie.value = response.token; | ||
| await navigateTo('/'); | ||
| } catch (error: unknown) { | ||
| if (error && typeof error === 'object' && 'data' in error) { | ||
| const fetchError = error as { data?: { message?: string } }; | ||
| errorMessage.value = fetchError.data?.message ?? 'Unable to sign in. Please try again.'; | ||
| } else if (error instanceof Error) { | ||
| errorMessage.value = error.message; | ||
| } else { | ||
| errorMessage.value = 'Unable to sign in. Please try again.'; | ||
| } | ||
| } finally { | ||
| loading.value = false; | ||
| } |
There was a problem hiding this comment.
Set the session cookie with Secure outside of development
Right now the auth token cookie is created without the Secure flag, so any HTTP access to the same host can leak it. Because this token authenticates every subsequent call, that’s a security problem once the app is served over HTTPS. Please ensure the cookie is marked secure whenever you’re not in dev (and keep it disabled locally so the cookie still sets over plain HTTP). (nuxt.com)
Apply this diff:
- const tokenCookie = useCookie('sendook_token', {
- maxAge: 60 * 60 * 24 * 7,
- sameSite: 'lax'
- });
+ const tokenCookie = useCookie('sendook_token', {
+ maxAge: 60 * 60 * 24 * 7,
+ sameSite: 'lax',
+ secure: !import.meta.dev
+ });📝 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.
| const tokenCookie = useCookie('sendook_token', { | |
| maxAge: 60 * 60 * 24 * 7, | |
| sameSite: 'lax' | |
| }); | |
| tokenCookie.value = response.token; | |
| await navigateTo('/'); | |
| } catch (error: unknown) { | |
| if (error && typeof error === 'object' && 'data' in error) { | |
| const fetchError = error as { data?: { message?: string } }; | |
| errorMessage.value = fetchError.data?.message ?? 'Unable to sign in. Please try again.'; | |
| } else if (error instanceof Error) { | |
| errorMessage.value = error.message; | |
| } else { | |
| errorMessage.value = 'Unable to sign in. Please try again.'; | |
| } | |
| } finally { | |
| loading.value = false; | |
| } | |
| const tokenCookie = useCookie('sendook_token', { | |
| maxAge: 60 * 60 * 24 * 7, | |
| sameSite: 'lax', | |
| secure: !import.meta.dev | |
| }); | |
| tokenCookie.value = response.token; | |
| await navigateTo('/'); | |
| } catch (error: unknown) { | |
| if (error && typeof error === 'object' && 'data' in error) { | |
| const fetchError = error as { data?: { message?: string } }; | |
| errorMessage.value = fetchError.data?.message ?? 'Unable to sign in. Please try again.'; | |
| } else if (error instanceof Error) { | |
| errorMessage.value = error.message; | |
| } else { | |
| errorMessage.value = 'Unable to sign in. Please try again.'; | |
| } | |
| } finally { | |
| loading.value = false; | |
| } |
🤖 Prompt for AI Agents
In app/app/pages/login.vue around lines 86 to 104, the auth token cookie is
created without the Secure flag; update the useCookie options to include a
secure property that is true when not running in development (for example
secure: process.env.NODE_ENV !== 'development' or using Nuxt's runtime/dev
flag), so the cookie is marked Secure in production/HTTPS but remains unset for
local HTTP development; keep all other options (maxAge, sameSite) as-is and
ensure the secure value is computed before assigning tokenCookie.value.
| <div class="user-avatar">M</div> | ||
| <div class="user-details"> | ||
| <p class="user-name">Marc Whitbread</p> | ||
| <p class="user-email">marc@sendook.dev</p> |
There was a problem hiding this comment.
Critical: Hardcoded user data must be replaced with dynamic values.
The user avatar, name, and email are hardcoded in the template. This will display the same information for all users and is a blocker for production deployment.
Replace the hardcoded values with dynamic user data from your authentication system:
<div class="user-avatar">{{ userInitial }}</div>
<div class="user-details">
<p class="user-name">{{ userName }}</p>
<p class="user-email">{{ userEmail }}</p>
</div>Then add computed properties or props in your script section to provide these values from useRequireAuth() or a similar composable.
🤖 Prompt for AI Agents
In app/layouts/default.vue around lines 29 to 32 the template currently contains
hardcoded user info (avatar "M", name "Marc Whitbread", email
"marc@sendook.dev"); replace these with dynamic template bindings (e.g.
userAvatar/initial, userName, userEmail) and wire them up in the script section
by importing your auth composable (useRequireAuth or equivalent), deriving
userName and userEmail from the returned user object and computing userInitial
(first letter of name or email) as a computed property; ensure the template uses
the computed values and that the composable is awaited/handled appropriately so
no undefined values render.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
api/controllers/SESController.ts (1)
64-100: Restore BCC delivery in the SES MIME flow.
MailComposer never receives abccfield, so BCC recipients are dropped. Even if we add it, the composer strips the header unless we callcompiled.keepBcc = true, and SES still refuses to deliver unless we pass the flattened recipient list asDestinations. Right now every BCC recipient silently misses the email. Please wire the BCC data through and keep it in the built MIME.Apply this diff to preserve BCC recipients:
const mailOptions: any = { from: fromName ? `"${fromName}" <${from}>` : from, to: (to ?? []).join(", "), cc: (cc ?? []).join(", "), + bcc: (bcc ?? []).join(", "), subject, text, html, @@ - const composer = new MailComposer(mailOptions); - const mimeMessage = await new Promise<Buffer>((resolve, reject) => { - composer.compile().build((err, message) => { + const composer = new MailComposer(mailOptions); + const compiled = composer.compile(); + compiled.keepBcc = true; + + const mimeMessage = await new Promise<Buffer>((resolve, reject) => { + compiled.build((err, message) => { if (err) return reject(err); resolve(message); }); }); + const destinations = Array.from( + new Set([...(to ?? []), ...(cc ?? []), ...(bcc ?? [])].filter(Boolean)) + ); + const command = new SendRawEmailCommand({ RawMessage: { Data: mimeMessage }, + Destinations: destinations.length ? destinations : undefined, Tags: [ { Name: "message",api/routes/v1/inboxes/messages.ts (1)
193-204: Block cross-inbox replies.
We still only check organization ownership here, so a client can reply to a message that belongs to a different inbox under the same org and we’ll file the response under the wrong inbox/thread. That mixes conversations and leaks context. Please reject whenreplyToMessage.inboxIddoesn’t match the requested inbox.Apply this diff to enforce the guard:
if ( !replyToMessage || replyToMessage.organizationId.toString() !== req.organization._id.toString() ) { return res.status(404).json({ error: "Message not found" }); } + if (replyToMessage.inboxId.toString() !== req.params.inboxId) { + return res.status(404).json({ error: "Message not found" }); + }
🧹 Nitpick comments (3)
app/app/pages/domains.vue (3)
91-96: Add domain name format validation.The domain input only has the
requiredattribute but doesn't validate the domain format. Users could submit invalid values like "not a domain" or "example..com", leading to backend errors.Add pattern validation to the input:
<input v-model="form.name" type="text" placeholder="example.com" required + pattern="^([a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}$" + title="Please enter a valid domain name (e.g., example.com)" :disabled="saving">Alternatively, add validation in
handleCreateDomainbefore the API call:const domainPattern = /^([a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}$/; if (!domainPattern.test(form.name.trim())) { errorMessage.value = 'Please enter a valid domain name (e.g., example.com)'; return; }
196-226: Domain loading errors are not visible to users.When
loadDomainsfails, the error is logged to the console and setserrorMessage.value(line 221), but this error is not displayed anywhere in the UI. TheerrorMessageis only shown in the create domain dialog, not in the main domains view.Add a dedicated state and UI element for loading errors:
const errorMessage = ref(''); +const loadError = ref(''); const loadDomains = async () => { // ... existing code ... + loadError.value = ''; try { // ... existing code ... } catch (error) { console.error('Failed to load domains', error); - errorMessage.value = 'Unable to load domains. Please try again.'; + loadError.value = 'Unable to load domains. Please try again.'; domains.value = []; } finally { loading.value = false; } };Then display it in the template:
<template v-else> + <div v-if="loadError" class="banner banner-error" role="alert"> + {{ loadError }} + </div> <div v-if="verificationMessage" class="banner banner-success"> {{ verificationMessage }} </div> <!-- ... --> </template>
217-218: Consider simplifying response type handling.The nested ternary operator for handling different response formats works correctly but is hard to read.
Extract into a helper function for clarity:
+const normalizeDomains = (data: unknown): Domain[] => { + if (Array.isArray(data)) return data; + if (data && typeof data === 'object' && 'data' in data && Array.isArray(data.data)) { + return data.data; + } + return []; +}; const response = await fetch(/* ... */); const data = await response.json(); -domains.value = Array.isArray(data) ? data : Array.isArray(data?.data) ? data.data : []; +domains.value = normalizeDomains(data);
📜 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 (7)
api/controllers/ApiKeyController.ts(2 hunks)api/controllers/SESController.ts(3 hunks)api/db/redis/index.ts(1 hunks)api/package.json(2 hunks)api/routes/v1/api_keys/index.ts(1 hunks)api/routes/v1/inboxes/messages.ts(8 hunks)app/app/pages/domains.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/controllers/ApiKeyController.ts
- api/package.json
- api/routes/v1/api_keys/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
api/routes/v1/inboxes/messages.ts (5)
api/db/redis/index.ts (1)
redis(3-3)api/controllers/InboxController.ts (1)
getInboxByOrganizationIdAndInboxId(32-43)api/controllers/ThreadController.ts (1)
createThread(4-17)api/controllers/MessageController.ts (1)
getMessages(86-110)api/controllers/SESController.ts (1)
sendSESMessage(37-101)
🔇 Additional comments (1)
app/app/pages/domains.vue (1)
146-383: Well-structured component with proper security practices.The component demonstrates good practices:
- Proper use of Vue 3 Composition API
- Secure API calls with bearer token authentication
- Appropriate error handling and loading states
- Reactive session-based data loading
- Clean separation of concerns
| </section> | ||
|
|
||
| <section v-else class="domains-grid"> | ||
| <article v-for="domain in domains" :key="domain._id ?? domain.name" class="domain-card"> |
There was a problem hiding this comment.
Inconsistent domain identifier usage across operations.
The code uses different fallback orders for domain identifiers:
- Line 37: Key uses
domain._id ?? domain.name - Lines 68-69, 253, 266: Uses
domain.name ?? domain._id - Line 338, 349: Uses only
pendingDelete.value?._id
This inconsistency could cause issues if the backend expects a specific identifier type (name vs _id) in API endpoints. For example, the verify endpoint might receive a domain name while the delete endpoint receives an _id for the same domain.
Standardize identifier usage across all operations. Determine whether the backend expects name or _id in URL paths, then use that consistently:
-const domainId = domain.name ?? domain._id;
+// If backend expects name:
+const domainId = domain.name;
+// OR if backend expects _id:
+const domainId = domain._id;
// Also standardize the key in the v-for:
-:key="domain._id ?? domain.name"
+:key="domain._id"Additionally, add validation to ensure the required identifier exists before making API calls.
Also applies to: 68-69, 253-253, 266-266, 338-338, 349-349
| <footer v-if="!domain.verified"> | ||
| <span>{{ domain.verified ? 'Verified' : 'DNS pending verification' }}</span> |
There was a problem hiding this comment.
Dead code: 'Verified' text is unreachable.
The footer only renders when !domain.verified (line 63), but line 64 includes a ternary that checks domain.verified. The "Verified" branch will never execute.
Simplify to remove the dead code:
<footer v-if="!domain.verified">
- <span>{{ domain.verified ? 'Verified' : 'DNS pending verification' }}</span>
+ <span>DNS pending verification</span>📝 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.
| <footer v-if="!domain.verified"> | |
| <span>{{ domain.verified ? 'Verified' : 'DNS pending verification' }}</span> | |
| <footer v-if="!domain.verified"> | |
| <span>DNS pending verification</span> |
🤖 Prompt for AI Agents
In app/app/pages/domains.vue around lines 63 to 64, the footer is only rendered
when !domain.verified but the span contains a ternary that includes a "Verified"
branch which is unreachable; simplify by removing the ternary and render the
appropriate static text (e.g., "DNS pending verification") or adjust the
conditional so the span reflects the correct state (either show the span only
when not verified with static "DNS pending verification", or render a separate
footer for the verified case).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/app/pages/inbox/[inboxId].vue (2)
86-88: Remove commented code.The
routerimport is commented out and unused. Clean up dead code to improve maintainability.const route = useRoute(); -// const router = useRouter(); const session = useRequireAuth();
190-192: Consider structured error logging over console.error.Using
console.errordirectly logs errors only to the browser console. For production environments, consider using a structured logging service or error tracking tool (e.g., Sentry) for better observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/app/components/dashboard/DashboardShell.vue(1 hunks)app/app/pages/inbox/[inboxId].vue(1 hunks)app/app/pages/inboxes.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/app/pages/inboxes.vue
🔇 Additional comments (8)
app/app/pages/inbox/[inboxId].vue (4)
119-119: Verify fallback to_idfor inbox address display.Falling back to the MongoDB
_idwhen
121-131: LGTM!The client-side search filtering logic correctly handles optional fields, filters falsy values, and performs case-insensitive matching across multiple message properties.
133-147: LGTM!The date formatting helper correctly validates input, handles invalid dates gracefully, and provides locale-aware formatting.
198-206: LGTM!The watcher correctly tracks all dependencies and refetches messages when organization, inbox, or auth token changes, with immediate execution on mount.
app/app/components/dashboard/DashboardShell.vue (4)
60-74: LGTM!The user display name logic correctly prioritizes first/last name over email, with proper type narrowing and filtering of empty values.
78-83: LGTM!The active link detection correctly handles the root path with exact matching and nested paths with prefix matching.
85-90: LGTM!The logout handler correctly clears the auth cookie client-side and redirects using
replaceto prevent navigating back to authenticated state. Thetypeof documentcheck ensures SSR safety.
94-111: Verify sidebar width and shell padding alignment.The dashboard shell has
padding-left: 240px(line 96) but the sidebar width is200px(line 108), creating a 40px gap. Verify whether this spacing is intentional or if the values should match.
| @media (max-width: 1024px) { | ||
| .dashboard-shell { | ||
| grid-template-columns: 1fr; | ||
| } | ||
| .dashboard-sidebar { | ||
| flex-direction: row; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| } | ||
| .dashboard-sidebar ul { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| } | ||
| .dashboard-main { | ||
| padding: 1.75rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Responsive layout is broken.
The media query attempts to set grid-template-columns: 1fr on .dashboard-shell (line 234), but the shell doesn't use CSS Grid layout—it uses padding-left positioning. This rule has no effect, and the responsive layout won't work correctly.
Fix the responsive layout by removing the fixed positioning and adjusting the shell padding:
@media (max-width: 1024px) {
.dashboard-shell {
- grid-template-columns: 1fr;
+ padding-left: 0;
}
.dashboard-sidebar {
+ position: relative;
+ width: 100%;
+ height: auto;
flex-direction: row;
align-items: center;
justify-content: space-between;
}You'll also need to adjust the sidebar layout significantly for mobile, as the current fixed positioning with transformed flex-direction won't provide a usable mobile experience.
📝 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.
| @media (max-width: 1024px) { | |
| .dashboard-shell { | |
| grid-template-columns: 1fr; | |
| } | |
| .dashboard-sidebar { | |
| flex-direction: row; | |
| align-items: center; | |
| justify-content: space-between; | |
| } | |
| .dashboard-sidebar ul { | |
| display: flex; | |
| flex-wrap: wrap; | |
| } | |
| .dashboard-main { | |
| padding: 1.75rem; | |
| } | |
| } | |
| @media (max-width: 1024px) { | |
| .dashboard-shell { | |
| padding-left: 0; | |
| } | |
| .dashboard-sidebar { | |
| position: relative; | |
| width: 100%; | |
| height: auto; | |
| flex-direction: row; | |
| align-items: center; | |
| justify-content: space-between; | |
| } | |
| .dashboard-sidebar ul { | |
| display: flex; | |
| flex-wrap: wrap; | |
| } | |
| .dashboard-main { | |
| padding: 1.75rem; | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/app/components/dashboard/DashboardShell.vue around lines 232 to 251, the
media query sets grid-template-columns on .dashboard-shell even though the shell
uses fixed positioning and padding-left for layout; remove/override the fixed
positioning and left padding rules for mobile, switch the shell to a
single-column flow (e.g., make it static/position:initial and remove the left
padding so content can stack), and update .dashboard-sidebar to be a normal
block-level section that spans full width (remove transforms/fixed behavior),
change the sidebar list to a vertical column (no wrap, display: block or column
flex) and adjust .dashboard-main padding to the provided smaller value so
content is usable on small screens; ensure all position/transform/fixed styles
applied elsewhere for desktop are overridden inside this media query so the
mobile layout stacks correctly.
| <section v-if="loading" class="placeholder-card"> | ||
| <h2>Loading messages…</h2> | ||
| <p>Fetching the conversation history for this inbox.</p> | ||
| </section> | ||
|
|
||
| <section v-else-if="messages.length === 0" class="placeholder-card"> | ||
| <h2>No messages yet</h2> | ||
| <p>Send a test email to this inbox or wait for new activity to appear here.</p> | ||
| </section> |
There was a problem hiding this comment.
Error state is tracked but never displayed.
The component tracks an error ref (line 111) and populates it in the catch block (line 192), but there's no corresponding error display in the template. Users won't see fetch failures.
Add an error display section:
+ <section v-else-if="error" class="placeholder-card error-card">
+ <h2>Unable to load messages</h2>
+ <p>{{ error }}</p>
+ </section>
+
<section v-else-if="messages.length === 0" class="placeholder-card">
<h2>No messages yet</h2>
<p>Send a test email to this inbox or wait for new activity to appear here.</p>
</section>You'll also need to add error styling:
.error-card {
border-color: rgba(248, 113, 113, 0.2);
background: rgba(248, 113, 113, 0.05);
}
.error-card h2 {
color: #fecaca;
}🤖 Prompt for AI Agents
In app/app/pages/inbox/[inboxId].vue around lines 24 to 32, the template never
shows the tracked error ref; insert an error display section (e.g., <section
v-else-if="error" class="error-card"> with an h2 and a paragraph showing a short
message and the error text) between the loading and empty states so the order
becomes: loading -> error -> no-messages -> messages; also add the provided CSS
rules (.error-card and .error-card h2) to the component/style block so the error
state is visually distinct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node-sdk/openapi.yaml (2)
50-59: Add pagination parameters andmaxItemsconstraints to list endpoint responses.Array responses lack
maxItemsconstraints, and list endpoints (/v1/api_keys,/v1/inboxes,/v1/inboxes/{inboxId}/messages,/v1/inboxes/{inboxId}/threads) have no pagination parameters documented. Consider adding:
- Query parameters for pagination (e.g.,
limit,offset, orpage,pageSize)maxItemsconstraint on array schemas to prevent unbounded responses- Clearer documentation of how clients should handle large result sets
For example, update
listApiKeysto include pagination:get: tags: - API Keys summary: List API keys operationId: listApiKeys + parameters: + - name: limit + in: query + required: false + description: Maximum number of items to return + schema: + type: integer + default: 25 + maximum: 100 + - name: offset + in: query + required: false + description: Number of items to skip + schema: + type: integer + default: 0 responses: "200": description: A collection of API keys content: application/json: schema: type: object properties: data: type: array + maxItems: 100 items: $ref: '#/components/schemas/ApiKey'Also applies to: 220-224, 339-342, 387-390
555-561: Reconsider strict requirement for bothtextandhtmlin message requests.Both
MessageSendRequestandMessageReplyRequestrequire bothtextandhtmlfields. In practice, many email APIs allow either/or or just one. If your backend supports sending text-only or HTML-only messages, adjust the schema to reflect that flexibility:MessageSendRequest: type: object - required: - - to - - subject - - text - - html + required: + - to + - subject properties: to: type: array items: type: string format: email description: Primary recipients. cc: type: array items: type: string format: email bcc: type: array items: type: string format: email labels: type: array items: type: string subject: type: string text: type: string + nullable: true html: type: string + nullable: true(Adjust based on your actual backend constraints.)
Also applies to: 589-598
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node-sdk/openapi.yaml(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
node-sdk/openapi.yaml
[medium] 57-61: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (2)
node-sdk/openapi.yaml (2)
555-588: Missing attachment support in message schemas.The PR summary mentions "multi-recipient messages with embedded attachments," but the
MessageSendRequestandMessageReplyRequestschemas lack anattachmentsfield. Verify whether attachments are intended to be part of these payloads or handled separately, and update the schema accordingly.
1-70: Well-structured and comprehensive OpenAPI specification.The overall organization, endpoint design, consistent parameter naming, and proper HTTP status codes align well with REST conventions. The spec clearly documents the API contract for all five resource groups and will be useful for SDK generation and client integration.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
landing/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (6)
landing/content/1.docs/3.api/1.index.md(1 hunks)landing/content/1.docs/3.api/2.api-keys.md(1 hunks)landing/content/1.docs/3.api/3.domains.md(1 hunks)landing/content/1.docs/3.api/4.inboxes.md(1 hunks)landing/content/1.docs/3.api/5.messages.md(1 hunks)landing/content/1.docs/3.api/6.threads.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- landing/content/1.docs/3.api/2.api-keys.md
- landing/content/1.docs/3.api/1.index.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
landing/content/1.docs/3.api/4.inboxes.md
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
landing/content/1.docs/3.api/4.inboxes.md (1)
1-95: Documentation structure and content look good.The inbox API documentation is comprehensive, well-organized, and clearly describes each operation with appropriate request/response examples. Field descriptions are helpful, and the delete endpoint properly communicates side effects.
Also applies to: 99-101
landing/content/1.docs/3.api/3.domains.md (1)
1-76: Domain API documentation is complete and well-structured.The documentation clearly describes the domain lifecycle from creation through verification, with appropriate status values and async handling via polling. The post-creation guidance about DKIM/SPF is helpful for onboarding flows.
Also applies to: 80-99
landing/content/1.docs/3.api/5.messages.md (1)
1-137: Message API documentation is comprehensive and well-organized.The endpoint descriptions, request/response examples, and status codes are clear and accurate. The async send pattern with 202 Accepted is properly documented, and the querying/filtering mechanism is well-explained.
landing/content/1.docs/3.api/6.threads.md (1)
1-81: Thread API documentation is clear and complete.The documentation effectively explains the thread concept and provides clear examples of both list and retrieve operations. The note about message ordering (newest→oldest) and guidance on accessing full message metadata is helpful for API consumers building conversational UIs.
| ``` | ||
| <empty> | ||
| ``` |
There was a problem hiding this comment.
Specify language tag for empty code block.
Line 77 contains a fenced code block without a language specification. The formatting should be updated to resolve the markdown linting warning.
Apply this diff to resolve the linting issue:
### Response `204 No Content`
-```
-<empty>
-```
+```text
+<empty>
+```🤖 Prompt for AI Agents
In landing/content/1.docs/3.api/3.domains.md around lines 77 to 79, the fenced
code block on line 77 is missing a language tag; update the fence to include a
language (use "text") so the block becomes ```text, keep the existing empty
content inside the block, and close with ```, ensuring the diff replaces the
empty ``` with ```text and the content remains unchanged.
| ``` | ||
| <empty> | ||
| ``` |
There was a problem hiding this comment.
Specify language tag for empty code block.
Line 96 contains a fenced code block without a language specification, which triggers markdown linting warnings. While the empty response is semantically correct for 204 No Content, the formatting should be updated.
Apply this diff to resolve the linting issue:
### Response `204 No Content`
-```
-<empty>
-```
+```text
+<empty>
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In landing/content/1.docs/3.api/4.inboxes.md around lines 96-98 there is a
fenced code block for an empty response that lacks a language tag, causing
markdown lint warnings; update the opening fence to include a language tag (use
"text") so the block reads as a fenced code block with language "text", keep the
"<empty>" line inside and preserve the closing fence.
| ## Message schema | ||
|
|
||
| The Message schema includes the following core fields (additional metadata may be present): | ||
|
|
||
| | Field | Type | Description | | ||
| | ----- | ---- | ----------- | | ||
| | `id` | string | Unique message identifier. | | ||
| | `subject` | string | Email subject line. | | ||
| | `text` | string | Plain-text content. | | ||
| | `html` | string | HTML content. | | ||
| | `labels` | string[] | Custom labels assigned to the message. | | ||
| | `createdAt` | string (date-time) | Timestamp when the message was created. | | ||
|
|
There was a problem hiding this comment.
Add missing fields to Message schema table.
The Message schema table is incomplete. It documents only id, subject, text, html, labels, and createdAt, but the request/response examples throughout the documentation include to, cc, bcc fields that should be documented in the schema. Additionally, the AI summary references embedded attachments, which should also be included if supported by the API.
Expand the schema table to include all documented message fields:
| Field | Type | Description |
| ----- | ---- | ----------- |
+| `to` | string[] | Primary recipient email addresses. |
+| `cc` | string[] | Carbon copy recipient email addresses. |
+| `bcc` | string[] | Blind carbon copy recipient email addresses. |
| `id` | string | Unique message identifier. |
| `subject` | string | Email subject line. |
| `text` | string | Plain-text content. |
| `html` | string | HTML content. |
| `labels` | string[] | Custom labels assigned to the message. |
+| `attachments` | object[] | Embedded attachments with content, name, and contentType. |
| `createdAt` | string (date-time) | Timestamp when the message was created. |📝 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.
| ## Message schema | |
| The Message schema includes the following core fields (additional metadata may be present): | |
| | Field | Type | Description | | |
| | ----- | ---- | ----------- | | |
| | `id` | string | Unique message identifier. | | |
| | `subject` | string | Email subject line. | | |
| | `text` | string | Plain-text content. | | |
| | `html` | string | HTML content. | | |
| | `labels` | string[] | Custom labels assigned to the message. | | |
| | `createdAt` | string (date-time) | Timestamp when the message was created. | | |
| ## Message schema | |
| The Message schema includes the following core fields (additional metadata may be present): | |
| | Field | Type | Description | | |
| | ----- | ---- | ----------- | | |
| | `to` | string[] | Primary recipient email addresses. | | |
| | `cc` | string[] | Carbon copy recipient email addresses. | | |
| | `bcc` | string[] | Blind carbon copy recipient email addresses. | | |
| | `id` | string | Unique message identifier. | | |
| | `subject` | string | Email subject line. | | |
| | `text` | string | Plain-text content. | | |
| | `html` | string | HTML content. | | |
| | `labels` | string[] | Custom labels assigned to the message. | | |
| | `attachments` | object[] | Embedded attachments with content, name, and contentType. | | |
| | `createdAt` | string (date-time) | Timestamp when the message was created. | |
🤖 Prompt for AI Agents
In landing/content/1.docs/3.api/5.messages.md around lines 138 to 150, the
Message schema table is missing documented fields; update the table to add the
`to`, `cc`, and `bcc` recipient fields and the `attachments` field (and any
other message fields shown in examples) with appropriate types and short
descriptions (e.g., `to`, `cc`, `bcc` as array of recipient objects or strings;
`attachments` as array of attachment objects with
filename/content-type/size/URL), ensuring the types match the request/response
examples and include notes about optionality and structure.
| const router = Router(); | ||
|
|
||
| router.post("/", async (req, res) => { | ||
| console.log("Received SES delivery notification", req.body); |
There was a problem hiding this comment.
Avoid logging potentially sensitive data in production.
SES delivery notifications may contain email addresses, recipient information, bounce details, or message content. Logging the entire req.body could expose PII and create compliance risks (GDPR, CCPA).
Consider one of these approaches:
- console.log("Received SES delivery notification", req.body);
+ // Log only the notification type and timestamp, not the full payload
+ console.log("Received SES delivery notification", { type: payload?.Type, timestamp: new Date().toISOString() });Or remove the log entirely and rely on structured observability tools that support PII redaction.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/routes/webhooks/ses.ts around line 7, the code logs the entire req.body
which can expose PII; remove the raw console.log and either (1) redact sensitive
fields before logging (only include non-PII fields like eventType, timestamp,
messageId), (2) switch to structured logging at a debug level so it’s disabled
in production, or (3) remove the log entirely and rely on observability tooling
with PII redaction; implement one of these options and ensure no raw req.body is
written to logs.
|
|
||
| router.post("/", async (req, res) => { | ||
| console.log("Received SES delivery notification", req.body); | ||
| const payload = JSON.parse(req.body); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify JSON.parse usage and add error handling.
If Express body-parser middleware is configured for this route, req.body will already be parsed as an object, and JSON.parse() will throw a TypeError. However, SES sends notifications with Content-Type: text/plain, so raw body parsing may be intentional.
Please verify the middleware configuration and add error handling:
Regardless of the outcome, add error handling to prevent uncaught exceptions:
- const payload = JSON.parse(req.body);
+ let payload;
+ try {
+ payload = typeof req.body === 'string' ? JSON.parse(req.body) : req.body;
+ } catch (error) {
+ console.error("Failed to parse SES notification", error);
+ return res.status(400).send();
+ }🏁 Script executed:
#!/bin/bash
# Description: Check for body-parser or express.json() middleware configuration
# and verify how this webhook route is mounted
# Look for middleware configuration
rg -n "express\.json|bodyParser|express\.text" --type=ts -C3
# Find where the SES webhook route is mounted
rg -n "webhooks/ses" --type=ts -C3Length of output: 225
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full webhooks.ts file to see how sesRouter is mounted
cat api/routes/webhooks.ts
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Get the full ses.ts file to see current implementation
cat api/routes/webhooks/ses.tsLength of output: 474
🏁 Script executed:
#!/bin/bash
# Search for express app setup and middleware configuration
rg -n "app\.use|express\.json|bodyParser" --type=ts -C2Length of output: 1045
🏁 Script executed:
#!/bin/bash
# Find where webhooks router is mounted in the main app
rg -n "webhooks" --type=ts -C2Length of output: 1686
---
🏁 Script executed:
```shell
#!/bin/bash
# Check imports in api/index.ts to confirm text() is from express
head -20 api/index.ts | grep -E "^import|^const"
Length of output: 544
Add error handling for JSON.parse failure.
The text() middleware is applied globally (api/index.ts:28), so req.body is a string and JSON.parse() is correct. However, without error handling, malformed JSON from SES will throw an uncaught exception.
const payload = JSON.parse(req.body);Add try-catch to handle parsing failures:
- const payload = JSON.parse(req.body);
+ let payload;
+ try {
+ payload = JSON.parse(req.body);
+ } catch (error) {
+ console.error("Failed to parse SES notification", error);
+ return res.status(400).send();
+ }📝 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.
| const payload = JSON.parse(req.body); | |
| let payload; | |
| try { | |
| payload = JSON.parse(req.body); | |
| } catch (error) { | |
| console.error("Failed to parse SES notification", error); | |
| return res.status(400).send(); | |
| } |
🤖 Prompt for AI Agents
In api/routes/webhooks/ses.ts around line 8, JSON.parse(req.body) can throw on
malformed SES payloads; wrap the parse in a try-catch, log the parse error
(using the existing logger) and return an immediate HTTP 400 (or other
appropriate error response) to the caller instead of allowing an uncaught
exception to crash the handler; ensure the successful path uses the parsed
payload variable after the try block.
Summary by CodeRabbit
New Features
Documentation
Tests