feat: automate domain verification follow-ups#375
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Deploying usesend with
|
| Latest commit: |
5d5d50e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8aab7232.usesend.pages.dev |
| Branch Preview URL: | https://feat-domain-verification-fol.usesend.pages.dev |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
WalkthroughAdds a domain verification subsystem: Redis-backed verification state and cadence constants, functions to refresh domain verification and determine when rechecks are due, and notification logic for status changes. Introduces a BullMQ job (hourly) and worker to enqueue/process due domain verifications, a new email template for domain verification status, queue constant for "domain-verification", and comprehensive unit tests for the job and domain-service behaviors. Integrates the job startup hookup into server instrumentation. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/web/src/server/service/domain-service.ts (1)
18-20: Keep the new internal imports on the~/alias.These added imports are still using relative paths inside
apps/web/src. Please switch them to~/server/...to match the repo import convention.Suggested diff
-import { getRedis, redisKey } from "../redis"; -import { sendMail } from "../mailer"; -import { renderDomainVerificationStatusEmail } from "../email-templates"; +import { getRedis, redisKey } from "~/server/redis"; +import { sendMail } from "~/server/mailer"; +import { renderDomainVerificationStatusEmail } from "~/server/email-templates";As per coding guidelines, "apps/web/src/**/*.{ts,tsx}: Use the
~/alias for imports from src in apps/web (e.g.,import { x } from "~/utils/x")".🤖 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 18 - 20, The imports getRedis, redisKey, sendMail, and renderDomainVerificationStatusEmail are using relative paths; update them to use the repo alias by importing from "~/server/redis", "~/server/mailer", and "~/server/email-templates" respectively so they follow the apps/web src alias convention; keep the same imported symbols (getRedis, redisKey, sendMail, renderDomainVerificationStatusEmail) and ensure existing code referencing those functions remains unchanged.apps/web/src/server/email-templates/DomainVerificationStatusEmail.tsx (1)
5-8: Use the~/alias for these internal imports.The new
./components/...imports bypass theapps/web/srcalias convention. Please switch them to~/server/email-templates/components/...for consistency with the rest of the app.Suggested diff
-import { EmailLayout } from "./components/EmailLayout"; -import { EmailHeader } from "./components/EmailHeader"; -import { EmailFooter } from "./components/EmailFooter"; -import { EmailButton } from "./components/EmailButton"; +import { EmailLayout } from "~/server/email-templates/components/EmailLayout"; +import { EmailHeader } from "~/server/email-templates/components/EmailHeader"; +import { EmailFooter } from "~/server/email-templates/components/EmailFooter"; +import { EmailButton } from "~/server/email-templates/components/EmailButton";As per coding guidelines, "apps/web/src/**/*.{ts,tsx}: Use the
~/alias for imports from src in apps/web (e.g.,import { x } from "~/utils/x")".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/email-templates/DomainVerificationStatusEmail.tsx` around lines 5 - 8, Update the internal component imports in DomainVerificationStatusEmail.tsx to use the apps/web src alias: replace the relative paths importing EmailLayout, EmailHeader, EmailFooter, and EmailButton from "./components/..." with the aliased paths under "~/server/email-templates/components/..." so the file imports EmailLayout, EmailHeader, EmailFooter, and EmailButton via the `~/` alias consistent with the repo convention.apps/web/src/server/service/domain-service.unit.test.ts (1)
129-300: Please add regression coverage for notification races and mail failures.These tests cover the serial happy/failure paths, but they would still pass if two concurrent refreshes double-sent the same status email or if
sendMailcausedrefreshDomainVerification()to throw after the DB update.As per coding guidelines, "apps/web/**/*.{unit,trpc,api,integration}.test.{ts,tsx}: Add tests when changes impact logic, APIs, or behavior in apps/web".
🤖 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.unit.test.ts` around lines 129 - 300, Add unit tests around refreshDomainVerification to cover two regressions: (1) a race where two concurrent refreshDomainVerification(domain) calls could double-send the same status email — simulate by calling refreshDomainVerification twice in parallel with mockRedis.mget returning values that indicate notifications are due and assert mockSendMail was called only once and mockDb.domain.update was called appropriately; (2) a mail-sender failure after the DB update — simulate mockSendMail throwing on the second call (or after mockDb.domain.update resolves) and assert that refreshDomainVerification still returns the updated domain/state and does not leave isVerifying stuck or rethrow the error (verify mockDb.domain.update was called and result.status reflects the DB update). Reference refreshDomainVerification, mockSendMail, mockDb.domain.update, mockRedis.mget and isDomainVerificationDue to locate and implement these tests.apps/web/src/instrumentation.ts (1)
2-2: Use the src alias here as well.
apps/web/srcimports should use~/...instead of relative paths, so this should come from~/utils/common.Suggested diff
-import { isCloud, isEmailCleanupEnabled } from "./utils/common"; +import { isCloud, isEmailCleanupEnabled } from "~/utils/common";As per coding guidelines, "apps/web/src/**/*.{ts,tsx}: Use the
~/alias for imports from src in apps/web (e.g.,import { x } from "~/utils/x")".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/instrumentation.ts` at line 2, Replace the relative import in apps/web/src/instrumentation.ts with the src alias: change the import of isCloud and isEmailCleanupEnabled from "./utils/common" to use the "~/..." alias (import { isCloud, isEmailCleanupEnabled } from "~/utils/common") so it follows the apps/web/src import convention and resolves from the project src root.
🤖 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/instrumentation.ts`:
- Line 28: The unconditional dynamic import of
"~/server/jobs/domain-verification-job" causes Redis/BullMQ initialization (via
getRedis()) at module load and can break startup/builds without Redis; change
the job module to export a function (e.g., initDomainVerificationJob) that
encapsulates BullMQ/getRedis initialization, update the job module to avoid side
effects at top-level, then replace the dynamic import in instrumentation.ts with
a top-level static import of initDomainVerificationJob and call
initDomainVerificationJob() only when the env/feature gate is enabled (the same
condition used elsewhere), thereby preventing Redis connection attempts during
builds and complying with the no-dynamic-import rule.
In `@apps/web/src/server/service/domain-service.ts`:
- Around line 466-525: The code reads verificationState.lastNotifiedStatus,
decides to send, then updates Redis after sending which allows a race where two
concurrent refreshDomainVerification() callers both send the same notification;
fix by performing an atomic check-and-reserve in Redis before sending:
add/replace usage of setLastNotifiedDomainStatus/getDomainVerificationState with
a new atomic reserveNotificationForStatus(domainId, status) (or implement
compare-and-set in setLastNotifiedDomainStatus) that uses Redis SET NX (or a Lua
script/CAS) to verify lastNotifiedStatus != status and simultaneously set it
(with an optional TTL) so only the winning caller proceeds to call
sendDomainStatusNotification; call this reserve function immediately before
sendDomainStatusNotification and only send if it returns success.
- Around line 519-524: Wrap the call to sendDomainStatusNotification in a
try/catch so any rejection is logged/handled and not rethrown, and ensure
setLastNotifiedDomainStatus(domain.id, updatedDomain.status) always runs (e.g.,
call it after the catch or in a finally block); reference
sendDomainStatusNotification and setLastNotifiedDomainStatus and avoid changing
behavior of getDomain/isVerifying—just prevent notification failures from
bubbling up and skipping the webhook path.
---
Nitpick comments:
In `@apps/web/src/instrumentation.ts`:
- Line 2: Replace the relative import in apps/web/src/instrumentation.ts with
the src alias: change the import of isCloud and isEmailCleanupEnabled from
"./utils/common" to use the "~/..." alias (import { isCloud,
isEmailCleanupEnabled } from "~/utils/common") so it follows the apps/web/src
import convention and resolves from the project src root.
In `@apps/web/src/server/email-templates/DomainVerificationStatusEmail.tsx`:
- Around line 5-8: Update the internal component imports in
DomainVerificationStatusEmail.tsx to use the apps/web src alias: replace the
relative paths importing EmailLayout, EmailHeader, EmailFooter, and EmailButton
from "./components/..." with the aliased paths under
"~/server/email-templates/components/..." so the file imports EmailLayout,
EmailHeader, EmailFooter, and EmailButton via the `~/` alias consistent with the
repo convention.
In `@apps/web/src/server/service/domain-service.ts`:
- Around line 18-20: The imports getRedis, redisKey, sendMail, and
renderDomainVerificationStatusEmail are using relative paths; update them to use
the repo alias by importing from "~/server/redis", "~/server/mailer", and
"~/server/email-templates" respectively so they follow the apps/web src alias
convention; keep the same imported symbols (getRedis, redisKey, sendMail,
renderDomainVerificationStatusEmail) and ensure existing code referencing those
functions remains unchanged.
In `@apps/web/src/server/service/domain-service.unit.test.ts`:
- Around line 129-300: Add unit tests around refreshDomainVerification to cover
two regressions: (1) a race where two concurrent
refreshDomainVerification(domain) calls could double-send the same status email
— simulate by calling refreshDomainVerification twice in parallel with
mockRedis.mget returning values that indicate notifications are due and assert
mockSendMail was called only once and mockDb.domain.update was called
appropriately; (2) a mail-sender failure after the DB update — simulate
mockSendMail throwing on the second call (or after mockDb.domain.update
resolves) and assert that refreshDomainVerification still returns the updated
domain/state and does not leave isVerifying stuck or rethrow the error (verify
mockDb.domain.update was called and result.status reflects the DB update).
Reference refreshDomainVerification, mockSendMail, mockDb.domain.update,
mockRedis.mget and isDomainVerificationDue to locate and implement these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d94d0089-b8fe-4ef3-b8eb-18de6190eb29
📒 Files selected for processing (8)
apps/web/src/instrumentation.tsapps/web/src/server/email-templates/DomainVerificationStatusEmail.tsxapps/web/src/server/email-templates/index.tsapps/web/src/server/jobs/domain-verification-job.tsapps/web/src/server/jobs/domain-verification-job.unit.test.tsapps/web/src/server/queue/queue-constants.tsapps/web/src/server/service/domain-service.tsapps/web/src/server/service/domain-service.unit.test.ts
Comment on lines
+519
to
+524
| await sendDomainStatusNotification({ | ||
| domain: updatedDomain, | ||
| previousStatus, | ||
| verificationError, | ||
| }); | ||
| await setLastNotifiedDomainStatus(domain.id, updatedDomain.status); |
Contributor
There was a problem hiding this comment.
Don’t let notification failures turn a successful refresh into a thrown request error.
If sendDomainStatusNotification() rejects, this function throws after the domain row is already updated. Because getDomain() calls this inline while isVerifying is true, a transient mail failure can surface as a 500 on domain fetches and also skip the webhook path below.
Suggested diff
if (
shouldSendDomainStatusNotification({
currentStatus: updatedDomain.status,
hasEverVerified:
verificationState.hasEverVerified ||
updatedDomain.status === DomainStatus.SUCCESS,
lastNotifiedStatus: verificationState.lastNotifiedStatus,
})
) {
- await sendDomainStatusNotification({
- domain: updatedDomain,
- previousStatus,
- verificationError,
- });
- await setLastNotifiedDomainStatus(domain.id, updatedDomain.status);
+ try {
+ await sendDomainStatusNotification({
+ domain: updatedDomain,
+ previousStatus,
+ verificationError,
+ });
+ await setLastNotifiedDomainStatus(domain.id, updatedDomain.status);
+ } catch (error) {
+ logger.error(
+ { err: error, domainId: domain.id },
+ "[DomainService]: Failed to send domain status notification",
+ );
+ }
}🤖 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 519 - 524, Wrap
the call to sendDomainStatusNotification in a try/catch so any rejection is
logged/handled and not rethrown, and ensure
setLastNotifiedDomainStatus(domain.id, updatedDomain.status) always runs (e.g.,
call it after the catch or in a finally block); reference
sendDomainStatusNotification and setLastNotifiedDomainStatus and avoid changing
behavior of getDomain/isVerifying—just prevent notification failures from
bubbling up and skipping the webhook path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/instrumentation.ts (1)
28-30:⚠️ Potential issue | 🟠 MajorDon't let scheduler init failures take down the app.
process.env.REDIS_URLonly gates presence.initDomainVerificationJob()still opens Redis/BullMQ and can reject on a bad URL or a temporary outage, andregister()awaits it inline. That makes a background follow-up job capable of aborting web startup. Catch/log the failure here and let the app continue booting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/instrumentation.ts` around lines 28 - 30, Wrap the await initDomainVerificationJob() call in a try/catch so failures opening Redis/BullMQ don't abort startup: in instrumentation.ts, around the existing check for process.env.REDIS_URL, change the inline await initDomainVerificationJob() to be inside a try block, catch any error from initDomainVerificationJob(), log the error with context (e.g., using the app logger or console.error) and do not rethrow so the app can continue booting; ensure the catch message references initDomainVerificationJob to aid debugging.
🤖 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/server/email-templates/DomainVerificationStatusEmail.tsx`:
- Around line 39-40: The email body is rendering raw enum values; update
DomainVerificationStatusEmail to humanize currentStatus and previousStatus the
same way as the preview; replace direct uses of currentStatus and previousStatus
in the JSX (the same area covered by preview and lines ~55-68) with a small
helper or inline expression like currentStatus.toLowerCase().replaceAll("_", "
") (and same for previousStatus) so both the subject/preview and body show
human-friendly labels.
In `@apps/web/src/server/service/domain-service.ts`:
- Line 628: After db.domain.delete() and SES succeed, make the Redis cleanup in
clearDomainVerificationState(id) best-effort by wrapping that call in a
try/catch so any thrown error is logged (use the existing logger/process logger)
and swallowed instead of rethrowing; do not prevent the subsequent webhook call
from running — ensure the webhook invocation that follows the
clearDomainVerificationState(id) call still executes even when the cleanup
fails.
- Around line 501-516: The code updates domain state with updatedDomain (setting
dkimStatus, spfDetails, status, isVerifying) and then calls
sendDomainStatusNotification which may fail and is only logged; change this so
notification delivery is durable: instead of calling
sendDomainStatusNotification synchronously inside the same transaction, record a
pending notification row or enqueue a retryable job (e.g., create a
DomainNotification record or push to NotificationQueue) referencing
updatedDomain.id and the new status, and have the notification worker/process
read those pending notifications and retry with backoff until success; update
the flow around sendDomainStatusNotification (and the similar block at lines
521-547) to write the durable pointer (pending flag or queue message)
before/after the domain update and remove the pending entry only once the
notification is successfully delivered.
---
Duplicate comments:
In `@apps/web/src/instrumentation.ts`:
- Around line 28-30: Wrap the await initDomainVerificationJob() call in a
try/catch so failures opening Redis/BullMQ don't abort startup: in
instrumentation.ts, around the existing check for process.env.REDIS_URL, change
the inline await initDomainVerificationJob() to be inside a try block, catch any
error from initDomainVerificationJob(), log the error with context (e.g., using
the app logger or console.error) and do not rethrow so the app can continue
booting; ensure the catch message references initDomainVerificationJob to aid
debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c6732a4-56c2-4473-99ca-b783d2693d2b
📒 Files selected for processing (6)
apps/web/src/instrumentation.tsapps/web/src/server/email-templates/DomainVerificationStatusEmail.tsxapps/web/src/server/jobs/domain-verification-job.tsapps/web/src/server/jobs/domain-verification-job.unit.test.tsapps/web/src/server/service/domain-service.tsapps/web/src/server/service/domain-service.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/server/jobs/domain-verification-job.ts
Comment on lines
+39
to
+40
| const preview = `${domainName} is now ${currentStatus.toLowerCase().replaceAll("_", " ")}`; | ||
|
|
Contributor
There was a problem hiding this comment.
Humanize the status labels in the body too.
The preview already formats enum values for humans, but the body still renders raw DomainStatus values like TEMPORARY_FAILURE and NOT_STARTED. Reuse the same formatter for the current and previous status text so the email reads cleanly.
Also applies to: 55-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/server/email-templates/DomainVerificationStatusEmail.tsx` around
lines 39 - 40, The email body is rendering raw enum values; update
DomainVerificationStatusEmail to humanize currentStatus and previousStatus the
same way as the preview; replace direct uses of currentStatus and previousStatus
in the JSX (the same area covered by preview and lines ~55-68) with a small
helper or inline expression like currentStatus.toLowerCase().replaceAll("_", "
") (and same for previousStatus) so both the subject/preview and body show
human-friendly labels.
Comment on lines
+501
to
+516
| const updatedDomain = await db.domain.update({ | ||
| where: { | ||
| id: domain.id, | ||
| }, | ||
| data: { | ||
| dkimStatus: dkimStatus ?? null, | ||
| spfDetails: spfDetails ?? null, | ||
| status: verificationStatus, | ||
| errorMessage: verificationError, | ||
| dmarcAdded: Boolean(dmarcRecord), | ||
| isVerifying: shouldContinueVerifying( | ||
| verificationStatus, | ||
| dkimStatus, | ||
| spfDetails, | ||
| status: verificationStatus ?? "NOT_STARTED", | ||
| dmarcAdded: dmarcRecord ? true : false, | ||
| isVerifying: | ||
| verificationStatus === "SUCCESS" && | ||
| dkimStatus === "SUCCESS" && | ||
| spfDetails === "SUCCESS" | ||
| ? false | ||
| : true, | ||
| }, | ||
| }); | ||
| ), | ||
| }, |
Contributor
There was a problem hiding this comment.
Notification failures need a durable retry path.
By the time this block runs, the new status and isVerifying decision are already persisted. If sendDomainStatusNotification() fails once, this catch only logs it; for an initial terminal FAILED that can mean the team is never notified, and for previously verified domains the next automatic retry may be 30 days later. Track pending notification delivery separately or move it onto its own retryable job.
Also applies to: 521-547
🤖 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 501 - 516, The
code updates domain state with updatedDomain (setting dkimStatus, spfDetails,
status, isVerifying) and then calls sendDomainStatusNotification which may fail
and is only logged; change this so notification delivery is durable: instead of
calling sendDomainStatusNotification synchronously inside the same transaction,
record a pending notification row or enqueue a retryable job (e.g., create a
DomainNotification record or push to NotificationQueue) referencing
updatedDomain.id and the new status, and have the notification worker/process
read those pending notifications and retry with backoff until success; update
the flow around sendDomainStatusNotification (and the similar block at lines
521-547) to write the durable pointer (pending flag or queue message)
before/after the domain update and remove the pending entry only once the
notification is successfully delivered.
| } | ||
|
|
||
| const deletedRecord = await db.domain.delete({ where: { id } }); | ||
| await clearDomainVerificationState(id); |
Contributor
There was a problem hiding this comment.
Make Redis cleanup best-effort on delete.
SES and db.domain.delete() have already succeeded before this call. If clearDomainVerificationState() throws, the API returns a 500 for a domain that is already gone and the webhook below is skipped. This cleanup should be caught and logged instead of unwinding the delete.
🧹 Suggested change
const deletedRecord = await db.domain.delete({ where: { id } });
- await clearDomainVerificationState(id);
+ try {
+ await clearDomainVerificationState(id);
+ } catch (error) {
+ logger.error(
+ { err: error, domainId: id },
+ "[DomainService]: Failed to clear domain verification state",
+ );
+ }📝 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
| await clearDomainVerificationState(id); | |
| const deletedRecord = await db.domain.delete({ where: { id } }); | |
| try { | |
| await clearDomainVerificationState(id); | |
| } catch (error) { | |
| logger.error( | |
| { err: error, domainId: id }, | |
| "[DomainService]: Failed to clear domain verification state", | |
| ); | |
| } |
🤖 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` at line 628, After
db.domain.delete() and SES succeed, make the Redis cleanup in
clearDomainVerificationState(id) best-effort by wrapping that call in a
try/catch so any thrown error is logged (use the existing logger/process logger)
and swallowed instead of rethrowing; do not prevent the subsequent webhook call
from running — ensure the webhook invocation that follows the
clearDomainVerificationState(id) call still executes even when the cleanup
fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
pnpm test:web:unit -- --runInBand domain-service.unit.test.ts domain-verification-job.unit.test.tsNEXTAUTH_SECRET=dummy pnpm buildSummary by cubic
Automates domain verification follow-ups with an hourly
bullmqjob. Rechecks unverified domains every 6 hours and verified domains every 30 days, sends one email per status change with concurrency-safe locking, stores cadence/notification state in Redis, and only starts the worker when Redis is configured.REDIS_URLis present; worker runs with concurrency 1 and refreshes only due domains.Written for commit 5d5d50e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests