Uh oh!
There was an error while loading. Please reload this page.
feat: add double opt-in - #237
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds double opt-in support across the stack: a Prisma migration and schema changes (Domain.defaultFrom; ContactBook.defaultDomainId, doubleOptInEnabled, doubleOptInTemplateId; indexes and FKs). Introduces a new double-opt-in service (URL/hash generation, template validation, send/confirm flows), domain helpers (resolveFromAddress, getVerifiedDomains), contact-book and contact-service validations/flows, API additions (getContactBookSettings, extended updateContactBook), UI pages (contact book settings and /confirm), mailer changes to use domain-derived from addresses, team initialization hook, and a design doc (double-opt-in.md). Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Deploying usesend with |
| Latest commit: | 9e513ea |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2b943280.usesend.pages.dev |
| Branch Preview URL: | https://km-2025-09-18-double-opt-in.usesend.pages.dev |
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| const nextDoubleOptInEnabled = | ||
| data.doubleOptInEnabled ?? contactBook.doubleOptInEnabled; | ||
| const nextTemplateId = | ||
| data.doubleOptInTemplateId ?? contactBook.doubleOptInTemplateId; | ||
| const nextDomainId = data.defaultDomainId ?? contactBook.defaultDomainId; | ||
| if (nextDoubleOptInEnabled) { | ||
| if (!nextDomainId) { |
There was a problem hiding this comment.
[P1] Clearing double-opt‑in dependencies bypasses server-side validation
The validation in updateContactBook uses nullish coalescing when calculating nextTemplateId and nextDomainId. If a client sends an update with doubleOptInEnabled left true but explicitly sets defaultDomainId or doubleOptInTemplateId to null, the next* variables still carry the previous non-null values and the checks pass, yet Prisma persists the null values from data. This leaves the contact book marked as double opt-in enabled while missing a verified domain and/or template, so downstream flows treat the book as disabled (doubleOptInEnabled true but required IDs null) and no confirmations are ever sent. Consider distinguishing between undefined and null when determining next* values so clearing fields while enabled is rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (18)
apps/web/prisma/schema.prisma (2)
180-202: Nit: Relation field casing inconsistent (prefer lowerCamelCase).In Domain, the relation field is named
ContactBook. Project-wide you use lowerCamelCase (e.g.,domains,templates,contactBooks). Consider renaming for consistency.- ContactBook ContactBook[]+ contactBooks ContactBook[]Note: This is a schema rename and will require a follow-up migration and code updates where accessed.
381-385: Add uniqueness for (teamId, name) on Template to back the seed logic and prevent dupes.You seed/find the “Double Opt In” template per-team by name. Enforce this at the DB level.
model Template { id String @id @default(cuid()) name String teamId Int subject String html String? content String? createdAt DateTime @default(now()) updatedAt DateTime @updatedAt team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) - ContactBook ContactBook[]+ contactBooks ContactBook[] @@index([createdAt(sort: Desc)]) + @@unique([teamId, name]) }apps/web/prisma/migrations/20250930120000_add_double_opt_in/migration.sql (1)
19-45: Enforce one “Double Opt In” template per team at the DB layer.Seed logic assumes per-team uniqueness by name; add a unique constraint to keep it invariant.
-- Seed default double opt-in template per team when missing +-- (consider adding a unique constraint to enforce uniqueness)+-- ALTER TABLE "Template" ADD CONSTRAINT "Template_teamId_name_unique" UNIQUE ("teamId", "name");If existing data may violate this, apply in a follow-up migration after deduplication.
apps/web/src/server/api/routers/domain.ts (1)
100-104: Avoidas anyand pass the typed Domain directly to resolveFromAddress.
db.domain.findFirstreturns a typed model includingdefaultFromafter schema update. Remove the unsafe cast.- const fromAddress = resolveFromAddress({- name: domain.name,- defaultFrom: (domain as any).defaultFrom ?? null,- });+ const fromAddress = resolveFromAddress(domain);Also applies to: 109-109
apps/web/src/server/mailer.ts (1)
5-5: Self-hosted path: use verified domains, fix import alias, and dropas any.
- Import with the ~/ alias per repo convention.
- Prefer verified domains to avoid runtime failures in sendEmail validation.
- Pass the domain directly to
resolveFromAddress.-import { getDomains, resolveFromAddress } from "./service/domain-service";+import { getVerifiedDomains, resolveFromAddress } from "~/server/service/domain-service";- const domains = await getDomains(team.id);+ const domains = await getVerifiedDomains(team.id);- const fromAddress = resolveFromAddress({- name: domain.name,- defaultFrom: (domain as any).defaultFrom ?? null,- });+ const fromAddress = resolveFromAddress(domain);Also applies to: 92-104, 104-108, 112-112
apps/web/src/server/service/domain-service.ts (2)
61-69: Simplify typing:Domainalready includesdefaultFrom.Drop the extra helper type and use
Pick<Domain, "name" | "defaultFrom">(or justDomain) to reduce redundancy.-type DomainWithDefaultFrom = Domain & { defaultFrom: string | null };--export function resolveFromAddress(- domain: Pick<DomainWithDefaultFrom, "name" | "defaultFrom">-) {+export function resolveFromAddress(domain: Pick<Domain, "name" | "defaultFrom">) { if (domain.defaultFrom && domain.defaultFrom.trim().length > 0) { return domain.defaultFrom.trim(); } return `hello@${domain.name}`; }
251-261: Use Prisma enum for status instead of string literal.Improves type-safety and auto-complete.
-export async function getVerifiedDomains(teamId: number) {+export async function getVerifiedDomains(teamId: number) { return db.domain.findMany({ where: { teamId, - status: "SUCCESS",+ status: "SUCCESS", // consider: DomainStatus.SUCCESS }, orderBy: { createdAt: "desc", }, }); }If you adopt this, also import
DomainStatusfrom@prisma/client.apps/web/src/app/confirm/page.tsx (2)
12-14: Handle array-shaped search params safely.Next can supply
string | string[]. Normalize before use to avoid accidental'[object Object]'or undefined.- const id = params.id as string;- const hash = params.hash as string;+ const id = Array.isArray(params.id) ? params.id[0] : (params.id ?? "");+ const hash = Array.isArray(params.hash) ? params.hash[0] : (params.hash ?? "");
48-51: Add rel="noopener noreferrer" to external links opened in a new tab.Security/a11y best practice; also flagged by Biome.
- <a href="https://usesend.com" className="font-bold" target="_blank">+ <a href="https://usesend.com" className="font-bold" target="_blank" rel="noopener noreferrer"> useSend </a>Also applies to: 70-73
apps/web/src/server/service/contact-book-service.ts (2)
76-83: Type breadth for properties may be too narrow.If
propertiescan hold non-string values (current schema isJson), preferRecord<string, unknown>to avoid friction.- properties?: Record<string, string>;+ properties?: Record<string, unknown>;
161-202: Minor: Limit template fields returned (or filter at query) to reduce payload.You immediately filter for DOI-capable templates; consider selecting only fields the settings UI needs.
apps/web/src/server/api/routers/contacts.ts (1)
81-101: Prefer ctx.contactBook.id over trusting client inputWithin whereConditions, use the server-derived id to avoid depending on client-sent contactBookId and to match the pattern used elsewhere in this router.
Apply this diff:
- .query(async ({ ctx: { db }, input }) => {+ .query(async ({ ctx: { db, contactBook }, input }) => { @@ - const whereConditions: Prisma.ContactFindManyArgs["where"] = {- contactBookId: input.contactBookId,+ const whereConditions: Prisma.ContactFindManyArgs["where"] = {+ contactBookId: contactBook.id,apps/web/src/server/service/contact-service.ts (2)
44-48: Coerce doubleOptInActive to a strict booleanUsing && returns the last truthy operand (string/number), not a boolean. Coerce to boolean to avoid subtle type/logic issues.
- const doubleOptInActive =- contactBook.doubleOptInEnabled &&- contactBook.doubleOptInTemplateId &&- contactBook.defaultDomainId;+ const doubleOptInActive = Boolean(+ contactBook.doubleOptInEnabled &&+ contactBook.doubleOptInTemplateId &&+ contactBook.defaultDomainId+ );
137-141: Also coerce boolean in updateContactSame boolean coercion applies here for consistency and clarity.
- const doubleOptInActive =- existing.contactBook.doubleOptInEnabled &&- existing.contactBook.doubleOptInTemplateId &&- existing.contactBook.defaultDomainId;+ const doubleOptInActive = Boolean(+ existing.contactBook.doubleOptInEnabled &&+ existing.contactBook.doubleOptInTemplateId &&+ existing.contactBook.defaultDomainId+ );apps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsx (1)
114-122: Optional: render error stateCurrently shows skeleton on any !data. Consider a small error state when
settingsQuery.isError.double-opt-in.md (1)
50-59: Doc/impl mismatch: confirmation route pathThe doc alternates between
/confirmand/api/confirm-subscription. The implementation exportsDOUBLE_OPT_IN_ROUTE = "/confirm". Please align the document with the actual route.apps/web/src/server/service/double-opt-in-service.ts (2)
124-130: Use domain error type for invalid linkThrowing plain
Errorleaks as 500s in many handlers. UseUnsendApiErrorwith BAD_REQUEST for invalid hashes.- if (hash !== expectedHash) {- throw new Error("Invalid confirmation link");- }+ if (hash !== expectedHash) {+ throw new UnsendApiError({ code: "BAD_REQUEST", message: "Invalid confirmation link" });+ }
124-129: Optional: constant‑time hash compareUse
timingSafeEqualto avoid micro‑timing differences.- if (hash !== expectedHash) {+ if (+ hash.length !== expectedHash.length ||+ !crypto.timingSafeEqual(Buffer.from(hash), Buffer.from(expectedHash))+ ) { throw new UnsendApiError({ code: "BAD_REQUEST", message: "Invalid confirmation link" }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/prisma/migrations/20250930120000_add_double_opt_in/migration.sql(1 hunks)apps/web/prisma/schema.prisma(3 hunks)apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsx(1 hunks)apps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsx(1 hunks)apps/web/src/app/confirm/page.tsx(1 hunks)apps/web/src/server/api/routers/contacts.ts(1 hunks)apps/web/src/server/api/routers/domain.ts(2 hunks)apps/web/src/server/mailer.ts(2 hunks)apps/web/src/server/service/contact-book-service.ts(3 hunks)apps/web/src/server/service/contact-service.ts(3 hunks)apps/web/src/server/service/domain-service.ts(3 hunks)apps/web/src/server/service/double-opt-in-service.ts(1 hunks)double-opt-in.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/server/service/contact-service.tsapps/web/src/server/api/routers/domain.tsapps/web/src/app/confirm/page.tsxapps/web/src/server/mailer.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsxapps/web/src/server/api/routers/contacts.tsapps/web/src/server/service/contact-book-service.tsapps/web/src/server/service/double-opt-in-service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use 2-space indentation in TypeScript code (enforced by Prettier)
Use semicolons in TypeScript code (enforced by Prettier)
Do not use dynamic imports
Files:
apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/server/service/contact-service.tsapps/web/src/server/api/routers/domain.tsapps/web/src/app/confirm/page.tsxapps/web/src/server/mailer.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsxapps/web/src/server/api/routers/contacts.tsapps/web/src/server/service/contact-book-service.tsapps/web/src/server/service/double-opt-in-service.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier 3 via pnpm format for TypeScript and Markdown files
Files:
apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/server/service/contact-service.tsdouble-opt-in.mdapps/web/src/server/api/routers/domain.tsapps/web/src/app/confirm/page.tsxapps/web/src/server/mailer.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsxapps/web/src/server/api/routers/contacts.tsapps/web/src/server/service/contact-book-service.tsapps/web/src/server/service/double-opt-in-service.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase (e.g., AppSideBar.tsx)
Files:
apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/app/confirm/page.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/web, use the
/ alias for src imports (e.g., import { x } from "/utils/x")
Files:
apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/server/service/contact-service.tsapps/web/src/server/api/routers/domain.tsapps/web/src/app/confirm/page.tsxapps/web/src/server/mailer.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsxapps/web/src/server/api/routers/contacts.tsapps/web/src/server/service/contact-book-service.tsapps/web/src/server/service/double-opt-in-service.ts
🧠 Learnings (1)
📚 Learning: 2025-09-10T12:33:42.667Z
Learnt from: KMKoushik
PR: usesend/useSend#224
File: apps/web/src/server/public-api/api/emails/get-email.ts:63-74
Timestamp: 2025-09-10T12:33:42.667Z
Learning: In the useSend project, Prisma's findUnique method works with composite where clauses including id, teamId, and optional domainId fields in apps/web/src/server/public-api/api/emails/get-email.ts
Applied to files:
apps/web/src/server/mailer.tsapps/web/src/server/service/contact-book-service.ts
🧬 Code graph analysis (8)
apps/web/src/server/service/contact-service.ts (3)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)apps/web/src/server/service/double-opt-in-service.ts (2)
templateSupportsDoubleOptIn(47-73)sendDoubleOptInEmail(85-122)
apps/web/src/server/api/routers/domain.ts (2)
apps/web/src/server/service/domain-service.ts (1)
resolveFromAddress(61-69)apps/web/src/server/service/email-service.ts (1)
sendEmail(50-295)
apps/web/src/app/confirm/page.tsx (1)
apps/web/src/server/service/double-opt-in-service.ts (1)
confirmContactFromLink(124-165)
apps/web/src/server/mailer.ts (2)
apps/web/src/server/service/domain-service.ts (1)
resolveFromAddress(61-69)apps/web/src/server/service/email-service.ts (1)
sendEmail(50-295)
apps/web/src/server/service/domain-service.ts (1)
packages/python-sdk/usesend/types.py (1)
Domain(26-42)
apps/web/src/server/api/routers/contacts.ts (1)
apps/web/src/server/api/trpc.ts (1)
contactBookProcedure(191-209)
apps/web/src/server/service/contact-book-service.ts (3)
apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)apps/web/src/server/service/double-opt-in-service.ts (2)
assertTemplateSupportsDoubleOptIn(75-83)templateSupportsDoubleOptIn(47-73)apps/web/src/server/service/domain-service.ts (1)
getVerifiedDomains(251-261)
apps/web/src/server/service/double-opt-in-service.ts (5)
apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)apps/web/src/server/service/domain-service.ts (1)
resolveFromAddress(61-69)apps/web/src/server/service/email-service.ts (1)
sendEmail(50-295)apps/web/src/server/db.ts (1)
db(20-20)
🪛 Biome (2.1.2)
apps/web/src/app/confirm/page.tsx
[error] 48-48: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 70-70: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🔇 Additional comments (9)
apps/web/prisma/schema.prisma (1)
284-302: LGTM: New DOI fields and indexes on ContactBook.Fields, relations, and indexes look correct and match the intended FK semantics (ON DELETE SET NULL).
apps/web/prisma/migrations/20250930120000_add_double_opt_in/migration.sql (1)
1-18: LGTM: Columns, indexes, and FKs align with the Prisma schema.No issues with ON DELETE/UPDATE behavior or index coverage.
apps/web/src/server/service/contact-book-service.ts (1)
102-147: LGTM: Enabling DOI validates verified domain + eligible template.Solid checks with clear API errors; assertion for placeholder is good.
Please confirm UI disables the toggle until a domain and eligible template are selected to prevent avoidable 400s.
apps/web/src/server/api/routers/contacts.ts (3)
44-48: LGTM: scoped settings fetch is correctThe settings query is correctly scoped via contactBookProcedure and delegates to the service layer.
57-60: Input schema extension looks rightThe new fields and nullability line up with the UI payload and Prisma types.
63-65: Good: sanitize away id before forwarding to serviceDropping contactBookId from the payload prevents accidental mass updates and centralizes the target id in ctx.
apps/web/src/server/service/contact-service.ts (2)
65-71: Confirm intended behavior: cannot unsubscribe via addOrUpdate when DOI is activeUnder DOI,
subscribedValuepreserves the existing state for updates, which also blocks settingsubscribed: false. If unsubscribe should be allowed here, tweak logic; otherwise document that onlyupdateContact/unsubscribe API should be used.
195-204: Bypasses DOI guardrails
subscribeContactunconditionally flips subscribed to true. Ensure this isn’t exposed to public callers or gate it behind confirmation logic/role checks.apps/web/src/server/service/double-opt-in-service.ts (1)
108-122: LGTM: email assembly respects config and idempotenceValidations, from‑address resolution, and variable injection look correct.
| <Link href={`/contacts/${contactBookId}/settings`}> | ||
| <Button variant="outline" type="button"> | ||
| Settings | ||
| </Button> | ||
| </Link> |
There was a problem hiding this comment.
Fix invalid interactive nesting: inside .
A Button inside a Link renders a button inside an anchor, which is invalid and harms a11y. Use the Button’s asChild to render the anchor as the button.
- <Link href={`/contacts/${contactBookId}/settings`}>- <Button variant="outline" type="button">- Settings- </Button>- </Link>+ <Button asChild variant="outline">+ <Link href={`/contacts/${contactBookId}/settings`}>Settings</Link>+ </Button>📝 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.
| <Linkhref={`/contacts/${contactBookId}/settings`}> | |
| <Buttonvariant="outline"type="button"> | |
| Settings | |
| </Button> | |
| </Link> | |
| <ButtonasChildvariant="outline"> | |
| <Linkhref={`/contacts/${contactBookId}/settings`}>Settings</Link> | |
| </Button> |
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsx around lines
123 to 127, the current code nests a <Button> inside a <Link>, creating an
invalid <button> inside <a> structure; change to use the Button's asChild prop
so the anchor is rendered as the button: replace the Link-wrapping-Button
pattern with a Button that has asChild and contains the Link (keep the href),
remove the type="button" since the rendered element will be an anchor, and
ensure styling/variant props remain on Button while accessibility and focus
behavior are preserved.
| @@ -0,0 +1,243 @@ | |||
| "use client"; | |||
| import { use, useEffect } from "react"; | |||
There was a problem hiding this comment.
Do not use React’s use() in a client component
use() is not supported in client components. Also, params shouldn’t be a Promise here. This will break at runtime/build.
-import { use, useEffect } from "react";+import { useEffect } from "react";📝 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.
| import{use,useEffect}from"react"; | |
| import{useEffect}from"react"; |
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsx
around line 3, remove the import and usage of React's experimental use() because
it is not supported in client components and params must not be treated as a
Promise; either convert this page to a server component (remove any "use client"
directive, make the page component async, await any data/fetches directly and
use params synchronously) or keep it a client component and instead accept
resolved data/params from a parent server component (resolve fetches server-side
and pass results as props), and delete the use import and any use() calls so
nothing treats params as a Promise at runtime.
| export default function ContactBookSettingsPage({ | ||
| params, | ||
| }: { | ||
| params: Promise<{ contactBookId: string }>; | ||
| }) { | ||
| const { contactBookId } = use(params); | ||
| const utils = api.useUtils(); |
There was a problem hiding this comment.
Fix component props: accept params directly and drop use(params)
Align with Next.js App Router conventions for client components.
-export default function ContactBookSettingsPage({- params,-}: {- params: Promise<{ contactBookId: string }>;-}) {- const { contactBookId } = use(params);+export default function ContactBookSettingsPage({+ params,+}: {+ params: { contactBookId: string };+}) {+ const { contactBookId } = params;📝 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.
| exportdefaultfunctionContactBookSettingsPage({ | |
| params, | |
| }: { | |
| params: Promise<{contactBookId: string}>; | |
| }){ | |
| const{ contactBookId }=use(params); | |
| constutils=api.useUtils(); | |
| exportdefaultfunctionContactBookSettingsPage({ | |
| params, | |
| }: { | |
| params: {contactBookId: string}; | |
| }){ | |
| const{ contactBookId }=params; | |
| constutils=api.useUtils(); |
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/contacts/[contactBookId]/settings/page.tsx
around lines 56-63, the component currently types params as a Promise and calls
use(params); change the component to accept params directly as { params: {
contactBookId: string } } (not a Promise), remove the use(params) call, and read
contactBookId from params.contactBookId; update the function signature/type
accordingly so the component follows Next.js App Router conventions for client
components.
| subscribed: subscribedValue, | ||
| ...(doubleOptInActive ? { unsubscribeReason: null } : {}), | ||
| }, | ||
| update: { | ||
| firstName: contact.firstName, | ||
| lastName: contact.lastName, | ||
| properties: contact.properties ?? {}, | ||
| subscribed: contact.subscribed, | ||
| subscribed: subscribedValue, | ||
| ...(doubleOptInActive && requestedSubscribed | ||
| ? { unsubscribeReason: null } | ||
| : {}), | ||
| }, |
There was a problem hiding this comment.
Bug: update path overwrites properties to {} when not provided
properties: contact.properties ?? {} will erase existing properties on update when the caller omits properties. Only include the field when provided.
update: {
firstName: contact.firstName,
lastName: contact.lastName,
- properties: contact.properties ?? {},+ ...(contact.properties !== undefined+ ? { properties: contact.properties }+ : {}),
subscribed: subscribedValue,
...(doubleOptInActive && requestedSubscribed
? { unsubscribeReason: null }
: {}),
},📝 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.
| subscribed: subscribedValue, | |
| ...(doubleOptInActive ? {unsubscribeReason: null} : {}), | |
| }, | |
| update: { | |
| firstName: contact.firstName, | |
| lastName: contact.lastName, | |
| properties: contact.properties??{}, | |
| subscribed: contact.subscribed, | |
| subscribed: subscribedValue, | |
| ...(doubleOptInActive&&requestedSubscribed | |
| ? {unsubscribeReason: null} | |
| : {}), | |
| }, | |
| subscribed: subscribedValue, | |
| ...(doubleOptInActive ? {unsubscribeReason: null} : {}), | |
| }, | |
| update: { | |
| firstName: contact.firstName, | |
| lastName: contact.lastName, | |
| ...(contact.properties!==undefined ? {properties: contact.properties} : {}), | |
| subscribed: subscribedValue, | |
| ...(doubleOptInActive&&requestedSubscribed | |
| ? {unsubscribeReason: null} | |
| : {}), | |
| }, |
🤖 Prompt for AI Agents
In apps/web/src/server/service/contact-service.ts around lines 85 to 96, the
update block unconditionally sets properties: contact.properties ?? {} which
wipes existing properties when the caller omits properties; change the update
payload to only include the properties field when contact.properties is
explicitly provided (e.g., conditionally spread the properties key only if
contact.properties !== undefined) so that missing properties do not overwrite
stored values, and keep the existing subscribed/unsubscribeReason conditional
logic intact.
| export function createDoubleOptInIdentifier( | ||
| contactId: string, | ||
| contactBookId: string | ||
| ) { | ||
| return `${contactId}-${contactBookId}`; | ||
| } |
There was a problem hiding this comment.
Delimiter collision: id parsing breaks for hyphenated ids
id = "${contactId}-${contactBookId}" and id.split("-") will fail if either id contains - (e.g., UUIDs). Use a robust encoding (e.g., base64url of a JSON payload).
-export function createDoubleOptInIdentifier(- contactId: string,- contactBookId: string-) {- return `${contactId}-${contactBookId}`;-}+export function createDoubleOptInIdentifier(+ contactId: string,+ contactBookId: string+) {+ // base64url(JSON.stringify({ c: contactId, b: contactBookId }))+ const payload = JSON.stringify({ c: contactId, b: contactBookId });+ return Buffer.from(payload, "utf8").toString("base64url");+}
@@
- const [contactId, contactBookId] = id.split("-");-- if (!contactId || !contactBookId) {- throw new Error("Invalid confirmation link");- }+ let contactId: string | undefined;+ let contactBookId: string | undefined;+ try {+ const decoded = JSON.parse(+ Buffer.from(id, "base64url").toString("utf8")+ ) as { c?: string; b?: string };+ contactId = decoded.c;+ contactBookId = decoded.b;+ } catch {+ // fall through to uniform error below+ }+ if (!contactId || !contactBookId) {+ throw new UnsendApiError({ code: "BAD_REQUEST", message: "Invalid confirmation link" });+ }Also applies to: 131-135
| function createDoubleOptInHash(identifier: string) { | ||
| return createHash("sha256") | ||
| .update(`${identifier}-${env.NEXTAUTH_SECRET}`) | ||
| .digest("hex"); | ||
| } |
There was a problem hiding this comment.
Guard missing env secrets
If NEXTAUTH_SECRET is unset, the hash becomes predictable. Fail fast with a clear error.
function createDoubleOptInHash(identifier: string) {
- return createHash("sha256")- .update(`${identifier}-${env.NEXTAUTH_SECRET}`)+ const secret = env.NEXTAUTH_SECRET;+ if (!secret) {+ throw new UnsendApiError({+ code: "INTERNAL_SERVER_ERROR",+ message: "Server misconfiguration: NEXTAUTH_SECRET is not set",+ });+ }+ return createHash("sha256")+ .update(`${identifier}-${secret}`)
.digest("hex");
}📝 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.
| functioncreateDoubleOptInHash(identifier: string){ | |
| returncreateHash("sha256") | |
| .update(`${identifier}-${env.NEXTAUTH_SECRET}`) | |
| .digest("hex"); | |
| } | |
| functioncreateDoubleOptInHash(identifier: string){ | |
| constsecret=env.NEXTAUTH_SECRET; | |
| if(!secret){ | |
| thrownewUnsendApiError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Server misconfiguration: NEXTAUTH_SECRET is not set", | |
| }); | |
| } | |
| returncreateHash("sha256") | |
| .update(`${identifier}-${secret}`) | |
| .digest("hex"); | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/server/service/double-opt-in-service.ts around lines 31 to 35,
the function createDoubleOptInHash uses env.NEXTAUTH_SECRET directly which makes
the hash predictable if the secret is unset; modify the code to validate that
env.NEXTAUTH_SECRET is present and non-empty and throw a clear, fast-failing
Error (or assert) if it is missing before computing the hash so the service
fails early with an explanatory message.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
apps/web/src/server/service/double-opt-in-service.ts (4)
30-35: Identifier delimiter collision; use base64url JSONHyphen-splitting breaks for hyphenated IDs (e.g., UUIDs). Encode a JSON payload.
export function createDoubleOptInIdentifier( contactId: string, contactBookId: string ) { - return `${contactId}-${contactBookId}`;+ const payload = JSON.stringify({ c: contactId, b: contactBookId });+ return Buffer.from(payload, "utf8").toString("base64url"); }
37-41: Use HMAC-SHA256 and guard missing NEXTAUTH_SECRETPrefer createHmac with a required secret; fail fast if unset.
-import { createHash } from "crypto";+import { createHmac, timingSafeEqual } from "crypto"; @@ -function createDoubleOptInHash(identifier: string) {- return createHash("sha256")- .update(`${identifier}-${env.NEXTAUTH_SECRET}`)- .digest("hex");-}+function createDoubleOptInHash(identifier: string) {+ const secret = env.NEXTAUTH_SECRET;+ if (!secret) {+ throw new UnsendApiError({+ code: "INTERNAL_SERVER_ERROR",+ message: "Server misconfiguration: NEXTAUTH_SECRET is not set",+ });+ }+ return createHmac("sha256", secret).update(identifier).digest("hex");+}
160-165: Robust id parsing (matches identifier change)Splitting on "-" will break once identifiers contain hyphens. Decode the base64url JSON as shown above.
153-164: Timing-safe hash check and consistent API errorsAvoid simple string equality for hashes and use typed API errors.
export async function confirmContactFromLink(id: string, hash: string) { - const expectedHash = createDoubleOptInHash(id);-- if (hash !== expectedHash) {- throw new Error("Invalid confirmation link");- }+ let expected: Buffer;+ let actual: Buffer;+ try {+ expected = Buffer.from(createDoubleOptInHash(id), "hex");+ actual = Buffer.from(hash, "hex");+ } catch {+ throw new UnsendApiError({ code: "BAD_REQUEST", message: "Invalid confirmation link" });+ }+ if (actual.length !== expected.length || !timingSafeEqual(actual, expected)) {+ throw new UnsendApiError({ code: "BAD_REQUEST", message: "Invalid confirmation link" });+ }-- const [contactId, contactBookId] = id.split("-");+ // Decode base64url identifier { c: contactId, b: contactBookId }+ let contactId: string | undefined;+ let contactBookId: string | undefined;+ try {+ const decoded = JSON.parse(Buffer.from(id, "base64url").toString("utf8")) as {+ c?: string;+ b?: string;+ };+ contactId = decoded.c;+ contactBookId = decoded.b;+ } catch {+ // fall through+ }- if (!contactId || !contactBookId) {- throw new Error("Invalid confirmation link");- }+ if (!contactId || !contactBookId) {+ throw new UnsendApiError({ code: "BAD_REQUEST", message: "Invalid confirmation link" });+ }
🧹 Nitpick comments (2)
apps/web/src/server/service/team-service.ts (1)
13-13: Use ~/ alias for imports in apps/webFollow repo convention for src imports.
-import { ensureDefaultDoubleOptInTemplate } from "./double-opt-in-service";+import { ensureDefaultDoubleOptInTemplate } from "~/server/service/double-opt-in-service";apps/web/src/server/service/double-opt-in-service.ts (1)
43-51: Build URLs robustly and validate NEXTAUTH_URLUse URL/URLSearchParams; ensures proper encoding and base handling.
export function createDoubleOptInUrl( contactId: string, contactBookId: string ) { const identifier = createDoubleOptInIdentifier(contactId, contactBookId); const hash = createDoubleOptInHash(identifier); -- return `${env.NEXTAUTH_URL}${DOUBLE_OPT_IN_ROUTE}?id=${identifier}&hash=${hash}`;+ const base = env.NEXTAUTH_URL;+ if (!base) {+ throw new UnsendApiError({+ code: "INTERNAL_SERVER_ERROR",+ message: "Server misconfiguration: NEXTAUTH_URL is not set",+ });+ }+ const url = new URL(DOUBLE_OPT_IN_ROUTE, base);+ url.searchParams.set("id", identifier);+ url.searchParams.set("hash", hash);+ return url.toString(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/prisma/migrations/20250930120000_add_double_opt_in/migration.sql(1 hunks)apps/web/src/server/service/contact-book-service.ts(3 hunks)apps/web/src/server/service/double-opt-in-service.ts(1 hunks)apps/web/src/server/service/team-service.ts(2 hunks)double-opt-in.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- double-opt-in.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/server/service/contact-book-service.ts
- apps/web/prisma/migrations/20250930120000_add_double_opt_in/migration.sql
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/service/team-service.tsapps/web/src/server/service/double-opt-in-service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use 2-space indentation in TypeScript code (enforced by Prettier)
Use semicolons in TypeScript code (enforced by Prettier)
Do not use dynamic imports
Files:
apps/web/src/server/service/team-service.tsapps/web/src/server/service/double-opt-in-service.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier 3 via pnpm format for TypeScript and Markdown files
Files:
apps/web/src/server/service/team-service.tsapps/web/src/server/service/double-opt-in-service.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/web, use the
/ alias for src imports (e.g., import { x } from "/utils/x")
Files:
apps/web/src/server/service/team-service.tsapps/web/src/server/service/double-opt-in-service.ts
🧬 Code graph analysis (2)
apps/web/src/server/service/team-service.ts (1)
apps/web/src/server/service/double-opt-in-service.ts (1)
ensureDefaultDoubleOptInTemplate(91-112)
apps/web/src/server/service/double-opt-in-service.ts (6)
packages/python-sdk/usesend/types.py (2)
Domain(26-42)Contact(282-291)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/service/domain-service.ts (1)
resolveFromAddress(61-69)apps/web/src/server/service/email-service.ts (1)
sendEmail(50-295)
| export async function ensureDefaultDoubleOptInTemplate(teamId: number) { | ||
| const existing = await db.template.findFirst({ | ||
| where: { | ||
| teamId, | ||
| name: DOUBLE_OPT_IN_TEMPLATE_NAME, | ||
| }, | ||
| }); | ||
| if (existing) { | ||
| return existing; | ||
| } | ||
| return db.template.create({ | ||
| data: { | ||
| teamId, | ||
| name: DOUBLE_OPT_IN_TEMPLATE_NAME, | ||
| subject: DOUBLE_OPT_IN_TEMPLATE_SUBJECT, | ||
| html: DOUBLE_OPT_IN_TEMPLATE_HTML, | ||
| content: DOUBLE_OPT_IN_TEMPLATE_CONTENT, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Race-safe template provisioning (duplicate creation possible)
Two concurrent calls can both create the default template. Use upsert (with a unique on [teamId,name]) or handle P2002 and re-read.
+import { Prisma } from "@prisma/client";
@@
- return db.template.create({- data: {- teamId,- name: DOUBLE_OPT_IN_TEMPLATE_NAME,- subject: DOUBLE_OPT_IN_TEMPLATE_SUBJECT,- html: DOUBLE_OPT_IN_TEMPLATE_HTML,- content: DOUBLE_OPT_IN_TEMPLATE_CONTENT,- },- });+ try {+ return await db.template.create({+ data: {+ teamId,+ name: DOUBLE_OPT_IN_TEMPLATE_NAME,+ subject: DOUBLE_OPT_IN_TEMPLATE_SUBJECT,+ html: DOUBLE_OPT_IN_TEMPLATE_HTML,+ content: DOUBLE_OPT_IN_TEMPLATE_CONTENT,+ },+ });+ } catch (err) {+ if (+ err instanceof Prisma.PrismaClientKnownRequestError &&+ err.code === "P2002"+ ) {+ // Created by a concurrent request; return the existing one.+ return (await db.template.findFirst({+ where: { teamId, name: DOUBLE_OPT_IN_TEMPLATE_NAME },+ }))!;+ }+ throw err;+ }If [teamId, name] isn’t unique yet, add a unique index in the migration.
📝 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.
| exportasyncfunctionensureDefaultDoubleOptInTemplate(teamId: number){ | |
| constexisting=awaitdb.template.findFirst({ | |
| where: { | |
| teamId, | |
| name: DOUBLE_OPT_IN_TEMPLATE_NAME, | |
| }, | |
| }); | |
| if(existing){ | |
| returnexisting; | |
| } | |
| returndb.template.create({ | |
| data: { | |
| teamId, | |
| name: DOUBLE_OPT_IN_TEMPLATE_NAME, | |
| subject: DOUBLE_OPT_IN_TEMPLATE_SUBJECT, | |
| html: DOUBLE_OPT_IN_TEMPLATE_HTML, | |
| content: DOUBLE_OPT_IN_TEMPLATE_CONTENT, | |
| }, | |
| }); | |
| } | |
| import{Prisma}from"@prisma/client"; | |
| exportasyncfunctionensureDefaultDoubleOptInTemplate(teamId: number){ | |
| constexisting=awaitdb.template.findFirst({ | |
| where: { | |
| teamId, | |
| name: DOUBLE_OPT_IN_TEMPLATE_NAME, | |
| }, | |
| }); | |
| if(existing){ | |
| returnexisting; | |
| } | |
| try{ | |
| returnawaitdb.template.create({ | |
| data: { | |
| teamId, | |
| name: DOUBLE_OPT_IN_TEMPLATE_NAME, | |
| subject: DOUBLE_OPT_IN_TEMPLATE_SUBJECT, | |
| html: DOUBLE_OPT_IN_TEMPLATE_HTML, | |
| content: DOUBLE_OPT_IN_TEMPLATE_CONTENT, | |
| }, | |
| }); | |
| }catch(err){ | |
| if( | |
| errinstanceofPrisma.PrismaClientKnownRequestError&& | |
| err.code==="P2002" | |
| ){ | |
| // Created by a concurrent request; return the existing one. | |
| return(awaitdb.template.findFirst({ | |
| where: {teamId,name: DOUBLE_OPT_IN_TEMPLATE_NAME}, | |
| }))!; | |
| } | |
| throwerr; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/server/service/double-opt-in-service.ts around lines 91 to 112,
the current logic can create duplicate templates when two requests run
concurrently; change to a race-safe approach by using db.template.upsert keyed
on a unique constraint covering [teamId, name] (and add that unique index in a
migration if it doesn't exist), or wrap the create in a try/catch that detects
Prisma P2002 (unique constraint) errors and, on P2002, re-query
db.template.findFirst and return the existing record; ensure the final code
always returns the single canonical template record.
| if (!contact.contactBook.doubleOptInEnabled) { | ||
| return { contact, confirmed: contact.subscribed }; | ||
| } | ||
| if (contact.subscribed) { | ||
| return { contact, confirmed: true }; | ||
| } | ||
| const updated = await db.contact.update({ | ||
| where: { id: contact.id }, | ||
| data: { | ||
| subscribed: true, | ||
| unsubscribeReason: null, | ||
| }, | ||
| }); | ||
| return { contact: updated, confirmed: true }; | ||
| } |
There was a problem hiding this comment.
Don’t override explicit unsubscribes/bounces/complaints
Current logic will resubscribe any contact with a valid link. That’s a compliance risk. Only confirm contacts pending double opt-in; never override explicit unsubscribes.
if (!contact.contactBook.doubleOptInEnabled) {
return { contact, confirmed: contact.subscribed };
}
if (contact.subscribed) {
return { contact, confirmed: true };
}
+ // Do not override explicit unsubscribes/suppressions; only allow pending DOI.+ if (contact.unsubscribeReason && contact.unsubscribeReason !== "DOUBLE_OPT_IN_PENDING") {+ logger.info(+ { contactId: contact.id, contactBookId },+ "Skipping resubscribe via double opt-in: contact previously unsubscribed/suppressed"+ );+ return { contact, confirmed: false };+ }+
const updated = await db.contact.update({
where: { id: contact.id },
data: {
subscribed: true,
unsubscribeReason: null,
},
});If there’s no “DOUBLE_OPT_IN_PENDING” status today, add one (or an equivalent flag) when initially queuing DOI to make this decision unambiguous.
📝 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.
| if(!contact.contactBook.doubleOptInEnabled){ | |
| return{ contact,confirmed: contact.subscribed}; | |
| } | |
| if(contact.subscribed){ | |
| return{ contact,confirmed: true}; | |
| } | |
| constupdated=awaitdb.contact.update({ | |
| where: {id: contact.id}, | |
| data: { | |
| subscribed: true, | |
| unsubscribeReason: null, | |
| }, | |
| }); | |
| return{contact: updated,confirmed: true}; | |
| } | |
| if(!contact.contactBook.doubleOptInEnabled){ | |
| return{ contact,confirmed: contact.subscribed}; | |
| } | |
| if(contact.subscribed){ | |
| return{ contact,confirmed: true}; | |
| } | |
| // Do not override explicit unsubscribes/suppressions; only allow pending DOI. | |
| if(contact.unsubscribeReason&&contact.unsubscribeReason!=="DOUBLE_OPT_IN_PENDING"){ | |
| logger.info( | |
| {contactId: contact.id, contactBookId }, | |
| "Skipping resubscribe via double opt-in: contact previously unsubscribed/suppressed" | |
| ); | |
| return{ contact,confirmed: false}; | |
| } | |
| constupdated=awaitdb.contact.update({ | |
| where: {id: contact.id}, | |
| data: { | |
| subscribed: true, | |
| unsubscribeReason: null, | |
| }, | |
| }); | |
| return{contact: updated,confirmed: true}; | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/server/service/double-opt-in-service.ts around lines 177-194,
the current logic unconditionally resubscribes any contact after a valid link
which can override explicit unsubscribes/bounces/complaints; change it so the
service only confirms contacts that are in a pending double-opt-in state (e.g.,
status === "DOUBLE_OPT_IN_PENDING" or an equivalent flag) and otherwise returns
without modifying subscription state. Modify the DB update to run only when
contact is pending DOI and ensure it does not clear or overwrite
unsubscribe/bounce/complaint fields; if there is no pending status, add a NEW
"DOUBLE_OPT_IN_PENDING" status (or equivalent flag) at the point where DOI is
initially queued so the check is unambiguous.
| await ensureDefaultDoubleOptInTemplate(created.id); | ||
| // Warm cache for the new team | ||
| await TeamService.refreshTeamCache(created.id); |
There was a problem hiding this comment.
Don’t block team creation on template seeding failures
A transient DB error here will fail createTeam. Log and continue; cache warm still runs.
- await ensureDefaultDoubleOptInTemplate(created.id);+ try {+ await ensureDefaultDoubleOptInTemplate(created.id);+ } catch (err) {+ logger.warn({ err, teamId: created.id }, "Failed to ensure default double opt-in template; proceeding");+ }📝 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.
| awaitensureDefaultDoubleOptInTemplate(created.id); | |
| // Warm cache for the new team | |
| awaitTeamService.refreshTeamCache(created.id); | |
| try{ | |
| awaitensureDefaultDoubleOptInTemplate(created.id); | |
| }catch(err){ | |
| logger.warn({ err,teamId: created.id},"Failed to ensure default double opt-in template; proceeding"); | |
| } | |
| // Warm cache for the new team | |
| awaitTeamService.refreshTeamCache(created.id); |
🤖 Prompt for AI Agents
In apps/web/src/server/service/team-service.ts around lines 97 to 99, the call
to ensureDefaultDoubleOptInTemplate can throw and currently blocks team
creation; wrap that call in a try/catch so any errors are caught, log the error
(including context like team id and error message) and continue execution
without rethrowing, ensuring TeamService.refreshTeamCache(created.id) still runs
regardless of template seeding failure.
KMKoushik
commented
Nov 28, 2025
this is not the right approach, need to redo it with better idea |
Summary by CodeRabbit
New Features
Documentation