Domain fixes #17
Domain fixes #17
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds TXT DNS resolution and DMARC/SPF checks, derives domain.verified from SES DKIM verification status, exposes GET /v1/domains/:domainId/dns, updates the frontend with a DNS details dialog, and extends the Node SDK/types/tests with a domain.dns method. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend (domains.vue)
participant API as API Routes
participant SESCtrl as SESController
participant DomainCtrl as DomainController
participant DNSCtrl as DNSController
participant DNS as External DNS
participant SES as AWS SES
User->>Frontend: Click "View Details"
Frontend->>API: GET /v1/domains/{domainId}/dns
API->>SESCtrl: getDomainVerificationDkimAttributes(domain)
SESCtrl->>SES: VerifyDomainDkimCommand
SES-->>SESCtrl: DKIM tokens
API->>DNSCtrl: getDNSTXTRecords(domain) (SPF/DMARC)
DNSCtrl->>DNS: resolveTxt()
DNS-->>DNSCtrl: TXT records
API->>API: Build MX, SPF, DMARC, DKIM CNAMEs
API-->>Frontend: DNS records array
Frontend->>Frontend: Open DNS details dialog
alt User triggers verification
User->>Frontend: Click "Verify Domain"
Frontend->>API: POST /v1/domains/{domainId}/verify
API->>DNSCtrl: getDNSTXTRecords(domain)
DNSCtrl->>DNS: resolveTxt()
DNS-->>DNSCtrl: TXT records
API->>SESCtrl: getDomainVerificationStatus(domain)
SESCtrl->>SES: GetIdentityDkimAttributesCommand
SES-->>SESCtrl: DKIM verification status
API->>DomainCtrl: Update domain.verified based on DKIM status
API-->>Frontend: Verification result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/controllers/DomainController.ts (1)
76-108: DMARC verification is using the wrong host and a weak matchIn
verifyDomainDNS, DMARC TXT is resolved withgetDNSTXTRecords({ domain: domain.name })and then matched only bytype === "TXT"andvalue. Given the default record inDomain.tsusesname: "_dmarc", you likely want to:
- Resolve
_dmarc.${domain.name}instead of the root domain.- Match on both
type === "TXT"andname === "_dmarc"(or equivalent) to avoid accidentally validating unrelated TXT records.- Drop the
console.log(dmarcRecord);or replace it with structured logging.For example:
- const dmarcRecord = await getDNSTXTRecords({ domain: domain.name }); - console.log(dmarcRecord); + const dmarcRecord = await getDNSTXTRecords({ domain: `_dmarc.${domain.name}` }); let dmarcRecordFound; if (dmarcRecord && Array.isArray(dmarcRecord)) { dmarcRecordFound = domain.records.find( (domainRecord) => - domainRecord.type === "TXT" && - dmarcRecord.find((dnsRecord) => dnsRecord === domainRecord.value) + domainRecord.type === "TXT" && + domainRecord.name === "_dmarc" && + dmarcRecord.some((dnsRecord) => dnsRecord === domainRecord.value) ); }
🧹 Nitpick comments (10)
api/db/mongo/schemas/Domain.ts (1)
21-33: DMARC default record looks fine, but ensure it matches verification logicThe new default TXT record for DMARC is reasonable structurally, but
verifyDomainDNScurrently matches TXT records only bytypeandvalue, and performs TXT lookups againstdomain.namerather than_dmarc.${domain.name}. Please double‑check that:
- The UI/instructions expect customers to publish
v=DMARC1; p=reject;at_dmarc.<domain>.- The verification logic (TXT lookup + record matching) is aligned with this
name: "_dmarc"convention so the record can actually be verified.node-sdk/types/sendook-api.d.ts (1)
29-31: Newdnsmethod typing is consistent, but confirm it matches the runtime API
GetDomainDNSParamsandDomainMethods.dnsare consistent with the existing style (params object +Promise<any>return). Just make sure the implementation innode-sdk/index.tsactually accepts{ domainId }and that the declared return type (Promise<any>) matches what the tests and consumers expect (likely an array of DNS records).Also applies to: 67-72
api/controllers/DomainController.ts (1)
110-113: Add error handling and documentation for DKIM verification dependencyThe AWS SES
GetIdentityDkimAttributesCommandresponse shape is correct as used. However, the code atapi/controllers/DomainController.tslines 110-113 lacks error handling. If AWS returns an unexpected response or the domain identity doesn't exist, the code could fail silently or throw unhandled errors. Consider wrapping thegetDomainVerificationStatuscall in a try/catch block and/or documenting the AWS SES API dependency so future maintainers understand this coupling.node-sdk/tests/domainAPI.spec.ts (1)
45-55: Stabilise DNS test by preparing the domain fixture instead of relying on external stateThis test assumes
marcwhitbread.comalready exists for the API key and always has DNS records configured. If that precondition ever changes, the test will fail for environmental reasons rather than a regression.Consider mirroring the
verifyDomaintest’s pattern here (delete-if-exists, create, optionally verify) before callingdomain.dns, and asserting on the record shape (type/name/value) rather than onlylength > 0. That will make this integration test much less flaky and more meaningful.node-sdk/index.ts (1)
115-126: Newdomain.dnsclient wrapper looks correct; consider tightening the return typeThe DNS method is wired consistently with the other domain methods (URL, auth header, and
response.datapassthrough), so the client-side integration with/v1/domains/{domainId}/dnslooks good.If you want better DX later, you could update
DomainMethodsto return a specificDNSRecord[]-style type here instead ofany, and reuse that type in the UI. That can be done as a follow‑up without blocking this PR.api/routes/v1/domains/index.ts (3)
35-37: Domain validation regex is reasonable, but note limitations for non-standard domainsThe regex does a solid job of enforcing conventional FQDN structure and avoiding obviously invalid input.
Just be aware it intentionally excludes single-label/internal hostnames and IDN/punycode TLDs that contain hyphens (for example,
xn--TLDs). If you ever need to support those cases, you’ll need to relax this pattern slightly or special-case them.
87-88: Commented-out SES status call: either wire it through or drop the placeholderThe commented
getDomainVerificationStatuscall is a useful hint for future work (surfacing SES status afterverifyDomainDNS). If you don’t plan to use it imminently, consider removing the commented line to keep the handler lean; otherwise, wiring it up and returning richer verification status to clients would be a nice enhancement.
12-15: DNS records endpoint is sound, but consider SES coupling, region, and DMARC configurabilityThe new
/:domainId/dnsroute does the right high‑level things: it enforces domain ownership viagetDomainByOrganizationIdAndName, fetches DKIM tokens from SES, and returns MX/TXT/CNAME records in a simple array shape that the SDK/UI can consume.A few points to consider:
- AWS SES call semantics –
getDomainVerificationDkimAttributescurrently issues aVerifyDomainDkimcall (per the controller snippet), which is an AWS SES operation intended to initiate DKIM verification and return tokens, not just a read‑only “get attributes” call. For an endpoint that may be hit often by the UI, you probably want a read‑only or cached behaviour (for example, storing the tokens on the Domain record after the first call, or switching to aGetIdentityDkimAttributes-style helper) to reduce coupling and unnecessary SES traffic.- Region-specific MX host –
inbound-smtp.us-east-2.amazonaws.comis hard‑coded. If you ever support SES in other regions, or allow region selection per domain, deriving this host from configuration (or a domain field) will prevent mismatches between the DNS MX you show and the actual SES region in use.- DMARC policy constant – the DMARC TXT value is also hard‑coded to
v=DMARC1; p=reject;. If the Domain schema’s DMARC settings become user-editable or environment-specific, you’ll likely want to pull this from the database or configuration instead of inlining it here.None of these need to block the PR, but they’re worth addressing before this endpoint becomes heavily used.
Also applies to: 93-132
app/app/pages/domains.vue (2)
63-72: DNS details dialog UX is clear; verify flow entry point change looks intentionalMoving the “verify” action into a dedicated “View Details” dialog with DNS records, loading, empty, and error states makes the flow much clearer and easier to reason about. The conditional rendering (
loadingDNS/dnsError/ empty / table) and thedetailsErrormessage below the table are laid out well.One small defensive tweak you might consider:
handleVerifyDomain(pendingDetails!)relies on the non‑null assertion. Today that’s safe because you always setpendingDetailsbefore opening the dialog, but if this method ever gets reused elsewhere it may be worth either guarding in the handler (if (!domain) return;) or adjusting the type so the template never needs!.Also applies to: 142-195
216-239: DNS load/verify logic is solid; consider guarding against overlapping loadsThe new DNS state and handlers (
DNSRecord,dnsRecords,loadingDNS,dnsError,detailsError,openDetailsDialog,closeDetailsDialog,loadDNSRecords, and the updatedhandleVerifyDomain) are consistent with the rest of the page:
loadDNSRecordscorrectly builds the/domains/{domainId}/dnsURL, surfaces backend error messages (using eithermessageorerror), and populatesdnsRecordsonly when a valid array is returned.handleVerifyDomainuses the samename ?? _idlogic asloadDNSRecords, shows a success banner, reloads the domain list, and then refreshes DNS records when the dialog is open, which keeps the view in sync.One edge case to keep in mind: because
openDetailsDialogisasyncand callsawait loadDNSRecords(domain), it’s possible (e.g. on a slow network) for users to trigger multiple DNS loads in sequence. The last-resolvingloadDNSRecordscall will win, even ifpendingDetailshas since been changed to a different domain, which could briefly show DNS records for A under the header for B.If you want to harden this, you could:
- Track a
currentDnsDomainIdref, set it at the start ofloadDNSRecords, and before writingdnsRecords/dnsErrorcheck that it still matchespendingDetails.value.- Or, compare
domainIdtopendingDetails.value?.name ?? pendingDetails.value?._idbefore mutating state.That would avoid any stale updates while keeping the rest of this logic intact.
Also applies to: 313-370, 372-407
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/controllers/DNSController.ts(2 hunks)api/controllers/DomainController.ts(2 hunks)api/controllers/SESController.ts(1 hunks)api/db/mongo/schemas/Domain.ts(1 hunks)api/routes/v1/domains/index.ts(3 hunks)app/app/pages/domains.vue(8 hunks)node-sdk/index.ts(1 hunks)node-sdk/tests/domainAPI.spec.ts(1 hunks)node-sdk/types/sendook-api.d.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
api/controllers/DomainController.ts (2)
api/controllers/DNSController.ts (1)
getDNSTXTRecords(17-29)api/controllers/SESController.ts (1)
getDomainVerificationStatus(31-35)
api/routes/v1/domains/index.ts (2)
api/controllers/DomainController.ts (1)
getDomainByOrganizationIdAndName(31-42)api/controllers/SESController.ts (1)
getDomainVerificationDkimAttributes(26-29)
🔇 Additional comments (2)
api/controllers/SESController.ts (1)
26-35: SES helper renames are correct and properly usedThe callers are correctly aligned with their intent:
getDomainVerificationDkimAttributes(line 108 of api/routes/v1/domains/index.ts) retrieves DKIM tokens as expected, checking forDkimTokens.getDomainVerificationStatus(line 110 of api/controllers/DomainController.ts inverifyDomainDNS) checks the verification status, examiningDkimVerificationStatus === "Success".AWS SES documentation confirms that
VerifyDomainDkimis safe to call multiple times, returning DKIM tokens on each call, so repeated verification calls pose no risk.app/app/pages/domains.vue (1)
762-765: DNS dialog/table styling is consistent with the existing designThe
dialog-card-largesizing and the DNS-specific styles (dns-loading/dns-empty, table, monospacedns-name/dns-valuewithword-break: break-all, and responsive adjustments in the media query) fit nicely with the existing dialog patterns and should handle long DKIM/DMARC values well.No issues from a layout/UX perspective.
Also applies to: 862-952, 936-951
| @@ -1,4 +1,4 @@ | |||
| import { resolveMx } from 'node:dns/promises'; | |||
| import { resolveMx, resolveTxt } from 'node:dns/promises'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
TXT lookup only inspects the first TXT RR; consider flattening all records
resolveTxt returns string[][], but getDNSTXTRecords currently returns only records[0] ?? []. If the DMARC TXT is not in the first RR, verifyDomainDNS will never see it. Prefer flattening all strings:
export async function getDNSTXTRecords({
domain,
}: {
domain: string;
}): Promise<string[]> {
try {
- const records = await resolveTxt(domain);
- return records[0] ?? [];
+ const records = await resolveTxt(domain);
+ return records.flat();
} catch (error) {
console.error('Error resolving TXT records:', error);
return [];
}
}Also applies to: 17-29
🤖 Prompt for AI Agents
In api/controllers/DNSController.ts around lines 1 and 17-29, getDNSTXTRecords
currently returns only the first TXT RR (records[0] ?? []), but resolveTxt
returns string[][]; update the function to flatten all TXT record arrays (e.g.,
concat/flat the string[][] into a single string[]), then search that flattened
list for the DMARC TXT entry so lookups work even when DMARC is not in the first
RR.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
api/controllers/DomainController.ts (3)
96-107: Consider normalizing TXT record comparison.The exact string match (
===) on Line 102 might miss valid DMARC records with minor whitespace or formatting variations. Consider normalizing both sides (trim, lowercase, collapse whitespace) before comparison.Example refinement:
if (dmarcRecord && Array.isArray(dmarcRecord)) { dmarcRecordFound = domain.records.find( (domainRecord) => domainRecord.type === "TXT" && - dmarcRecord.find((dnsRecord) => dnsRecord === domainRecord.value) + dmarcRecord.find((dnsRecord) => + dnsRecord.trim().toLowerCase() === domainRecord.value.trim().toLowerCase() + ) ); }
109-120: Same normalization consideration applies here.The SPF record matching uses the same exact string comparison pattern as DMARC. Apply the same normalization refinement suggested for DMARC verification to improve robustness.
122-124: Simplify redundant ternary.The expression
=== "Success" ? true : falseis redundant since the comparison already yields a boolean.Apply this diff:
- domain.verified = verifiedDomainStatus.DkimAttributes?.[domain.name]?.DkimVerificationStatus === "Success" ? true : false; + domain.verified = verifiedDomainStatus.DkimAttributes?.[domain.name]?.DkimVerificationStatus === "Success";api/routes/v1/domains/index.ts (1)
105-110: Add error handling for AWS SES API call.The call to
getDomainVerificationDkimAttributescan throw various AWS SDK errors (network issues, authentication failures, rate limiting, etc.). Without error handling, this will result in an unhandled promise rejection.Apply this diff:
+ try { const verifiedDomainDkimAttributes = await getDomainVerificationDkimAttributes({ domain: domain.name }); - if (!verifiedDomainDkimAttributes.DkimTokens) { + if (!verifiedDomainDkimAttributes.DkimTokens || verifiedDomainDkimAttributes.DkimTokens.length === 0) { return res.status(400).json({ error: "Failed to get DKIM attributes" }); } + } catch (error) { + console.error('Error fetching DKIM attributes:', error); + return res.status(500).json({ error: "Failed to retrieve DKIM attributes" }); + }Note: This also adds a check for empty
DkimTokensarray.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/controllers/DomainController.ts(2 hunks)api/controllers/SESController.ts(2 hunks)api/db/mongo/schemas/Domain.ts(1 hunks)api/routes/v1/domains/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/db/mongo/schemas/Domain.ts
- api/controllers/SESController.ts
🧰 Additional context used
🧬 Code graph analysis (2)
api/controllers/DomainController.ts (2)
api/controllers/DNSController.ts (1)
getDNSTXTRecords(17-29)api/controllers/SESController.ts (1)
getDomainVerificationStatus(31-35)
api/routes/v1/domains/index.ts (2)
api/controllers/DomainController.ts (1)
getDomainByOrganizationIdAndName(31-42)api/controllers/SESController.ts (1)
getDomainVerificationDkimAttributes(26-29)
🔇 Additional comments (1)
api/controllers/DomainController.ts (1)
3-5: LGTM!The new imports are properly utilized in the enhanced domain verification flow.
| import { | ||
| getDomainVerificationDkimAttributes, | ||
| getDomainVerificationStatus, | ||
| } from "../../../controllers/SESController"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if getDomainVerificationStatus is used.
The import includes getDomainVerificationStatus but it doesn't appear to be used in this file. Only getDomainVerificationDkimAttributes is used (Line 106).
Run this script to verify the usage:
🏁 Script executed:
#!/bin/bash
# Description: Check if getDomainVerificationStatus is used in this routes file
# Search for usage of getDomainVerificationStatus in the file
rg -n 'getDomainVerificationStatus' api/routes/v1/domains/index.tsLength of output: 33
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if getDomainVerificationStatus is re-exported from this file
rg -n 'export.*getDomainVerificationStatus' api/routes/v1/domains/index.ts
# Check if other files import getDomainVerificationStatus from this routes file
rg -n "from.*api/routes/v1/domains" --type=ts | head -20
Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full content of the file to verify getDomainVerificationStatus usage
cat api/routes/v1/domains/index.ts | grep -i 'getDomainVerificationStatus\|export'
Length of output: 53
Remove the unused getDomainVerificationStatus import.
Verification confirms that getDomainVerificationStatus is imported but never used in this file. Only getDomainVerificationDkimAttributes is actively used. Remove the unused import from lines 12-15.
🤖 Prompt for AI Agents
In api/routes/v1/domains/index.ts around lines 12 to 15, the import list
includes getDomainVerificationStatus which is not used in this file; remove
getDomainVerificationStatus from the import statement so only
getDomainVerificationDkimAttributes is imported, then save the file and run a
quick lint/type check to confirm no unused-import warnings remain.
| return res.json([ | ||
| { | ||
| type: "MX", | ||
| name: "@", | ||
| value: "inbound-smtp.us-east-2.amazonaws.com", | ||
| }, | ||
| { | ||
| type: "TXT", | ||
| name: "@", | ||
| value: "v=spf1 include:amazonses.com ~all", | ||
| }, | ||
| { | ||
| type: "TXT", | ||
| name: "_dmarc", | ||
| value: "v=DMARC1; p=reject;", | ||
| }, |
There was a problem hiding this comment.
Critical: Hardcoded AWS region and email policies.
Several hardcoded values create deployment and configuration concerns:
-
AWS Region (Line 116): The MX record hardcodes
us-east-2, which will break email delivery if the application is deployed in a different AWS region or needs to support multiple regions. -
Email Security Policies: The SPF and DMARC policies are hardcoded with specific settings that may not suit all organizations:
- SPF
~all(softfail) vs-all(hardfail) - DMARC
p=rejectis the strictest policy
- SPF
Consider extracting these values to environment variables or configuration:
+ // In a config file or from environment variables
+ const AWS_SES_REGION = process.env.AWS_SES_REGION || 'us-east-2';
+ const SPF_POLICY = process.env.SPF_POLICY || 'v=spf1 include:amazonses.com ~all';
+ const DMARC_POLICY = process.env.DMARC_POLICY || 'v=DMARC1; p=reject;';
return res.json([
{
type: "MX",
name: "@",
- value: "inbound-smtp.us-east-2.amazonaws.com",
+ value: `inbound-smtp.${AWS_SES_REGION}.amazonaws.com`,
},
{
type: "TXT",
name: "@",
- value: "v=spf1 include:amazonses.com ~all",
+ value: SPF_POLICY,
},
{
type: "TXT",
name: "_dmarc",
- value: "v=DMARC1; p=reject;",
+ value: DMARC_POLICY,
},📝 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.
| return res.json([ | |
| { | |
| type: "MX", | |
| name: "@", | |
| value: "inbound-smtp.us-east-2.amazonaws.com", | |
| }, | |
| { | |
| type: "TXT", | |
| name: "@", | |
| value: "v=spf1 include:amazonses.com ~all", | |
| }, | |
| { | |
| type: "TXT", | |
| name: "_dmarc", | |
| value: "v=DMARC1; p=reject;", | |
| }, | |
| // In a config file or from environment variables | |
| const AWS_SES_REGION = process.env.AWS_SES_REGION || 'us-east-2'; | |
| const SPF_POLICY = process.env.SPF_POLICY || 'v=spf1 include:amazonses.com ~all'; | |
| const DMARC_POLICY = process.env.DMARC_POLICY || 'v=DMARC1; p=reject;'; | |
| return res.json([ | |
| { | |
| type: "MX", | |
| name: "@", | |
| value: `inbound-smtp.${AWS_SES_REGION}.amazonaws.com`, | |
| }, | |
| { | |
| type: "TXT", | |
| name: "@", | |
| value: SPF_POLICY, | |
| }, | |
| { | |
| type: "TXT", | |
| name: "_dmarc", | |
| value: DMARC_POLICY, | |
| }, |
🤖 Prompt for AI Agents
In api/routes/v1/domains/index.ts around lines 112 to 127, the MX host and
SPF/DMARC strings are hardcoded (us-east-2, v=spf1 ... ~all, v=DMARC1;
p=reject;); change this to read AWS_REGION, SPF_POLICY and DMARC_POLICY (or a
config object) from environment/config with sensible defaults and validation,
construct the MX value dynamically as `inbound-smtp.{AWS_REGION}.amazonaws.com`
using the region value, and replace the hardcoded SPF/DMARC strings with the
env-driven values so deploys in other regions or with different security
policies work correctly (include fallbacks and a brief error/log if required env
vars are missing or invalid).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/controllers/InboxController.ts (1)
62-71: Remove unusedgetNewRandomInboxEmailfunction and dead imports.Verification confirms
getNewRandomInboxEmailis unused after the change on line 21. The function only contains a self-recursive call and has no external callers. Additionally, remove the dead imports fromapi/controllers/AuthController.ts(line 6) andapi/tests/MessageController.spec.ts(line 9).
| : undefined; | ||
| inbox.name = name; | ||
| inbox.email = email || await getNewRandomInboxEmail({ name }); | ||
| inbox.email = email || `${name.replace(/[^a-zA-Z0-9]/g, "-")}@${process.env.DEFAULT_EMAIL_DOMAIN}`; |
There was a problem hiding this comment.
🧩 Analysis chain
Risk of duplicate emails without uniqueness check.
The deterministic email generation can create duplicate emails when multiple inboxes share the same name. The previous getNewRandomInboxEmail implementation included a recursive uniqueness check that's now bypassed.
Additionally, names containing only non-alphanumeric characters (e.g., "!!!") would produce an invalid email format: @${DEFAULT_EMAIL_DOMAIN}.
Verify whether the Inbox schema enforces a unique constraint on the email field:
If no unique constraint exists, either:
- Add a unique constraint to the email field in the database schema, OR
- Restore the uniqueness check by keeping the random suffix approach
If a unique constraint does exist, the save() call on line 22 will throw a duplicate key error that should be handled gracefully with user-friendly error messages.
🏁 Script executed:
#!/bin/bash
# Description: Check if the Inbox schema has a unique constraint on the email field
# Search for the Inbox schema definition
ast-grep --pattern 'email: {
$$$
}'
# Also search for unique index definitions
rg -n --type=ts -A5 -B5 'email.*unique|unique.*email' api/db/mongo/schemas/Length of output: 410
🏁 Script executed:
#!/bin/bash
# Description: Verify getNewRandomInboxEmail usage and error handling
# Search for all calls to getNewRandomInboxEmail
echo "=== Searching for getNewRandomInboxEmail usage ==="
rg -n '\bgetNewRandomInboxEmail\b' --type=ts
# Check context around line 22 for error handling
echo "=== Checking context around line 21-22 for error handling ==="
sed -n '15,35p' api/controllers/InboxController.ts
# Search for try-catch or error handling around save operations
echo "=== Searching for error handling patterns in InboxController ==="
rg -n --type=ts -A3 '\.save\(' api/controllers/InboxController.tsLength of output: 1309
🏁 Script executed:
#!/bin/bash
# Description: Check getNewRandomInboxEmail usage in AuthController
echo "=== AuthController usage of getNewRandomInboxEmail ==="
rg -n --type=ts -B5 -A5 'getNewRandomInboxEmail' api/controllers/AuthController.tsLength of output: 605
🏁 Script executed:
#!/bin/bash
# Description: Search entire AuthController for getNewRandomInboxEmail function calls
echo "=== Full AuthController file ==="
cat api/controllers/AuthController.ts | grep -n "getNewRandomInboxEmail"Length of output: 124
Error handling required for duplicate email constraint violations and invalid email format edge case.
The Inbox schema does enforce a unique constraint on the email field (confirmed at api/db/mongo/schemas/Inbox.ts:30), so duplicate emails are prevented at the database level. However, two critical issues remain:
-
Missing error handling: The
await inbox.save()call (line 22) has no try-catch, so duplicate key errors from the unique constraint will cause unhandled promise rejections. -
Invalid email format edge case: Names containing only non-alphanumeric characters (e.g.,
"!!!") produce invalid emails like@${DEFAULT_EMAIL_DOMAIN}.
Additionally, the getNewRandomInboxEmail function (lines 62-71) is now dead code and can be removed, as can its unused import in api/controllers/AuthController.ts:6.
🤖 Prompt for AI Agents
In api/controllers/InboxController.ts around line 21 (and nearby save at line
22) the code assigns inbox.email using name which can produce an empty
local-part and then calls await inbox.save() with no error handling; add
validation to ensure the generated local-part is non-empty (e.g., strip
non-alphanumerics and if result is empty generate a safe fallback local-part or
use a random/UUID segment) before composing the email, and wrap the await
inbox.save() in a try-catch that specifically handles Mongo duplicate key errors
(E11000) by returning a controlled error response or retry logic, and
rethrow/propagate unexpected errors; additionally remove the now-dead
getNewRandomInboxEmail function from lines ~62-71 in this file and delete its
unused import from api/controllers/AuthController.ts (line 6) to eliminate dead
code and unused dependencies.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Documentation