feat: add campaign data recovery from orphaned recipients#295
Merged
Conversation
Recovers lost campaign rows by scanning campaign_recipients for entries referencing non-existent campaigns, then reconstructing the campaign metadata (subject, body) via the Resend API and recomputing counters from recipient statuses. https://claude.ai/code/session_014Jp5Zrx4TWnCDGYwFyCDRG
- Add ON CONFLICT (id) DO NOTHING for safe retries on partial failure - Sanitize console.warn/error to avoid logging resend_id or raw error objects - Consolidate setval into single call after loop (avoid wasting sequence values) - Guard against all-pending recipients being marked as 'sent' (use 'draft') - Add 10 tests covering auth, recovery, Resend integration, and error paths https://claude.ai/code/session_014uzXrEL3FBvtvyMbu7jTy9
📝 WalkthroughWalkthroughAdds an admin-only campaign recovery feature: a frontend hook and button trigger a POST to a new backend route that finds orphaned campaign IDs from Changes
Sequence DiagramsequenceDiagram
actor Admin as Admin User
participant Client as React Client
participant Server as Backend API
participant DB as Database
participant Resend as Resend API
Admin->>Client: Click "Recover lost campaigns"
Client->>Server: POST /api/admin/campaigns/recover
Server->>Server: Verify auth & isPowerUser & owner
Server->>DB: SELECT orphaned campaign_ids FROM campaign_recipients
loop For each orphaned campaign
Server->>DB: Aggregate recipient counts & timestamps
Server->>DB: SELECT sample_nonnull_resend_id
alt resend_id exists
Server->>Resend: emails.get(resend_id)
alt Resend returns subject/html
Resend-->>Server: subject, html_body
else Resend fails
Resend-->>Server: error
Server->>Server: use placeholder subject/html
end
else no resend_id
Server->>Server: use placeholder subject/html
end
Server->>DB: INSERT INTO campaigns (...) ON CONFLICT(id) DO NOTHING
DB-->>Server: insert result (rowCount)
end
Server->>DB: setval('campaigns_id_seq', (SELECT MAX(id)))
DB-->>Server: seq updated
Server-->>Client: { recovered: n, campaigns: [...] }
Client->>Client: invalidate campaigns & dashboard queries
Client->>Admin: show toast (success or message)
Security & Correctness Flags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes.campaignRecover.test.ts`:
- Around line 397-440: The test suite lacks coverage for the idempotent retry
behavior of the INSERT with ON CONFLICT DO NOTHING; add a new test (e.g.,
it("skips already-recovered campaigns (idempotent retry)")) that uses
mockDbExecute and callCount to simulate: first call returning an orphaned
campaign id (e.g., 42), second call returning campaign stats, third call
returning { rows: [], rowCount: 0 } to represent the INSERT being a no-op, then
assert that callHandler("post", ENDPOINT, req) returns status 200, recovered ===
0 and campaigns length === 0; use the existing patterns for mockGetUser and
mockGetResendClient to mirror other tests.
In `@server/routes.ts`:
- Around line 2386-2393: The status assignment logic can mark campaigns as
"sent" even when there are failures; update the conditional using the same
variables (status, sentCount, failedCount, totalCount) so that: if sentCount ===
0 && failedCount === 0 set status = "draft"; else if failedCount > 0 set status
= "partially_sent" (regardless of sentCount + failedCount === totalCount);
otherwise set status = "sent". This ensures any failures cause "partially_sent"
instead of "sent".
- Around line 2316-2321: The error responses in the authorization block (where
you read req.user?.claims?.sub, call authStorage.getUser, check user.tier and
compare to APP_OWNER_ID) return JSON with only message; update each
res.status(...).json(...) to include a machine-readable code field per
guidelines (e.g. for missing auth use { message: "Unauthorized", code:
"unauthorized" }, for non-power tier use { message: "Admin access required",
code: "admin_required" }, and for non-owner use { message: "Owner access
required", code: "owner_required" }); keep existing status codes and messages
but add the code field consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4319d902-74d2-45d3-a8cf-e51feac4f2c8
📒 Files selected for processing (4)
client/src/hooks/use-campaigns.tsclient/src/pages/AdminCampaigns.tsxserver/routes.campaignRecover.test.tsserver/routes.ts
Comment on lines
+2316
to
+2321
| try { | ||
| const userId = req.user?.claims?.sub; | ||
| if (!userId) return res.status(401).json({ message: "Unauthorized" }); | ||
| const user = await authStorage.getUser(userId); | ||
| if (!user || user.tier !== "power") return res.status(403).json({ message: "Admin access required" }); | ||
| if (userId !== APP_OWNER_ID) return res.status(403).json({ message: "Owner access required" }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add code field to error responses per coding guidelines.
The error responses are missing the code field. Per coding guidelines, Express routes should return JSON errors with { message, code } format.
Suggested fix
- if (!userId) return res.status(401).json({ message: "Unauthorized" });
+ if (!userId) return res.status(401).json({ message: "Unauthorized", code: "UNAUTHORIZED" });
const user = await authStorage.getUser(userId);
- if (!user || user.tier !== "power") return res.status(403).json({ message: "Admin access required" });
- if (userId !== APP_OWNER_ID) return res.status(403).json({ message: "Owner access required" });
+ if (!user || user.tier !== "power") return res.status(403).json({ message: "Admin access required", code: "FORBIDDEN" });
+ if (userId !== APP_OWNER_ID) return res.status(403).json({ message: "Owner access required", code: "FORBIDDEN" });Also at line 2443:
- res.status(500).json({ message: "Failed to recover campaigns" });
+ res.status(500).json({ message: "Failed to recover campaigns", code: "INTERNAL_ERROR" });Based on learnings: "Express routes must be defined in server/routes.ts with JSON responses using { message, code } format for errors".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const userId = req.user?.claims?.sub; | |
| if (!userId) return res.status(401).json({ message: "Unauthorized" }); | |
| const user = await authStorage.getUser(userId); | |
| if (!user || user.tier !== "power") return res.status(403).json({ message: "Admin access required" }); | |
| if (userId !== APP_OWNER_ID) return res.status(403).json({ message: "Owner access required" }); | |
| try { | |
| const userId = req.user?.claims?.sub; | |
| if (!userId) return res.status(401).json({ message: "Unauthorized", code: "UNAUTHORIZED" }); | |
| const user = await authStorage.getUser(userId); | |
| if (!user || user.tier !== "power") return res.status(403).json({ message: "Admin access required", code: "FORBIDDEN" }); | |
| if (userId !== APP_OWNER_ID) return res.status(403).json({ message: "Owner access required", code: "FORBIDDEN" }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes.ts` around lines 2316 - 2321, The error responses in the
authorization block (where you read req.user?.claims?.sub, call
authStorage.getUser, check user.tier and compare to APP_OWNER_ID) return JSON
with only message; update each res.status(...).json(...) to include a
machine-readable code field per guidelines (e.g. for missing auth use { message:
"Unauthorized", code: "unauthorized" }, for non-power tier use { message: "Admin
access required", code: "admin_required" }, and for non-owner use { message:
"Owner access required", code: "owner_required" }); keep existing status codes
and messages but add the code field consistently.
…nt retry test Status logic previously marked campaigns as "sent" when failedCount > 0 but sentCount + failedCount === totalCount. Now any failures result in "partially_sent". Also adds test coverage for ON CONFLICT DO NOTHING idempotent retry behavior. https://claude.ai/code/session_014uzXrEL3FBvtvyMbu7jTy9
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
server/routes.ts (1)
2318-2321:⚠️ Potential issue | 🟡 MinorReturn
{ message, code }from the new error branches.Lines 2318-2321 and 2442-2443 still emit
{ message }only. That breaks the API error contract for this endpoint and forces downstream callers and tests to special-case it.Based on learnings, "Express routes must be defined in
server/routes.tswith JSON responses using{ message, code }format for errors".Also applies to: 2442-2443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes.ts` around lines 2318 - 2321, The three error branches in the route (checks around userId, authStorage.getUser() and the owner/tier checks using user.tier and APP_OWNER_ID) return JSON with only { message }, violating the endpoint's error contract; update each res.status(...).json(...) call in this block to return { message, code } instead (use appropriate error codes/strings consistent with other routes) so all failures (unauthorized, admin access required, owner access required) follow the { message, code } format expected by callers and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes.campaignRecover.test.ts`:
- Around line 182-190: ensureRoutes() currently calls registerRoutes(), which
starts the softDeleteCleanupInterval and leaves a live timer; after awaiting
registerRoutes(mockHttpServer, app) call stopRouteTimers() (the function
exported from "./routes") to clear the interval and prevent CI hangs. Do the
same fix in the other nearly identical block (the second ensureRoutes usage
referenced around the other occurrence) so both places call stopRouteTimers()
immediately after registerRoutes() returns.
- Around line 455-493: The current test only asserts the recovered campaign ID
but doesn't validate status logic; add a new assertion (or a new test case) that
covers the boundary where sent + failed === total with failed > 0 and assert the
route returns status "partially_sent". To make this reliable, extract the status
derivation into an exported helper (e.g., deriveCampaignStatus(stats) or export
the existing function in the campaign recovery route) and unit-test that helper
directly, then update the test in server/routes.campaignRecover.test.ts (use the
same mocks: mockDbExecute, mockGetUser, callHandler, ENDPOINT) to simulate stats
where total === sent + failed and failed > 0 and assert the returned campaign
status is "partially_sent".
In `@server/routes.ts`:
- Around line 2382-2393: The status logic in server/routes.ts incorrectly marks
campaigns as "sent" when there are pending recipients; update the if/else chain
that sets the status (using sentCount, failedCount, totalCount, status) to add a
branch before the final "sent" case that checks if totalCount > sentCount +
failedCount and sets status = "sending" (or another "unfinished" state), keeping
the existing "draft" and "partially_sent" branches intact so in-progress
recoveries are not considered complete.
- Line 2315: The route "/api/admin/campaigns/recover" is hardcoded in
server/routes.ts; add a new entry api.admin.campaigns.recover in
shared/routes.ts (including method, path, optional input schema and responses)
and update server/routes.ts to use that constant instead of the literal string
(the app.post call that currently uses "/api/admin/campaigns/recover" and its
isAuthenticated wrapper). Also update any client hook and tests that reference
the route to import and use api.admin.campaigns.recover so path, input and
response typings stay in sync.
- Around line 2324-2437: The route handler currently performs orphan discovery,
stats aggregation, Resend lookups, campaign inserts and sequence setval directly
(variables/functions: orphanRows/orphanedIds, getResendClient,
resend.emails.get, db.execute, recovered, setval), which must be moved behind
IStorage; implement a new IStorage method (e.g.
recoverOrphanedCampaigns(resendClient?: ResendClient):
Promise<Array<{id:number,name:string,subject:string,totalRecipients:number}>>)
inside server/storage.ts that encapsulates the SELECT of orphan campaign_ids,
the per-campaign stats aggregation, fetching sample email via resend.emails.get,
inserting campaigns with ON CONFLICT and advancing the campaigns_id_seq, and
then update the route to call
storage.recoverOrphanedCampaigns(getResendClient()) and return its result as
JSON (keep auth/HTTP concerns in the handler only). Ensure the storage
implementation uses existing DB helpers (db.execute/sql) and is unit-testable so
tests mock IStorage instead of db.execute call order.
---
Duplicate comments:
In `@server/routes.ts`:
- Around line 2318-2321: The three error branches in the route (checks around
userId, authStorage.getUser() and the owner/tier checks using user.tier and
APP_OWNER_ID) return JSON with only { message }, violating the endpoint's error
contract; update each res.status(...).json(...) call in this block to return {
message, code } instead (use appropriate error codes/strings consistent with
other routes) so all failures (unauthorized, admin access required, owner access
required) follow the { message, code } format expected by callers and tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37bfffa8-a066-4342-b894-bc5ed515e515
📒 Files selected for processing (2)
server/routes.campaignRecover.test.tsserver/routes.ts
Comment on lines
+182
to
+190
| async function ensureRoutes() { | ||
| if (routesRegistered) return; | ||
| process.env.APP_OWNER_ID = "owner-123"; | ||
| const { registerRoutes } = await import("./routes"); | ||
| const app = makeMockApp(); | ||
| const mockHttpServer = {} as any; | ||
| await registerRoutes(mockHttpServer, app as any); | ||
| routesRegistered = true; | ||
| } |
There was a problem hiding this comment.
Stop the route timers after registering server/routes.
ensureRoutes() executes registerRoutes(), and registerRoutes() starts softDeleteCleanupInterval. afterAll restores APP_OWNER_ID only, so this suite leaves a live interval behind and can hang or flap in CI. Call stopRouteTimers() here.
Proposed fix
-afterAll(() => {
+afterAll(async () => {
+ const { stopRouteTimers } = await import("./routes");
+ stopRouteTimers();
if (previousAppOwnerId === undefined) {
delete process.env.APP_OWNER_ID;
} else {
process.env.APP_OWNER_ID = previousAppOwnerId;
}
});Also applies to: 192-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes.campaignRecover.test.ts` around lines 182 - 190, ensureRoutes()
currently calls registerRoutes(), which starts the softDeleteCleanupInterval and
leaves a live timer; after awaiting registerRoutes(mockHttpServer, app) call
stopRouteTimers() (the function exported from "./routes") to clear the interval
and prevent CI hangs. Do the same fix in the other nearly identical block (the
second ensureRoutes usage referenced around the other occurrence) so both places
call stopRouteTimers() immediately after registerRoutes() returns.
Comment on lines
+455
to
+493
| it("sets status to partially_sent when there are failures and not all done", async () => { | ||
| mockGetUser.mockResolvedValue({ tier: "power" }); | ||
| mockGetResendClient.mockReturnValue(null); | ||
|
|
||
| // Call sequence: 1=orphans, 2=stats, 3=INSERT, 4=setval | ||
| let callCount = 0; | ||
| mockDbExecute.mockImplementation(() => { | ||
| callCount++; | ||
| if (callCount === 1) { | ||
| return Promise.resolve({ rows: [{ campaign_id: 55 }] }); | ||
| } | ||
| if (callCount === 2) { | ||
| // failed > 0 and sent + failed !== total → partially_sent | ||
| return Promise.resolve({ | ||
| rows: [{ | ||
| total: 10, sent: 5, failed: 2, delivered: 4, | ||
| opened: 1, clicked: 0, | ||
| first_sent: "2026-01-15T00:00:00Z", | ||
| last_sent: "2026-01-15T00:30:00Z", | ||
| }], | ||
| }); | ||
| } | ||
| if (callCount === 3) { | ||
| // INSERT | ||
| return Promise.resolve({ rows: [], rowCount: 1 }); | ||
| } | ||
| // setval | ||
| return Promise.resolve({ rows: [] }); | ||
| }); | ||
|
|
||
| const req = { user: { claims: { sub: "owner-123" } }, body: {} }; | ||
| const res = await callHandler("post", ENDPOINT, req); | ||
|
|
||
| expect(res._status).toBe(200); | ||
| expect(res._json.recovered).toBe(1); | ||
| // We can't directly check the DB INSERT args since db.execute is a generic mock, | ||
| // but we verify the endpoint succeeded and returned the campaign | ||
| expect(res._json.campaigns[0].id).toBe(55); | ||
| }); |
There was a problem hiding this comment.
This test never proves the partially_sent fix.
The fixture keeps sent + failed < total, and the only assertion is the recovered campaign ID. If the regression from this PR came back on the all-terminal branch (sent + failed === total with failures), this test would still pass. Add that boundary case and assert the derived status, ideally after extracting the status calculation out of the route.
As per coding guidelines, "Tests must cover edge cases and error paths - include assertions for edge cases (empty inputs, boundary values, null/undefined), error paths (invalid input, unauthorized access, not found), and security-relevant scenarios (ownership violations, SSRF attempts)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes.campaignRecover.test.ts` around lines 455 - 493, The current
test only asserts the recovered campaign ID but doesn't validate status logic;
add a new assertion (or a new test case) that covers the boundary where sent +
failed === total with failed > 0 and assert the route returns status
"partially_sent". To make this reliable, extract the status derivation into an
exported helper (e.g., deriveCampaignStatus(stats) or export the existing
function in the campaign recovery route) and unit-test that helper directly,
then update the test in server/routes.campaignRecover.test.ts (use the same
mocks: mockDbExecute, mockGetUser, callHandler, ENDPOINT) to simulate stats
where total === sent + failed and failed > 0 and assert the returned campaign
status is "partially_sent".
| // Recover campaigns whose rows were lost but whose recipients still exist. | ||
| // Uses Resend API to fetch subject/body from a sample recipient's resend_id, | ||
| // then reconstructs the campaign row with accurate counters. | ||
| app.post("/api/admin/campaigns/recover", isAuthenticated, async (req: any, res) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move the new endpoint contract into @shared/routes.
Line 2315 hardcodes a new client/server contract in server/routes.ts. That bypasses the shared api definition, so the client hook, server handler, and tests can drift on path, input schema, and response typing. Please add api.admin.campaigns.recover in shared/routes.ts and consume that constant here and downstream.
As per coding guidelines, "Define route constants in the api object in shared/routes.ts with method, path, responses, and optional input. Never hardcode route path strings like '/api/monitors' in server or client code. Reference these constants throughout the codebase."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes.ts` at line 2315, The route "/api/admin/campaigns/recover" is
hardcoded in server/routes.ts; add a new entry api.admin.campaigns.recover in
shared/routes.ts (including method, path, optional input schema and responses)
and update server/routes.ts to use that constant instead of the literal string
(the app.post call that currently uses "/api/admin/campaigns/recover" and its
isAuthenticated wrapper). Also update any client hook and tests that reference
the route to import and use api.admin.campaigns.recover so path, input and
response typings stay in sync.
Comment on lines
+2324
to
+2437
| const orphanRows = await db.execute(sql` | ||
| SELECT DISTINCT cr.campaign_id | ||
| FROM campaign_recipients cr | ||
| LEFT JOIN campaigns c ON c.id = cr.campaign_id | ||
| WHERE c.id IS NULL | ||
| `); | ||
|
|
||
| const orphanedIds = (orphanRows.rows as { campaign_id: number }[]).map(r => r.campaign_id); | ||
| if (orphanedIds.length === 0) { | ||
| return res.json({ recovered: 0, campaigns: [], message: "No orphaned recipients found — campaign data appears intact." }); | ||
| } | ||
|
|
||
| const resend = getResendClient(); | ||
| const recovered: Array<{ id: number; name: string; subject: string; totalRecipients: number }> = []; | ||
|
|
||
| for (const campaignId of orphanedIds) { | ||
| // Aggregate counters from recipient rows | ||
| const statsResult = await db.execute(sql` | ||
| SELECT | ||
| COUNT(*)::int AS total, | ||
| COUNT(*) FILTER (WHERE status IN ('sent','delivered','opened','clicked'))::int AS sent, | ||
| COUNT(*) FILTER (WHERE status = 'failed' OR status = 'bounced' OR status = 'complained')::int AS failed, | ||
| COUNT(*) FILTER (WHERE status IN ('delivered','opened','clicked'))::int AS delivered, | ||
| COUNT(*) FILTER (WHERE status IN ('opened','clicked'))::int AS opened, | ||
| COUNT(*) FILTER (WHERE status = 'clicked')::int AS clicked, | ||
| MIN(sent_at) AS first_sent, | ||
| MAX(sent_at) AS last_sent | ||
| FROM campaign_recipients | ||
| WHERE campaign_id = ${campaignId} | ||
| `); | ||
|
|
||
| const stats = statsResult.rows[0] as any; | ||
|
|
||
| // Try to recover subject/body from Resend API using a sample resend_id | ||
| let subject = `Recovered Campaign #${campaignId}`; | ||
| let htmlBody = "<p>(Email body could not be recovered)</p>"; | ||
|
|
||
| if (resend) { | ||
| const sampleRow = await db.execute(sql` | ||
| SELECT resend_id FROM campaign_recipients | ||
| WHERE campaign_id = ${campaignId} AND resend_id IS NOT NULL | ||
| LIMIT 1 | ||
| `); | ||
|
|
||
| const sampleResendId = (sampleRow.rows[0] as { resend_id: string } | undefined)?.resend_id; | ||
| if (sampleResendId) { | ||
| try { | ||
| const emailData = await resend.emails.get(sampleResendId); | ||
| if (emailData.data) { | ||
| subject = emailData.data.subject ?? subject; | ||
| htmlBody = emailData.data.html ?? htmlBody; | ||
| } | ||
| } catch (e) { | ||
| console.warn(`[CampaignRecover] Could not fetch sample email for campaign ${campaignId}:`, e instanceof Error ? e.message : "unknown error"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Determine status from counters | ||
| const sentCount = Number(stats.sent); | ||
| const failedCount = Number(stats.failed); | ||
| const totalCount = Number(stats.total); | ||
| let status: string; | ||
| if (sentCount === 0 && failedCount === 0) { | ||
| status = "draft"; | ||
| } else if (failedCount > 0) { | ||
| status = "partially_sent"; | ||
| } else { | ||
| status = "sent"; | ||
| } | ||
|
|
||
| // Re-insert the campaign row with its original ID using raw SQL | ||
| // so that the foreign key from campaign_recipients is satisfied again. | ||
| // ON CONFLICT DO NOTHING makes retries safe if a prior attempt partially completed. | ||
| const insertResult = await db.execute(sql` | ||
| INSERT INTO campaigns (id, name, subject, html_body, status, type, total_recipients, | ||
| sent_count, failed_count, delivered_count, opened_count, clicked_count, | ||
| created_at, sent_at, completed_at) | ||
| VALUES ( | ||
| ${campaignId}, | ||
| ${`Recovered Campaign #${campaignId}`}, | ||
| ${subject}, | ||
| ${htmlBody}, | ||
| ${status}, | ||
| 'manual', | ||
| ${Number(stats.total)}, | ||
| ${Number(stats.sent)}, | ||
| ${Number(stats.failed)}, | ||
| ${Number(stats.delivered)}, | ||
| ${Number(stats.opened)}, | ||
| ${Number(stats.clicked)}, | ||
| COALESCE(${stats.first_sent}::timestamptz, NOW()), | ||
| ${stats.first_sent ?? null}::timestamptz, | ||
| ${stats.last_sent ?? null}::timestamptz | ||
| ) | ||
| ON CONFLICT (id) DO NOTHING | ||
| `); | ||
|
|
||
| // Skip if already recovered (idempotent retry) | ||
| if (insertResult.rowCount === 0) continue; | ||
|
|
||
| recovered.push({ | ||
| id: campaignId, | ||
| name: `Recovered Campaign #${campaignId}`, | ||
| subject, | ||
| totalRecipients: Number(stats.total), | ||
| }); | ||
| } | ||
|
|
||
| // Advance the serial sequence past all recovered IDs so future inserts don't collide | ||
| if (recovered.length > 0) { | ||
| await db.execute(sql` | ||
| SELECT setval('campaigns_id_seq', (SELECT MAX(id) FROM campaigns)) | ||
| `); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Push the recovery SQL behind IStorage.
Lines 2324-2437 do orphan discovery, stats aggregation, inserts, and sequence repair directly in the route. That violates the storage boundary and is why the tests have to mock brittle db.execute call order. Move this flow into server/storage.ts or a dedicated service and keep the handler focused on auth + HTTP mapping.
As per coding guidelines, "Never put database queries or Drizzle ORM calls directly in route handlers — all database access must go through methods on the IStorage interface implemented in server/storage.ts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes.ts` around lines 2324 - 2437, The route handler currently
performs orphan discovery, stats aggregation, Resend lookups, campaign inserts
and sequence setval directly (variables/functions: orphanRows/orphanedIds,
getResendClient, resend.emails.get, db.execute, recovered, setval), which must
be moved behind IStorage; implement a new IStorage method (e.g.
recoverOrphanedCampaigns(resendClient?: ResendClient):
Promise<Array<{id:number,name:string,subject:string,totalRecipients:number}>>)
inside server/storage.ts that encapsulates the SELECT of orphan campaign_ids,
the per-campaign stats aggregation, fetching sample email via resend.emails.get,
inserting campaigns with ON CONFLICT and advancing the campaigns_id_seq, and
then update the route to call
storage.recoverOrphanedCampaigns(getResendClient()) and return its result as
JSON (keep auth/HTTP concerns in the handler only). Ensure the storage
implementation uses existing DB helpers (db.execute/sql) and is unit-testable so
tests mock IStorage instead of db.execute call order.
Comment on lines
+2382
to
+2393
| // Determine status from counters | ||
| const sentCount = Number(stats.sent); | ||
| const failedCount = Number(stats.failed); | ||
| const totalCount = Number(stats.total); | ||
| let status: string; | ||
| if (sentCount === 0 && failedCount === 0) { | ||
| status = "draft"; | ||
| } else if (failedCount > 0) { | ||
| status = "partially_sent"; | ||
| } else { | ||
| status = "sent"; | ||
| } |
There was a problem hiding this comment.
Pending recipients are being recovered as a completed campaign.
If totalCount > sentCount + failedCount and failedCount === 0, Lines 2387-2392 fall through to status = "sent". That marks an in-progress recovery as complete even though some recipient rows are still pending. Add an unfinished-state branch (e.g. sending) before the terminal sent case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes.ts` around lines 2382 - 2393, The status logic in
server/routes.ts incorrectly marks campaigns as "sent" when there are pending
recipients; update the if/else chain that sets the status (using sentCount,
failedCount, totalCount, status) to add a branch before the final "sent" case
that checks if totalCount > sentCount + failedCount and sets status = "sending"
(or another "unfinished" state), keeping the existing "draft" and
"partially_sent" branches intact so in-progress recoveries are not considered
complete.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an owner-only admin endpoint to recover campaign rows that were lost while their recipient data still exists in the database. The endpoint scans for orphaned
campaign_recipientsrecords, aggregates stats, optionally recovers email subject/body from the Resend API, and reconstructs campaign rows with accurate counters.Changes
Server (
server/routes.ts)POST /api/admin/campaigns/recoverendpoint with triple-gated auth (isAuthenticated + power tier + APP_OWNER_ID)INSERT ... ON CONFLICT (id) DO NOTHINGfor idempotent retriessetval('campaigns_id_seq', (SELECT MAX(id) FROM campaigns))after loopClient (
client/src/hooks/use-campaigns.ts,client/src/pages/AdminCampaigns.tsx)useRecoverCampaigns()mutation hookTests (
server/routes.campaignRecover.test.ts)How to test
POST /api/admin/campaigns/recoverwithout auth → 401; with non-power user → 403; with non-owner → 403{ recovered: 0, campaigns: [] }{ recovered: 0 }(no duplicates)npm run check && npm run test→ all 1855 tests passhttps://claude.ai/code/session_014uzXrEL3FBvtvyMbu7jTy9
Summary by CodeRabbit
New Features
Tests