Improve reward sync cron job#3944
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors reward handling into a versioned, batched job pipeline: new job schema and queueing, Redis versioning, a processing cron, orphaned-rewards cleanup, action handlers now enqueue jobs, and partner notification/email templates updated. ChangesReward Processing Pipeline Refactor
Sequence Diagram(s)sequenceDiagram
participant Action as Reward Action
participant Queue as queueRewardProcessing
participant Redis as Redis
participant Cron as Process Cron
participant Prisma as Prisma
participant Notify as notifyPartnerRewardChange
participant Email as queueBatchEmail
Action->>Queue: queueRewardProcessing(event, groupId, rewardSnapshot)
Queue->>Redis: incrementRewardVersion(groupId,event)
Queue->>Cron: publish job to /api/cron/rewards/process (includes version)
Cron->>Prisma: fetch reward & partnerGroup
Cron->>Redis: isStaleRewardVersion(version)
Cron->>Prisma: query enrollments (max 300)
alt enrollments exist
Cron->>Prisma: updateMany (create/delete)
Cron->>Notify: notifyPartnerRewardChange(...)
Notify->>Email: queueBatchEmail(user, PartnerRewardUpdated)
Cron->>Queue: enqueue next batch
else no more enrollments
Cron->>Prisma: delete reward (if reward-deleted)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
Use Redis reward versions to skip stale cron jobs, process enrollments in batches of 300, and add email idempotency keys.
|
@coderabbitai full review please |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch PR details
gh pr view 3944 --repo dubinc/dub --json title,body,files,additions,deletions,commits
echo "=== PR DIFF ==="
gh pr diff 3944 --repo dubinc/dubLength of output: 46461
Review: Improve reward sync cron jobSummaryThis PR replaces the old 🔴 High
Partner users are fetched without a // route.ts — programEnrollments query
partner: {
users: {
select: {
user: {
select: {
name: true,
email: true,
},
},
},
+ // Add filter:
+ where: {
+ user: {
+ email: {
+ not: null,
+ },
+ },
+ },
},
},Or alternatively filter in const validUsers = users.filter((u): u is typeof u & { email: string } => u.email != null);🟡 Medium
// Before
export type RewardJob = z.input<typeof rewardJobSchema>;
// After — use output type so defaults are baked in
export type RewardJob = z.infer<typeof rewardJobSchema>;
// Then the version check becomes simply:
const version =
params.version !== 1 // or store a sentinel like undefined separatelyOr keep two distinct types — one for the queue call (input, version omittable) and one for the parsed job body (output, version always set).
If many rewards become orphaned at once (e.g. a mass delete-then-create pattern), cleaning them up at 10 per cron run could be very slow. Consider adding an // Add ordering for determinism
orderBy: {
updatedAt: "asc",
},
take: 50, // or 100
The notification emails are sent before if (shouldNotify) {
await notifyPartnerRewardChange({ ... }); // emails sent
}
startingAfter = programEnrollments[...].id;
await queueRewardProcessing({ ...input, batchNumber: batchNumber + 1 }); // then next batch queuedIf 🟢 Low / Nit
The file was renamed from -export default function PartnerRewardsUpdated({
+export default function PartnerRewardUpdated({
If a Redis key expires in the 24-hour window (which would be unusual but possible if Redis is flushed),
The catch block logs and flushes but then throws a new generic error with the same user-facing message. The original Architecture ObservationThe removal of QStash ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/app/(ee)/api/cron/cleanup/orphaned-rewards/route.ts (1)
15-26: ⚡ Quick winProcess a stable batch order before applying
take: 10.This query limits the job to 10 rows but doesn't specify an order, so cleanup becomes nondeterministic under backlog. Add an oldest-first sort so each run makes predictable progress.
Suggested diff
const rewards = await prisma.reward.findMany({ where: { programId: null, updatedAt: { lt: subMinutes(new Date(), 30), // 30 minutes ago }, }, + orderBy: [ + { updatedAt: "asc" }, + { id: "asc" }, + ], select: { id: true, event: true, }, take: 10,🤖 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/cleanup/orphaned-rewards/route.ts around lines 15 - 26, The query using prisma.reward.findMany with take: 10 is nondeterministic because it lacks an order; modify the query that populates rewards to add a stable oldest-first ordering (e.g., orderBy: { updatedAt: 'asc' } and include a tie-breaker like id: 'asc') so each run processes the oldest records deterministically before applying take: 10.
🤖 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/cleanup/orphaned-rewards/route.ts:
- Around line 65-78: The current loop swallows deletion errors by catching them
inside the loop (the try/catch around prisma.reward.delete for reward.id) so the
handler always returns success via logAndRespond; remove the per-item try/catch
(or instead collect failed IDs) and let errors propagate to withCron so failures
are surfaced—either delete the try/catch that wraps prisma.reward.delete in
route.ts (referencing prisma.reward.delete and reward.id) or push failed reward
IDs into an array and after the loop throw a new Error listing those IDs before
calling logAndRespond, so withCron can mark the run as failed.
In `@apps/web/app/`(ee)/api/cron/rewards/process/route.ts:
- Around line 193-199: In the catch block that inspects error.code when deleting
a reward (route.ts, inside the handler that hard-deletes rewards and references
reward.id), narrow the thrown value by first testing error instanceof
Prisma.PrismaClientKnownRequestError before accessing error.code, and only treat
P2025 as a success if that instanceof check passes; otherwise rethrow or wrap
the error as currently done. This keeps the existing error message logic for
non-P2025 cases but prevents runtime crashes when non-Prisma values are thrown.
In `@apps/web/lib/api/rewards/queue-reward-processing.ts`:
- Around line 35-42: The code computes `version` using `incrementRewardVersion`
before the surrounding `try` block, which lets errors escape structured logging
and the normalized error path; move the conditional computation of `version`
(the ternary that calls `incrementRewardVersion({ groupId: params.groupId,
event: params.rewardSnapshot.event })`) into the existing `try` block so any
thrown error is caught and handled by the function's catch/logging flow
(preserve the branch that uses `params.version` when provided).
---
Nitpick comments:
In `@apps/web/app/`(ee)/api/cron/cleanup/orphaned-rewards/route.ts:
- Around line 15-26: The query using prisma.reward.findMany with take: 10 is
nondeterministic because it lacks an order; modify the query that populates
rewards to add a stable oldest-first ordering (e.g., orderBy: { updatedAt: 'asc'
} and include a tie-breaker like id: 'asc') so each run processes the oldest
records deterministically before applying take: 10.
🪄 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: 152b1585-3a90-4015-8486-7fc867cf98ff
📒 Files selected for processing (13)
apps/web/app/(ee)/api/cron/cleanup/orphaned-rewards/route.tsapps/web/app/(ee)/api/cron/rewards/process/route.tsapps/web/app/(ee)/api/cron/rewards/sync-enrollments/route.tsapps/web/lib/actions/partners/create-reward.tsapps/web/lib/actions/partners/delete-reward.tsapps/web/lib/actions/partners/update-reward.tsapps/web/lib/api/partners/notify-partner-reward-change.tsapps/web/lib/api/partners/notify-partners-reward-changed.tsapps/web/lib/api/rewards/queue-reward-enrollment-sync.tsapps/web/lib/api/rewards/queue-reward-processing.tsapps/web/lib/api/rewards/reward-version.tsapps/web/lib/email/email-templates-map.tspackages/email/src/templates/partner-reward-updated.tsx
💤 Files with no reviewable changes (3)
- apps/web/lib/api/partners/notify-partners-reward-changed.ts
- apps/web/lib/api/rewards/queue-reward-enrollment-sync.ts
- apps/web/app/(ee)/api/cron/rewards/sync-enrollments/route.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/web/lib/api/rewards/queue-reward-processing.ts (2)
35-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove version resolution into the
tryblock.Line 36 performs
incrementRewardVersionoutside thetry, so failures skip your structured logging/error path.Suggested change
export async function queueRewardProcessing(params: RewardJob) { - const version = - params.version !== undefined - ? params.version - : await incrementRewardVersion({ - groupId: params.groupId, - event: params.rewardSnapshot.event, - }); - try { + const version = + params.version !== undefined + ? params.version + : await incrementRewardVersion({ + groupId: params.groupId, + event: params.rewardSnapshot.event, + }); + const response = await qstash.publishJSON({🤖 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/api/rewards/queue-reward-processing.ts` around lines 35 - 42, The version resolution (currently using params.version or await incrementRewardVersion(...)) must be moved inside the existing try block so any failures from incrementRewardVersion are caught and routed through your structured logging/error handling; update the code that currently reads params.version and calls incrementRewardVersion to run inside the try, preserving the fallback behavior (use params.version if defined, otherwise await incrementRewardVersion with { groupId: params.groupId, event: params.rewardSnapshot.event }) and remove the earlier top-level await so errors aren't thrown outside the try/catch.
32-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a parsed/output job type for internal queue logic.
Line 32 currently uses
z.input<typeof rewardJobSchema>, which keepsversion/batchNumberoptional at type level and weakens the version-gating contract in recursive calls.Suggested change
-export type RewardJob = z.input<typeof rewardJobSchema>; +export type RewardJobInput = z.input<typeof rewardJobSchema>; +export type RewardJob = z.infer<typeof rewardJobSchema>; -export async function queueRewardProcessing(params: RewardJob) { +export async function queueRewardProcessing(params: RewardJobInput) { + const parsed = rewardJobSchema.parse(params);🤖 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/api/rewards/queue-reward-processing.ts` around lines 32 - 34, The internal job type should use the parsed/output schema type so required fields like version and batchNumber are enforced; replace the current type alias RewardJob (which uses z.input<typeof rewardJobSchema>) with the parsed/output type (e.g. z.infer<typeof rewardJobSchema> or z.output equivalent) and update the queueRewardProcessing signature to accept that parsed type so recursive/internal calls honor the version-gating contract.apps/web/app/(ee)/api/cron/rewards/process/route.ts (1)
116-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter out users with null email before enqueueing notifications.
Lines 123 and 167 pass users without an email-null guard, so notification payloads can include invalid recipients.
Suggested change
- const users = programEnrollments.flatMap(({ partner }) => - partner.users.map(({ user }) => user), - ); + const users = programEnrollments.flatMap(({ partner }) => + partner.users + .map(({ user }) => user) + .filter((user) => user.email != null), + );Also applies to: 157-168
🤖 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/rewards/process/route.ts around lines 116 - 124, The partner->users query returns user objects where user.email may be null; before building/enqueuing notification payloads in the route handler (the exported request handler in route.ts) filter partners.select.users to only include entries with user.email != null (e.g., users.filter(u => u.user?.email)); apply this guard in both places where you iterate partner.select.users and call the enqueue/create-notification logic so you never enqueue notifications with a null recipient.
🧹 Nitpick comments (1)
apps/web/app/(ee)/api/cron/cleanup/orphaned-rewards/route.ts (1)
15-27: ⚡ Quick winAdd
orderByfor deterministic batch processing.Without ordering, the query may return different subsets of orphaned rewards across runs, potentially leaving some orphans unprocessed indefinitely while repeatedly fetching others. Adding
orderBy: { updatedAt: 'asc' }ensures oldest orphans are processed first and makes progress predictable.Suggested fix
const rewards = await prisma.reward.findMany({ where: { programId: null, updatedAt: { lt: subMinutes(new Date(), 30), // 30 minutes ago }, }, select: { id: true, event: true, }, + orderBy: { + updatedAt: "asc", + }, take: 10, });🤖 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/cleanup/orphaned-rewards/route.ts around lines 15 - 27, The prune query using prisma.reward.findMany that fetches orphaned rewards into the rewards variable lacks an orderBy clause, causing non-deterministic batches; update the prisma.reward.findMany call to include orderBy: { updatedAt: 'asc' } (or similar deterministic ordering) so the oldest orphaned rewards are processed first and batch progress is predictable.
🤖 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/lib/api/partners/notify-partner-reward-change.ts`:
- Around line 34-35: Filter out users with null or undefined emails before
constructing the batch payload to avoid enqueuing invalid recipients: in the
function that builds the batch (the code using users.map and constructing { to:
user.email!, ... } and the similar block referencing partner.email), replace the
direct map with a pre-filter like users.filter(u => u.email) (or equivalent) so
any entries with missing emails are excluded, then map over the filtered array
to build the payload; apply the same guard where partner.email is used to ensure
partner.email is always a non-null string before adding to the batch.
---
Duplicate comments:
In `@apps/web/app/`(ee)/api/cron/rewards/process/route.ts:
- Around line 116-124: The partner->users query returns user objects where
user.email may be null; before building/enqueuing notification payloads in the
route handler (the exported request handler in route.ts) filter
partners.select.users to only include entries with user.email != null (e.g.,
users.filter(u => u.user?.email)); apply this guard in both places where you
iterate partner.select.users and call the enqueue/create-notification logic so
you never enqueue notifications with a null recipient.
In `@apps/web/lib/api/rewards/queue-reward-processing.ts`:
- Around line 35-42: The version resolution (currently using params.version or
await incrementRewardVersion(...)) must be moved inside the existing try block
so any failures from incrementRewardVersion are caught and routed through your
structured logging/error handling; update the code that currently reads
params.version and calls incrementRewardVersion to run inside the try,
preserving the fallback behavior (use params.version if defined, otherwise await
incrementRewardVersion with { groupId: params.groupId, event:
params.rewardSnapshot.event }) and remove the earlier top-level await so errors
aren't thrown outside the try/catch.
- Around line 32-34: The internal job type should use the parsed/output schema
type so required fields like version and batchNumber are enforced; replace the
current type alias RewardJob (which uses z.input<typeof rewardJobSchema>) with
the parsed/output type (e.g. z.infer<typeof rewardJobSchema> or z.output
equivalent) and update the queueRewardProcessing signature to accept that parsed
type so recursive/internal calls honor the version-gating contract.
---
Nitpick comments:
In `@apps/web/app/`(ee)/api/cron/cleanup/orphaned-rewards/route.ts:
- Around line 15-27: The prune query using prisma.reward.findMany that fetches
orphaned rewards into the rewards variable lacks an orderBy clause, causing
non-deterministic batches; update the prisma.reward.findMany call to include
orderBy: { updatedAt: 'asc' } (or similar deterministic ordering) so the oldest
orphaned rewards are processed first and batch progress is predictable.
🪄 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: 4b40368b-e1b9-4a6b-993e-5b324f75db64
📒 Files selected for processing (13)
apps/web/app/(ee)/api/cron/cleanup/orphaned-rewards/route.tsapps/web/app/(ee)/api/cron/rewards/process/route.tsapps/web/app/(ee)/api/cron/rewards/sync-enrollments/route.tsapps/web/lib/actions/partners/create-reward.tsapps/web/lib/actions/partners/delete-reward.tsapps/web/lib/actions/partners/update-reward.tsapps/web/lib/api/partners/notify-partner-reward-change.tsapps/web/lib/api/partners/notify-partners-reward-changed.tsapps/web/lib/api/rewards/queue-reward-enrollment-sync.tsapps/web/lib/api/rewards/queue-reward-processing.tsapps/web/lib/api/rewards/reward-version.tsapps/web/lib/email/email-templates-map.tspackages/email/src/templates/partner-reward-updated.tsx
💤 Files with no reviewable changes (3)
- apps/web/lib/api/partners/notify-partners-reward-changed.ts
- apps/web/lib/api/rewards/queue-reward-enrollment-sync.ts
- apps/web/app/(ee)/api/cron/rewards/sync-enrollments/route.ts
|
@coderabbitai, I've fixed the issues mentioned above. Do another round of review. Any potential breaking changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 3944 --repo dubinc/dubLength of output: 40203 🏁 Script executed: #!/bin/bash
# Check for any remaining imports of removed exports
rg -n "getRewardEmailSnapshot|notifyPartnersRewardChanged|RewardSnapshot|sync-enrollments|PartnerRewardsUpdated|queue-reward-enrollment-sync" --type ts --type tsx -g '!*.test.*' 2>/dev/null | grep -v "^Binary" || trueLength of output: 525 Good news — the search for removed symbols came back clean. No stray imports of SummaryOne remaining issue to fix: the // Callers use the input type (version/batchNumber optional)
type RewardJobInput = z.input<typeof rewardJobSchema>;
// Parsed cron payload uses the output type (defaults resolved)
export type RewardJob = z.infer<typeof rewardJobSchema>;
export async function queueRewardProcessing(params: RewardJobInput) {
const version =
params.version !== undefined
? params.version
: await incrementRewardVersion({
groupId: params.groupId,
event: params.rewardSnapshot.event,
});
// ...
}On breaking changes:
Everything else looks solid. The null-email fix (both at query and notification layer), the |
Summary by CodeRabbit
New Features
Refactor
Chores