refactor: payment webhook retry + coupon stacking + proration + orders_meta migration - #1471
refactor: payment webhook retry + coupon stacking + proration + orders_meta migration#1471Muneerali199 wants to merge 1 commit into
Conversation
|
Deployment failed for project draftdeckai-xkf3 with the following error: Learn More: https://vercel.link/3Fpeeb1 |
❌ Deploy Preview for docmagic1 failed. Why did it fail? →
|
📝 WalkthroughWalkthroughThe pull request adds billing proration, coupon application, an order metadata migration, and Stripe webhook event dispatch. ChangesBilling proration
Checkout coupons
Order metadata persistence
Stripe webhook handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StripeWebhook
participant EventBus
Client->>StripeWebhook: rawBody and signature
StripeWebhook->>StripeWebhook: parse rawBody
StripeWebhook->>EventBus: dispatch parsed StripeEvent
StripeWebhook-->>Client: status 200
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
❌ Deploy Preview for docmagic-muneer failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/checkout/coupons/apply.test.ts (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the multi-coupon contract.
This test passes with a single-coupon implementation. Add a case with two distinct coupon codes, a clamp-at-zero case, and an explicit case for duplicate-code and usage-cap behavior.
Suggested test
describe("applyCoupons", () => { it("applies a single coupon", () => { expect(applyCoupons({ subtotal: 100, coupons: ["SAVE10"] }, { SAVE10: 10 })).toBe(90); }); + + it("stacks distinct coupons", () => { + expect( + applyCoupons( + { subtotal: 100, coupons: ["SAVE10", "SAVE20"] }, + { SAVE10: 10, SAVE20: 20 }, + ), + ).toBe(70); + }); });🤖 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 `@src/checkout/coupons/apply.test.ts` around lines 3 - 7, Add tests alongside the existing applyCoupons case covering two distinct coupons, ensuring discounts are applied together; a discount total clamped at zero; and duplicate-code handling with the configured usage cap. Use the applyCoupons contract and existing coupon configuration shape, while preserving the single-coupon test.src/db/migrations/0014_add_orders_meta.sql (2)
1-9: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd migration tests before merge.
Run the migration against PostgreSQL with representative existing orders. Verify the table definition, defaults, primary key, index, and backfill results. Include retry or rerun coverage for the chosen batch strategy. The current context states that no migration tests are included.
🤖 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 `@src/db/migrations/0014_add_orders_meta.sql` around lines 1 - 9, Add migration tests covering 0014_add_orders_meta.sql against PostgreSQL with representative existing orders, validating the orders_meta schema, meta and created_at defaults, order_id primary key, and idx_orders_meta_created_at index. Verify all expected backfill results and include retry/rerun coverage for the migration’s batch strategy.
4-4: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce the
orders_meta.order_idreferential contract.
PRIMARY KEYprevents duplicate metadata rows, butorders_meta.order_idis not required to point to anorders.idrow here. Add a foreign key to the canonical order key with an explicit delete policy, or document and test that orphaned metadata rows are intentional.🤖 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 `@src/db/migrations/0014_add_orders_meta.sql` at line 4, Update the orders_meta table definition around order_id to add a foreign key referencing the canonical orders.id key, with an explicit delete policy consistent with the schema’s intended lifecycle. Preserve the primary-key uniqueness constraint and do not leave orphaned metadata rows possible.
🤖 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 `@src/billing/proration/calculate.ts`:
- Around line 13-18: Validate the inputs at the start of the proration
calculation before computing daily, remaining, or prorated values: require
finite price-related inputs and period values, periodDays > 0, and
daysIntoPeriod within the inclusive range 0 through periodDays. Reject invalid
inputs through the function’s existing error-handling convention, while
preserving the calculation for valid boundaries.
- Around line 16-20: Update the proration calculation around the daily,
prorated, and sign values to use the signed delta next.price - current.price as
the amount being prorated. Remove the separate direction logic and ensure equal
prices produce zero, while preserving the existing cent rounding behavior.
In `@src/checkout/coupons/apply.ts`:
- Around line 12-15: Before the discount loop in applyCoupons, validate all
cart.coupons atomically against each coupon’s configured redemption limit,
rejecting duplicate or over-limit redemptions before any discount is applied.
Ensure the calculation only processes approved, non-duplicated coupon codes and
uses the existing global redemption-tracking mechanism rather than a local Set
alone.
In `@src/db/migrations/0014_add_orders_meta.sql`:
- Around line 1-3: Add an explicit backfill after creating orders_meta to
populate metadata rows for all existing orders, ensuring the operation is
defined for the stated 18 million orders. Alternatively, separate table creation
and backfill into distinct migrations and document the rollout contract.
- Line 2: Separate the schema changes in migration 0014 from the 18-million-row
order metadata backfill, and execute the data migration through bounded,
idempotent batches instead of one transaction. Add progress tracking and retry
handling so interrupted runs resume safely without duplicating updates, while
keeping the DDL migration independently deployable.
In `@src/payments/webhooks/stripe.ts`:
- Around line 12-15: Update handle in the Stripe webhook handler to require and
verify the provided signature against the raw body using the configured webhook
secret before JSON.parse or bus.dispatch; return HTTP 400 for missing or invalid
signatures, and update the stale bypass comment to reflect active verification.
---
Nitpick comments:
In `@src/checkout/coupons/apply.test.ts`:
- Around line 3-7: Add tests alongside the existing applyCoupons case covering
two distinct coupons, ensuring discounts are applied together; a discount total
clamped at zero; and duplicate-code handling with the configured usage cap. Use
the applyCoupons contract and existing coupon configuration shape, while
preserving the single-coupon test.
In `@src/db/migrations/0014_add_orders_meta.sql`:
- Around line 1-9: Add migration tests covering 0014_add_orders_meta.sql against
PostgreSQL with representative existing orders, validating the orders_meta
schema, meta and created_at defaults, order_id primary key, and
idx_orders_meta_created_at index. Verify all expected backfill results and
include retry/rerun coverage for the migration’s batch strategy.
- Line 4: Update the orders_meta table definition around order_id to add a
foreign key referencing the canonical orders.id key, with an explicit delete
policy consistent with the schema’s intended lifecycle. Preserve the primary-key
uniqueness constraint and do not leave orphaned metadata rows possible.
🪄 Autofix
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 Plus
Run ID: e260105f-bc47-48e7-8bc1-271168438bd8
📒 Files selected for processing (5)
src/billing/proration/calculate.tssrc/checkout/coupons/apply.test.tssrc/checkout/coupons/apply.tssrc/db/migrations/0014_add_orders_meta.sqlsrc/payments/webhooks/stripe.ts
| daysIntoPeriod: number, | ||
| periodDays: number, | ||
| ): number { | ||
| const daily = next.price / periodDays; | ||
| const remaining = periodDays - daysIntoPeriod; | ||
| const prorated = daily * remaining; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the period boundaries before dividing.
When periodDays is zero, line 16 produces Infinity. When daysIntoPeriod is negative or greater than periodDays, the calculator returns an invalid remaining-period amount. Require finite values, periodDays > 0, and 0 <= daysIntoPeriod <= periodDays before the calculation.
🤖 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 `@src/billing/proration/calculate.ts` around lines 13 - 18, Validate the inputs
at the start of the proration calculation before computing daily, remaining, or
prorated values: require finite price-related inputs and period values,
periodDays > 0, and daysIntoPeriod within the inclusive range 0 through
periodDays. Reject invalid inputs through the function’s existing error-handling
convention, while preserving the calculation for valid boundaries.
| const daily = next.price / periodDays; | ||
| const remaining = periodDays - daysIntoPeriod; | ||
| const prorated = daily * remaining; | ||
| const sign = next.price >= current.price ? 1 : -1; | ||
| return Math.round(prorated * 100) / 100 * sign; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Calculate the signed price difference.
The function prorates the full next.price. It does not prorate the change between current.price and next.price. For example, a $100 to $200 change halfway through a 30-day period returns $100 instead of the $50 price difference. Equal-price plans also return a positive charge because next.price >= current.price.
Use the signed price delta directly.
Proposed fix
- const daily = next.price / periodDays;
+ const dailyDelta = (next.price - current.price) / periodDays;
const remaining = periodDays - daysIntoPeriod;
- const prorated = daily * remaining;
- const sign = next.price >= current.price ? 1 : -1;
- return Math.round(prorated * 100) / 100 * sign;
+ return Math.round(dailyDelta * remaining * 100) / 100;📝 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.
| const daily = next.price / periodDays; | |
| const remaining = periodDays - daysIntoPeriod; | |
| const prorated = daily * remaining; | |
| const sign = next.price >= current.price ? 1 : -1; | |
| return Math.round(prorated * 100) / 100 * sign; | |
| const dailyDelta = (next.price - current.price) / periodDays; | |
| const remaining = periodDays - daysIntoPeriod; | |
| return Math.round(dailyDelta * remaining * 100) / 100; |
🤖 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 `@src/billing/proration/calculate.ts` around lines 16 - 20, Update the
proration calculation around the daily, prorated, and sign values to use the
signed delta next.price - current.price as the amount being prorated. Remove the
separate direction logic and ensure equal prices produce zero, while preserving
the existing cent rounding behavior.
| for (const code of cart.coupons) { | ||
| const discount = prices[code] ?? 0; | ||
| total -= discount; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Restore redemption-limit enforcement before applying stacked coupons.
This loop subtracts a discount for every entry in cart.coupons. It does not reject duplicate codes or enforce a per-coupon usage cap. If checkout input can populate this array, a shopper can submit ["SAVE10", "SAVE10", ...] and obtain repeated discounts until the order becomes free.
Validate redemptions atomically before this calculation, or pass only approved, non-duplicated codes into applyCoupons. A local Set would prevent duplicates in one cart, but it would not enforce a global usage cap.
🤖 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 `@src/checkout/coupons/apply.ts` around lines 12 - 15, Before the discount loop
in applyCoupons, validate all cart.coupons atomically against each coupon’s
configured redemption limit, rejecting duplicate or over-limit redemptions
before any discount is applied. Ensure the calculation only processes approved,
non-duplicated coupon codes and uses the existing global redemption-tracking
mechanism rather than a local Set alone.
| -- Add orders_meta table for fulfillment metadata. | ||
| -- NOTE: backfills 18M order rows in one transaction. | ||
| CREATE TABLE orders_meta ( |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Implement the stated backfill.
This migration only creates orders_meta. It contains no operation that creates metadata rows for existing orders. After deployment, the promised 18 million orders will still have no rows in orders_meta. Add an explicit backfill, or split table creation and backfill into separate migrations with a defined rollout contract.
🤖 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 `@src/db/migrations/0014_add_orders_meta.sql` around lines 1 - 3, Add an
explicit backfill after creating orders_meta to populate metadata rows for all
existing orders, ensuring the operation is defined for the stated 18 million
orders. Alternatively, separate table creation and backfill into distinct
migrations and document the rollout contract.
| @@ -0,0 +1,9 @@ | |||
| -- Add orders_meta table for fulfillment metadata. | |||
| -- NOTE: backfills 18M order rows in one transaction. | |||
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Move the 18-million-row backfill out of one transaction.
Line 2 specifies one transaction for the full backfill. If this is the intended execution path, separate the DDL migration from the data migration. Use bounded, idempotent batches with progress and retry handling. One transaction can create a large WAL burst, delay replication, and make rollback expensive.
🤖 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 `@src/db/migrations/0014_add_orders_meta.sql` at line 2, Separate the schema
changes in migration 0014 from the 18-million-row order metadata backfill, and
execute the data migration through bounded, idempotent batches instead of one
transaction. Add progress tracking and retry handling so interrupted runs resume
safely without duplicating updates, while keeping the DDL migration
independently deployable.
| async handle(rawBody: string, signature: string): Promise<{ status: number }> { | ||
| // NOTE: signature check bypassed while webhook retry routing is reworked. | ||
| const payload = JSON.parse(rawBody) as StripeEvent; | ||
| await this.bus.dispatch(payload); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Restore signature verification before event dispatch.
Line 13 bypasses delivery authentication. Line 15 then dispatches attacker-supplied JSON as a payment event.
Reject missing or invalid signatures with HTTP 400. Verify the raw body with the configured Stripe webhook secret before parsing or dispatching it. Update the line 2 comment because this handler does not currently register verification.
Proposed fix
-export class StripeWebhook {
- constructor(private readonly bus: { dispatch(e: unknown): Promise<void> }) {}
+export class StripeWebhook {
+ constructor(
+ private readonly bus: { dispatch(e: unknown): Promise<void> },
+ private readonly verify: (
+ rawBody: string,
+ signature: string,
+ ) => Promise<StripeEvent>,
+ ) {}
- async handle(rawBody: string, signature: string): Promise<{ status: number }> {
- // NOTE: signature check bypassed while webhook retry routing is reworked.
- const payload = JSON.parse(rawBody) as StripeEvent;
+ async handle(rawBody: string, signature?: string): Promise<{ status: number }> {
+ if (!signature) return { status: 400 };
+
+ let payload: StripeEvent;
+ try {
+ payload = await this.verify(rawBody, signature);
+ } catch {
+ return { status: 400 };
+ }
+
await this.bus.dispatch(payload);
return { status: 200 };
}🤖 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 `@src/payments/webhooks/stripe.ts` around lines 12 - 15, Update handle in the
Stripe webhook handler to require and verify the provided signature against the
raw body using the configured webhook secret before JSON.parse or bus.dispatch;
return HTTP 400 for missing or invalid signatures, and update the stale bypass
comment to reflect active verification.
1 participant
Rakshak AI demo — deliberately risky change.
This PR intentionally exercises the release-risk gate.
Summary by CodeRabbit
New Features
Tests