Fix partner account merge failing on overlapping program enrollments#4047
Fix partner account merge failing on overlapping program enrollments#4047pepeladeira wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe partner account merge logic is migrated from a synchronous cron handler to an Upstash Workflow-backed API route with durable steps. The new route validates input, loads a merge plan to compute overlapping vs. transfer program enrollments, processes each enrollment with idempotent durable steps, transfers bounties and syncs links/commissions, deletes source user and cleanup fraud events, removes the source partner record, and sends notifications. The workflow is triggered via ChangesPartner Account Merge Workflow Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/email/src/templates/partner-account-merge-failed.tsx (1)
17-21: 💤 Low valueConsider a more generic default email value.
The default email
"panic@thedis.co"appears to be a test/placeholder value. While this default is unlikely to be used in production (since the route always passes the actualtargetEmail), consider using a more generic placeholder like"user@example.com"or adding a comment to clarify this is for development/testing only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/email/src/templates/partner-account-merge-failed.tsx` around lines 17 - 21, The default email value in the PartnerAccountMergeFailed function parameter is set to a test/placeholder value "panic@thedis.co" which appears unprofessional. Either replace this default email with a more generic placeholder value like "user@example.com", or add a comment above the function explaining that this default is for development/testing purposes only and is not used in production since the actual targetEmail is always passed by the route.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/app/`(ee)/api/cron/partners/merge-accounts/route.ts:
- Around line 549-573: The PartnerAccountMergeFailed email template always
indicates that no data was merged, but when mergedProgramIds.length > 0, some
enrollments have already been transferred before the failure. Pass
mergedProgramIds to the PartnerAccountMergeFailed template component (in the
react prop of the sendEmail call) so it can conditionally display different
messaging indicating that a partial merge occurred and some data was
transferred, or alternatively use a separate email template when
mergedProgramIds.length > 0 to accurately reflect the partial merge scenario to
the user.
In `@packages/email/src/templates/partner-account-merge-failed.tsx`:
- Around line 42-45: The email message in the Text component (lines 42-45)
incorrectly claims "no data was merged or deleted" unconditionally, but the code
shows that partial merges are possible and explicitly tracked via
mergedProgramIds. Update the email template to conditionally display different
messages based on whether a partial merge occurred: if mergedProgramIds contains
transferred enrollments, the message should inform the user which programs were
merged and suggest they contact support; if no transfers occurred, use the
current message. Alternatively, confirm through code review that errors can only
occur before any transfers begin, in which case remove the partial-merge
tracking logic (mergedProgramIds population and the conditional check at line
550) to eliminate the contradiction.
---
Nitpick comments:
In `@packages/email/src/templates/partner-account-merge-failed.tsx`:
- Around line 17-21: The default email value in the PartnerAccountMergeFailed
function parameter is set to a test/placeholder value "panic@thedis.co" which
appears unprofessional. Either replace this default email with a more generic
placeholder value like "user@example.com", or add a comment above the function
explaining that this default is for development/testing purposes only and is not
used in production since the actual targetEmail is always passed by the route.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a4872ec-8b08-4424-bf40-c993ed2e1b8f
📒 Files selected for processing (2)
apps/web/app/(ee)/api/cron/partners/merge-accounts/route.tspackages/email/src/templates/partner-account-merge-failed.tsx
|
@coderabbitai full review. any breaking changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== merge-accounts route ==="
cat apps/web/app/\(ee\)/api/cron/partners/merge-accounts/route.ts
echo ""
echo "=== partner-account-merge-failed email template ==="
cat packages/email/src/templates/partner-account-merge-failed.tsx
echo ""
echo "=== Check email exports/index ==="
fd "index" packages/email/src --max-depth 2 -e ts -e tsx | head -5 | xargs cat 2>/dev/null || true
echo ""
echo "=== Check if the email template is exported ==="
rg "PartnerAccountMergeFailed\|merge-failed\|merge_failed" packages/email/src --include="*.ts" --include="*.tsx" -nLength of output: 21336 🏁 Script executed: #!/bin/bash
echo "=== Look for existing email sends in the route for pattern reference ==="
rg "sendEmail\|from.*email\|PartnerAccount" apps/web/app/\(ee\)/api/cron/partners/merge-accounts/ -n -C2 2>/dev/null || true
echo ""
echo "=== Check how other email templates are exported ==="
cat packages/email/src/index.ts 2>/dev/null || fd "index" packages/email/src -e ts | head -3 | xargs cat 2>/dev/null || true
echo ""
echo "=== Check for ProgramEnrollment model fields (especially applicationId and tenantId) ==="
rg "applicationId\|tenantId" apps/web/app/\(ee\)/api/cron/partners/merge-accounts/route.ts -n || true
echo ""
echo "=== Check prisma schema for ProgramEnrollment ==="
rg "model ProgramEnrollment" -A 40 --include="*.prisma" -r . || true
echo ""
echo "=== Check for any existing tests for this cron route ==="
fd "merge-accounts\|merge_accounts" --include="*.test.*" --include="*.spec.*" . 2>/dev/null || fd "merge" . --max-depth 8 -e test.ts -e spec.ts 2>/dev/null | head -10 || trueLength of output: 3487 🏁 Script executed: #!/bin/bash
echo "=== Full route file ==="
cat "apps/web/app/(ee)/api/cron/partners/merge-accounts/route.ts"Length of output: 16817 🏁 Script executed: #!/bin/bash
# Use find instead
find . -path "*/cron/partners/merge-accounts/route.ts" -exec cat {} \;Length of output: 16786 🏁 Script executed: #!/bin/bash
# Try different quoting
file_path=$(find . -name "route.ts" -path "*/merge-accounts/*" 2>/dev/null | head -1)
echo "Found: $file_path"
cat "$file_path"Length of output: 16917 🏁 Script executed: #!/bin/bash
find . -name "route.ts" -path "*merge*" 2>/dev/null
find . -name "*.tsx" -path "*merge-failed*" 2>/dev/nullLength of output: 290 🏁 Script executed: #!/bin/bash
ls apps/web/app/ 2>/dev/null | head -20
ls apps/web/app/api/ 2>/dev/null | head -20
find apps -type d -name "merge-accounts" 2>/dev/null
find packages/email -name "*merge*" 2>/dev/nullLength of output: 819 🏁 Script executed: #!/bin/bash
# Check git diff for the PR
git diff origin/main --name-only 2>/dev/null || git log --oneline -5
git show --stat HEAD 2>/dev/null | head -20Length of output: 899 🏁 Script executed: #!/bin/bash
git log --oneline -3
git diff HEAD~1 --name-only 2>/dev/nullLength of output: 332 🏁 Script executed: #!/bin/bash
git log --oneline -5
git diff HEAD~1 HEAD -- . 2>/dev/null | head -500Length of output: 1574 🏁 Script executed: #!/bin/bash
git show HEAD --statLength of output: 356 🏁 Script executed: #!/bin/bash
# Try to get the PR diff via GitHub CLI
gh pr diff 4047 --repo dubinc/dub 2>/dev/null | head -800Length of output: 20178 🏁 Script executed: #!/bin/bash
gh pr view 4047 --repo dubinc/dub --json files 2>/dev/null | jq '.files[].path' 2>/dev/null || true
gh pr diff 4047 --repo dubinc/dub 2>/dev/null | head -1000Length of output: 20365 🏁 Script executed: #!/bin/bash
gh pr diff 4047 --repo dubinc/dub 2>/dev/nullLength of output: 20126
PR Review: Fix partner account merge failing on overlapping program enrollmentsSummary of Changes
Breaking ChangesNo breaking changes. The public contract is identical:
Root Cause Fixed ✅The old code deleted the source if (sourceEnrollment.applicationId) {
await tx.programEnrollment.update({
where: { id: sourceEnrollment.id },
data: { applicationId: null },
});
}
await tx.programEnrollment.delete({ where: { id: sourceEnrollment.id } });Issues Found1. 🔴 Stale
|
| Behavior | Before | After |
|---|---|---|
| Overlapping program enrollments | ❌ Crashed (FK violation) | ✅ Handled correctly |
| Data transfer for overlapping programs | ❌ Skipped | ✅ Done |
applicationId cleanup before delete |
❌ Missing | ✅ Added |
tenantId uniqueness check |
❌ Missing | ✅ Added |
| Alert on failure | Silent | mention: true (pings team) |
| Failure email | None | Sent to targetEmail |
| Partial merge forensics | Basic error message | Detailed log with program IDs |
Email Template (partner-account-merge-failed.tsx) ✅
Clean and consistent with the existing partner-account-merged template. Copy is clear and actionable. The panic@thedis.co default is an internal-only fallback — no concern.
Overall Assessment
The core bug fix is correct and the architecture (sequential per-enrollment transactions with an in-transaction re-read of targetEnrollment) is sound. The main concerns are around partial-merge retry safety (#2) and stale source enrollment data (#1), which are related and could be addressed together by re-fetching sourceEnrollment inside the transaction. These are edge cases and don't affect the primary bug fix.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/(ee)/api/cron/partners/merge-accounts/route.ts (1)
278-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-fetch
sourceEnrollmentinside the transaction to avoid stale-data bugs.The
sourceEnrollmentobject is fetched outside the transaction (line 250) but used inside (lines 291, 299). If another process modifies or deletes this enrollment between the snapshot and execution, the code operates on stale data—leading to incorrect status comparisons, wrongapplicationIdchecks, or tenantId mismatches.Additionally, on retry after a mid-loop failure, already-deleted source enrollments would cause "record not found" errors since there's no existence check.
🔧 Suggested fix: re-fetch and guard inside the transaction
await prisma.$transaction(async (tx) => { + // Re-fetch source enrollment to ensure it still exists and get fresh data + const freshSourceEnrollment = await tx.programEnrollment.findUnique({ + where: { id: sourceEnrollment.id }, + }); + + if (!freshSourceEnrollment) { + // Already processed in a previous run or deleted externally + console.log(`Source enrollment ${sourceEnrollment.id} no longer exists, skipping`); + return; + } + const targetEnrollment = await tx.programEnrollment.findUnique({ where: { partnerId_programId: { partnerId: mergeTargetPartnerId, - programId: sourceEnrollment.programId, + programId: freshSourceEnrollment.programId, }, }, }); if (targetEnrollment) { mergedAsOverlap = true; await mergeOverlappingProgramEnrollment(tx, { - sourceEnrollment, + sourceEnrollment: freshSourceEnrollment, targetEnrollment, mergeSourcePartnerId, mergeTargetPartnerId, }); return; } await transferProgramEnrollment(tx, { - sourceEnrollment, + sourceEnrollment: freshSourceEnrollment, mergeSourcePartnerId, mergeTargetPartnerId, }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/`(ee)/api/cron/partners/merge-accounts/route.ts around lines 278 - 304, The sourceEnrollment object is fetched outside the transaction but used inside the prisma.$transaction block, creating a race condition where concurrent processes could modify or delete it between the initial fetch and execution. Inside the transaction callback, before using sourceEnrollment (in the mergeOverlappingProgramEnrollment and transferProgramEnrollment calls), re-fetch it using tx.programEnrollment.findUnique with the same query parameters used to find targetEnrollment. Add a guard check to ensure the re-fetched sourceEnrollment exists; if it does not exist, return early or throw an appropriate error to handle the case where the enrollment was deleted between the initial query and transaction execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/app/`(ee)/api/cron/partners/merge-accounts/route.ts:
- Around line 278-304: The sourceEnrollment object is fetched outside the
transaction but used inside the prisma.$transaction block, creating a race
condition where concurrent processes could modify or delete it between the
initial fetch and execution. Inside the transaction callback, before using
sourceEnrollment (in the mergeOverlappingProgramEnrollment and
transferProgramEnrollment calls), re-fetch it using
tx.programEnrollment.findUnique with the same query parameters used to find
targetEnrollment. Add a guard check to ensure the re-fetched sourceEnrollment
exists; if it does not exist, return early or throw an appropriate error to
handle the case where the enrollment was deleted between the initial query and
transaction execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3249224c-2e25-42b7-8e65-5e862caf5d10
📒 Files selected for processing (1)
apps/web/app/(ee)/api/cron/partners/merge-accounts/route.ts
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/cron/qstash-workflow.ts (1)
52-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact merge emails before logging workflow failures.
This new workflow carries
sourceEmailandtargetEmail; loggingworkflowsor returning those fields incorrelationwrites raw email addresses into operational logs/alerts. Keep correlation useful without storing the emails directly.Proposed fix
- console.error("QStash workflow trigger failed", { error, workflows }); + console.error("QStash workflow trigger failed", { + error, + workflows: workflows.map((workflow) => ({ + workflowType: workflow.workflowType, + workflowLabel: workflow.workflowLabel, + correlation: getWorkflowConfig(workflow).correlation, + })), + });correlation: { userId: body.userId, - sourceEmail: body.sourceEmail, - targetEmail: body.targetEmail, + sourceEmailPresent: typeof body.sourceEmail === "string", + targetEmailPresent: typeof body.targetEmail === "string", },Also applies to: 110-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/cron/qstash-workflow.ts` at line 52, Remove sensitive email address fields from the workflows object before logging it in the console.error call at line 52 and before including it in any correlation data returned around lines 110-114. Create a redacted version of the workflows data that excludes sourceEmail and targetEmail fields, and use that redacted version for logging and correlation purposes instead of passing the raw workflows object directly to console.error or returning unredacted email fields in correlation data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/app/`(ee)/api/workflows/merge-partner-account/route.ts:
- Around line 437-443: The delete and update operations on enrollments in this
route only filter by enrollment id, which creates a race condition where another
process could reassign the enrollment to a different partner before the mutation
executes, causing the operation to affect the wrong partner's data. Add
sourcePartnerId as an additional WHERE clause condition to all database delete
and update operations that modify enrollments (at the lines mentioned and any
similar operations) to ensure mutations only affect enrollments that still
belong to the source partner, preventing unauthorized modifications to other
partners' enrollments.
- Around line 598-611: The Promise.allSettled call completes successfully even
when individual promises reject, preventing failed syncs from being retried.
After the Promise.allSettled resolves, check the results array for any rejected
promises from recordLink, linkCache.expireMany, or syncTotalCommissions calls,
and throw an error if any rejections are found so the durable step fails and can
be retried. This ensures post-merge sync failures in Tinybird, cache operations,
or commission calculations are not silently ignored.
- Around line 58-61: In the early return path where plan.proceed is false, the
merge cache is not being cleared, causing the cache entry for the user to remain
set and prevent clean retries. Before the return statement in the if
(!plan.proceed) block, add a cache clear operation using the key format
${CACHE_KEY_PREFIX}:${userId} to ensure the cache is properly cleaned up when
skipping the merge due to "not found" or "same account" conditions.
- Around line 733-749: The current try-catch block swallows the error from the
DELETE FROM Partner database operation, which causes the workflow to continue
and send success emails even when the deletion fails. Instead of catching and
returning a log for all errors in this block, restructure the code to let
database deletion errors propagate and fail the workflow while handling image
cleanup separately as a non-critical operation. Move the storage.delete call
into its own try-catch block that only logs errors without affecting the
workflow outcome, and either remove the try-catch from the Partner deletion or
rethrow the error after logging if you need to perform cleanup before the
deletion fails.
- Around line 187-221: The failureFunction in the merge-partner-account workflow
failure handler currently only clears cache and logs internally but does not
send a failure notification to the target email. Add code in the failureFunction
to send the failed-merge notification template to the target email address with
an idempotency key before the function returns. Extract the target email from
the context or inputSchema as needed, and ensure the notification is sent with
proper error context (such as correlation, workflowRunId, and failStatus) to
inform the partner that their merge operation failed.
In `@apps/web/lib/actions/partners/merge-partner-accounts.ts`:
- Around line 343-355: The triggerQStashWorkflow call does not check its return
value, which can be null when retries are exhausted, causing the action to
proceed as if the workflow was successfully enqueued when it actually failed.
Capture the return value from the triggerQStashWorkflow function call and check
if it returns null. If it does return null, throw an error to properly propagate
the failure to the caller so they know the merge workflow failed to enqueue
instead of silently continuing.
---
Outside diff comments:
In `@apps/web/lib/cron/qstash-workflow.ts`:
- Line 52: Remove sensitive email address fields from the workflows object
before logging it in the console.error call at line 52 and before including it
in any correlation data returned around lines 110-114. Create a redacted version
of the workflows data that excludes sourceEmail and targetEmail fields, and use
that redacted version for logging and correlation purposes instead of passing
the raw workflows object directly to console.error or returning unredacted email
fields in correlation data.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dd8747c-0056-4832-bb25-07ac8cb64db1
📒 Files selected for processing (4)
apps/web/app/(ee)/api/cron/partners/merge-accounts/route.tsapps/web/app/(ee)/api/workflows/merge-partner-account/route.tsapps/web/lib/actions/partners/merge-partner-accounts.tsapps/web/lib/cron/qstash-workflow.ts
💤 Files with no reviewable changes (1)
- apps/web/app/(ee)/api/cron/partners/merge-accounts/route.ts
…nup-source-account
|
@coderabbitai full review. any breaking changes or security issues? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get full PR diff
gh pr diff 4047 --repo dubinc/dub 2>/dev/nullLength of output: 51517 🏁 Script executed: #!/bin/bash
# Find and read the new workflow route
find . -path "*workflows/merge-partner-account/route.ts" 2>/dev/null | head -3
find . -path "*merge-partner-account*" 2>/dev/null
# Check how other workflow routes handle auth
find . -path "*workflows*" -name "route.ts" 2>/dev/null | head -10Length of output: 1082
Breaking ChangesNone. The public API contract is unchanged:
Security Review✅ Authentication on the new workflow routeThe new route uses ✅ E2E trigger endpoint is properly sandboxed
This endpoint cannot be used to trigger merges in production. ✅ SQL injection — safeawait conn.execute(`DELETE FROM Partner WHERE id = ?`, [sourcePartnerId]);Parameterized query. ✅ No sensitive data exposure
✅ Concurrent merge prevention
🟡
|
Summary by CodeRabbit