Skip to content

feat: MAIL FROM label override and aggregate domain verification#392

Open
amanjuman wants to merge 1 commit intousesend:mainfrom
amanjuman:feat/mail-from-label-upstream-pr
Open

feat: MAIL FROM label override and aggregate domain verification#392
amanjuman wants to merge 1 commit intousesend:mainfrom
amanjuman:feat/mail-from-label-upstream-pr

Conversation

@amanjuman
Copy link
Copy Markdown

@amanjuman amanjuman commented Apr 18, 2026

Adds optional mailFromLabel on Domain, SES MAIL FROM sync, aggregate verification status (identity + DKIM + SPF), domain UI and API updates, and webhook payload field mailFromLabel.

Also fixes contactBookId type in webhook-service unit test (string) so apps/web tsc passes.


Summary by cubic

Adds an optional MAIL FROM label override and an aggregate domain verification status (identity + DKIM + SPF) across UI, API, SES, and webhooks to make setup clearer and safer. Domains are “Verified” only when all checks pass; MX/SPF DNS names now follow the effective MAIL FROM label.

  • New Features

    • Database: add mailFromLabel to Domain (nullable; defaults to using the SES region as the label).
    • API: domain.setMailFromLabel(id, mailFromLabel) with validation (1–63 chars, letters/digits/hyphens, no leading/trailing hyphen). Syncs SES via putEmailIdentityMailFromDomain; changing the label sets SPF to PENDING and re-verifies.
    • DNS/SES: MX and SPF TXT record names use the effective label (custom or region). Default MAIL FROM is {region}.{domain}; resetting sets it back to default.
    • UI: Domain Settings adds a MAIL FROM label field with save/reset, effective label preview, and guidance. Domain list and admin teams show the aggregate verification status.
    • Status: new aggregateDomainStatus (worst-of identity, DKIM, SPF). Exposed as aggregateStatus in the domain API schema and used for badges, verification emails, webhook event type selection, and from-email validation.
    • Webhooks: DomainPayload now includes mailFromLabel.
  • Bug Fixes

    • Test type fix: contactBookId is a string in webhook-service unit tests so apps/web TypeScript passes.

Written for commit 249f9bc. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Configure custom MAIL FROM labels for your domains to enhance email deliverability
    • Domain verification status now displays aggregate results across all verification methods (SES identity, DKIM, and MAIL FROM)
  • Chores

    • Database schema updates to support MAIL FROM label configuration
    • Type definitions and webhook event structure enhancements

Adds optional mailFromLabel on Domain, SES MAIL FROM sync, aggregate verification status (identity + DKIM + SPF), domain UI and API updates, and webhook payload field mailFromLabel.

Also fixes contactBookId type in webhook-service unit test (string) so apps/web tsc passes.

Made-with: Cursor
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 18, 2026

@amanjuman is attempting to deploy a commit to the kmkoushik's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 18, 2026

Walkthrough

This PR adds custom MAIL FROM label support to the domain management system. Changes include: a database migration adding mailFromLabel column to the Domain table; a new aggregateDomainStatus utility function that computes the worst-case verification status across SES identity, DKIM, and SPF; updates to the Prisma and Zod schemas; a new setMailFromLabel mutation endpoint and service function; AWS SES integration to configure custom MAIL FROM domains; UI updates to display aggregate status; updated webhook payloads to include mailFromLabel; and corresponding tests and type definitions.

Possibly related PRs

Suggested labels

codex

Suggested reviewers

  • KMKoushik
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.55% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately and specifically summarizes the two main changes: MAIL FROM label override support and aggregate domain verification status computation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@amanjuman amanjuman marked this pull request as ready for review April 21, 2026 20:36
Copilot AI review requested due to automatic review settings April 21, 2026 20:36
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for customizing the SES MAIL FROM subdomain label per domain and introduces an aggregate “verified” status (identity + DKIM + SPF) to drive UI, API responses, and verification-related behavior.

Changes:

  • Add mailFromLabel to the Domain model + migration, wire up SES MAIL FROM updates, and expose it via API/UI/webhooks.
  • Introduce aggregateDomainStatus and surface aggregateStatus on domain responses for UI badges and verification gating.
  • Update tests and webhook payload typings (including a contactBookId type fix).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/lib/src/webhook/webhook-events.ts Extends domain webhook payload type with mailFromLabel.
apps/web/src/types/domain.ts Adds aggregateStatus to enriched domain type used by UI.
apps/web/src/server/service/webhook-service.unit.test.ts Fixes contactBookId type to string for TS correctness.
apps/web/src/server/service/domain-service.unit.test.ts Adds unit test coverage for MAIL FROM label changes.
apps/web/src/server/service/domain-service.ts Implements aggregate status, MAIL FROM DNS record changes, and setMailFromLabel.
apps/web/src/server/jobs/domain-verification-job.unit.test.ts Updates test fixtures for new Domain field.
apps/web/src/server/aws/ses.ts Changes default SES MAIL FROM domain and adds helper to update it.
apps/web/src/server/api/routers/domain.ts Adds domain.setMailFromLabel mutation.
apps/web/src/lib/zod/domain-schema.ts Updates API schema to include aggregateStatus and mailFromLabel.
apps/web/src/lib/domain-aggregate-status.ts Adds aggregate status computation helper.
apps/web/src/lib/domain-aggregate-status.unit.test.ts Adds unit tests for aggregate status logic.
apps/web/src/app/(dashboard)/domains/domain-list.tsx Uses aggregateStatus for status indicator/badge.
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx Uses aggregateStatus and adds MAIL FROM label settings UI.
apps/web/src/app/(dashboard)/admin/teams/page.tsx Shows aggregate domain verification state in admin team view.
apps/web/prisma/schema.prisma Adds nullable mailFromLabel field to Domain model.
apps/web/prisma/migrations/20260419140000_domain_mail_from_label/migration.sql DB migration for mailFromLabel.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

const firstLabel = stored ?? domain.region;
const mailFromFqdn = `${firstLabel}.${domain.name}`;
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setMailFromLabel configures SES with mailFromFqdn = ${firstLabel}.${domain.name}``, but the DNS records for MX/SPF are built using effectiveMailFromFirstLabel(domain) plus `subdomainSuffix` (when `domain.subdomain` is set). For domains with a `subdomain`, this can make the SES MAIL FROM domain not match the DNS records shown to the user. Consider constructing the SES MAIL FROM FQDN using the same helper/logic as `buildDnsRecords` (including `subdomainSuffix` when present) so the configured MAIL FROM domain and displayed DNS records stay consistent.

Suggested change
const mailFromFqdn = `${firstLabel}.${domain.name}`;
const subdomainSuffix = domain.subdomain ? `.${domain.subdomain}` : "";
const mailFromFqdn = `${firstLabel}${subdomainSuffix}.${domain.name}`;

Copilot uses AI. Check for mistakes.
.openapi({ description: "DNS record name", example: "mail" }),
name: z.string().openapi({
description:
"DNS record name (hostname label). For custom MAIL FROM MX and SPF TXT records, this is the first label of the MAIL FROM host: the domain `mailFromLabel` if set, otherwise the SES `region` value.",
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OpenAPI description for DomainDnsRecordSchema.name says this is a single “hostname label”, but the code can return values containing dots/underscores (e.g. ${selector}._domainkey${subdomainSuffix} and ${label}${subdomainSuffix}). Update the description (and ideally the example) to reflect that this is the record name relative to the domain/zone and may include multiple labels.

Suggested change
"DNS record name (hostname label). For custom MAIL FROM MX and SPF TXT records, this is the first label of the MAIL FROM host: the domain `mailFromLabel` if set, otherwise the SES `region` value.",
"DNS record name relative to the domain/zone. This may be a single label or multiple labels (for example, `selector1._domainkey` or a MAIL FROM subdomain such as `bounce`). For custom MAIL FROM MX and SPF TXT records, this is the MAIL FROM host name relative to the domain: `mailFromLabel` if set, otherwise the SES `region` value.",
example: "selector1._domainkey",

Copilot uses AI. Check for mistakes.
Comment on lines +227 to 239
team.domains.map((domain) => {
const agg = aggregateDomainStatus(domain);
return (
<div
key={domain.id}
className="flex items-center justify-between rounded-md bg-background px-3 py-2 text-sm"
>
<span>{domain.name}</span>
<Badge variant={domain.status === "SUCCESS" ? "outline" : "secondary"}>
{domain.status === "SUCCESS"
<Badge variant={agg === "SUCCESS" ? "outline" : "secondary"}>
{agg === "SUCCESS"
? "Verified"
: domain.status.toLowerCase()}
: agg.toLowerCase()}
</Badge>
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aggregateDomainStatus(domain) here is computed from team.domains, but the admin API selection for domains (see teamAdminSelection in apps/web/src/server/api/routers/admin.ts) only includes status/isVerifying and does not include dkimStatus/spfDetails. With missing fields, aggregateDomainStatus treats them as NOT_STARTED, so even a SUCCESS domain will display as not_started. Fix by including dkimStatus and spfDetails (and any other required fields) in the admin query selection, or by using domain.status directly when those fields aren’t present.

Copilot uses AI. Check for mistakes.
Comment on lines 331 to 336
function buildDomainPayload(domain: Domain): DomainPayload {
return {
id: domain.id,
name: domain.name,
status: domain.status,
status: aggregateDomainStatus(domain),
region: domain.region,
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildDomainPayload now sets status to the aggregate verification result. That changes the meaning of DomainPayload.status for webhook consumers (it used to reflect the SES identity verification status while dkimStatus/spfDetails carried the other checks). To avoid a breaking change/inconsistency, keep status as the identity status and add a new field (e.g. aggregateStatus) to the webhook payload (or bump/version the webhook payload if this semantic change is intended).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/web/src/server/service/domain-service.ts (1)

554-580: ⚠️ Potential issue | 🟠 Major

Use aggregate status for verification state, notifications, and polling cadence.

This path still treats raw SES identity status as verified. If identity is SUCCESS while DKIM/SPF is PENDING or FAILED, the domain can be marked “ever verified,” notifications can be skipped, and future rechecks can move to the 30-day cadence even though aggregateStatus is not SUCCESS.

🐛 Proposed direction
+  const normalizedDomain = {
+    ...updatedDomain,
+    dkimStatus: dkimStatus ?? null,
+    spfDetails: spfDetails ?? null,
+    dmarcAdded: Boolean(dmarcRecord),
+  } satisfies Domain;
+
+  const previousAggregate = aggregateDomainStatus(domain);
+  const nextAggregate = aggregateDomainStatus(normalizedDomain);
+
-  if (updatedDomain.status === DomainStatus.SUCCESS) {
+  if (nextAggregate === DomainStatus.SUCCESS) {
     await markDomainEverVerified(domain.id);
   }

   if (
     shouldSendDomainStatusNotification({
-      previousStatus,
-      currentStatus: updatedDomain.status,
+      previousStatus: previousAggregate,
+      currentStatus: nextAggregate,
       hasEverVerified:
         verificationState.hasEverVerified ||
-        updatedDomain.status === DomainStatus.SUCCESS,
+        nextAggregate === DomainStatus.SUCCESS,
       lastNotifiedStatus: verificationState.lastNotifiedStatus,
     })
   ) {
     const reservedNotification = await reserveDomainStatusNotification(
       domain.id,
-      updatedDomain.status,
+      nextAggregate,
     );

     if (reservedNotification) {
       try {
         await sendDomainStatusNotification({
-          domain: updatedDomain,
-          previousStatus,
+          domain: normalizedDomain,
+          previousStatus: previousAggregate,
         });
-        await setLastNotifiedDomainStatus(domain.id, updatedDomain.status);
+        await setLastNotifiedDomainStatus(domain.id, nextAggregate);
       } catch (error) {
         logger.error(
-          { err: error, domainId: domain.id, status: updatedDomain.status },
+          { err: error, domainId: domain.id, status: nextAggregate },
           "[DomainService]: Failed to send domain status notification",
         );
       }
     }
   }

-  const normalizedDomain = {
-    ...updatedDomain,
-    dkimStatus: dkimStatus ?? null,
-    spfDetails: spfDetails ?? null,
-    dmarcAdded: Boolean(dmarcRecord),
-  } satisfies Domain;
-
   const domainWithDns = withDnsRecords(normalizedDomain);
 export async function isDomainVerificationDue(domain: Domain) {
   const verificationState = await getDomainVerificationState(domain.id);
+  const aggregateStatus = aggregateDomainStatus(domain);

   if (
     !verificationState.hasEverVerified &&
-    domain.status === DomainStatus.FAILED &&
+    aggregateStatus === DomainStatus.FAILED &&
     !domain.isVerifying
   ) {
     return false;
   }

   const now = Date.now();
   const lastCheckedAt = verificationState.lastCheckedAt?.getTime() ?? 0;
   const intervalMs =
     verificationState.hasEverVerified ||
-    VERIFIED_DOMAIN_STATUSES.has(domain.status)
+    VERIFIED_DOMAIN_STATUSES.has(aggregateStatus)
       ? DOMAIN_VERIFIED_RECHECK_MS
       : DOMAIN_UNVERIFIED_RECHECK_MS;

Also applies to: 604-632, 788-802

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/server/service/domain-service.ts` around lines 554 - 580, The
code is using the raw SES identity status (updatedDomain.status) instead of the
domain's aggregateStatus when deciding verification state changes,
notifications, and polling cadence; change references so that decisions use
updatedDomain.aggregateStatus (or the aggregateStatus field on the updated
domain object) everywhere: when calling markDomainEverVerified, when computing
hasEverVerified in the shouldSendDomainStatusNotification call, when
reserving/sending notifications (reserveDomainStatusNotification,
sendDomainStatusNotification, setLastNotifiedDomainStatus) and when passing
previousStatus/currentStatus into shouldSendDomainStatusNotification; ensure
previousStatus/currentStatus values passed are the aggregate statuses (not raw
identity status) so notifications and cadence reflect the true aggregated
verification state.
🧹 Nitpick comments (1)
apps/web/src/lib/domain-aggregate-status.ts (1)

6-12: Guard against future DomainStatus enum additions.

STATUS_WORST_FIRST currently lists all 5 enum values, but if a new DomainStatus is added to Prisma, indexOf will return -1 and that status will be silently skipped in the worst-case computation — potentially reporting SUCCESS when the new status is actually a failure mode. Consider an exhaustiveness check so this fails at compile time.

♻️ Suggested guard
+// Compile-time exhaustiveness: ensure every DomainStatus is ranked.
+const _exhaustive: Record<DomainStatus, number> = {
+  [DomainStatus.FAILED]: 0,
+  [DomainStatus.TEMPORARY_FAILURE]: 1,
+  [DomainStatus.PENDING]: 2,
+  [DomainStatus.NOT_STARTED]: 3,
+  [DomainStatus.SUCCESS]: 4,
+};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/lib/domain-aggregate-status.ts` around lines 6 - 12,
STATUS_WORST_FIRST enumerates DomainStatus values but has no exhaustiveness
guard, so future additions to the DomainStatus enum will be missed; add a
compile-time exhaustiveness check after STATUS_WORST_FIRST that forces a TS
error if the array doesn’t contain every DomainStatus. Concretely, keep
STATUS_WORST_FIRST as is and then add a type-level conditional using
DomainStatus and typeof STATUS_WORST_FIRST[number] (a conditional type that
resolves to never when a mismatch exists) and assign it to a const (e.g.,
_exhaustiveDomainStatusCheck) so the compiler fails if any DomainStatus is
missing; optionally also add a runtime check that throws if
Object.values(DomainStatus) contains a value not in STATUS_WORST_FIRST for
safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/web/src/lib/zod/domain-schema.ts`:
- Around line 11-14: The openapi description for the zod schema field name
(z.string().openapi(...) in domain-schema.ts) misleadingly states "the first
label" but buildDnsRecords actually returns the full relative record name
including subdomain (e.g., for sub.example.com a custom label becomes
"bounce.sub" not just "bounce"); update the description text in the name field
to clarify it is the full relative DNS record name (first label plus any
subdomain segment) and include a brief example such as "for sub.example.com and
custom label 'bounce' the record name is 'bounce.sub'".

In `@apps/web/src/types/domain.ts`:
- Around line 14-15: The updateDomain() handler returns a bare Domain missing
the aggregateStatus that other domain APIs include; wrap its returned object
with withDnsRecords(returnedDomain) before returning so the Domain always
includes aggregateStatus (consistent with
createDomain/getDomain/getDomains/setMailFromLabel/refreshDomainVerification)
and preserves type safety for the Domain type and its aggregateStatus field.

---

Outside diff comments:
In `@apps/web/src/server/service/domain-service.ts`:
- Around line 554-580: The code is using the raw SES identity status
(updatedDomain.status) instead of the domain's aggregateStatus when deciding
verification state changes, notifications, and polling cadence; change
references so that decisions use updatedDomain.aggregateStatus (or the
aggregateStatus field on the updated domain object) everywhere: when calling
markDomainEverVerified, when computing hasEverVerified in the
shouldSendDomainStatusNotification call, when reserving/sending notifications
(reserveDomainStatusNotification, sendDomainStatusNotification,
setLastNotifiedDomainStatus) and when passing previousStatus/currentStatus into
shouldSendDomainStatusNotification; ensure previousStatus/currentStatus values
passed are the aggregate statuses (not raw identity status) so notifications and
cadence reflect the true aggregated verification state.

---

Nitpick comments:
In `@apps/web/src/lib/domain-aggregate-status.ts`:
- Around line 6-12: STATUS_WORST_FIRST enumerates DomainStatus values but has no
exhaustiveness guard, so future additions to the DomainStatus enum will be
missed; add a compile-time exhaustiveness check after STATUS_WORST_FIRST that
forces a TS error if the array doesn’t contain every DomainStatus. Concretely,
keep STATUS_WORST_FIRST as is and then add a type-level conditional using
DomainStatus and typeof STATUS_WORST_FIRST[number] (a conditional type that
resolves to never when a mismatch exists) and assign it to a const (e.g.,
_exhaustiveDomainStatusCheck) so the compiler fails if any DomainStatus is
missing; optionally also add a runtime check that throws if
Object.values(DomainStatus) contains a value not in STATUS_WORST_FIRST for
safety.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bf1302d5-a00b-4831-87eb-83e2626e2e25

📥 Commits

Reviewing files that changed from the base of the PR and between 5b9788e and 249f9bc.

📒 Files selected for processing (16)
  • apps/web/prisma/migrations/20260419140000_domain_mail_from_label/migration.sql
  • apps/web/prisma/schema.prisma
  • apps/web/src/app/(dashboard)/admin/teams/page.tsx
  • apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
  • apps/web/src/app/(dashboard)/domains/domain-list.tsx
  • apps/web/src/lib/domain-aggregate-status.ts
  • apps/web/src/lib/domain-aggregate-status.unit.test.ts
  • apps/web/src/lib/zod/domain-schema.ts
  • apps/web/src/server/api/routers/domain.ts
  • apps/web/src/server/aws/ses.ts
  • apps/web/src/server/jobs/domain-verification-job.unit.test.ts
  • apps/web/src/server/service/domain-service.ts
  • apps/web/src/server/service/domain-service.unit.test.ts
  • apps/web/src/server/service/webhook-service.unit.test.ts
  • apps/web/src/types/domain.ts
  • packages/lib/src/webhook/webhook-events.ts

Comment on lines +11 to +14
name: z.string().openapi({
description:
"DNS record name (hostname label). For custom MAIL FROM MX and SPF TXT records, this is the first label of the MAIL FROM host: the domain `mailFromLabel` if set, otherwise the SES `region` value.",
}),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clarify MAIL FROM DNS record names for subdomains.

The schema says this is “the first label,” but buildDnsRecords returns the full relative record name; for sub.example.com, a custom label becomes bounce.sub, not just bounce.

📝 Proposed wording update
   name: z.string().openapi({
     description:
-      "DNS record name (hostname label). For custom MAIL FROM MX and SPF TXT records, this is the first label of the MAIL FROM host: the domain `mailFromLabel` if set, otherwise the SES `region` value.",
+      "DNS record name relative to the domain's DNS zone. For MAIL FROM MX and SPF TXT records, this starts with `mailFromLabel` when set, otherwise the SES `region`, and may include the domain subdomain prefix.",
   }),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/lib/zod/domain-schema.ts` around lines 11 - 14, The openapi
description for the zod schema field name (z.string().openapi(...) in
domain-schema.ts) misleadingly states "the first label" but buildDnsRecords
actually returns the full relative record name including subdomain (e.g., for
sub.example.com a custom label becomes "bounce.sub" not just "bounce"); update
the description text in the name field to clarify it is the full relative DNS
record name (first label plus any subdomain segment) and include a brief example
such as "for sub.example.com and custom label 'bounce' the record name is
'bounce.sub'".

Comment on lines +14 to +15
/** Worst of identity verification, DKIM, and MAIL FROM (SPF); use for UI badges. */
aggregateStatus: DomainStatus;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find places that build DomainWithDnsRecords-shaped objects without aggregateStatus.
rg -nP -C3 'DomainWithDnsRecords\b'
rg -nP -C2 'aggregateStatus\s*:'

Repository: usesend/useSend

Length of output: 4858


🏁 Script executed:

#!/bin/bash
# Find all places where DomainWithDnsRecords is returned or constructed
# Check API routes and service methods
rg -nP 'DomainWithDnsRecords|\.domain\.' apps/web/src --type ts --type tsx -C2 | head -80

# Look for router/API procedures that might return domains
rg -nP 'router\.' apps/web/src/server -C2 | grep -A5 -B5 'domain'

Repository: usesend/useSend

Length of output: 86


🏁 Script executed:

#!/bin/bash
# Find all places where DomainWithDnsRecords is returned or constructed
rg 'DomainWithDnsRecords' apps/web/src --glob '*.ts' --glob '*.tsx' -C2

# Look for API routes/procedures that might return or manipulate domains
rg -nP 'return.*domain|domain.*\{' apps/web/src/server --glob '*.ts' --glob '*.tsx' | head -100

Repository: usesend/useSend

Length of output: 13558


🏁 Script executed:

#!/bin/bash
# Find tRPC router procedures for domains
rg -nP 'domain\.' apps/web/src/server/api --glob '*.ts' --glob '*.tsx' -A5 -B5

# Also check for return types with Domain or domain in the router file
fd -e ts -e tsx . apps/web/src/server/api | xargs grep -l "domain" | head -5

Repository: usesend/useSend

Length of output: 4973


🏁 Script executed:

#!/bin/bash
# Look at the domain router file to find all procedures
cat apps/web/src/server/api/routers/domain.ts

Repository: usesend/useSend

Length of output: 3052


🏁 Script executed:

#!/bin/bash
# Check the domain service functions to see what they return
# Look at getDomains, updateDomain, getDomain, setMailFromLabel, createDomain functions
rg -nP 'export async function (getDomains|updateDomain|getDomain|setMailFromLabel|createDomain|deleteDomain)' apps/web/src/server/service/domain-service.ts -A20

Repository: usesend/useSend

Length of output: 3074


🏁 Script executed:

#!/bin/bash
# Check refreshDomainVerification return type
rg -nP 'async function refreshDomainVerification' apps/web/src/server/service/domain-service.ts -A30

Repository: usesend/useSend

Length of output: 1274


🏁 Script executed:

#!/bin/bash
# Get more of the refreshDomainVerification function to see what it returns
rg -nP 'async function refreshDomainVerification' apps/web/src/server/service/domain-service.ts -A80 | tail -60

Repository: usesend/useSend

Length of output: 2220


🏁 Script executed:

#!/bin/bash
# Get the complete return statement of refreshDomainVerification
rg -nP 'async function refreshDomainVerification' apps/web/src/server/service/domain-service.ts -A120 | tail -40

Repository: usesend/useSend

Length of output: 1371


🏁 Script executed:

#!/bin/bash
# Get the final return statement
rg -nP 'async function refreshDomainVerification' apps/web/src/server/service/domain-service.ts -A130 | tail -20

Repository: usesend/useSend

Length of output: 771


🏁 Script executed:

#!/bin/bash
# Check what the updateDomain tRPC procedure returns and how it's used on frontend
rg -nP 'updateDomain' apps/web/src/app --glob '*.ts' --glob '*.tsx' -B2 -A5

Repository: usesend/useSend

Length of output: 4544


🏁 Script executed:

#!/bin/bash
# Check the exact line where updateDomain returns
sed -n '697,710p' apps/web/src/server/service/domain-service.ts

Repository: usesend/useSend

Length of output: 326


updateDomain() must wrap return value with withDnsRecords() to ensure aggregateStatus is always present.

Found that updateDomain() at line 708 returns the bare Domain object without calling withDnsRecords(), unlike all other domain-returning functions (createDomain, getDomain, getDomains, setMailFromLabel, refreshDomainVerification). This means the tRPC mutation returns a domain missing the required aggregateStatus field. While the frontend mitigates this by invalidating the cache, it breaks type safety and inconsistency with other procedures.

Change:

export async function updateDomain(
  id: number,
  data: { clickTracking?: boolean; openTracking?: boolean },
) {
  const updated = await db.domain.update({
    where: { id },
    data,
  });

  await emitDomainEvent(updated, "domain.updated");

- return updated;
+ return withDnsRecords(updated);
}
📝 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.

Suggested change
/** Worst of identity verification, DKIM, and MAIL FROM (SPF); use for UI badges. */
aggregateStatus: DomainStatus;
export async function updateDomain(
id: number,
data: { clickTracking?: boolean; openTracking?: boolean },
) {
const updated = await db.domain.update({
where: { id },
data,
});
await emitDomainEvent(updated, "domain.updated");
return withDnsRecords(updated);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/types/domain.ts` around lines 14 - 15, The updateDomain()
handler returns a bare Domain missing the aggregateStatus that other domain APIs
include; wrap its returned object with withDnsRecords(returnedDomain) before
returning so the Domain always includes aggregateStatus (consistent with
createDomain/getDomain/getDomains/setMailFromLabel/refreshDomainVerification)
and preserves type safety for the Domain type and its aggregateStatus field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants