fix(dev-plans): reprice drafted renewal on tier change - #2909
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a Stripe renewal-invoice re-pricing helper for dev-plan tier changes, wires it into change and cancel downgrade flows, and expands unit and real-Stripe test coverage for adjustment, skip, failure, reversal, and final billing behavior. ChangesDraft renewal invoice re-pricing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DevPlansRoute
participant Stripe
participant repriceDraftRenewalInvoice
Client->>DevPlansRoute: POST /change-tier
DevPlansRoute->>Stripe: update subscription item price
DevPlansRoute->>repriceDraftRenewalInvoice: repriceDraftRenewalInvoice(...)
repriceDraftRenewalInvoice->>Stripe: invoices.list(drafted renewal)
repriceDraftRenewalInvoice->>Stripe: prices.retrieve(target tier)
alt draft total differs
repriceDraftRenewalInvoice->>Stripe: invoiceItems.create(adjustment)
else draft total matches
repriceDraftRenewalInvoice-->>DevPlansRoute: no adjustment
end
DevPlansRoute-->>Client: response
Client->>DevPlansRoute: POST /cancel-downgrade
DevPlansRoute->>Stripe: restore current tier price
DevPlansRoute->>repriceDraftRenewalInvoice: repriceDraftRenewalInvoice(...)
repriceDraftRenewalInvoice->>Stripe: invoiceItems.create(inverse adjustment)
DevPlansRoute-->>Client: response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58dac2012f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await stripe.invoiceItems.create({ | ||
| customer: customerId, | ||
| invoice: draft.id, | ||
| amount: adjustmentCents, | ||
| currency: draft.currency, |
There was a problem hiding this comment.
When two tier-change requests for the same subscription run during the pre-renewal draft window, both can list the draft before either new adjustment line is visible. Since this invoiceItems.create call has no per-draft/new-tier idempotency key or lock, duplicate requests can each add the full adjustment (for example, two pro→lite requests add two -$50 lines), leaving the renewal invoice over-adjusted. Serialize per subscription/invoice or make the adjustment creation idempotent.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| const billedCents = draft.lines.data.reduce((sum, line) => { |
There was a problem hiding this comment.
Stripe only embeds the first handful of invoice lines on an Invoice object, but this sum uses only draft.lines.data. If a user schedules/cancels/supersedes enough changes in the draft window, prior devPlanRenewalReprice lines can fall outside that embedded page, so billedCents is computed from incomplete data and the next adjustment can move the renewal away from the target tier price. Retrieve the full paginated invoice lines before computing the net billed amount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Addresses a Stripe edge case where a dev plan tier change close to renewal can leave an already-drafted subscription_cycle invoice billing the old tier price while the renewal logic applies the new tier (credits/description), causing mismatched charges and entitlements.
Changes:
- Added
repriceDraftRenewalInvoice(...)to detect a draft renewal invoice and apply an adjustment invoice item to reconcile the draft’s net plan amount to the new tier price. - Invoked draft re-pricing after tier-change price swaps in
change-tierand when reverting incancel-downgrade. - Added unit tests covering re-pricing behavior, idempotency/no-op scenarios, best-effort failure behavior, and cancel-downgrade netting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/api/src/routes/dev-plans.ts | Adds best-effort logic to re-price already-drafted Stripe renewal invoices after dev plan tier swaps (and on cancel-downgrade). |
| apps/api/src/routes/dev-plans.spec.ts | Adds unit tests validating draft renewal invoice re-pricing and resilience/idempotency cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // downgrade inside that window swaps the subscription price too late for | ||
| // the draft, so without an adjustment the renewal bills the old (higher) | ||
| // tier while the webhook grants the lower tier's credits. | ||
| process.env.STRIPE_DEV_PLAN_LITE_PRICE_ID = "price_lite"; |
| it("skips the draft adjustment when the draft already bills the new price", async () => { | ||
| // The draft can be created concurrently with (or after) the price swap, in | ||
| // which case it already bills the new tier and must not be adjusted again. | ||
| process.env.STRIPE_DEV_PLAN_LITE_PRICE_ID = "price_lite"; |
| it("schedules the downgrade even if re-pricing the draft fails", async () => { | ||
| // Re-pricing is best-effort: a Stripe failure there must not leave the | ||
| // price swapped without the pending tier recorded (the reverse mismatch). | ||
| process.env.STRIPE_DEV_PLAN_LITE_PRICE_ID = "price_lite"; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/routes/dev-plans.spec.ts (1)
986-1281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared Stripe mock boilerplate.
The four new tests each repeat near-identical
subscriptions.retrieve/subscriptions.updatemock payloads (differing only in a few fields like tier/price). A small factory (e.g.mockDevPlanSubscription({ devPlan, priceId })) would reduce duplication for this new block, consistent with the DRY guideline. Since this repeats an existing pattern already used throughout the file, this is optional and not blocking.As per coding guidelines, "Apply DRY principles for code reuse."
🤖 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/api/src/routes/dev-plans.spec.ts` around lines 986 - 1281, The new dev-plan Stripe tests duplicate the same `stripeMock.subscriptions.retrieve` and `stripeMock.subscriptions.update` payloads across multiple cases. Extract a small reusable factory/helper in `dev-plans.spec.ts` (for example around these test blocks) that builds the shared subscription mock from inputs like tier and price ID, and use it in the affected tests to remove repetition while keeping the test-specific invoice and pricing assertions.Source: Coding guidelines
🤖 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/api/src/routes/dev-plans.ts`:
- Around line 228-256: The dev plan renewal adjustment in dev-plans.ts is
marking only the invoice item, but the dedupe logic checks draft invoice line
metadata on draft.lines.data. Update the Stripe invoice item creation path so
the marker survives onto the generated draft invoice line, and ensure the
repricing/cancel flow in the dev-plan renewal handling uses a reliable persisted
marker for previously applied adjustments.
---
Nitpick comments:
In `@apps/api/src/routes/dev-plans.spec.ts`:
- Around line 986-1281: The new dev-plan Stripe tests duplicate the same
`stripeMock.subscriptions.retrieve` and `stripeMock.subscriptions.update`
payloads across multiple cases. Extract a small reusable factory/helper in
`dev-plans.spec.ts` (for example around these test blocks) that builds the
shared subscription mock from inputs like tier and price ID, and use it in the
affected tests to remove repetition while keeping the test-specific invoice and
pricing assertions.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 036e09de-d664-4677-afc6-8e3c4dab670f
📒 Files selected for processing (2)
apps/api/src/routes/dev-plans.spec.tsapps/api/src/routes/dev-plans.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/src/routes/dev-plans-reprice.e2e.ts (2)
153-166: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTight timeout margin in draft-invoice test.
waitForClockcan take up to 120s (60 × 2000ms) insideadvanceTo, but the enclosing test also has a 120000ms Vitest timeout (line 166) that must additionally cover the subsequentgetDraftRenewallist call and assertions. In a slow sandbox this leaves little to no margin and could cause an intermittent timeout failure unrelated to the actual behavior under test.Consider giving this test (and others calling
advanceTo) a larger timeout, e.g. 150000ms, to decouple clock-polling time from assertion overhead.🤖 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/api/src/routes/dev-plans-reprice.e2e.ts` around lines 153 - 166, The draft-invoice e2e test has too little timeout headroom because advanceTo() can consume most of the current Vitest timeout before getDraftRenewal() and assertions run. Increase the timeout on this test, and any similar tests that call advanceTo(), so the total budget comfortably exceeds waitForClock’s polling window; use the test name and advanceTo() as the key spots to update.
97-99: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
unit_amount ?? 0fallback can mask a misconfigured price.If either
PRO_PRICE_IDorLITE_PRICE_IDresolves to a price withunit_amount: null(e.g., a metered/tiered price), the fallback to0lets the subsequentexpect(proUnit).toBeGreaterThan(liteUnit)pass or fail for the wrong reason instead of failing clearly on a misconfigured fixture.🔧 Suggested fix to fail fast on unexpected null unit_amount
- proUnit = (await stripe.prices.retrieve(PRO_PRICE_ID)).unit_amount ?? 0; - liteUnit = (await stripe.prices.retrieve(LITE_PRICE_ID)).unit_amount ?? 0; + const proPrice = await stripe.prices.retrieve(PRO_PRICE_ID); + const litePrice = await stripe.prices.retrieve(LITE_PRICE_ID); + expect(proPrice.unit_amount, "PRO price must have a unit_amount").not.toBeNull(); + expect(litePrice.unit_amount, "LITE price must have a unit_amount").not.toBeNull(); + proUnit = proPrice.unit_amount!; + liteUnit = litePrice.unit_amount!;🤖 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/api/src/routes/dev-plans-reprice.e2e.ts` around lines 97 - 99, The `dev-plans-reprice.e2e.ts` assertion is masking bad Stripe fixtures by falling back to 0 when `stripe.prices.retrieve(...).unit_amount` is null. Update the test around `PRO_PRICE_ID` and `LITE_PRICE_ID` to fail fast if either retrieved price has a null `unit_amount` instead of defaulting, so `expect(proUnit).toBeGreaterThan(liteUnit)` only runs with valid amounts and clearly surfaces misconfigured prices.
🤖 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.
Nitpick comments:
In `@apps/api/src/routes/dev-plans-reprice.e2e.ts`:
- Around line 153-166: The draft-invoice e2e test has too little timeout
headroom because advanceTo() can consume most of the current Vitest timeout
before getDraftRenewal() and assertions run. Increase the timeout on this test,
and any similar tests that call advanceTo(), so the total budget comfortably
exceeds waitForClock’s polling window; use the test name and advanceTo() as the
key spots to update.
- Around line 97-99: The `dev-plans-reprice.e2e.ts` assertion is masking bad
Stripe fixtures by falling back to 0 when
`stripe.prices.retrieve(...).unit_amount` is null. Update the test around
`PRO_PRICE_ID` and `LITE_PRICE_ID` to fail fast if either retrieved price has a
null `unit_amount` instead of defaulting, so
`expect(proUnit).toBeGreaterThan(liteUnit)` only runs with valid amounts and
clearly surfaces misconfigured prices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e737072-3b6f-4864-aeb5-42f3d4d97a43
📒 Files selected for processing (2)
apps/api/src/routes/dev-plans-reprice.e2e.tsapps/api/src/routes/dev-plans.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/routes/dev-plans.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fd2c8a3f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // If Stripe already drafted the upcoming renewal invoice (tier change | ||
| // within the pre-renewal finalization window), it still bills the old | ||
| // price — re-price it to match the tier taking effect at renewal. | ||
| await repriceDraftRenewalInvoice({ |
There was a problem hiding this comment.
When scheduling a downgrade in the pre-renewal draft window, this best-effort Stripe work runs after the subscription price was changed but before devPlanPendingTier is persisted below. If Stripe finalizes and the invoice.payment_succeeded webhook runs during these awaits, the webhook computes the renewal tier from organization.devPlanPendingTier ?? organization.devPlan (apps/api/src/stripe.ts:3246), so it can grant/record the old tier even though the draft may already have been adjusted to the new lower price, and then this request leaves a pending downgrade for the next cycle. Write the pending tier before repricing, or make the webhook derive the renewal tier from Stripe, so the renewal state matches the charge.
Useful? React with 👍 / 👎.
|
|
||
| // The downgrade may have already re-priced a drafted renewal invoice to | ||
| // the lower tier; re-price it back so the renewal bills the current tier. | ||
| await repriceDraftRenewalInvoice({ |
There was a problem hiding this comment.
When cancelling a downgrade, this reprice runs after the Stripe price is reverted but before devPlanPendingTier is cleared below. If the renewal invoice finalizes during these Stripe calls, the webhook still sees the stale pending tier and uses it as the effective renewal tier at apps/api/src/stripe.ts:3246, so a customer can be charged the restored higher/current price while the org is renewed onto the cancelled lower tier. Clear the pending tier before the best-effort reprice, or otherwise make the webhook ignore the stale pending tier once cancellation starts.
Useful? React with 👍 / 👎.
Stripe drafts the subscription_cycle renewal invoice up to ~an hour before finalizing and charging it. A scheduled tier change (a downgrade, or an upgrade deferred to renewal) swaps the subscription item's price with proration suppressed and no cycle re-anchor, expecting the renewal to bill the new tier. But a change landing inside that draft window leaves the already-drafted invoice billing the OLD price, so a pro->lite downgrade scheduled minutes before renewal charged the PRO price ($79) while the renewal webhook granted LITE credits ($87) and labeled the transaction "Dev Plan LITE renewed". Add repriceDraftRenewalInvoice() next to voidPendingCycleRenewalInvoices (both handle the same pre-finalization draft window; the apply-now upgrade path voids the stale draft because it re-anchors, while a scheduled change keeps the cycle so its draft must be re-priced instead). It adds an adjustment invoice item for the difference between the new tier's price and the draft's net plan amount (subscription lines plus earlier tagged adjustments), which is idempotent across consecutive changes and a no-op when the draft already bills the new price. Wired into the scheduled-change branch (both directions) and cancel-downgrade. Best-effort: a failure only reverts to the pre-existing near-boundary mismatch and never fails the tier change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an opt-in Stripe Test Clock e2e that reproduces the incident against a live sandbox: subscribe PRO, advance to the renewal boundary so Stripe drafts the subscription_cycle invoice, then downgrade to LITE mid-window and run the real repriceDraftRenewalInvoice helper. Confirms end-to-end: Stripe drafts the renewal at the PRO price before finalizing; the item price swap does NOT re-price the existing draft (root cause); the helper adjusts the draft to the LITE price idempotently; cancel-downgrade nets it back to PRO; and the invoice finalizes and charges the re-priced LITE amount once the clock passes finalization. Gated behind STRIPE_TESTCLOCK_E2E so it is skipped in normal CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7fd2c8a to
d57ac4a
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Problem
A DevPass subscriber downgraded pro → lite at 06:37 PM and was renewed at 07:01 PM the same day — charged the PRO price ($79) but granted LITE credits ($87) with the transaction labeled "Dev Plan LITE renewed". Net effect: overcharged ~$50 for a lite cycle.
Root cause
Stripe drafts the
subscription_cyclerenewal invoice up to ~an hour before finalizing and charging it. A tier change swaps the subscription item's price immediately (proration_behavior: "none"), but an already-drafted renewal invoice keeps billing the price it was drafted with — the swap lands too late. The renewal webhook then desyncs three ways:amountcomes from Stripe'sinvoice.amount_paid(old tier), while credits, tier, and description followdevPlanPendingTier(new tier).Any tier change (or cancel-downgrade) inside the pre-renewal finalization window hits this, in either direction.
Fix
After every tier-change price swap (
change-tier, both directions, andcancel-downgrade), look for a draftsubscription_cycleinvoice on the subscription and re-price it by adding an adjustment invoice item for the difference between the new tier's price and the draft's net plan amount (subscription lines + earlier tagged adjustment lines).devPlanRenewalRepricemetadata so only our own adjustments are netted.Tests
Unit (
dev-plans.spec.ts, mocked Stripe, 18 passing): downgrade re-prices a drafted renewal (−$50), skips when the draft already bills the new price, downgrade still succeeds when re-pricing fails, cancel-downgrade nets the earlier adjustment back out (+$50).Real Stripe Test Clock e2e (
dev-plans-reprice.e2e.ts, opt-in viaSTRIPE_TESTCLOCK_E2E, skipped in CI): reproduces the incident against a live sandbox and runs the actualrepriceDraftRenewalInvoicehelper. All 6 pass:🤖 Generated with Claude Code
Summary by CodeRabbit