feat(self-hosted): custom SES tracking domain, HTTPS toggle, unsubscribe tracking fixes (#345)#391
Open
amanjuman wants to merge 4 commits intousesend:mainfrom
Open
Conversation
…scribe fixes Add SES custom tracking hostname for self-hosted installs (DNS, verification, per-domain configuration sets). Store tracking HTTPS preference on the domain (OPTIONAL vs REQUIRE) instead of a global env var; document Cloudflare proxy as a simple TLS option for tracking links. Apply ses:no-track to unsubscribe links so opt-outs are not click-tracked, and broaden campaign analytics to ignore unsubscribe destination URLs. Include Prisma migrations, root docker-compose for app/postgres/redis, ignore .pnpm-store, and update unit tests. Made-with: Cursor
Drop fork-local compose file not needed upstream; keep .env.selfhost.example aligned with existing AWS vars only. Made-with: Cursor
|
@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. |
![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 custom tracking domain support across the stack: Prisma schema and migrations for new Domain fields, DB unique index, tRPC endpoint to set/clear custom tracking hostnames, extensive domain-service logic for SES identity/configuration-set lifecycle and verification polling, SES v2 helper functions (HTTPS policy and configuration set operations), UI for managing custom tracking hostname and HTTPS requirement, HTML utility to mark unsubscribe links with Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds self-hosted support for per-domain SES click/open tracking using a custom tracking hostname, including DNS record guidance, optional HTTPS enforcement for tracking URLs, and fixes to exclude unsubscribe interactions from engagement metrics.
Changes:
- Add per-domain custom SES tracking identity + per-domain configuration sets (general/click/open/full) and selection logic in the email queue.
- Add unsubscribe tracking exemptions: inject
ses:no-trackon unsubscribe links and adjust webhook click parsing to avoid counting opt-outs as engagement. - Extend domain DNS record models/UI to include tracking CNAME/TXT records plus per-domain HTTPS toggle; include Prisma schema + migrations.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/utils/ses-utils.ts | Chooses per-domain vs global SES configuration set names. |
| apps/web/src/types/domain.ts | Extends DNS record types to include CNAME. |
| apps/web/src/server/utils/ses-tracking-html.ts | Adds helper to inject ses:no-track on unsubscribe links. |
| apps/web/src/server/utils/ses-tracking-html.unit.test.ts | Unit tests for ses:no-track injection helper. |
| apps/web/src/server/service/webhook-service.unit.test.ts | Fixes contactBookId test payload type (string). |
| apps/web/src/server/service/ses-hook-parser.ts | Exempts unsubscribe URLs from campaign engagement analytics. |
| apps/web/src/server/service/email-queue-service.ts | Passes domain tracking config fields into configuration set selection. |
| apps/web/src/server/service/domain-service.ts | Implements custom tracking hostname lifecycle, DNS record generation, provisioning/cleanup, and verification polling. |
| apps/web/src/server/service/domain-service.unit.test.ts | Updates domain test fixtures with new tracking fields. |
| apps/web/src/server/jobs/domain-verification-job.unit.test.ts | Updates domain test fixtures with new tracking fields. |
| apps/web/src/server/aws/ses.ts | Adds SES helpers for tracking identity + config set tracking options; injects ses:no-track before sending. |
| apps/web/src/server/api/routers/domain.ts | Adds tRPC mutation to set custom tracking hostname + HTTPS requirement. |
| apps/web/src/lib/zod/domain-schema.ts | Updates OpenAPI schema to allow CNAME DNS record type. |
| apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx | Adds UI for custom tracking hostname + HTTPS toggle; polls verification status. |
| apps/web/prisma/schema.prisma | Adds new Domain fields for custom tracking + per-domain config sets + HTTPS requirement. |
| apps/web/prisma/migrations/20260418120000_custom_tracking_domain/migration.sql | Migration adding custom tracking identity + config set columns. |
| apps/web/prisma/migrations/20260418153000_domain_tracking_https_required/migration.sql | Migration adding trackingHttpsRequired. |
| .gitignore | Ignores .pnpm-store. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @see https://docs.aws.amazon.com/ses/latest/dg/faqs-metrics.html | ||
| */ | ||
| export function addSesNoTrackToUnsubscribeLinks(html: string): string { | ||
| if (!html.includes("unsubscribe")) { |
Comment on lines
+424
to
+428
| const normalized = trimmed.toLowerCase(); | ||
| if ( | ||
| !/^(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\.)+[a-z]{2,}$/i.test( | ||
| normalized, | ||
| ) |
Comment on lines
+309
to
+313
| try { | ||
| await ses.addWebhookConfiguration( | ||
| configGeneral, | ||
| topicArn, | ||
| SES_GENERAL_EVENTS, |
Comment on lines
+375
to
+379
| export async function setCustomTrackingHostname( | ||
| domainId: number, | ||
| teamId: number, | ||
| hostname: string | null, | ||
| trackingHttpsRequired?: boolean, |
![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: 9
🤖 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/prisma/schema.prisma`:
- Around line 199-210: The schema allows multiple Domain rows to share the same
customTrackingHostname which can cause deletion of SES resources still used by
another Domain; update the Prisma schema by adding a unique constraint/index on
the Domain model for the field customTrackingHostname (i.e., make
customTrackingHostname unique) and create the corresponding Prisma migration to
apply this change so the database enforces uniqueness and prevents cross-domain
resource deletion.
In `@apps/web/src/server/aws/ses.ts`:
- Around line 207-221: putConfigurationSetHttpsTracking currently returns false
on non-200 responses which callers (e.g. functions in domain-service.ts that
await it) ignore, allowing silent failures; change
putConfigurationSetHttpsTracking to throw an Error when
response.$metadata.httpStatusCode !== 200 (include the status code and any
response metadata or message in the error text) so failures bubble up and are
handled by callers instead of being silently ignored; update the function body
around getSesClient/PutConfigurationSetTrackingOptionsCommand to check the
response and throw, leaving callers to catch or propagate the exception.
In `@apps/web/src/server/service/domain-service.ts`:
- Around line 166-180: The background "due" check in isDomainVerificationDue
currently ignores custom tracking status, so a domain with an already-verified
primary status can leave a new tracking hostname stuck PENDING; update
isDomainVerificationDue to also consider
shouldPollCustomTrackingVerification(domain) (call the existing function) and
return true when it indicates polling is needed, ensuring customTracking
verification is evaluated independently of the primary domain cadence and
triggers background checks even if the primary domain is recently checked.
- Around line 446-459: The code currently persists trackingHttpsRequired via
db.domain.update before calling reapplyCustomTrackingSesPolicy, which means a
failed SES reapply leaves the DB/UI inconsistent; change the flow so the SES
reapply succeeds before committing the DB change (or perform the DB update
inside a transaction and roll it back if reapplyCustomTrackingSesPolicy throws).
Specifically, call reapplyCustomTrackingSesPolicy using the existing domain
state (or the intended new value) first and only call db.domain.update (and then
emitDomainEvent) after reapplyCustomTrackingSesPolicy returns successfully; on
reapply failure, propagate or handle the error without persisting
trackingHttpsRequired for domainId so the DB and SES remain consistent.
- Around line 424-436: The current validation accepts apex hostnames like
"example.com" which later cause buildTrackingDnsRecords() to skip emitting the
routing CNAME; update the validation in the same block that computes normalized
to reject hostnames without a subdomain by parsing the host (e.g., using
tldts.parse(normalized).subdomain) and throwing the same UnsendApiError (code
"BAD_REQUEST") when subdomain is missing; place this check before calling
assertTrackingHostnameAllowed(domain.name, normalized) so only hostnames with a
subdomain proceed and the routing CNAME will be generated.
- Around line 464-489: The code currently deletes old SES tracking resources
with removeCustomTrackingResources(domain) before creating the new identity via
ses.addTrackingEmailIdentity and updating the DB; change the sequence so you
first compute selector (domain.customTrackingDkimSelector ?? "utrack"), call
ses.addTrackingEmailIdentity(normalized, domain.region, domain.sesTenantId ??
undefined, selector) and get publicKey, then perform the db.domain.update to
point to the new hostname/publicKey/selector and statuses, and only after the
new identity and DB update succeed call removeCustomTrackingResources for the
previous customTrackingHostname (if it existed) to avoid leaving the DB pointing
to deleted config sets; update logic in the function containing these calls
(references: removeCustomTrackingResources, addTrackingEmailIdentity,
db.domain.update, customTrackingHostname, customTrackingDkimSelector)
accordingly.
- Around line 309-365: The provisioning is not idempotent because
addWebhookConfiguration (ses.addWebhookConfiguration) does not handle AWS
AlreadyExistsException; update the addWebhookConfiguration implementation to
catch AlreadyExistsException and treat it as success (mirror how
deleteConfigurationSet handles NotFoundException) so retries don’t fail when a
configuration set already exists, or alternatively ensure partial resources are
rolled back before rethrowing—modify the addWebhookConfiguration function in
ses.ts (where the call is implemented) to swallow/handle AlreadyExistsException
and log it as a non-error.
In `@apps/web/src/server/utils/ses-tracking-html.ts`:
- Around line 8-13: The early-return guard uses html.includes("unsubscribe")
which is case-sensitive and can skip mixed-case unsubscribe links; change the
pre-check to be case-insensitive (e.g., use
html.toLowerCase().includes("unsubscribe") or use /unsubscribe/i.test(html)) so
the subsequent replace call (the regex with the i flag) always runs for any case
variation and properly adds the ses:no-track attribute.
🪄 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: c3537cac-30fc-49f6-b05a-f218a5b0287f
📒 Files selected for processing (18)
.gitignoreapps/web/prisma/migrations/20260418120000_custom_tracking_domain/migration.sqlapps/web/prisma/migrations/20260418153000_domain_tracking_https_required/migration.sqlapps/web/prisma/schema.prismaapps/web/src/app/(dashboard)/domains/[domainId]/page.tsxapps/web/src/lib/zod/domain-schema.tsapps/web/src/server/api/routers/domain.tsapps/web/src/server/aws/ses.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.tsapps/web/src/server/service/email-queue-service.tsapps/web/src/server/service/ses-hook-parser.tsapps/web/src/server/service/webhook-service.unit.test.tsapps/web/src/server/utils/ses-tracking-html.tsapps/web/src/server/utils/ses-tracking-html.unit.test.tsapps/web/src/types/domain.tsapps/web/src/utils/ses-utils.ts
Comment on lines
+199
to
+210
| /// Self-hosted: custom hostname for SES click/open tracking (e.g. track.example.com). Requires DNS + verification in SES. | ||
| customTrackingHostname String? | ||
| customTrackingPublicKey String? | ||
| customTrackingDkimSelector String? @default("utrack") | ||
| customTrackingDkimStatus String? | ||
| customTrackingStatus DomainStatus @default(NOT_STARTED) | ||
| trackingConfigGeneral String? | ||
| trackingConfigClick String? | ||
| trackingConfigOpen String? | ||
| trackingConfigFull String? | ||
| /// Self-hosted: when true, SES uses HTTPS for tracking links (REQUIRE). Needs valid TLS on the tracking hostname (e.g. Cloudflare proxy). When false, OPTIONAL (HTTP allowed; CNAME-only to awstrack). | ||
| trackingHttpsRequired Boolean @default(false) |
Contributor
There was a problem hiding this comment.
Make customTrackingHostname unique to prevent cross-domain resource deletion.
Right now two Domain rows can point at the same SES tracking hostname. Clearing or deleting one domain then calls custom tracking cleanup and can remove SES resources still referenced by the other domain.
Suggested schema hardening
- customTrackingHostname String?
+ customTrackingHostname String? `@unique`Add the matching Prisma migration as well.
📝 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
| /// Self-hosted: custom hostname for SES click/open tracking (e.g. track.example.com). Requires DNS + verification in SES. | |
| customTrackingHostname String? | |
| customTrackingPublicKey String? | |
| customTrackingDkimSelector String? @default("utrack") | |
| customTrackingDkimStatus String? | |
| customTrackingStatus DomainStatus @default(NOT_STARTED) | |
| trackingConfigGeneral String? | |
| trackingConfigClick String? | |
| trackingConfigOpen String? | |
| trackingConfigFull String? | |
| /// Self-hosted: when true, SES uses HTTPS for tracking links (REQUIRE). Needs valid TLS on the tracking hostname (e.g. Cloudflare proxy). When false, OPTIONAL (HTTP allowed; CNAME-only to awstrack). | |
| trackingHttpsRequired Boolean @default(false) | |
| /// Self-hosted: custom hostname for SES click/open tracking (e.g. track.example.com). Requires DNS + verification in SES. | |
| customTrackingHostname String? `@unique` | |
| customTrackingPublicKey String? | |
| customTrackingDkimSelector String? `@default`("utrack") | |
| customTrackingDkimStatus String? | |
| customTrackingStatus DomainStatus `@default`(NOT_STARTED) | |
| trackingConfigGeneral String? | |
| trackingConfigClick String? | |
| trackingConfigOpen String? | |
| trackingConfigFull String? | |
| /// Self-hosted: when true, SES uses HTTPS for tracking links (REQUIRE). Needs valid TLS on the tracking hostname (e.g. Cloudflare proxy). When false, OPTIONAL (HTTP allowed; CNAME-only to awstrack). | |
| trackingHttpsRequired Boolean `@default`(false) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/prisma/schema.prisma` around lines 199 - 210, The schema allows
multiple Domain rows to share the same customTrackingHostname which can cause
deletion of SES resources still used by another Domain; update the Prisma schema
by adding a unique constraint/index on the Domain model for the field
customTrackingHostname (i.e., make customTrackingHostname unique) and create the
corresponding Prisma migration to apply this change so the database enforces
uniqueness and prevents cross-domain resource deletion.
Comment on lines
+464
to
+489
| if (domain.customTrackingHostname) { | ||
| await removeCustomTrackingResources(domain); | ||
| } | ||
|
|
||
| const selector = domain.customTrackingDkimSelector ?? "utrack"; | ||
| const publicKey = await ses.addTrackingEmailIdentity( | ||
| normalized, | ||
| domain.region, | ||
| domain.sesTenantId ?? undefined, | ||
| selector, | ||
| ); | ||
|
|
||
| const updated = await db.domain.update({ | ||
| where: { id: domainId }, | ||
| data: { | ||
| customTrackingHostname: normalized, | ||
| customTrackingPublicKey: publicKey, | ||
| customTrackingDkimSelector: selector, | ||
| customTrackingDkimStatus: null, | ||
| customTrackingStatus: DomainStatus.PENDING, | ||
| trackingConfigGeneral: null, | ||
| trackingConfigClick: null, | ||
| trackingConfigOpen: null, | ||
| trackingConfigFull: null, | ||
| trackingHttpsRequired: trackingHttpsRequired ?? false, | ||
| }, |
Contributor
There was a problem hiding this comment.
Create the new tracking identity before deleting the old resources.
When changing hostnames, the old SES identity/config sets are deleted before addTrackingEmailIdentity() and the DB update. If the new SES call fails, the DB still points to deleted config sets, so subsequent sends can use stale configuration names and fail.
Safer sequencing
- if (domain.customTrackingHostname) {
- await removeCustomTrackingResources(domain);
- }
+ const previousTrackingDomain = domain.customTrackingHostname ? domain : null;
const selector = domain.customTrackingDkimSelector ?? "utrack";
const publicKey = await ses.addTrackingEmailIdentity(
normalized,
domain.region,
@@
await emitDomainEvent(updated, "domain.updated");
+
+ if (previousTrackingDomain) {
+ await removeCustomTrackingResources(previousTrackingDomain);
+ }
+
return updated;
}🤖 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 464 - 489, The
code currently deletes old SES tracking resources with
removeCustomTrackingResources(domain) before creating the new identity via
ses.addTrackingEmailIdentity and updating the DB; change the sequence so you
first compute selector (domain.customTrackingDkimSelector ?? "utrack"), call
ses.addTrackingEmailIdentity(normalized, domain.region, domain.sesTenantId ??
undefined, selector) and get publicKey, then perform the db.domain.update to
point to the new hostname/publicKey/selector and statuses, and only after the
new identity and DB update succeed call removeCustomTrackingResources for the
previous customTrackingHostname (if it existed) to avoid leaving the DB pointing
to deleted config sets; update logic in the function containing these calls
(references: removeCustomTrackingResources, addTrackingEmailIdentity,
db.domain.update, customTrackingHostname, customTrackingDkimSelector)
accordingly.
Comment on lines
+35
to
+54
| /** Destination URLs for opt-out should not increment campaign click/open-style engagement. */ | ||
| function isUnsubscribeEngagementExemptLink(link: string | undefined): boolean { | ||
| if (!link) { | ||
| return false; | ||
| } | ||
| try { | ||
| const base = new URL(env.NEXTAUTH_URL); | ||
| const u = new URL(link); | ||
| if (u.origin !== base.origin) { | ||
| return false; | ||
| } | ||
| return /\bunsubscribe\b/i.test(`${u.pathname}${u.search}`); | ||
| } catch { | ||
| const prefix = env.NEXTAUTH_URL.replace(/\/$/, ""); | ||
| return ( | ||
| link.startsWith(`${prefix}/unsubscribe`) || | ||
| /\/api\/unsubscribe/i.test(link) | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t count valid external unsubscribe URLs as campaign engagement.
isUnsubscribeEngagementExemptLink only exempts unsubscribe links on NEXTAUTH_URL in the normal URL path. A valid branded/custom unsubscribe URL such as https://example.com/unsubscribe returns false, so Line 288 allows it to increment campaign click analytics despite being an opt-out destination.
Proposed fix
try {
- const base = new URL(env.NEXTAUTH_URL);
const u = new URL(link);
- if (u.origin !== base.origin) {
- return false;
- }
return /\bunsubscribe\b/i.test(`${u.pathname}${u.search}`);
} catch {
const prefix = env.NEXTAUTH_URL.replace(/\/$/, "");Also applies to: 286-289
…ema uniqueness) - Case-insensitive unsubscribe pre-check; reject apex tracking hostnames - PutConfigurationSetTrackingOptions throws on non-200; addWebhookConfiguration treats AlreadyExists as success and still returns boolean - Poll custom tracking verification on 6h cadence when tracking DNS is pending - Apply HTTPS policy to SES before persisting DB; create new tracking identity before removing old SES resources - Unique customTrackingHostname to avoid cross-domain SES cleanup clashes - Tests: mixed-case unsubscribe, verification due with pending tracking Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/server/aws/ses.ts (1)
1-17:⚠️ Potential issue | 🟠 MajorAdd
UpdateConfigurationSetEventDestinationCommandimport and update existing SES event destinations instead of silently continuing.When an event destination already exists, the current code returns success without reconciling
MatchingEventTypesorTopicArn. If a retry occurs after a partial deploy, SES settings change, or the event types list changes, the webhook will remain wired to stale settings while the database records success.Required changes
Add the import at the top of the file:
import { SESv2Client, CreateEmailIdentityCommand, DeleteEmailIdentityCommand, GetEmailIdentityCommand, PutEmailIdentityMailFromAttributesCommand, SendEmailCommand, CreateConfigurationSetEventDestinationCommand, CreateConfigurationSetCommand, DeleteConfigurationSetCommand, PutConfigurationSetTrackingOptionsCommand, + UpdateConfigurationSetEventDestinationCommand, EventType,Update the event destination creation logic to reconcile changes:
+ const eventDestination = { + Enabled: true, + MatchingEventTypes: eventTypes, + SnsDestination: { + TopicArn: topicArn, + }, + }; + const command = new CreateConfigurationSetEventDestinationCommand({ ConfigurationSetName: configName, EventDestinationName: "usesend_destination", - EventDestination: { - Enabled: true, - MatchingEventTypes: eventTypes, - SnsDestination: { - TopicArn: topicArn, - }, - }, + EventDestination: eventDestination, }); try { const response = await sesClient.send(command); if (response.$metadata.httpStatusCode !== 200) { throw new Error("Failed to create configuration set event destination"); } } catch (error: unknown) { if (!isAlreadyExistsError(error)) { throw error; } + const response = await sesClient.send( + new UpdateConfigurationSetEventDestinationCommand({ + ConfigurationSetName: configName, + EventDestinationName: "usesend_destination", + EventDestination: eventDestination, + }), + ); + if (response.$metadata.httpStatusCode !== 200) { + throw new Error("Failed to update configuration set event destination"); + } logger.debug( { configName, region }, - "SES event destination already exists; continuing", + "SES event destination already exists; updated", ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/aws/ses.ts` around lines 1 - 17, Add the missing UpdateConfigurationSetEventDestinationCommand import from "@aws-sdk/client-sesv2" and change the CreateConfigurationSetEventDestinationCommand error handling so that if the event destination already exists you call UpdateConfigurationSetEventDestinationCommand to reconcile MatchingEventTypes and TopicArn (instead of returning success silently); specifically, update the logic around CreateConfigurationSetEventDestinationCommand to detect the existing-destination condition and invoke UpdateConfigurationSetEventDestinationCommand with the desired EventDestination configuration so the SES settings are updated to match the intended webhook configuration.
♻️ Duplicate comments (1)
apps/web/src/server/service/domain-service.ts (1)
1138-1156:⚠️ Potential issue | 🟠 MajorRun the custom tracking due check before the failed-domain short-circuit.
A never-verified primary domain in
FAILEDstate returnsfalsebefore evaluating pending custom tracking verification, so the tracking hostname can still be skipped by background checks. This is the same ordering issue noted previously.Proposed fix
export async function isDomainVerificationDue(domain: Domain) { const verificationState = await getDomainVerificationState(domain.id); + if (shouldPollCustomTrackingVerification(domain)) { + const now = Date.now(); + const lastCheckedAt = verificationState.lastCheckedAt?.getTime() ?? 0; + if (!verificationState.lastCheckedAt) { + return true; + } + return now - lastCheckedAt >= DOMAIN_UNVERIFIED_RECHECK_MS; + } + if ( !verificationState.hasEverVerified && domain.status === DomainStatus.FAILED && !domain.isVerifying ) { return false; } - if (shouldPollCustomTrackingVerification(domain)) { - const now = Date.now(); - const lastCheckedAt = verificationState.lastCheckedAt?.getTime() ?? 0; - if (!verificationState.lastCheckedAt) { - return true; - } - return now - lastCheckedAt >= DOMAIN_UNVERIFIED_RECHECK_MS; - } -🤖 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 1138 - 1156, In isDomainVerificationDue, the early return that short-circuits when a never-verified primary domain is in DomainStatus.FAILED and !domain.isVerifying runs before checking pending custom tracking; move the shouldPollCustomTrackingVerification block (using shouldPollCustomTrackingVerification(domain), verificationState.lastCheckedAt, DOMAIN_UNVERIFIED_RECHECK_MS) so it executes before the FAILED/never-verified short-circuit, preserving the same logic for lastCheckedAt and timing, and keep the FAILED && !domain.isVerifying check after that so custom tracking rechecks are not skipped.
🤖 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/service/domain-service.ts`:
- Around line 166-179: The current shouldPollCustomTrackingVerification function
stops polling when customTrackingStatus === DomainStatus.SUCCESS even if the
provisioning fields are empty; update shouldPollCustomTrackingVerification so it
only returns false when running in cloud OR when customTrackingStatus ===
DomainStatus.SUCCESS and both domain.customTrackingHostname and
domain.customTrackingPublicKey are present (i.e., provisioning actually
completed). Otherwise continue polling (return true) so
ensureCustomTrackingProvisioned can retry when status is SUCCESS but tracking
config fields are missing (also allow retry on FAILED if desired). Reference:
shouldPollCustomTrackingVerification, ensureCustomTrackingProvisioned,
Domain.customTrackingHostname, Domain.customTrackingPublicKey,
domain.customTrackingStatus, DomainStatus.SUCCESS.
- Around line 484-497: The update currently forces trackingHttpsRequired to
false when the caller omits it; before calling db.domain.update (the block that
updates customTrackingHostname/customTrackingPublicKey/etc. for domainId), first
load the existing domain (e.g., via db.domain.findUnique by domainId) and then
set the update payload's trackingHttpsRequired to trackingHttpsRequired ??
existing.trackingHttpsRequired so the existing HTTPS policy is preserved when
the parameter is not provided; reference db.domain.update, domainId, and
trackingHttpsRequired when making this change.
---
Outside diff comments:
In `@apps/web/src/server/aws/ses.ts`:
- Around line 1-17: Add the missing
UpdateConfigurationSetEventDestinationCommand import from
"@aws-sdk/client-sesv2" and change the
CreateConfigurationSetEventDestinationCommand error handling so that if the
event destination already exists you call
UpdateConfigurationSetEventDestinationCommand to reconcile MatchingEventTypes
and TopicArn (instead of returning success silently); specifically, update the
logic around CreateConfigurationSetEventDestinationCommand to detect the
existing-destination condition and invoke
UpdateConfigurationSetEventDestinationCommand with the desired EventDestination
configuration so the SES settings are updated to match the intended webhook
configuration.
---
Duplicate comments:
In `@apps/web/src/server/service/domain-service.ts`:
- Around line 1138-1156: In isDomainVerificationDue, the early return that
short-circuits when a never-verified primary domain is in DomainStatus.FAILED
and !domain.isVerifying runs before checking pending custom tracking; move the
shouldPollCustomTrackingVerification block (using
shouldPollCustomTrackingVerification(domain), verificationState.lastCheckedAt,
DOMAIN_UNVERIFIED_RECHECK_MS) so it executes before the FAILED/never-verified
short-circuit, preserving the same logic for lastCheckedAt and timing, and keep
the FAILED && !domain.isVerifying check after that so custom tracking rechecks
are not skipped.
🪄 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: d3621dd2-5c1c-4d04-ad12-ab90e95d2665
📒 Files selected for processing (7)
apps/web/prisma/migrations/20260419120000_domain_custom_tracking_hostname_unique/migration.sqlapps/web/prisma/schema.prismaapps/web/src/server/aws/ses.tsapps/web/src/server/service/domain-service.tsapps/web/src/server/service/domain-service.unit.test.tsapps/web/src/server/utils/ses-tracking-html.tsapps/web/src/server/utils/ses-tracking-html.unit.test.ts
✅ Files skipped from review due to trivial changes (3)
- apps/web/prisma/migrations/20260419120000_domain_custom_tracking_hostname_unique/migration.sql
- apps/web/src/server/utils/ses-tracking-html.ts
- apps/web/prisma/schema.prisma
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/server/utils/ses-tracking-html.unit.test.ts
- apps/web/src/server/service/domain-service.unit.test.ts
…mption Keep polling when SES tracking is SUCCESS but config sets are missing; preserve trackingHttpsRequired when rotating hostnames; treat branded unsubscribe URLs as non-engagement without same-origin check. Extract unsubscribe helper for tests. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20672bea-1d09-417d-b7e7-7ed213b425d2
📒 Files selected for processing (5)
apps/web/src/server/service/domain-service.tsapps/web/src/server/service/domain-service.unit.test.tsapps/web/src/server/service/ses-hook-parser.tsapps/web/src/server/service/unsubscribe-engagement-exempt.tsapps/web/src/server/service/unsubscribe-engagement-exempt.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/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/service/ses-hook-parser.ts
Comment on lines
+10
to
+19
| try { | ||
| const u = new URL(link); | ||
| return /\bunsubscribe\b/i.test(`${u.pathname}${u.search}`); | ||
| } catch { | ||
| const prefix = env.NEXTAUTH_URL.replace(/\/$/, ""); | ||
| return ( | ||
| link.startsWith(`${prefix}/unsubscribe`) || | ||
| /\/api\/unsubscribe/i.test(link) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential false positives from broad unsubscribe substring match.
The \bunsubscribe\b word-boundary check on pathname + search will flag unrelated URLs that happen to contain the word (e.g., https://example.com/blog/how-to-unsubscribe-from-anything, https://example.com/?ref=unsubscribe-guide). Since this suppresses click engagement tracking, marketing/content pages that include the word in slugs or UTM values will silently be excluded from analytics.
If the intent is to match only real opt-out endpoints, consider anchoring to path boundaries like /unsubscribe / /api/unsubscribe (or a configured path list) rather than a free-floating word match across pathname+search. If the current permissiveness is intentional (defense in depth for any branded unsub URL), feel free to ignore.
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
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.
Closes #345
Summary
Self-hosted only: custom hostname for SES click/open tracking (DNS DKIM + CNAME), per-domain configuration sets, per-domain HTTPS option (
OPTIONALvsREQUIRE),ses:no-trackon unsubscribe links, and campaign analytics adjustments for unsubscribe URLs.Notes
Summary by cubic
Self‑hosted: add per‑domain SES tracking domains with a per‑domain HTTPS policy, and stop counting unsubscribe clicks/opens as engagement. Includes Prisma migrations and a unique index on
customTrackingHostname.New Features
OPTIONALorREQUIRE), with DKIM TXT + CNAME DNS guidance and auto‑polling of SES verification while pending.ses:no-trackon unsubscribe links at send time to prevent SES click wrapping.Bug Fixes
customTrackingHostname, and preserve the HTTPS preference when rotating hostnames.Written for commit fb600ad. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests