fix(commissions): enforce DB idempotency, unify commission engine, and harden webhook money paths - #14
Merged
Conversation
…d harden webhook money paths
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Deploy Preview for hex-diva ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideThis PR hardens commission and payout processing by enforcing database-level idempotency, centralizing commission calculation through a repository adapter, and adding explicit error-handling and write guards on money-related webhooks and engines, plus introduces a quality engine verifier script and test config plumbing. Sequence diagram for UpPromote order attribution commission processingsequenceDiagram
actor UpPromote
participant UppromoteWebhookRoute as UppromoteWebhookRoute
participant CommissionRepositoryAdapter as CommissionRepositoryAdapter
participant Supabase as Supabase
UpPromote->>UppromoteWebhookRoute: handleOrderAttributed(data)
UppromoteWebhookRoute->>Supabase: select referrals by referral_code
Supabase-->>UppromoteWebhookRoute: referral
UppromoteWebhookRoute->>CommissionRepositoryAdapter: processOrderCommission(referrer_id, order_id, amount)
CommissionRepositoryAdapter->>Supabase: select referral_stats.total_conversions
Supabase-->>CommissionRepositoryAdapter: stats
CommissionRepositoryAdapter->>Supabase: upsert commissions onConflict referrer_id,order_id
Supabase-->>CommissionRepositoryAdapter: commission row
CommissionRepositoryAdapter-->>UppromoteWebhookRoute: commission
UppromoteWebhookRoute->>Supabase: update referrals (conversions, commission_amount)
UppromoteWebhookRoute->>CommissionRepositoryAdapter: updateReferralStats(referrer_id)
CommissionRepositoryAdapter->>Supabase: select referral_stats by referrer_id
Supabase-->>CommissionRepositoryAdapter: stats or null
CommissionRepositoryAdapter->>Supabase: insert referral_stats if missing
Supabase-->>CommissionRepositoryAdapter: ok
UppromoteWebhookRoute-->>UpPromote: log Order attributed
Sequence diagram for UpPromote payoutProcessed idempotency guardsequenceDiagram
actor UpPromote
participant UppromoteWebhookRoute as UppromoteWebhookRoute
participant Supabase as Supabase
UpPromote->>UppromoteWebhookRoute: handlePayoutProcessed(data)
UppromoteWebhookRoute->>Supabase: select referral_stats.referrer_id by uppromote_affiliate_id
Supabase-->>UppromoteWebhookRoute: referrer_id or null
UppromoteWebhookRoute->>UppromoteWebhookRoute: [if no referrer_id] log warning and return
UppromoteWebhookRoute->>Supabase: select commission_payouts.id by referrer_id,stripe_transfer_id
Supabase-->>UppromoteWebhookRoute: existingPayout or null
UppromoteWebhookRoute->>UppromoteWebhookRoute: [if existingPayout] log and return
UppromoteWebhookRoute->>UppromoteWebhookRoute: validate status (processing|paid|pending)
UppromoteWebhookRoute->>Supabase: insert commission_payouts (user_id, amount, status, stripe_transfer_id, payout_date)
Supabase-->>UppromoteWebhookRoute: ok
UppromoteWebhookRoute->>Supabase: update referral_stats.paid_amount when status == paid
Supabase-->>UppromoteWebhookRoute: ok
UppromoteWebhookRoute-->>UpPromote: log Payout processed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (16)
🔥 Files not summarized due to errors (1)
WalkthroughThe PR adds commission integrity constraints and atomic referral-stat updates, routes UpPromote processing through a repository adapter, strengthens webhook error handling and idempotency, introduces a quality-engine verification CLI, adds Vitest setup, and documents execution plans and review findings. ChangesCommission integrity and webhook processing
Quality verification and test setup
Execution planning documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UpPromoteWebhook
participant CommissionRepositoryAdapter
participant Supabase
UpPromoteWebhook->>CommissionRepositoryAdapter: process order commission
CommissionRepositoryAdapter->>Supabase: upsert commission and referral statistics
UpPromoteWebhook->>Supabase: check payout reference
Supabase-->>UpPromoteWebhook: existing or new payout state
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- CommissionRepositoryAdapter.updateReferralStats currently only ensures the stats row exists and never actually updates any aggregates; wire it up to call the new update_referral_stats_atomic RPC (with the appropriate commission and order totals) or otherwise perform the atomic increments so UpPromote/order flows keep stats correct.
- The process-order webhook route no longer calls markWebhookProcessed on errors and also removed the second body parse; consider reintroducing failure marking using the already-parsed body so idempotency semantics remain consistent even when processing fails.
- The update_referral_stats_atomic RPC unconditionally sets volume_month_reset_at to NOW on every update, which will effectively disable month-boundary reset logic; review this so monthly volume and tier calculations continue to behave as intended (e.g., only resetting at month rollover).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- CommissionRepositoryAdapter.updateReferralStats currently only ensures the stats row exists and never actually updates any aggregates; wire it up to call the new update_referral_stats_atomic RPC (with the appropriate commission and order totals) or otherwise perform the atomic increments so UpPromote/order flows keep stats correct.
- The process-order webhook route no longer calls markWebhookProcessed on errors and also removed the second body parse; consider reintroducing failure marking using the already-parsed body so idempotency semantics remain consistent even when processing fails.
- The update_referral_stats_atomic RPC unconditionally sets volume_month_reset_at to NOW on every update, which will effectively disable month-boundary reset logic; review this so monthly volume and tier calculations continue to behave as intended (e.g., only resetting at month rollover).
## Individual Comments
### Comment 1
<location path="src/lib/adapters/CommissionRepositoryAdapter.ts" line_range="227-233" />
<code_context>
async updateReferralStats(referrerId: string): Promise<void> {
const { supabaseAdmin } = await import('@/lib/db')
</code_context>
<issue_to_address>
**issue (bug_risk):** updateReferralStats currently only ensures the row exists and never applies any aggregate updates.
The new migration adds an `update_referral_stats_atomic(p_referrer_id, p_commission_amount, p_order_total)` RPC to maintain `referral_stats`, but this method only does a best‑effort insert and never calls the RPC or updates aggregates. Since callers (e.g. the UpPromote webhook order handler) now depend on `updateReferralStats` to keep `total_conversions`, `total_commission_earned`, and `volume_month` correct, those values will remain stale. Please either wire this method to the RPC (with the required inputs) or implement the aggregate updates here, and remove or update any comments that imply additional work is happening to avoid silently incomplete behavior.
</issue_to_address>
### Comment 2
<location path="migrations/009_commission_integrity.sql" line_range="23-32" />
<code_context>
+CREATE OR REPLACE FUNCTION public.update_referral_stats_atomic(
</code_context>
<issue_to_address>
**issue (bug_risk):** The atomic stats RPC ignores existing monthly reset logic and volume_ytd, which may cause long-term stats drift.
Previously, `referral_stats` updates handled month rollovers (`volume_month` reset and `volume_month_reset_at` update) and maintained `volume_ytd`. The new `update_referral_stats_atomic` only increments `total_conversions`, `total_commission_earned`, and `volume_month`, sets `volume_month_reset_at` on insert, and never updates it on conflict, so month boundaries and YTD volume are no longer enforced and `volume_month_reset_at` can become stale. If these fields are still used (e.g., tiering/reporting), the RPC should implement equivalent reset/YTD logic or those fields should be explicitly deprecated to avoid inconsistent stats.
</issue_to_address>
### Comment 3
<location path="scripts/verify-quality-engine.mts" line_range="279-281" />
<code_context>
+ console.error("❌ No baseline found. Run with --baseline first.");
+ process.exit(1);
+ }
+ const baselineFindings: {file:string;title:string}[] = JSON.parse(fs.readFileSync(baselinePath, "utf8"));
+ const baselineSet = new Set(baselineFindings.map(f => `${f.file}:${f.title}`));
+ const newFindings = findings.filter(f => !baselineSet.has(`${f.file}:${f.title}`));
+ if (newFindings.length > 0) {
+ console.error("⚠️ qa-intel: New/changed issues found:");
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Baseline comparison keys only on file and title, which can conflate different rules or instances.
The baseline key is built as "${f.file}:${f.title}", so distinct rules with the same title in a file, or multiple findings with the same title at different locations, will be merged. That can cause new/changed issues to be treated as already baselined. Consider including the rule identifier (and possibly a location such as line/column) in the key to make comparison accurate and avoid missed findings.
Suggested implementation:
```typescript
if (compare) {
if (!fs.existsSync(baselinePath)) {
console.error("❌ No baseline found. Run with --baseline first.");
process.exit(1);
}
const makeFindingKey = (f: {
file: string;
title: string;
ruleId?: string;
rule?: string;
line?: number;
column?: number;
}): string => {
const rule = f.ruleId ?? f.rule ?? "";
const location =
f.line != null && f.column != null ? `:${f.line}:${f.column}` : "";
return `${f.file}:${rule}:${f.title}${location}`;
};
const baselineFindings: {
file: string;
title: string;
ruleId?: string;
rule?: string;
line?: number;
column?: number;
}[] = JSON.parse(fs.readFileSync(baselinePath, "utf8"));
const baselineSet = new Set(baselineFindings.map(makeFindingKey));
const newFindings = findings.filter(f => !baselineSet.has(makeFindingKey(f)));
```
```typescript
if (newFindings.length > 0) {
console.error("⚠️ qa-intel: New/changed issues found:");
console.error(JSON.stringify(newFindings, null, 2));
process.exit(1);
} else {
console.log("✅ qa-intel: No new issues since baseline.");
process.exit(0);
}
}
```
- Ensure that the `findings` objects being written to the baseline (earlier in this file) include `ruleId` or `rule` and, if available, `line` and `column`, so the comparison uses stable identifiers.
- If there is a shared `Finding` type/interface elsewhere in the codebase, update it to include `ruleId`/`rule` and location fields, then replace the inline type in `baselineFindings` and `makeFindingKey` with that shared type for consistency.
</issue_to_address>
### Comment 4
<location path="docs/testing/review-matrix.md" line_range="16" />
<code_context>
+| **REV-001** | Low | **P3** | [layout.tsx](file:///home/kellyb_dev/projects/hex-diva/src/app/layout.tsx) | Import Ordering | Framework import `next/script` is placed after type definitions. Should follow: framework → thirdparty → internal → types. | Reorder imports to group framework libraries first. |
</code_context>
<issue_to_address>
**nitpick (typo):** Consider changing “thirdparty” to the standard hyphenated form “third-party” in the import ordering description.
For clarity and consistency with common usage, consider updating the wording to use the hyphenated “third-party” in the import ordering description.
```suggestion
| **REV-001** | Low | **P3** | [layout.tsx](file:///home/kellyb_dev/projects/hex-diva/src/app/layout.tsx) | Import Ordering | Framework import `next/script` is placed after type definitions. Should follow: framework → third-party → internal → types. | Reorder imports to group framework libraries first. |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
227
to
233
| async updateReferralStats(referrerId: string): Promise<void> { | ||
| const { supabaseAdmin } = await import('@/lib/db') | ||
|
|
||
| // First ensure the record exists | ||
| const { data: stats, error: statsError } = await supabaseAdmin | ||
| .from('referral_stats') | ||
| .select('*') |
There was a problem hiding this comment.
issue (bug_risk): updateReferralStats currently only ensures the row exists and never applies any aggregate updates.
The new migration adds an update_referral_stats_atomic(p_referrer_id, p_commission_amount, p_order_total) RPC to maintain referral_stats, but this method only does a best‑effort insert and never calls the RPC or updates aggregates. Since callers (e.g. the UpPromote webhook order handler) now depend on updateReferralStats to keep total_conversions, total_commission_earned, and volume_month correct, those values will remain stale. Please either wire this method to the RPC (with the required inputs) or implement the aggregate updates here, and remove or update any comments that imply additional work is happening to avoid silently incomplete behavior.
Comment on lines
+23
to
+32
| CREATE OR REPLACE FUNCTION public.update_referral_stats_atomic( | ||
| p_referrer_id UUID, | ||
| p_commission_amount DECIMAL(10, 2), | ||
| p_order_total DECIMAL(10, 2) | ||
| ) | ||
| RETURNS VOID | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| AS $$ | ||
| BEGIN |
There was a problem hiding this comment.
issue (bug_risk): The atomic stats RPC ignores existing monthly reset logic and volume_ytd, which may cause long-term stats drift.
Previously, referral_stats updates handled month rollovers (volume_month reset and volume_month_reset_at update) and maintained volume_ytd. The new update_referral_stats_atomic only increments total_conversions, total_commission_earned, and volume_month, sets volume_month_reset_at on insert, and never updates it on conflict, so month boundaries and YTD volume are no longer enforced and volume_month_reset_at can become stale. If these fields are still used (e.g., tiering/reporting), the RPC should implement equivalent reset/YTD logic or those fields should be explicitly deprecated to avoid inconsistent stats.
Comment on lines
+279
to
+281
| const baselineFindings: {file:string;title:string}[] = JSON.parse(fs.readFileSync(baselinePath, "utf8")); | ||
| const baselineSet = new Set(baselineFindings.map(f => `${f.file}:${f.title}`)); | ||
| const newFindings = findings.filter(f => !baselineSet.has(`${f.file}:${f.title}`)); |
There was a problem hiding this comment.
suggestion (bug_risk): Baseline comparison keys only on file and title, which can conflate different rules or instances.
The baseline key is built as "${f.file}:${f.title}", so distinct rules with the same title in a file, or multiple findings with the same title at different locations, will be merged. That can cause new/changed issues to be treated as already baselined. Consider including the rule identifier (and possibly a location such as line/column) in the key to make comparison accurate and avoid missed findings.
Suggested implementation:
if (compare) {
if (!fs.existsSync(baselinePath)) {
console.error("❌ No baseline found. Run with --baseline first.");
process.exit(1);
}
const makeFindingKey = (f: {
file: string;
title: string;
ruleId?: string;
rule?: string;
line?: number;
column?: number;
}): string => {
const rule = f.ruleId ?? f.rule ?? "";
const location =
f.line != null && f.column != null ? `:${f.line}:${f.column}` : "";
return `${f.file}:${rule}:${f.title}${location}`;
};
const baselineFindings: {
file: string;
title: string;
ruleId?: string;
rule?: string;
line?: number;
column?: number;
}[] = JSON.parse(fs.readFileSync(baselinePath, "utf8"));
const baselineSet = new Set(baselineFindings.map(makeFindingKey));
const newFindings = findings.filter(f => !baselineSet.has(makeFindingKey(f))); if (newFindings.length > 0) {
console.error("⚠️ qa-intel: New/changed issues found:");
console.error(JSON.stringify(newFindings, null, 2));
process.exit(1);
} else {
console.log("✅ qa-intel: No new issues since baseline.");
process.exit(0);
}
}- Ensure that the
findingsobjects being written to the baseline (earlier in this file) includeruleIdorruleand, if available,lineandcolumn, so the comparison uses stable identifiers. - If there is a shared
Findingtype/interface elsewhere in the codebase, update it to includeruleId/ruleand location fields, then replace the inline type inbaselineFindingsandmakeFindingKeywith that shared type for consistency.
|
|
||
| | Finding ID | Severity | Priority | File | Category / Rule | Description | Recommended Fix | | ||
| | :--- | :--- | :--- | :--- | :--- | :--- | :--- | | ||
| | **REV-001** | Low | **P3** | [layout.tsx](file:///home/kellyb_dev/projects/hex-diva/src/app/layout.tsx) | Import Ordering | Framework import `next/script` is placed after type definitions. Should follow: framework → thirdparty → internal → types. | Reorder imports to group framework libraries first. | |
There was a problem hiding this comment.
nitpick (typo): Consider changing “thirdparty” to the standard hyphenated form “third-party” in the import ordering description.
For clarity and consistency with common usage, consider updating the wording to use the hyphenated “third-party” in the import ordering description.
Suggested change
| | **REV-001** | Low | **P3** | [layout.tsx](file:///home/kellyb_dev/projects/hex-diva/src/app/layout.tsx) | Import Ordering | Framework import `next/script` is placed after type definitions. Should follow: framework → thirdparty → internal → types. | Reorder imports to group framework libraries first. | | |
| | **REV-001** | Low | **P3** | [layout.tsx](file:///home/kellyb_dev/projects/hex-diva/src/app/layout.tsx) | Import Ordering | Framework import `next/script` is placed after type definitions. Should follow: framework → third-party → internal → types. | Reorder imports to group framework libraries first. | |
3 tasks
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.
Implements Wave 1A fixes: migrations, CommissionRepositoryAdapter atomic updates, unifying referrals/uppromote calculation engines, adding payout idempotency checks, and Shopify webhook write checks.
Summary by Sourcery
Strengthen commission and payout integrity by enforcing database idempotency, centralizing commission calculation through the repository adapter, and hardening webhook-driven money flows.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Deployment:
Documentation:
Chores:
Summary by cubic
Enforces single-commission per (referrer, order), routes all commission math through one engine, and hardens Shopify/UpPromote webhook money paths with strict idempotency and error handling.
Bug Fixes
referrer_id,order_idto avoid TOCTOU; duplicate fetch fallback added.src/lib/referrals(tier + rate) and uses the adapter in theuppromotewebhook.error; failed DB writes prevent marking webhooks as completed.uppromote.ts; added commission fields toDatabasetypes.Migration
009_commission_integrity.sql: dedupes existingcommissions, adds unique index on(referrer_id, order_id), and introducesupdate_referral_stats_atomicRPC for race-safe stats updates.Written for commit 8a6c1c1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation