feat: batch campaigns #227
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 campaign scheduling and batch processing across schema, backend, API, and UI. Prisma schema adds CampaignEmail, new Campaign fields (scheduledAt, batchSize, batchWindowMinutes, lastCursor, lastSentAt, timestamps), indexes, and CampaignStatus values RUNNING and PAUSED. Server adds CAMPAIGN_BATCH and CAMPAIGN_SCHEDULER queues, CampaignBatchService, CampaignSchedulerService, instrumentation to start the scheduler, and campaign batch/scheduling APIs (scheduleCampaign, pauseCampaign, resumeCampaign, latestEmails). Frontend adds ScheduleCampaign modal, TogglePauseCampaign, campaign card/list UI, campaign status badge, live stats, and related routing. UI library adds Calendar/Popover updates and new dependencies; migration SQL and npm scripts updated. Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Deploying usesend with
|
| Latest commit: |
5cec4c7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1f1f0a7e.usesend.pages.dev |
| Branch Preview URL: | https://km-2025-09-12-campaign-fixes.usesend.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
apps/web/prisma/migrations/20250912120627_campaign_status/migration.sql (3)
9-11: Enum extension may fail on PostgreSQL ≤ 11; verify DB version or split migration.Adding multiple enum values in one migration is unsupported on PG 11 and earlier. Confirm production is PG 12+ or split into two migrations (one ADD VALUE per migration).
13-16: GuardbatchSizewith a CHECK constraint.Prevent invalid configs by enforcing positive batch sizes.
ALTER TABLE "Campaign" ADD COLUMN "batchSize" INTEGER NOT NULL DEFAULT 500, ADD COLUMN "lastCursor" TEXT, ADD COLUMN "lastSentAt" TIMESTAMP(3), -ADD COLUMN "scheduledAt" TIMESTAMP(3); +ADD COLUMN "scheduledAt" TIMESTAMP(3); + +ALTER TABLE "Campaign" + ADD CONSTRAINT "campaign_batchSize_positive" CHECK ("batchSize" > 0);
19-25: Index creation may lock hot tables; consider CONCURRENTLY or low-traffic window.If Email/Contact are large and write-heavy,
CREATE INDEXwill block writes. If feasible, build these withCREATE INDEX CONCURRENTLY(outside a transaction) or schedule during maintenance.apps/web/prisma/schema.prisma (1)
264-266: Consider upgrading the(campaignId, contactId)index to UNIQUE for hard dedup.Your code does existence checks, but a UNIQUE constraint eliminates edge-case duplicates from unexpected producers. If historical data might violate uniqueness, plan a backfill/cleanup before adding it.
apps/web/src/server/service/campaign-service.ts (6)
61-76: Preserve RUNNING status when re-scheduling.Comment says “keep running if already running,” but code always sets SCHEDULED. Keep RUNNING to avoid needless state flips.
- await db.campaign.update({ + await db.campaign.update({ where: { id }, - data: { - status: "SCHEDULED", + data: { + status: campaign.status === "RUNNING" ? "RUNNING" : "SCHEDULED", total, scheduledAt: campaign.scheduledAt ?? new Date(), lastCursor: campaign.lastCursor ?? null, }, });
555-566: Add context logs for early returns.Silent returns make incident triage harder. Log reasons for skip.
- const campaign = await db.campaign.findUnique({ + const campaign = await db.campaign.findUnique({ where: { id: campaignId }, }); - if (!campaign) return; - if (!campaign.contactBookId) return; + if (!campaign) { + logger.warn({ campaignId }, "Batch skip: campaign not found"); + return; + } + if (!campaign.contactBookId) { + logger.warn({ campaignId }, "Batch skip: no contactBookId"); + return; + }
569-582: ClampbatchSizeto a sane, positive range.Avoid accidental 0/negative/huge values.
- const batchSize = campaign.batchSize ?? 500; + const batchSize = Math.max(1, Math.min(5000, campaign.batchSize ?? 500));
607-613: Log missing domain to avoid silent stalls.A missing domain currently halts progress without visibility.
const domain = await db.domain.findUnique({ where: { id: campaign.domainId }, }); - if (!domain) return; + if (!domain) { + logger.error({ campaignId, domainId: campaign.domainId }, "Batch skip: domain not found"); + return; + }
699-704: Don’t abort the whole tick if one enqueue fails.Catch per-campaign to keep scheduling others in the same tick.
- for (const c of campaigns) { - await CampaignBatchService.queueBatch({ - campaignId: c.id, - teamId: c.teamId, - }); - } + for (const c of campaigns) { + try { + await CampaignBatchService.queueBatch({ + campaignId: c.id, + teamId: c.teamId, + }); + } catch (e) { + logger.debug({ err: e, campaignId: c.id }, "Batch enqueue skipped/failed"); + } + }
718-721: Stabilize repeatable job identity.Set
repeat.jobIdto ensure one scheduler per environment even across redeploys (verify your BullMQ version supports this).- repeat: { every: SCHEDULER_TICK_MS }, + repeat: { every: SCHEDULER_TICK_MS, jobId: "campaign-scheduler" },campaign-fix.md (2)
48-52: Re-evaluate non-unique(campaignId, contactId)stance.If you can clean up legacy dupes, a UNIQUE constraint provides stronger guarantees than code-level checks.
64-64: Fix list indentation to satisfy markdownlint (MD005/MD007).- - In `queueCampaignBatch`, call `campaignQueue.add(name, { campaignId }, { jobId: campaign-batch:${campaignId}, ...DEFAULT_QUEUE_OPTIONS })` and catch “job exists” to avoid duplicates. + - In `queueCampaignBatch`, call `campaignQueue.add(name, { campaignId }, { jobId: campaign-batch:${campaignId}, ...DEFAULT_QUEUE_OPTIONS })` and catch “job exists” to avoid duplicates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/prisma/migrations/20250912120627_campaign_status/migration.sql(1 hunks)apps/web/prisma/schema.prisma(4 hunks)apps/web/src/server/queue/queue-constants.ts(1 hunks)apps/web/src/server/service/campaign-service.ts(3 hunks)campaign-fix.md(1 hunks)
🧰 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/queue/queue-constants.tsapps/web/src/server/service/campaign-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/queue/queue-constants.tsapps/web/src/server/service/campaign-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/queue/queue-constants.tsapps/web/src/server/service/campaign-service.tscampaign-fix.md
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/queue/queue-constants.tsapps/web/src/server/service/campaign-service.ts
🧬 Code graph analysis (1)
apps/web/src/server/service/campaign-service.ts (5)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/queue/bullmq-context.ts (2)
TeamJob(5-5)createWorkerHandler(10-24)apps/web/src/server/queue/queue-constants.ts (3)
CAMPAIGN_BATCH_QUEUE(3-3)DEFAULT_QUEUE_OPTIONS(6-11)CAMPAIGN_SCHEDULER_QUEUE(4-4)apps/web/src/server/redis.ts (1)
getRedis(6-13)apps/web/src/server/logger/log.ts (1)
logger(31-63)
🪛 markdownlint-cli2 (0.17.2)
campaign-fix.md
64-64: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
64-64: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
🔇 Additional comments (6)
apps/web/src/server/queue/queue-constants.ts (1)
3-4: LGTM—queues named and exported clearly.Names are concise and consistent with usage in services.
apps/web/prisma/schema.prisma (2)
322-323: LGTM—supporting indexes align with query patterns.
Contact(contactBookId, id)enables cursor paging;Campaign(status, scheduledAt)supports scheduler scans.Also applies to: 367-368
328-331: New statuses added—ensure app logic and type unions are updated.Verify all status checks (API/UI/workers) include RUNNING and PAUSED.
apps/web/src/server/service/campaign-service.ts (2)
17-19: LGTM—new queue constants are correctly imported.
77-81: LGTM—first batch is enqueued idempotently.campaign-fix.md (1)
16-22: Doc/code mismatch: enqueue error handling.Doc says to “catch and ignore” duplicate
jobId, but the scheduler currently catches only around the whole loop; a single failure can end the tick early. See suggested per-campaign try/catch in code review.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
KMKoushik
force-pushed
the
km/2025-09-12-campaign-fixes
branch
from
275e2d5 to
280cfb2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
KMKoushik
force-pushed
the
km/2025-09-12-campaign-fixes
branch
from
280cfb2 to
d97aaa8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@codex review |
|
@cubic-dev-ai |
@KMKoushik 👋 Hi! I'm cubic, Here's how I can help with your PR: Ask questions
Request reviews
Give feedback
Request fixes
|
|
@cubic-dev-ai review this |
@KMKoushik I've started the AI code review. It'll take a few minutes to complete. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 20 files
Prompt for AI agents (all 6 issues)
Understand the root cause of the following 6 issues and fix them.
<file name="apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx">
<violation number="1" location="apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx:106">
When chrono.parseDate fails, this code sets selectedDate to the current time, so clicking “Schedule” sends the campaign immediately instead of surfacing an error for the invalid input. Please leave selectedDate null on parse failure so the dialog can show an error instead of scheduling now.</violation>
</file>
<file name="campaign-fix.md">
<violation number="1" location="campaign-fix.md:32">
Resume batches need to skip the previously processed cursor row; otherwise the last cursor contact is read forever and the campaign never completes.</violation>
</file>
<file name="apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx">
<violation number="1" location="apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx:135">
Paused campaigns never render TogglePauseCampaign, so users cannot resume them from this list even though the component supports resume. Please include the PAUSED status in the condition.</violation>
</file>
<file name="apps/web/src/server/api/routers/campaign.ts">
<violation number="1" location="apps/web/src/server/api/routers/campaign.ts:234">
Campaign fetching and validation logic duplicates the `campaignProcedure`'s functionality. `scheduleCampaign` should use `campaignProcedure` instead of `teamProcedure`.</violation>
<violation number="2" location="apps/web/src/server/api/routers/campaign.ts:255">
Campaign content rendering logic duplicates code in `updateCampaign` procedure.</violation>
</file>
<file name="apps/web/prisma/schema.prisma">
<violation number="1" location="apps/web/prisma/schema.prisma:270">
Please add explicit @relation fields (with appropriate onDelete actions) for CampaignEmail so the database enforces integrity; otherwise deleting a campaign/contact/email leaves orphan rows in this dedupe table, and the batch worker will continue treating those contacts as already processed.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| if (parsed) { | ||
| setSelectedDate(parsed); | ||
| } else { | ||
| setSelectedDate(new Date()); |
There was a problem hiding this comment.
When chrono.parseDate fails, this code sets selectedDate to the current time, so clicking “Schedule” sends the campaign immediately instead of surfacing an error for the invalid input. Please leave selectedDate null on parse failure so the dialog can show an error instead of scheduling now.
Prompt for AI agents
Address the following comment on apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx at line 106:
<comment>When chrono.parseDate fails, this code sets selectedDate to the current time, so clicking “Schedule” sends the campaign immediately instead of surfacing an error for the invalid input. Please leave selectedDate null on parse failure so the dialog can show an error instead of scheduling now.</comment>
<file context>
@@ -0,0 +1,310 @@
+ if (parsed) {
+ setSelectedDate(parsed);
+ } else {
+ setSelectedDate(new Date());
+ }
+ };
</file context>
| setSelectedDate(new Date()); | |
| setSelectedDate(null); |
campaign-fix.md
Outdated
| - Load campaign, determine `batchSize` (use column or default). | ||
| - Fetch next page of contacts with cursor paging: | ||
| - `where: { contactBookId, subscribed: true }` | ||
| - `orderBy: { id: asc }`, `take: batchSize`, `cursor` from `lastCursor` if present. |
There was a problem hiding this comment.
Resume batches need to skip the previously processed cursor row; otherwise the last cursor contact is read forever and the campaign never completes.
Prompt for AI agents
Address the following comment on campaign-fix.md at line 32:
<comment>Resume batches need to skip the previously processed cursor row; otherwise the last cursor contact is read forever and the campaign never completes.</comment>
<file context>
@@ -0,0 +1,216 @@
+ - Load campaign, determine `batchSize` (use column or default).
+ - Fetch next page of contacts with cursor paging:
+ - `where: { contactBookId, subscribed: true }`
+ - `orderBy: { id: asc }`, `take: batchSize`, `cursor` from `lastCursor` if present.
+ - For each contact:
+ - Check if an email already exists for `(campaignId, contactId)`; if yes, skip.
</file context>
✅ Addressed in 95b1657
| <div className="flex gap-2"> | ||
| <div className="flex gap-2 items-center"> | ||
| {(campaign.status === CampaignStatus.SCHEDULED || | ||
| campaign.status === CampaignStatus.RUNNING) && ( |
There was a problem hiding this comment.
Paused campaigns never render TogglePauseCampaign, so users cannot resume them from this list even though the component supports resume. Please include the PAUSED status in the condition.
Prompt for AI agents
Address the following comment on apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx at line 135:
<comment>Paused campaigns never render TogglePauseCampaign, so users cannot resume them from this list even though the component supports resume. Please include the PAUSED status in the condition.</comment>
<file context>
@@ -118,7 +130,11 @@ export default function CampaignList() {
- <div className="flex gap-2">
+ <div className="flex gap-2 items-center">
+ {(campaign.status === CampaignStatus.SCHEDULED ||
+ campaign.status === CampaignStatus.RUNNING) && (
+ <TogglePauseCampaign campaign={campaign} />
+ )}
</file context>
| campaign.status === CampaignStatus.RUNNING) && ( | |
| campaign.status === CampaignStatus.RUNNING || campaign.status === CampaignStatus.PAUSED) && ( |
| }, | ||
| ), | ||
|
|
||
| scheduleCampaign: teamProcedure |
There was a problem hiding this comment.
Campaign fetching and validation logic duplicates the campaignProcedure's functionality. scheduleCampaign should use campaignProcedure instead of teamProcedure.
Prompt for AI agents
Address the following comment on apps/web/src/server/api/routers/campaign.ts at line 234:
<comment>Campaign fetching and validation logic duplicates the `campaignProcedure`'s functionality. `scheduleCampaign` should use `campaignProcedure` instead of `teamProcedure`.</comment>
<file context>
@@ -226,6 +231,78 @@ export const campaignRouter = createTRPCRouter({
},
),
+ scheduleCampaign: teamProcedure
+ .input(
+ z.object({
</file context>
✅ Addressed in d148a72
| } | ||
|
|
||
| // Pre-render HTML from content for consistency with sendCampaign | ||
| try { |
There was a problem hiding this comment.
Campaign content rendering logic duplicates code in updateCampaign procedure.
Prompt for AI agents
Address the following comment on apps/web/src/server/api/routers/campaign.ts at line 255:
<comment>Campaign content rendering logic duplicates code in `updateCampaign` procedure.</comment>
<file context>
@@ -226,6 +231,78 @@ export const campaignRouter = createTRPCRouter({
+ }
+
+ // Pre-render HTML from content for consistency with sendCampaign
+ try {
+ const jsonContent = JSON.parse(campaign.content);
+ const renderer = new EmailRenderer(jsonContent);
</file context>
✅ Addressed in d148a72
| @@index([createdAt(sort: Desc)]) | ||
| } | ||
|
|
||
| model CampaignEmail { |
There was a problem hiding this comment.
Please add explicit @relation fields (with appropriate onDelete actions) for CampaignEmail so the database enforces integrity; otherwise deleting a campaign/contact/email leaves orphan rows in this dedupe table, and the batch worker will continue treating those contacts as already processed.
Prompt for AI agents
Address the following comment on apps/web/prisma/schema.prisma at line 270:
<comment>Please add explicit @relation fields (with appropriate onDelete actions) for CampaignEmail so the database enforces integrity; otherwise deleting a campaign/contact/email leaves orphan rows in this dedupe table, and the batch worker will continue treating those contacts as already processed.</comment>
<file context>
@@ -263,9 +263,18 @@ model Email {
@@index([createdAt(sort: Desc)])
}
+model CampaignEmail {
+ campaignId String
+ contactId String
</file context>
There was a problem hiding this comment.
Actionable comments posted: 5
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/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
288-291: Avoid uncontrolled input when replyTo is undefinedNormalize value to an empty string.
Apply this diff:
- value={replyTo} + value={replyTo ?? ""}
♻️ Duplicate comments (4)
apps/web/src/server/service/campaign-service.ts (1)
519-561: Prevent duplicate emails and race by making create+enqueue idempotentCurrent flow can create duplicate Email rows and skip enqueue on unique violation. Prefer upsert with a unique (campaignId, contactId) and/or reserve via CampaignEmail first.
- Add UNIQUE on Email(campaignId, contactId) (see schema/migration comments).
- Use prisma.email.upsert on that unique key, and always attempt to insert CampaignEmail with ON CONFLICT DO NOTHING (or handle unique error) before enqueuing.
- Alternatively, insert the CampaignEmail reservation first (dedupe), then create Email and enqueue.
Also applies to: 537-551, 479-490
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
101-103: Guard against invalid JSON to avoid crashesWrap JSON.parse in a safe initializer.
Apply this diff:
- const [json, setJson] = useState<Record<string, any> | undefined>( - campaign.content ? JSON.parse(campaign.content) : undefined - ); + const [json, setJson] = useState<Record<string, any> | undefined>(() => { + try { + return campaign.content ? JSON.parse(campaign.content) : undefined; + } catch (e) { + console.error("Invalid campaign.content JSON", e); + toast.error("Invalid campaign content. Resetting to empty."); + return undefined; + } + });apps/web/prisma/schema.prisma (2)
266-268: Make (campaignId, contactId) unique on EmailPrevents duplicates per contact per campaign and enables upsert.
Apply this diff:
- @@index([campaignId, contactId]) + @@unique([campaignId, contactId])
270-277: Add relations for CampaignEmail with onDelete to avoid orphansDefine @relation fields and cascade on campaign/contact delete.
Apply this diff:
-model CampaignEmail { - campaignId String - contactId String - emailId String - createdAt DateTime @default(now()) - - @@id([campaignId, contactId]) -} +model CampaignEmail { + campaignId String + contactId String + emailId String + createdAt DateTime @default(now()) + + // Relations + campaign Campaign @relation(fields: [campaignId], references: [id], onDelete: Cascade) + contact Contact @relation(fields: [contactId], references: [id], onDelete: Cascade) + email Email @relation(fields: [emailId], references: [id], onDelete: Cascade) + + @@id([campaignId, contactId]) +}
🧹 Nitpick comments (6)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
135-138: Nit: fix typo in debounced callback nameImproves readability.
Apply this diff:
- const deboucedUpdateCampaign = useDebouncedCallback( + const debouncedUpdateCampaign = useDebouncedCallback( updateEditorContent, 1000 );And update its usage:
- deboucedUpdateCampaign(); + debouncedUpdateCampaign();apps/web/src/server/api/routers/campaign.ts (2)
228-244: Use ctx.campaign from campaignProcedure; drop redundant campaignId in inputAvoids duplicating validation and simplifies the API surface.
Apply this diff:
- scheduleCampaign: campaignProcedure - .input( - z.object({ - campaignId: z.string(), - scheduledAt: z.union([z.string().datetime(), z.date()]).optional(), - batchSize: z.number().min(1).max(100_000).optional(), - }) - ) - .mutation(async ({ ctx: { team }, input }) => { - await scheduleCampaignService({ - campaignId: input.campaignId, + scheduleCampaign: campaignProcedure + .input( + z.object({ + scheduledAt: z.union([z.string().datetime(), z.date()]).optional(), + batchSize: z.number().min(1).max(100_000).optional(), + }) + ) + .mutation(async ({ ctx: { team, campaign }, input }) => { + await scheduleCampaignService({ + campaignId: campaign.id, teamId: team.id, scheduledAt: input.scheduledAt, batchSize: input.batchSize, }); return { ok: true }; }),
181-184: Compute processed via CampaignEmail for accuracy and index usageCounting CampaignEmail aligns with dedupe semantics and avoids double-counting.
Apply this diff:
- const processed = await db.email.count({ - where: { campaignId: input.campaignId }, - }); + const processed = await db.campaignEmail.count({ + where: { campaignId: input.campaignId }, + });Also applies to: 189-196
apps/web/src/server/service/campaign-service.ts (3)
687-700: Set jobId in addBulk for idempotent enqueuesAvoid duplicate contact jobs across retries.
Apply this diff:
static async queueBulkContacts(data: CampaignEmailJob[]) { return await this.campaignQueue.addBulk( data.map((item) => ({ name: `contact-${item.contact.id}`, data: { ...item, teamId: item.emailConfig.teamId, }, - opts: { - ...DEFAULT_QUEUE_OPTIONS, - }, + opts: { + jobId: `campaign:${item.emailConfig.campaignId}:contact:${item.contact.id}`, + ...DEFAULT_QUEUE_OPTIONS, + }, })) ); }
785-805: Offload per-contact processing to CampaignEmailService for scalabilityProcessing contacts inline in the batch worker serializes heavy work. Queue them to the mail-processing queue for parallelism and isolation.
Replace the per-contact loop with a single call to queueBulkContacts:
- // Process each contact in this batch - for (const contact of contacts) { - if (existingSet.has(contact.id)) continue; - await processContactEmail({ - contact, - campaign, - emailConfig: { - from: campaign.from, - subject: campaign.subject, - replyTo: Array.isArray(campaign.replyTo) ? campaign.replyTo : [], - cc: Array.isArray(campaign.cc) ? campaign.cc : [], - bcc: Array.isArray(campaign.bcc) ? campaign.bcc : [], - teamId: campaign.teamId, - campaignId: campaign.id, - previewText: campaign.previewText ?? undefined, - domainId: domain.id, - region: domain.region, - }, - }); - } + await CampaignEmailService.queueBulkContacts( + contacts + .filter((c) => !existingSet.has(c.id)) + .map((contact) => ({ + contact, + campaign, + emailConfig: { + from: campaign.from, + subject: campaign.subject, + replyTo: Array.isArray(campaign.replyTo) ? campaign.replyTo : [], + cc: Array.isArray(campaign.cc) ? campaign.cc : [], + bcc: Array.isArray(campaign.bcc) ? campaign.bcc : [], + teamId: campaign.teamId, + campaignId: campaign.id, + previewText: campaign.previewText ?? undefined, + domainId: domain.id, + region: domain.region, + }, + })) + );
32-99: Consider removing sendCampaign or mark deprecatedRouter no longer exposes this flow. Reduce dead code.
- Remove sendCampaign if unused, or add a comment marking it deprecated in favor of scheduleCampaign + batch processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql(1 hunks)apps/web/prisma/schema.prisma(2 hunks)apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx(12 hunks)apps/web/src/server/api/routers/campaign.ts(8 hunks)apps/web/src/server/service/campaign-service.ts(7 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)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tsapps/web/src/server/api/routers/campaign.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript with 2-space indentation and semicolons (enforced by Prettier)
ESLint must pass with zero warnings using @usesend/eslint-config
Do not use dynamic imports (avoid import() and dynamic loading)
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tsapps/web/src/server/api/routers/campaign.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code and docs with Prettier 3
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tsapps/web/src/server/api/routers/campaign.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase (e.g., AppSideBar.tsx)
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: In apps/web, use the "/" alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC for API calls unless explicitly instructed otherwise
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tsapps/web/src/server/api/routers/campaign.ts
🧠 Learnings (1)
📚 Learning: 2025-09-19T12:10:31.256Z
Learnt from: CR
PR: usesend/useSend#0
File: AGENTS.md:0-0
Timestamp: 2025-09-19T12:10:31.256Z
Learning: Applies to apps/web/**/*.{ts,tsx} : In apps/web, use the "~/" alias for src imports (e.g., import { x } from "~/utils/x")
Applied to files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx
🧬 Code graph analysis (3)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
ScheduleCampaign(27-308)
apps/web/src/server/service/campaign-service.ts (7)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)packages/email-editor/src/renderer.tsx (1)
EmailRenderer(172-774)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/queue/bullmq-context.ts (2)
TeamJob(5-5)createWorkerHandler(10-24)apps/web/src/server/queue/queue-constants.ts (2)
CAMPAIGN_BATCH_QUEUE(3-3)DEFAULT_QUEUE_OPTIONS(6-11)apps/web/src/server/redis.ts (1)
getRedis(6-13)
apps/web/src/server/api/routers/campaign.ts (3)
apps/web/src/server/api/trpc.ts (2)
publicProcedure(91-91)campaignProcedure(224-242)apps/web/src/server/nanoid.ts (1)
nanoid(8-11)apps/web/src/server/service/storage-service.ts (1)
getDocumentUploadUrl(38-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql (1)
9-10: Confirm Postgres major version
Please provide the Postgres major version used in your production and staging environments to verify that multipleADD VALUEstatements in a single migration are supported.
| CREATE TABLE "CampaignEmail" ( | ||
| "campaignId" TEXT NOT NULL, | ||
| "contactId" TEXT NOT NULL, | ||
| "emailId" TEXT NOT NULL, | ||
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
|
|
||
| CONSTRAINT "CampaignEmail_pkey" PRIMARY KEY ("campaignId","contactId") | ||
| ); |
There was a problem hiding this comment.
Add foreign keys for CampaignEmail to enforce referential integrity
Define FKs to Campaign, Contact, and Email to prevent orphans and ensure cascading behavior.
Apply this diff:
CREATE TABLE "CampaignEmail" (
"campaignId" TEXT NOT NULL,
"contactId" TEXT NOT NULL,
"emailId" TEXT NOT NULL,
"createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
CONSTRAINT "CampaignEmail_pkey" PRIMARY KEY ("campaignId","contactId")
);
+
+-- Enforce referential integrity
+ALTER TABLE "CampaignEmail"
+ ADD CONSTRAINT "CampaignEmail_campaignId_fkey"
+ FOREIGN KEY ("campaignId") REFERENCES "Campaign"("id") ON DELETE CASCADE;
+ALTER TABLE "CampaignEmail"
+ ADD CONSTRAINT "CampaignEmail_contactId_fkey"
+ FOREIGN KEY ("contactId") REFERENCES "Contact"("id") ON DELETE CASCADE;
+ALTER TABLE "CampaignEmail"
+ ADD CONSTRAINT "CampaignEmail_emailId_fkey"
+ FOREIGN KEY ("emailId") REFERENCES "Email"("id") ON DELETE CASCADE;📝 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.
| CREATE TABLE "CampaignEmail" ( | |
| "campaignId" TEXT NOT NULL, | |
| "contactId" TEXT NOT NULL, | |
| "emailId" TEXT NOT NULL, | |
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| CONSTRAINT "CampaignEmail_pkey" PRIMARY KEY ("campaignId","contactId") | |
| ); | |
| CREATE TABLE "CampaignEmail" ( | |
| "campaignId" TEXT NOT NULL, | |
| "contactId" TEXT NOT NULL, | |
| "emailId" TEXT NOT NULL, | |
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| CONSTRAINT "CampaignEmail_pkey" PRIMARY KEY ("campaignId","contactId") | |
| ); | |
| -- Enforce referential integrity | |
| ALTER TABLE "CampaignEmail" | |
| ADD CONSTRAINT "CampaignEmail_campaignId_fkey" | |
| FOREIGN KEY ("campaignId") REFERENCES "Campaign"("id") ON DELETE CASCADE; | |
| ALTER TABLE "CampaignEmail" | |
| ADD CONSTRAINT "CampaignEmail_contactId_fkey" | |
| FOREIGN KEY ("contactId") REFERENCES "Contact"("id") ON DELETE CASCADE; | |
| ALTER TABLE "CampaignEmail" | |
| ADD CONSTRAINT "CampaignEmail_emailId_fkey" | |
| FOREIGN KEY ("emailId") REFERENCES "Email"("id") ON DELETE CASCADE; |
🤖 Prompt for AI Agents
In apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql
around lines 20 to 27, the CampaignEmail table lacks foreign key constraints;
add FK constraints referencing the Campaign, Contact, and Email tables (matching
column types/names) and specify cascading behavior (e.g., ON DELETE CASCADE ON
UPDATE CASCADE) so orphaned CampaignEmail rows cannot occur; modify the CREATE
TABLE to include CONSTRAINTs for campaignId -> Campaign(id), contactId ->
Contact(id), and emailId -> Email(id) with appropriate ON DELETE/ON UPDATE
clauses.
There was a problem hiding this comment.
i don't want to add fk, its fine
There was a problem hiding this comment.
@KMKoushik, understood! I'll leave the CampaignEmail table without foreign key constraints as you prefer. 👍
| -- CreateIndex | ||
| CREATE INDEX "Email_campaignId_contactId_idx" ON "Email"("campaignId", "contactId"); |
There was a problem hiding this comment.
Enforce one Email per (campaignId, contactId)
Make the (campaignId, contactId) pair unique on Email to avoid duplicates under concurrency and ensure idempotency.
Replace the non-unique index with a unique constraint:
--- CreateIndex
-CREATE INDEX "Email_campaignId_contactId_idx" ON "Email"("campaignId", "contactId");
+-- Enforce uniqueness only when both are non-null
+CREATE UNIQUE INDEX "Email_campaignId_contactId_key"
+ ON "Email"("campaignId", "contactId");If you cannot tolerate a full-table lock, use a partial unique index instead:
CREATE UNIQUE INDEX "Email_campaignId_contactId_key"
ON "Email"("campaignId", "contactId")
WHERE "campaignId" IS NOT NULL AND "contactId" IS NOT NULL;🤖 Prompt for AI Agents
In apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql
around lines 35-36, the migration currently creates a non-unique index on
Email(campaignId, contactId); replace that with a unique constraint/index to
enforce one Email per (campaignId, contactId). Modify the migration to either
add a UNIQUE constraint on those columns or create a UNIQUE INDEX; if avoiding a
full-table lock is required, create a partial UNIQUE INDEX that only applies
when campaignId and contactId are NOT NULL. Ensure the migration name/comments
reflect the change and update any rollback statements accordingly.
| AccordionItem, | ||
| AccordionTrigger, | ||
| } from "@usesend/ui/src/accordion"; | ||
| import ScheduleCampaign from "../../schedule-campaign"; |
There was a problem hiding this comment.
Fix import path and named export for ScheduleCampaign
Use app alias and named import to match the component’s export.
Apply this diff:
-import ScheduleCampaign from "../../schedule-campaign";
+import { ScheduleCampaign } from "~/app/(dashboard)/campaigns/schedule-campaign";Also ensures ESLint rule for apps/web alias is satisfied. Based on learnings
Also applies to: 212-213
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx around line
44 (and also update lines 212-213), the import currently uses a relative path
and default import for ScheduleCampaign which doesn't match the component's
named export or the project's app alias ESLint rules; change the import to use
the app alias (e.g. import { ScheduleCampaign } from
"app/components/schedule-campaign" or the correct app path) and use a named
import { ScheduleCampaign } instead of a default import, and update the two
other occurrences at lines 212-213 similarly so all imports conform to the app
alias and named export convention.
| campaign.previewText | ||
| ); |
There was a problem hiding this comment.
Initialize previewText to null, not undefined
Type is string | null. Ensure consistent state.
Apply this diff:
- const [previewText, setPreviewText] = useState<string | null>(
- campaign.previewText
- );
+ const [previewText, setPreviewText] = useState<string | null>(
+ campaign.previewText ?? 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.
| campaign.previewText | |
| ); | |
| const [previewText, setPreviewText] = useState<string | null>( | |
| campaign.previewText ?? null | |
| ); |
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx around
lines 113-114, the campaign.previewText state is being initialized to undefined
despite its type being string | null; change the initialization to null (e.g.,
useState<string | null>(null) or set previewText: null in the initial object) so
the state consistently uses null rather than undefined.
d148a72 to
bcdb38c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx (1)
133-137: Include PAUSED to render TogglePauseCampaign
Paused campaigns can’t be resumed from the list otherwise.Apply this diff:
- {(campaign.status === CampaignStatus.SCHEDULED || - campaign.status === CampaignStatus.RUNNING) && ( + {(campaign.status === CampaignStatus.SCHEDULED || + campaign.status === CampaignStatus.RUNNING || + campaign.status === CampaignStatus.PAUSED) && ( <TogglePauseCampaign campaign={campaign} /> )}apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (3)
113-114: Initialize previewText to null consistently
Type is string | null; avoid undefined.- const [previewText, setPreviewText] = useState<string | null>( - campaign.previewText - ); + const [previewText, setPreviewText] = useState<string | null>( + campaign.previewText ?? null + );
44-44: Fix import path and named export for ScheduleCampaign
Use app alias and named import.Apply this diff:
-import ScheduleCampaign from "../../schedule-campaign"; +import { ScheduleCampaign } from "~/app/(dashboard)/campaigns/schedule-campaign";As per coding guidelines
101-103: Guard JSON.parse to prevent crashes on bad content
Wrap in try/catch for safe initialization.- const [json, setJson] = useState<Record<string, any> | undefined>( - campaign.content ? JSON.parse(campaign.content) : undefined - ); + const [json, setJson] = useState<Record<string, any> | undefined>(() => { + try { + return campaign.content ? JSON.parse(campaign.content) : undefined; + } catch (e) { + console.error("Invalid campaign.content JSON", e); + toast.error("Invalid campaign content. Resetting to empty."); + return undefined; + } + });apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (1)
144-147: Render pause/resume only for SCHEDULED/RUNNING/PAUSED
Avoid exposing actions for DRAFT/SENT.- <div className="flex items-center gap-2"> - <TogglePauseCampaign campaign={campaign} /> - </div> + <div className="flex items-center gap-2"> + {(campaign.status === CampaignStatus.SCHEDULED || + campaign.status === CampaignStatus.RUNNING || + campaign.status === CampaignStatus.PAUSED) && ( + <TogglePauseCampaign campaign={campaign} /> + )} + </div>apps/web/prisma/schema.prisma (2)
266-266: Past review concern remains valid: TOCTOU race between Email and CampaignEmail creation.The non-unique index allows duplicate Email rows if two workers process the same contact simultaneously. The race window exists between
Email.create(line 524 in campaign-service.ts) andcampaignEmail.create(line 542). While CampaignEmail has a unique constraint via its composite PK, the Email table does not enforce uniqueness on (campaignId, contactId).Consider applying the recommended unique constraint from the past review or reordering operations to create campaignEmail first with an upsert pattern.
270-277: Past review concern remains: Missing foreign key relations with cascade deletes.Without explicit
@relationfields andonDeleteactions, deleting a campaign, contact, or email leaves orphan rows in CampaignEmail. These orphans would cause the batch worker to incorrectly skip contacts, believing they were already processed.Add FK relations as recommended in the past review.
apps/web/src/server/service/campaign-service.ts (1)
790-809: Past review concern about create+enqueue atomicity remains.The batch loop calls
processContactEmail, which can create Email and campaignEmail records but fail to enqueue the send job. As noted in the past review, this requires either a transactional outbox pattern or enqueue-after-commit with retry.apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
99-108: Invalid input still falls back to "now" instead of showing error.When
chrono.parseDatefails (line 102), the code setsselectedDate = new Date()(line 106), causing the Schedule button to schedule immediately instead of surfacing an error. Past reviews flagged this and marked it as "Addressed in commits c440a71 to d97aaa8", but the fallback is still present in the current code.Apply this diff:
const onScheduleInputChange = (input: string) => { setScheduleInput(input); if (error) setError(null); const parsed = chrono.parseDate(input); if (parsed) { setSelectedDate(parsed); } else { - setSelectedDate(new Date()); + setSelectedDate(null); } };Also update the Calendar to handle null:
<Calendar mode="single" - selected={selectedDate ?? new Date()} + selected={selectedDate ?? undefined} onSelect={(d) => { if (d) setDatePreserveTime(d); }} className="rounded-md border w-[250px] h-[300px] shrink-0" />
🧹 Nitpick comments (3)
packages/ui/src/calendar.tsx (1)
175-206: Use defaultClassNames.day_button for the DayButton overrideYou’re styling the DayButton with defaultClassNames.day (container) instead of button-specific class. Prefer day_button to keep semantics and avoid conflicting styles.
Apply this diff:
function CalendarDayButton({ @@ }: React.ComponentProps<typeof DayButton>) { - const defaultClassNames = getDefaultClassNames(); + const defaultClassNames = getDefaultClassNames(); @@ - className={cn( + className={cn( "data-[selected-single=true]:bg-primary data-[selected-single=true]:text-primary-foreground data-[range-middle=true]:bg-accent data-[range-middle=true]:text-accent-foreground data-[range-start=true]:bg-primary data-[range-start=true]:text-primary-foreground data-[range-end=true]:bg-primary data-[range-end=true]:text-primary-foreground group-data-[focused=true]/day:border-ring group-data-[focused=true]/day:ring-ring/50 dark:hover:text-accent-foreground flex aspect-square size-auto w-full min-w-(--cell-size) flex-col gap-1 leading-none font-normal group-data-[focused=true]/day:relative group-data-[focused=true]/day:z-10 group-data-[focused=true]/day:ring-[3px] data-[range-end=true]:rounded-md data-[range-end=true]:rounded-r-md data-[range-middle=true]:rounded-none data-[range-start=true]:rounded-md data-[range-start=true]:rounded-l-md [&>span]:text-xs [&>span]:opacity-70", - defaultClassNames.day, + defaultClassNames.day_button, className )}apps/web/prisma/schema.prisma (1)
333-333: Consider a partial index for cursor pagination over subscribed contacts.The batch worker always queries
WHERE subscribed: true(line 751 in campaign-service.ts). A partial indexWHERE subscribed = truewould reduce index size and improve scan performance by excluding unsubscribed contacts.PostgreSQL example:
CREATE INDEX CONCURRENTLY idx_contact_book_subscribed_id ON "Contact" ("contactBookId","id") WHERE subscribed = true;campaign-fix.md (1)
34-36: Documentation doesn't address create+enqueue atomicity.The per-contact flow description doesn't mention the atomicity concern: if Email creation succeeds but enqueueing fails, the email is considered processed but never sent. Consider documenting the intended reliability pattern (transactional outbox or enqueue-after-commit with retry).
Based on learnings from past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
AGENTS.md(1 hunks)apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql(1 hunks)apps/web/prisma/schema.prisma(2 hunks)apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx(12 hunks)apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx(4 hunks)apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx(5 hunks)apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx(1 hunks)apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx(1 hunks)apps/web/src/instrumentation.ts(1 hunks)apps/web/src/server/api/routers/campaign.ts(8 hunks)apps/web/src/server/jobs/campaign-scheduler-job.ts(1 hunks)apps/web/src/server/queue/queue-constants.ts(1 hunks)apps/web/src/server/service/campaign-service.ts(7 hunks)campaign-fix.md(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/button.tsx(1 hunks)packages/ui/src/calendar.tsx(1 hunks)packages/ui/src/dialog.tsx(1 hunks)packages/ui/src/popover.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/ui/src/dialog.tsx
- AGENTS.md
- packages/ui/src/button.tsx
- apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql
- apps/web/src/server/queue/queue-constants.ts
- apps/web/src/server/jobs/campaign-scheduler-job.ts
- apps/web/src/instrumentation.ts
🧰 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)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tsapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxpackages/ui/src/popover.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/server/api/routers/campaign.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript with 2-space indentation and semicolons (enforced by Prettier)
ESLint must pass with zero warnings using @usesend/eslint-config
Do not use dynamic imports (avoid import() and dynamic loading)
Files:
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tsapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxpackages/ui/src/popover.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/server/api/routers/campaign.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code and docs with Prettier 3
Files:
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tscampaign-fix.mdapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxpackages/ui/src/popover.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/server/api/routers/campaign.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase (e.g., AppSideBar.tsx)
Files:
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxpackages/ui/src/popover.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: In apps/web, use the "/" alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC for API calls unless explicitly instructed otherwise
Files:
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.tsapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/server/api/routers/campaign.ts
🧬 Code graph analysis (5)
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx (1)
apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx (1)
TogglePauseCampaign(10-61)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
ScheduleCampaign(27-308)
apps/web/src/server/service/campaign-service.ts (6)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)packages/email-editor/src/renderer.tsx (1)
EmailRenderer(172-774)apps/web/src/server/queue/bullmq-context.ts (2)
TeamJob(5-5)createWorkerHandler(10-24)apps/web/src/server/queue/queue-constants.ts (2)
CAMPAIGN_BATCH_QUEUE(3-3)DEFAULT_QUEUE_OPTIONS(6-11)apps/web/src/server/redis.ts (1)
getRedis(6-13)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (1)
apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx (1)
TogglePauseCampaign(10-61)
apps/web/src/server/api/routers/campaign.ts (3)
apps/web/src/server/api/trpc.ts (2)
campaignProcedure(224-242)publicProcedure(91-91)apps/web/src/server/service/campaign-service.ts (1)
deleteCampaign(342-356)apps/web/src/server/service/storage-service.ts (1)
getDocumentUploadUrl(38-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (17)
packages/ui/src/popover.tsx (1)
12-14: LGTM! Container prop and data-slot additions are well-implemented.The changes correctly add portal container control to PopoverContent:
- Type definition properly extends Radix UI props with optional container
- Container is normalized (
?? undefined) before forwarding to Portal, which is explicit and safe- The
data-slot="popover-content"attribute aligns with the broader UI component identification patternAlso applies to: 18-20, 23-23
packages/ui/src/calendar.tsx (1)
28-172: Solid DayPicker wrapper and themingProps forwarding, classNames, components overrides, and chevrons look correct. Good default formatters/setup.
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx (3)
62-67: RUNNING/PAUSED filter options: LGTM
101-105: Edit routing for DRAFT/SCHEDULED: LGTM
117-122: Status badge mapping extended: LGTMapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (1)
28-41: Adaptive polling logic: LGTM
Refetch when RUNNING or dueNow is a good balance.apps/web/prisma/schema.prisma (1)
358-379: LGTM! Schema changes support the batching workflow.The added fields (scheduledAt, batchSize, batchWindowMinutes, lastCursor, lastSentAt, timestamps) properly support scheduled batch processing. The index on (status, scheduledAt) will optimize scheduler queries. Default values are sensible.
apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx (1)
1-64: LGTM! Clean pause/resume control implementation.The component properly handles pause/resume state transitions with appropriate mutations, query invalidation, and user feedback via toasts. UI clearly indicates paused (green play) vs active (orange pause) states with proper disabled state during mutations.
apps/web/src/server/service/campaign-service.ts (5)
32-99: LGTM! Refactored to use batch processing.The function now correctly validates HTML content and unsubscribe links before scheduling, counts contacts without loading all into memory, and delegates to batch processing. This addresses the memory bloat issue mentioned in the design doc.
101-204: LGTM! Comprehensive campaign scheduling with validation.The function properly validates all prerequisites (content, HTML, contact book, unsubscribe link, contact count) before scheduling. Error handling uses appropriate API errors. The cursor reset logic for DRAFT/SENT transitions is correct.
342-356: LGTM! Deletion wrapped in transaction.The function now uses a transaction to ensure atomic deletion of campaign and related campaignEmail records, addressing the past review concern. If foreign keys with CASCADE are added (as suggested in another review), this explicit delete of campaignEmail records would become redundant.
749-762: Missingskip: 1in cursor pagination causes reprocessing.Prisma includes the cursor row by default. Without
skip: 1whenlastCursoris set, the last processed contact is re-read and checked in every batch, wasting processing cycles.Apply this diff:
if (campaign.lastCursor) { pagination.cursor = { id: campaign.lastCursor }; - pagination.skip = 1; // do not include the cursor row + pagination.skip = 1; }Note: The comment on line 759 indicates awareness but the implementation has
skip: 1present. Wait, reviewing line 759 shows:pagination.skip = 1; // do not include the cursor row- this IS implemented correctly!
821-860: LGTM! Defensive batch window check with idempotent enqueueing.The method properly enforces batch windows before enqueueing and uses a deterministic jobId to ensure only one batch job per campaign exists. Early returns for PAUSED/SENT states are correct.
apps/web/src/server/api/routers/campaign.ts (4)
177-179: LGTM! Progress tracking via processed email count.The
processedfield computation provides necessary progress visibility for the UI. The query is only executed when viewing campaign details, so performance impact is acceptable.Also applies to: 185-185, 191-191
159-161: LGTM! Proper delegation to service layer.Delegates to the transactional
deleteCampaignservice function reviewed earlier.
224-240: LGTM! Scheduling mutation with proper validation.Uses
campaignProcedurefor campaign existence/ownership validation. Input constraints onbatchSize(1-100,000) are reasonable. Proper delegation to service layer.
242-260: LGTM! Pause and resume mutations correctly manage campaign state.Both mutations use
campaignProcedurefor validation. Setting status toPAUSEDstops the scheduler from enqueueing batches (verified at line 733 in campaign-service.ts). Resuming toSCHEDULEDallows the scheduler to continue. Logic is correct.
| import TogglePauseCampaign from "../toggle-pause-campaign"; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Fix import: TogglePauseCampaign is a named export and prefer alias path
Default import will fail; also use "~/" alias in apps/web.
-import TogglePauseCampaign from "../toggle-pause-campaign";
+import { TogglePauseCampaign } from "~/app/(dashboard)/campaigns/toggle-pause-campaign";As per coding guidelines
🏁 Script executed:
#!/bin/bash
# Confirm export form and locate default imports
rg -nP -C2 'export\s+(default\s+)?const\s+TogglePauseCampaign' apps/web
rg -nP -C2 'import\s+TogglePauseCampaign\b.*from\s+["\'].*toggle-pause-campaign["\']' apps/webLength of output: 599
Use named import for TogglePauseCampaign via “~/” alias
-import TogglePauseCampaign from "../toggle-pause-campaign";
+import { TogglePauseCampaign } from "~/app/(dashboard)/campaigns/toggle-pause-campaign";Default import fails because TogglePauseCampaign is a named export.
📝 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 TogglePauseCampaign from "../toggle-pause-campaign"; | |
| import { TogglePauseCampaign } from "~/app/(dashboard)/campaigns/toggle-pause-campaign"; |
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx around lines
19-20, the code uses a default import for TogglePauseCampaign but the component
is a named export; replace the default import from "../toggle-pause-campaign"
with a named import that uses the "~/” path alias to the component (import the
named export TogglePauseCampaign via the alias), and ensure the import path
points to the correct file exported under the alias so usages continue to work.
| import DeleteCampaign from "./delete-campaign"; | ||
| import Link from "next/link"; | ||
| import DuplicateCampaign from "./duplicate-campaign"; | ||
| import TogglePauseCampaign from "./toggle-pause-campaign"; |
There was a problem hiding this comment.
🧩 Analysis chain
Fix import: TogglePauseCampaign is a named export
toggle-pause-campaign exports TogglePauseCampaign as a named export; default import will break.
Apply this diff:
-import TogglePauseCampaign from "./toggle-pause-campaign";
+import { TogglePauseCampaign } from "./toggle-pause-campaign";🏁 Script executed:
#!/bin/bash
# Find default imports of toggle-pause and confirm the export form
rg -nP -C2 'export\s+(default\s+)?const\s+TogglePauseCampaign' apps/web
rg -nP -C2 'import\s+TogglePauseCampaign\b.*from\s+["\'].*toggle-pause-campaign["\']' apps/webLength of output: 599
Import TogglePauseCampaign as a named export
toggle-pause-campaign.tsx exports TogglePauseCampaign as a named export; update in apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx:20:
-import TogglePauseCampaign from "./toggle-pause-campaign";
+import { TogglePauseCampaign } from "./toggle-pause-campaign";📝 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 TogglePauseCampaign from "./toggle-pause-campaign"; | |
| import { TogglePauseCampaign } from "./toggle-pause-campaign"; |
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx around line 20, the
file currently imports TogglePauseCampaign as a default export but the component
is exported as a named export; update the import to a named import (use curly
braces) from the same path and ensure the import identifier matches the exported
name (TogglePauseCampaign).
| try { | ||
| await db.campaignEmail.create({ | ||
| data: { | ||
| campaignId: emailConfig.campaignId, | ||
| contactId: contact.id, | ||
| emailId: email.id, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| logger.error( | ||
| { err: error }, | ||
| "Failed to create campaign email record so skipping email sending" | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Failed campaignEmail creation can leave orphan Email records.
If campaignEmail.create fails after email.create succeeds (line 524), the function returns without queueing the email. On retry, the existence check (which queries campaignEmail) will miss the orphaned Email record and create a duplicate. This can accumulate orphan Email records over time.
Consider one of these approaches:
- Create campaignEmail first with a placeholder emailId, then update with the actual emailId
- Use a transaction to create both records atomically
- Add the unique constraint on Email (campaignId, contactId) as suggested in earlier reviews
🤖 Prompt for AI Agents
In apps/web/src/server/service/campaign-service.ts around lines 541 to 555, the
code creates Email then attempts to create campaignEmail separately, which can
leave orphan Email records if campaignEmail.create fails; modify the logic to
perform both creations inside a single database transaction (create
campaignEmail and Email atomically) so either both succeed or both roll back; if
your ORM/DB doesn't support a direct two-row transaction in this context,
instead create the campaignEmail first with a temporary/placeholder emailId and
then update campaignEmail.emailId after Email creation (or add a unique
constraint on Email (campaignId, contactId) to prevent duplicates) — implement
the transaction approach by wrapping both create calls in db.$transaction (or
equivalent), ensuring proper error handling and rollback on failure.
campaign-fix.md
Outdated
| 2. Add a composite index for fast existence checks (non-unique): | ||
|
|
||
| - `Email` non-unique composite index on `(campaignId, contactId)` | ||
| - Purpose: Speed up the pre-insert existence check. We will keep this non-unique by design and rely on an existence check in code. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Documentation describes non-unique index despite TOCTOU concerns.
The design doc specifies a non-unique index on (campaignId, contactId) and explicitly states "we will keep this non-unique by design". However, past reviews have flagged this as TOCTOU-prone, allowing duplicate Email rows under race conditions. The doc should either justify this decision with the accepted risks or update to recommend a unique constraint.
Consider updating the doc to reflect the recommended approach:
-2. Add a composite index for fast existence checks (non-unique):
- - `Email` non-unique composite index on `(campaignId, contactId)`
- - Purpose: Speed up the pre-insert existence check. We will keep this non-unique by design and rely on an existence check in code.
+2. Add a composite UNIQUE constraint for existence checks:
+ - `Email` UNIQUE composite constraint on `(campaignId, contactId)`
+ - Purpose: Enforce idempotency at DB-level. Use upsert/`ON CONFLICT DO NOTHING` to avoid races.Based on learnings from past review comments.
Also applies to: 43-43, 57-57
🤖 Prompt for AI Agents
In campaign-fix.md around lines 14 to 17 (also applies to lines 43 and 57), the
doc currently prescribes a non-unique composite index on (campaignId, contactId)
but does not address TOCTOU/race-condition risks; update the document to either
(a) justify and accept the risk explicitly (describe expected race window,
mitigation strategies like application-level locking, idempotent retries, or
background de-duplication), or (b) recommend and document making the
index/constraint unique (add migration/rollback notes and how to handle existing
duplicates). Ensure the chosen option is applied consistently in all mentioned
line ranges and include a short actionable mitigation plan for deployments.
campaign-fix.md
Outdated
| - Fetch next page of contacts with cursor paging: | ||
| - `where: { contactBookId, subscribed: true }` | ||
| - `orderBy: { id: asc }`, `take: batchSize`, `cursor` from `lastCursor` if present. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Documentation omits skip: 1 for cursor pagination.
The cursor pagination description doesn't mention skip: 1, which is essential to avoid reprocessing the cursor row. The implementation correctly includes it (line 759 in campaign-service.ts), but the doc should document this detail.
Apply this diff:
- Fetch next page of contacts with cursor paging:
- `where: { contactBookId, subscribed: true }`
- - `orderBy: { id: asc }`, `take: batchSize`, `cursor` from `lastCursor` if present.
+ - `orderBy: { id: asc }`, `take: batchSize`, `cursor` from `lastCursor` if present, `skip: 1` when cursor is set.Based on learnings from past review comments.
📝 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.
| - Fetch next page of contacts with cursor paging: | |
| - `where: { contactBookId, subscribed: true }` | |
| - `orderBy: { id: asc }`, `take: batchSize`, `cursor` from `lastCursor` if present. | |
| - Fetch next page of contacts with cursor paging: | |
| - `where: { contactBookId, subscribed: true }` | |
| - `orderBy: { id: asc }`, `take: batchSize`, `cursor` from `lastCursor` if present, `skip: 1` when cursor is set. |
🤖 Prompt for AI Agents
In campaign-fix.md around lines 30 to 32, the docs describing cursor pagination
for fetching next page of contacts omit the essential "skip: 1" detail; update
the text to explicitly state that when using a cursor (from lastCursor) you must
include skip: 1 to avoid reprocessing the cursor row (match the implementation
at campaign-service.ts line 759), and add a short example or parenthetical note
indicating skip: 1 is only needed when providing a cursor.
| "date-fns": "^4.1.0", | ||
| "framer-motion": "^12.9.2", | ||
| "hast-util-to-jsx-runtime": "^2.3.6", | ||
| "input-otp": "^1.4.2", | ||
| "lucide-react": "^0.503.0", | ||
| "next-themes": "^0.4.6", | ||
| "pnpm": "^10.9.0", | ||
| "react-day-picker": "^9.10.0", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
New date dependencies look right; ensure peer deps alignment
date-fns and react-day-picker are appropriate for Calendar. Also ensure @usesend/ui declares react and react-dom as peerDependencies to avoid duplicate React in consumers.
You can verify current imports and peer deps with:
🏁 Script executed:
#!/bin/bash
rg -nP '"peerDependencies"\s*:' -A8 packages/ui/package.json
rg -nP --type=tsx '(from|require)\(["\']react(-dom)?["\']\)' packages/ui/srcLength of output: 163
🏁 Script executed:
#!/bin/bash
# Show peerDependencies block
rg -n '"peerDependencies"' -A8 packages/ui/package.json
# Check for react and react-dom entries
rg -n '"react"' packages/ui/package.json
rg -n '"react-dom"' packages/ui/package.json
# Find react/react-dom imports in TSX files
rg -n --type=tsx '\bimport.*react\b' packages/ui/src
rg -n --type=tsx '\bimport.*react-dom\b' packages/ui/srcLength of output: 81
Declare React and React-DOM as peerDependencies
packages/ui/package.json currently lists React in dependencies (v19.1.0) and omits React-DOM. Move both into a peerDependencies block (and remove React from dependencies) to prevent duplicate React instances in downstream consumers.
🤖 Prompt for AI Agents
packages/ui/package.json lines 49-56: React is listed in dependencies and
react-dom is missing; this can cause duplicate React instances for downstream
consumers. Remove React from "dependencies", add both "react" and "react-dom"
entries under a new "peerDependencies" section (use the same major/minor range
as the current React version, e.g., ^19.1.0), and optionally keep them in
"devDependencies" for local development/testing only; ensure package.json
formatting remains valid and run a reinstall to verify no duplicate React
warnings.
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/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
46-48: Remove unused sendSchema.The
sendSchemais no longer used after removing the sendCampaign flow.Apply this diff to remove the unused code:
-const sendSchema = z.object({ - confirmation: z.string(), -}); -
♻️ Duplicate comments (5)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (3)
44-44: Fix import path and named export for ScheduleCampaign.Use the app alias and named import to match the component's export.
Apply this diff:
-import ScheduleCampaign from "../../schedule-campaign"; +import { ScheduleCampaign } from "~/app/(dashboard)/campaigns/schedule-campaign";This resolves the import path to use the
~/alias (as per coding guidelines for apps/web) and corrects the import to use a named import matching the component's named export. Based on coding guidelines
101-103: Guard against invalid JSON content to prevent page crash.
JSON.parsecan throw; add a safe initializer.Apply this diff:
- const [json, setJson] = useState<Record<string, any> | undefined>( - campaign.content ? JSON.parse(campaign.content) : undefined - ); + const [json, setJson] = useState<Record<string, any> | undefined>(() => { + try { + return campaign.content ? JSON.parse(campaign.content) : undefined; + } catch (e) { + console.error("Invalid campaign.content JSON", e); + toast.error("Invalid campaign content. Resetting to empty."); + return undefined; + } + });
112-114: Initialize previewText to null, not undefined.Type is
string | null. Ensure consistent state.Apply this diff:
- const [previewText, setPreviewText] = useState<string | null>( - campaign.previewText - ); + const [previewText, setPreviewText] = useState<string | null>( + campaign.previewText ?? null + );apps/web/src/server/service/campaign-service.ts (2)
678-697: Create+enqueue is not atomic and can lose sends.If
processContactEmail(which callsdb.email.createthendb.campaignEmail.create) succeeds at creating the Email row but fails to create the campaignEmail row, the email will be marked as "processed" (orphaned Email exists) but the existence check (lines 668-675) only queriescampaignEmail, so retries will create duplicate Email records.Implement one of these patterns:
- Transactional outbox: Create Email rows and outbox entries in a transaction, then have a separate process read the outbox and enqueue jobs.
- Enqueue-after-commit with retry: Collect enqueue payloads during processing, commit all Email creates in a transaction with campaignEmail, then enqueue with retries and mark failures in the Email table.
Based on learnings from past review comments.
522-536: Early return prevents sending but doesn't solve the atomicity issue.While returning early (line 535) prevents queueing an email without a
campaignEmailassociation, thecampaignEmail.createfails.On retry, the existence check in the worker (lines 668-675) queries
campaignEmail, not
- Duplicate Email records for the same contact/campaign
- Incomplete campaign analytics (orphaned Emails aren't counted)
Apply the transactional outbox or enqueue-after-commit pattern recommended in the previous review (see past comment on lines 678-697). Alternatively, create both records in a single transaction:
- const email = await db.email.create({ - data: { - to: filteredToEmails, - replyTo: emailConfig.replyTo, - cc: filteredCcEmails.length > 0 ? filteredCcEmails : undefined, - bcc: filteredBccEmails.length > 0 ? filteredBccEmails : undefined, - from: emailConfig.from, - subject: emailConfig.subject, - html, - text: emailConfig.previewText, - teamId: emailConfig.teamId, - campaignId: emailConfig.campaignId, - contactId: contact.id, - domainId: emailConfig.domainId, - }, - }); - - try { - await db.campaignEmail.create({ - data: { - campaignId: emailConfig.campaignId, - contactId: contact.id, - emailId: email.id, - }, - }); - } catch (error) { - logger.error( - { err: error }, - "Failed to create campaign email record so skipping email sending" - ); - return; - } - - // Queue email for sending - await EmailQueueService.queueEmail( - email.id, - emailConfig.teamId, - emailConfig.region, - false, - oneClickUnsubUrl - ); + const result = await db.$transaction(async (tx) => { + const email = await tx.email.create({ + data: { + to: filteredToEmails, + replyTo: emailConfig.replyTo, + cc: filteredCcEmails.length > 0 ? filteredCcEmails : undefined, + bcc: filteredBccEmails.length > 0 ? filteredBccEmails : undefined, + from: emailConfig.from, + subject: emailConfig.subject, + html, + text: emailConfig.previewText, + teamId: emailConfig.teamId, + campaignId: emailConfig.campaignId, + contactId: contact.id, + domainId: emailConfig.domainId, + }, + }); + + await tx.campaignEmail.create({ + data: { + campaignId: emailConfig.campaignId, + contactId: contact.id, + emailId: email.id, + }, + }); + + return email; + }); + + // Queue email for sending (outside transaction for idempotency) + await EmailQueueService.queueEmail( + result.id, + emailConfig.teamId, + emailConfig.region, + false, + oneClickUnsubUrl + );Then handle enqueue failures with retry or a separate outbox worker as suggested in the previous review.
🧹 Nitpick comments (2)
apps/web/src/server/service/campaign-service.ts (2)
28-95: Consider renamingsendCampaignto reflect scheduling semantics.The function no longer sends immediately; it validates, sets status to
SCHEDULED, and enqueues the first batch. The namesendCampaignis misleading.Consider renaming to
scheduleCampaignImmediatelyor similar to clarify intent, or merge its logic intoscheduleCampaignif they're functionally equivalent.
709-748: Defensive window check has benign TOCTOU but is redundant.The defensive check (lines 717-741) reads campaign state and decides whether to enqueue, but this has a time-of-check-to-time-of-use gap. Another process could enqueue between the check and line 743. However, the
jobIdon line 746 makes the enqueue idempotent, so the race is harmless.Since the comment on line 716 notes the scheduler already enforces the window, consider removing the defensive check to simplify the code. The idempotent
jobIdalone provides sufficient protection against duplicate enqueues.Alternatively, if you want to keep the optimization, document that it's purely advisory (not for correctness) and the race is benign.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/package.json(2 hunks)apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx(12 hunks)apps/web/src/server/service/campaign-service.ts(6 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)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript with 2-space indentation and semicolons (enforced by Prettier)
ESLint must pass with zero warnings using @usesend/eslint-config
Do not use dynamic imports (avoid import() and dynamic loading)
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code and docs with Prettier 3
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase (e.g., AppSideBar.tsx)
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: In apps/web, use the "/" alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC for API calls unless explicitly instructed otherwise
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/service/campaign-service.ts
🧬 Code graph analysis (2)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
ScheduleCampaign(27-308)
apps/web/src/server/service/campaign-service.ts (7)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)packages/email-editor/src/renderer.tsx (1)
EmailRenderer(172-774)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/queue/bullmq-context.ts (2)
TeamJob(5-5)createWorkerHandler(10-24)apps/web/src/server/queue/queue-constants.ts (2)
CAMPAIGN_BATCH_QUEUE(3-3)DEFAULT_QUEUE_OPTIONS(6-11)apps/web/src/server/redis.ts (1)
getRedis(6-13)
🔇 Additional comments (3)
apps/web/package.json (1)
18-19: Verify script paths and module format: noapps/web/scripts/memory-monitor.js,memory-profiler.js, orbaseline-test.jsfound—ensure these files exist at the referenced paths and, with"type": "module", use ESM exports/imports or rename to.cjs.apps/web/src/server/service/campaign-service.ts (2)
97-200: LGTM!The validation logic is comprehensive, error handling uses appropriate
UnsendApiErrorcodes, and the cursor reset logic correctly identifies campaigns that need a fresh start (DRAFT/SENT).
338-352: LGTM! Addresses previous review feedback.The transaction wrapper ensures atomic deletion and prevents orphaned
campaignEmailrecords. This resolves the concern raised in the previous review.
| "db:migrate-reset": "prisma migrate reset", | ||
| "memory:monitor": "node --expose-gc scripts/memory-monitor.js", | ||
| "memory:profile": "node --expose-gc scripts/memory-profiler.js", | ||
| "memory:test": "node --expose-gc -e \"const MemoryMonitor = require('./scripts/memory-monitor'); const monitor = new MemoryMonitor(); monitor.start(1000); setTimeout(() => monitor.stop(), 30000)\"", |
There was a problem hiding this comment.
memory:test will likely fail under ESM (require is not defined/ERR_REQUIRE_ESM).
Project uses "type": "module". If scripts/memory-monitor.js is ESM (likely), require('./scripts/memory-monitor') in -e will fail. Use dynamic import with --input-type=module.
Apply this diff to make memory:test ESM-safe:
- "memory:test": "node --expose-gc -e \"const MemoryMonitor = require('./scripts/memory-monitor'); const monitor = new MemoryMonitor(); monitor.start(1000); setTimeout(() => monitor.stop(), 30000)\"",
+ "memory:test": "node --expose-gc --input-type=module -e \"import MemoryMonitor from './scripts/memory-monitor.js'; const monitor = new MemoryMonitor(); monitor.start(1000); setTimeout(() => monitor.stop(), 30000);\"",📝 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.
| "memory:test": "node --expose-gc -e \"const MemoryMonitor = require('./scripts/memory-monitor'); const monitor = new MemoryMonitor(); monitor.start(1000); setTimeout(() => monitor.stop(), 30000)\"", | |
| "memory:test": "node --expose-gc --input-type=module -e \"import MemoryMonitor from './scripts/memory-monitor.js'; const monitor = new MemoryMonitor(); monitor.start(1000); setTimeout(() => monitor.stop(), 30000);\"", |
🤖 Prompt for AI Agents
In apps/web/package.json around line 20, the memory:test script uses require in
a node -e one-liner which will fail when the project is ESM ("type":"module");
change the script to run Node with --input-type=module and use a dynamic import
(e.g. an async IIFE that awaits import('./scripts/memory-monitor.js')) instead
of require, start the monitor and stop it after 30s; ensure the import path
includes the .js extension and preserve the same expose-gc and timing options.
| try { | ||
| await db.campaignEmail.create({ | ||
| data: { | ||
| campaignId: emailConfig.campaignId, | ||
| contactId: contact.id, | ||
| emailId: email.id, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| logger.error({ err: error }, "Failed to create campaign email record"); | ||
| } |
There was a problem hiding this comment.
Log-and-continue can orphan Email records in suppression path.
If campaignEmail.create fails, the Email row (already created as SUPPRESSED) becomes an orphan. While less severe than in the send path (no actual send is lost), it's still a data consistency issue.
Consider wrapping both db.email.create (line 435) and db.campaignEmail.create in a transaction, or add a background job to clean up orphaned Email records without a corresponding campaignEmail entry.
🤖 Prompt for AI Agents
In apps/web/src/server/service/campaign-service.ts around lines 435 to 474, the
current flow creates an Email row (marked SUPPRESSED) and then separately
creates the campaignEmail record, but if campaignEmail.create fails the Email
becomes orphaned; to fix, perform both db.email.create and
db.campaignEmail.create inside a single database transaction so both succeed or
both roll back, or if transactions are unavailable, implement a retry/backoff
for campaignEmail.create and enqueue a background cleanup job to remove Email
rows that lack a corresponding campaignEmail; update error handling to surface
failures (not just log-and-continue) and ensure any created Email is deleted on
failure if not using transactions.
update campaign ui stuff
KMKoushik
force-pushed
the
km/2025-09-12-campaign-fixes
branch
from
50e4cdb to
2a54752
Compare
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/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
44-206: Fix ScheduleCampaign import to use the app alias and named export.The component re-exports
ScheduleCampaignas a named export under the~/alias. Keeping the relative default import breaks type checking and violates the apps/web import convention enforced by ESLint.Switch to
import { ScheduleCampaign } from "~/app/(dashboard)/campaigns/schedule-campaign";(and drop the default import usage) to satisfy the lint rule and avoid runtimeundefined.
♻️ Duplicate comments (5)
apps/web/package.json (1)
18-21: Fixmemory:testfor ESM mode.Project runs in ESM (
"type": "module"), sorequire('./scripts/memory-monitor')insidenode -ethrowsERR_REQUIRE_ESM. Switch the script to--input-type=moduleand useawait import('./scripts/memory-monitor.js')(with the.jsextension) before instantiating the monitor.apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (1)
19-21: Fix TogglePauseCampaign import
toggle-pause-campaign.tsxonly exportsTogglePauseCampaignas a named export, so the default import resolves toundefinedand throws at runtime when rendered. Import it by name (and use the~/alias per project convention).-import TogglePauseCampaign from "../toggle-pause-campaign"; +import { TogglePauseCampaign } from "~/app/(dashboard)/campaigns/toggle-pause-campaign";apps/web/src/app/(dashboard)/campaigns/campaign-card.tsx (1)
13-16: Import campaign action components by name
DeleteCampaign,DuplicateCampaign, andTogglePauseCampaignare all named exports. Importing them as defaults yieldsundefined, so the card crashes when React tries to render these controls. Switch to named imports.-import DeleteCampaign from "./delete-campaign"; -import DuplicateCampaign from "./duplicate-campaign"; -import TogglePauseCampaign from "./toggle-pause-campaign"; +import { DeleteCampaign } from "./delete-campaign"; +import { DuplicateCampaign } from "./duplicate-campaign"; +import { TogglePauseCampaign } from "./toggle-pause-campaign";apps/web/src/server/service/campaign-service.ts (1)
581-595: Don’t swallow campaignEmail.create failuresIf
db.campaignEmail.createthrows (transient DB error, constraint hiccup, etc.), we log and bail out. At that point the Email row already exists,lastCursorwill still advance, and we skip queueing—meaning that recipient never gets processed and we accumulate orphan Email records. Please make the email + campaignEmail creation atomic and fail the job so BullMQ can retry (e.g., wrap both inserts in a singledb.$transactionand only queue once the transaction commits, or delete/rollback the Email row and rethrow). Otherwise a brief DB blip permanently drops sends.apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
101-109: Keep selectedDate null on parse failureWhen
chrono.parseDatefails you callsetSelectedDate(new Date()). BecauseonDialogScheduleprefersselectedDateover parsing, the dialog happily schedules “right now” even though the input was invalid. LeaveselectedDateasnullso the error message surfaces and nothing is scheduled.- const parsed = chrono.parseDate(input); - if (parsed) { - setSelectedDate(parsed); - } else { - setSelectedDate(new Date()); - } + const parsed = chrono.parseDate(input); + if (parsed) { + setSelectedDate(parsed); + } else { + setSelectedDate(null); + }
🧹 Nitpick comments (1)
apps/web/src/server/api/routers/campaign.ts (1)
253-269: Remove redundantcampaignIdinput and usectx.campaign.id.Replace the input schema and mutation signature to match other
campaignProcedureroutes:scheduleCampaign: campaignProcedure .input( z.object({ - campaignId: z.string(), scheduledAt: z.union([z.string().datetime(), z.date()]).optional(), batchSize: z.number().min(1).max(100_000).optional(), }) ) - .mutation(async ({ ctx: { team }, input }) => { + .mutation(async ({ ctx: { team, campaign }, input }) => { await campaignService.scheduleCampaign({ - campaignId: input.campaignId, + campaignId: campaign.id, teamId: team.id, scheduledAt: input.scheduledAt, batchSize: input.batchSize, }); return { ok: true }; }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
AGENTS.md(1 hunks)apps/web/package.json(2 hunks)apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql(1 hunks)apps/web/prisma/schema.prisma(2 hunks)apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx(12 hunks)apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx(4 hunks)apps/web/src/app/(dashboard)/campaigns/campaign-card.tsx(1 hunks)apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx(2 hunks)apps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsx(1 hunks)apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx(1 hunks)apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx(1 hunks)apps/web/src/instrumentation.ts(1 hunks)apps/web/src/server/api/routers/campaign.ts(9 hunks)apps/web/src/server/jobs/campaign-scheduler-job.ts(1 hunks)apps/web/src/server/queue/queue-constants.ts(1 hunks)apps/web/src/server/service/campaign-service.ts(6 hunks)packages/ui/package.json(1 hunks)packages/ui/src/button.tsx(1 hunks)packages/ui/src/calendar.tsx(1 hunks)packages/ui/src/dialog.tsx(1 hunks)packages/ui/src/popover.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/ui/src/dialog.tsx
- packages/ui/src/popover.tsx
- apps/web/src/server/queue/queue-constants.ts
- apps/web/src/instrumentation.ts
- apps/web/prisma/migrations/20251006185736_campaign_batching/migration.sql
- packages/ui/src/button.tsx
- AGENTS.md
🧰 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/server/service/campaign-service.tsapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/jobs/campaign-scheduler-job.tsapps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsxapps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/campaign-card.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxapps/web/src/server/api/routers/campaign.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript as the primary language; prefer .ts/.tsx over JavaScript
Use 2-space indentation and semicolons; rely on Prettier for enforcement
Files:
apps/web/src/server/service/campaign-service.tsapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/jobs/campaign-scheduler-job.tsapps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsxapps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/campaign-card.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxapps/web/src/server/api/routers/campaign.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: In apps/web, use the/ alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC for API communication unless explicitly instructed otherwise
Files:
apps/web/src/server/service/campaign-service.tsapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/jobs/campaign-scheduler-job.tsapps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsxapps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/campaign-card.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxapps/web/src/server/api/routers/campaign.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use dynamic imports; always place imports at the top of the file
Files:
apps/web/src/server/service/campaign-service.tsapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/server/jobs/campaign-scheduler-job.tsapps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsxapps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/campaign-card.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsxapps/web/src/server/api/routers/campaign.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase (e.g., AppSideBar.tsx)
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsxapps/web/src/app/(dashboard)/campaigns/campaign-list.tsxapps/web/src/app/(dashboard)/campaigns/campaign-card.tsxpackages/ui/src/calendar.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsxapps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx
🧬 Code graph analysis (8)
apps/web/src/server/service/campaign-service.ts (7)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)packages/email-editor/src/renderer.tsx (1)
EmailRenderer(172-774)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/queue/bullmq-context.ts (2)
TeamJob(5-5)createWorkerHandler(10-24)apps/web/src/server/queue/queue-constants.ts (2)
CAMPAIGN_BATCH_QUEUE(4-4)DEFAULT_QUEUE_OPTIONS(7-12)apps/web/src/server/redis.ts (1)
getRedis(6-13)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
ScheduleCampaign(27-310)
apps/web/src/server/jobs/campaign-scheduler-job.ts (6)
apps/web/src/server/queue/bullmq-context.ts (2)
TeamJob(5-5)createWorkerHandler(10-24)apps/web/src/server/queue/queue-constants.ts (2)
CAMPAIGN_SCHEDULER_QUEUE(5-5)DEFAULT_QUEUE_OPTIONS(7-12)apps/web/src/server/redis.ts (1)
getRedis(6-13)apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/service/campaign-service.ts (1)
CampaignBatchService(660-808)
apps/web/src/app/(dashboard)/campaigns/campaign-list.tsx (2)
apps/web/src/hooks/useUrlState.ts (1)
useUrlState(8-36)apps/web/src/app/(dashboard)/campaigns/campaign-card.tsx (1)
CampaignCard(35-146)
apps/web/src/app/(dashboard)/campaigns/campaign-card.tsx (5)
apps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsx (1)
CampaignStatusBadge(7-36)packages/ui/src/tooltip.tsx (4)
TooltipProvider(30-30)Tooltip(30-30)TooltipTrigger(30-30)TooltipContent(30-30)apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx (1)
TogglePauseCampaign(10-95)apps/web/src/app/(dashboard)/campaigns/duplicate-campaign.tsx (1)
DuplicateCampaign(18-79)apps/web/src/app/(dashboard)/campaigns/delete-campaign.tsx (1)
DeleteCampaign(35-134)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (4)
apps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsx (1)
CampaignStatusBadge(7-36)apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx (1)
TogglePauseCampaign(10-95)packages/ui/src/spinner.tsx (1)
Spinner(4-51)apps/web/src/app/(dashboard)/emails/email-status-badge.tsx (1)
EmailStatusBadge(3-39)
apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx (2)
packages/ui/src/toaster.tsx (1)
toast(31-31)packages/ui/src/button.tsx (1)
Button(80-80)
apps/web/src/server/api/routers/campaign.ts (3)
apps/web/src/server/api/trpc.ts (2)
campaignProcedure(224-242)publicProcedure(91-91)apps/web/src/server/nanoid.ts (1)
nanoid(8-11)apps/web/src/server/service/storage-service.ts (1)
getDocumentUploadUrl(38-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
apps/web/src/server/api/routers/campaign.ts (6)
29-29: LGTM! Search functionality enhances campaign discoverability.The search implementation correctly applies case-insensitive filtering on both campaign name and subject fields. The additional fields in the response (scheduledAt, total, sent, delivered, unsubscribed) properly support the new batch campaign features.
Also applies to: 45-60, 74-78
166-168: LGTM! Proper delegation to service layer.Moving the deletion logic to the service layer improves separation of concerns and maintainability.
197-222: Verify the ordering logic aligns with "latest" intent.The endpoint orders by
updatedAtfirst, thencreatedAt. This prioritizes recently updated emails (e.g., status changes) over recently created ones. If "latest" should mean "most recently created or scheduled," consider ordering bycreatedAt DESCorscheduledAt DESCinstead.
231-233: LGTM! Proper delegation to service layer.The mutation correctly delegates re-subscription logic to the service layer.
271-277: LGTM! Correctly delegates pause operation.The mutation properly uses the campaign from context and delegates to the service layer.
279-285: LGTM! Correctly delegates resume operation.The mutation properly uses the campaign from context and delegates to the service layer.
| model CampaignEmail { | ||
| campaignId String | ||
| contactId String | ||
| emailId String | ||
| createdAt DateTime @default(now()) | ||
| @@id([campaignId, contactId]) | ||
| } |
There was a problem hiding this comment.
Add foreign keys for CampaignEmail to enforce integrity.
CampaignEmail currently stores plain IDs without @relation bindings. Without FK constraints, deleting a campaign/contact/email leaves orphaned rows, so dedupe checks will falsely treat those contacts as already processed forever.
Define relations with onDelete: Cascade (and a unique constraint on emailId if needed) so these records stay consistent when parents are removed. Based on learnings.
🤖 Prompt for AI Agents
In apps/web/prisma/schema.prisma around lines 270-277, CampaignEmail currently
stores plain IDs without relations; add explicit Prisma relations to Campaign,
Contact, and Email with fields mapping and references to their primary key
(e.g., id) and set onDelete: Cascade so child rows are removed when parents are
deleted; update the model to include the relation fields (campaign Campaign
@relation(fields: [campaignId], references: [id], onDelete: Cascade), contact
Contact @relation(fields: [contactId], references: [id], onDelete: Cascade),
email Email @relation(fields: [emailId], references: [id], onDelete: Cascade))
and keep campaignId/contactId/emailId as String fields, and if you need emailId
uniqueness add @@unique([emailId]) or change the @@id composite accordingly to
enforce the intended uniqueness/primary key behavior.
| const [json, setJson] = useState<Record<string, any> | undefined>( | ||
| campaign.content ? JSON.parse(campaign.content) : undefined, | ||
| campaign.content ? JSON.parse(campaign.content) : undefined | ||
| ); | ||
| const [isSaving, setIsSaving] = useState(false); |
There was a problem hiding this comment.
Guard JSON parsing to prevent editor crashes.
JSON.parse(campaign.content) will throw if the stored content is invalid JSON (e.g., manual DB edits or legacy data). That exception bubbles during render and white-screens the entire editor.
Wrap the parse in a try/catch (or use a safe parser) and fall back to undefined, optionally surfacing a toast so the page stays usable.
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx around
lines 101 to 104, the direct call to JSON.parse(campaign.content) can throw and
crash the editor; wrap the parse in a try/catch (or use a safe JSON parser) when
initializing state so that on parse errors you return undefined (or a default
object) instead of letting the exception bubble, setJson should still work as
before, and optionally trigger a non-blocking toast/console.warn to surface the
invalid content to the user/admin.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
40-42: Critical: invalid/empty input still schedules “now” via fallbacks. Keep selectedDate null and block submit.Current fallbacks to new Date() make “Schedule” act like “Send Now” on parse failure or empty input.
Apply this diff to remove “now” fallbacks and require a valid date:
@@ - const [selectedDate, setSelectedDate] = useState<Date | null>( - initialScheduledAtDate ?? new Date() - ); + const [selectedDate, setSelectedDate] = useState<Date | null>( + initialScheduledAtDate ?? null + ); @@ - const now = new Date(); - setSelectedDate(now); + setSelectedDate(null); setScheduleInput(""); @@ const onScheduleInputChange = (input: string) => { setScheduleInput(input); if (error) setError(null); const parsed = chrono.parseDate(input); if (parsed) { setSelectedDate(parsed); } else { - setSelectedDate(new Date()); + setSelectedDate(null); } }; @@ - <Calendar + <Calendar mode="single" - selected={selectedDate ?? new Date()} + selected={selectedDate ?? undefined} onSelect={(d) => { if (d) setDatePreserveTime(d); }} className="rounded-md border w-[250px] h-[300px] shrink-0 font-mono" />Optionally, disable the “Schedule” button when selectedDate is null and scheduleInput doesn’t parse to a date.
Also applies to: 61-64, 103-112, 200-206
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
103-105: Guard JSON.parse to avoid editor crash on invalid content.Parsing can throw and white-screen the page; initialize state safely.
Apply:
- const [json, setJson] = useState<Record<string, any> | undefined>( - campaign.content ? JSON.parse(campaign.content) : undefined - ); + const [json, setJson] = useState<Record<string, any> | undefined>(() => { + try { + return campaign.content ? JSON.parse(campaign.content) : undefined; + } catch (e) { + console.error("Invalid campaign.content JSON", e); + toast.error("Invalid campaign content. Resetting to empty."); + return undefined; + } + });apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (1)
19-19: Fix import: TogglePauseCampaign is a named export and use app alias.Default import will break; switch to named import with "~/”.
As per coding guidelines
-import TogglePauseCampaign from "../toggle-pause-campaign"; +import { TogglePauseCampaign } from "~/app/(dashboard)/campaigns/toggle-pause-campaign";
🧹 Nitpick comments (5)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (3)
44-45: Use app alias for ScheduleCampaign import.Prefer the "~/” alias in apps/web.
As per coding guidelines
-import ScheduleCampaign from "../../schedule-campaign"; +import ScheduleCampaign from "~/app/(dashboard)/campaigns/schedule-campaign";
288-296: Make replyTo a controlled input (avoid undefined).Ensure value is a string.
- <input + <input type="text" - value={replyTo} + value={replyTo ?? ""} onChange={(e) => { setReplyTo(e.target.value); }}
145-146: Minor: remove debug log.Avoid console noise in production.
- console.log("file type: ", file.type); + // noopapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (2)
20-20: Use app alias for CampaignStatusBadge import.As per coding guidelines
-import CampaignStatusBadge from "../../campaigns/campaign-status-badge"; +import CampaignStatusBadge from "~/app/(dashboard)/campaigns/campaign-status-badge";
128-128: Prefer enum over string literal for status checks.Reduces typo risk.
- {campaign.status === "SCHEDULED" ? ( + {campaign.status === CampaignStatus.SCHEDULED ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx(12 hunks)apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx(4 hunks)apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx(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)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript as the primary language; prefer .ts/.tsx over JavaScript
Use 2-space indentation and semicolons; rely on Prettier for enforcement
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase (e.g., AppSideBar.tsx)
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: In apps/web, use the/ alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC for API communication unless explicitly instructed otherwise
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use dynamic imports; always place imports at the top of the file
Files:
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsxapps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx
🧬 Code graph analysis (2)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
apps/web/src/app/(dashboard)/campaigns/schedule-campaign.tsx (1)
ScheduleCampaign(27-312)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx (3)
apps/web/src/app/(dashboard)/campaigns/campaign-status-badge.tsx (1)
CampaignStatusBadge(7-36)apps/web/src/app/(dashboard)/campaigns/toggle-pause-campaign.tsx (1)
TogglePauseCampaign(10-95)apps/web/src/app/(dashboard)/emails/email-status-badge.tsx (1)
EmailStatusBadge(3-39)
| {campaign.status === "SCHEDULED" ? ( | ||
| <Link href={`/campaigns/${campaign.id}/edit`}> | ||
| <Button>Edit</Button> | ||
| </Link> | ||
| ) : ( | ||
| <TogglePauseCampaign campaign={campaign} mode="full" /> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Hide Pause/Resume for DRAFT/SENT; show only for RUNNING/PAUSED (Edit for SCHEDULED).
Prevents exposing invalid actions.
- {campaign.status === "SCHEDULED" ? (
- <Link href={`/campaigns/${campaign.id}/edit`}>
- <Button>Edit</Button>
- </Link>
- ) : (
- <TogglePauseCampaign campaign={campaign} mode="full" />
- )}
+ {campaign.status === CampaignStatus.SCHEDULED ? (
+ <Link href={`/campaigns/${campaign.id}/edit`}>
+ <Button>Edit</Button>
+ </Link>
+ ) : campaign.status === CampaignStatus.RUNNING ||
+ campaign.status === CampaignStatus.PAUSED ? (
+ <TogglePauseCampaign campaign={campaign} mode="full" />
+ ) : 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.
| {campaign.status === "SCHEDULED" ? ( | |
| <Link href={`/campaigns/${campaign.id}/edit`}> | |
| <Button>Edit</Button> | |
| </Link> | |
| ) : ( | |
| <TogglePauseCampaign campaign={campaign} mode="full" /> | |
| )} | |
| </div> | |
| {campaign.status === CampaignStatus.SCHEDULED ? ( | |
| <Link href={`/campaigns/${campaign.id}/edit`}> | |
| <Button>Edit</Button> | |
| </Link> | |
| ) : campaign.status === CampaignStatus.RUNNING || | |
| campaign.status === CampaignStatus.PAUSED ? ( | |
| <TogglePauseCampaign campaign={campaign} mode="full" /> | |
| ) : null} | |
| </div> |
🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/campaigns/[campaignId]/page.tsx around lines 128
to 135, the UI currently shows Edit for SCHEDULED and TogglePauseCampaign
otherwise; update the conditional rendering so Edit is shown only when
campaign.status === "SCHEDULED" and the pause/resume TogglePauseCampaign is
shown only when campaign.status is "RUNNING" or "PAUSED" (hide it for "DRAFT"
and "SENT"). Replace the current ternary with explicit checks (if SCHEDULED ->
Link/Edit; else if RUNNING or PAUSED -> TogglePauseCampaign; otherwise render
nothing) and ensure types/strings match the campaign status enum used across the
codebase.
1 participant
Summary by CodeRabbit