payments: rework refund flow to three-knob API#1429
Open
BilalG1 wants to merge 6 commits into
Open
Conversation
Replaces the per-entry `refund_entries: [{ entry_index, quantity, amount_usd }]`
schema with a flat `{ amount_usd, revoke_product, end_subscription? }` shape on
the admin refund endpoint. Refund state is now derived from the bulldozer ledger
(`refund:<sourceTxnId>:<uuid>` rows) rather than the legacy `refundedAt` Prisma
column, so multiple partial refunds can run against a single purchase up to the
remaining cap. Adds support for refunding any subscription invoice via
`invoice_id` (start or renewal). Refund rows surface in the listing endpoint as
`type: "refund"` with adjusted_by linkage that handles both new and legacy
formats. Stripe idempotency keys are now derived from
`(tenancyId, sourceTxnId, amount, prior_refunded)` so network retries dedupe at
Stripe while intentional partials still get distinct keys. Dashboard refund
dialog rebuilt around the three toggles. The `transaction-builder.ts` helpers
that the old listing path used are gone — the listing reads bulldozer directly.
Known follow-ups (documented in code): cap-check race window under concurrent
refunds (a Postgres advisory lock would help, but bulldozer's embedded
BEGIN/COMMIT prevents an outer Prisma tx from scoping the writes), and Stripe
vs. DB non-atomicity if a write fails after a successful Stripe refund.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR refactors the refund system from a complex per-entry interface to a simpler three-knob model. The route now accepts ChangesRefund Flow Refactor to Three-Knob Model
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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
- Restore SubscriptionAlreadyRefunded / OneTimePurchaseAlreadyRefunded KnownErrors as the legacy-refundedAt backstop, and TestModePurchaseNonRefundable for test-mode amount>0, so callers catching by error code still work. - Correct the misleading idempotency-key comment: the key is keyed on (tenancyId, sourceTxnId, amount, priorRefunded) — not refundTxnId — which means Stripe-success → DB-fail self-heals on retry, while DB-success → response-loss is the remaining hole. - Add e2e coverage for the invoice_id (renewal-invoice) refund path, plus rejection paths for unrelated invoice_id and invoice_id on OTPs.
- Fix Stripe error-code string for idempotent sub cancel (`subscription_already_canceled`, not `subscription_canceled`). - Reject end-only sub refund replay when the sub is already scheduled to end — otherwise `readPriorRefundSummary` doesn't see end-only events and the call is a forever-no-op accumulating empty rows. - Reject `revoke_product=true` with `invoice_id` pointing to a renewal invoice: the product grant lives on the sub-start txn, so a renewal-scoped revocation would write a back-reference to a non-existent entry. - Return the full refund txnId as the listing's `id` for refund rows so it matches `adjusted_by.transaction_id` linkage. - Document the dashboard's missing renewal-refund path as a gap. - Tests: +2 (end-only replay, revoke+renewal rejection); extended OTP-full-refund and renewal-invoice tests to assert id linkage.
… amountTotal currency guard - Dashboard: seed the refund dialog's amountUsd / revokeProduct / endSubscription state from the current transaction in the menu onClick before opening. ActionDialog's onOpenChange doesn't fire on the open transition for a controlled dialog, so the previous reset block was dead on open — admins hitting "Refund" on a paid purchase and submitting defaults would revoke/end at $0 instead of refunding the charged amount. - Refund product-revocation: persist the public-API entry index (`SUBSCRIPTION_START_PRODUCT_GRANT_ENTRY_INDEX = 0`), not the internal ledger position. `mapLedgerEntry` drops the hidden `active-subscription-start` entry, so the prior value of `1` pointed at the money-transfer entry (or out of range on test-mode subs) through the public listing. - Subscription cap: split the USD product cap from the actual cap. The invoice's `amountTotal` is the more accurate cap (proration, quantity changes, discounts), but `SubscriptionInvoice` doesn't persist currency — so always run `getTotalUsdStripeUnits` first as a USD pre-flight (it throws on non-USD pricing), and only then prefer `amountTotal`.
Contributor
Greptile SummaryThis PR replaces the per-entry refund schema with a flat three-knob API (
Confidence Score: 5/5Safe to merge; the refund flow is well-tested (19/19 e2e), documented limitations are intentional and tracked, and no correctness gaps were found that affect live-mode paths. The rework is thorough and self-consistent: the new bulldozer-derived prior-refund summary, idempotency-key derivation, legacy backstop, and end-only replay guard all hold up under review. The only latent issue found is the invoice ownership check comparing stripeSubscriptionId (null for test-mode subs), which is logically fragile if test-mode subscriptions ever start creating invoice records — but that path is presently unreachable and does not affect live-mode behavior. Known concurrency and atomicity gaps are transparently documented in comments and the PR description. apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx — specifically the invoice ownership check at line 396 (stripeSubscriptionId comparison). Important Files Changed
Sequence DiagramsequenceDiagram
participant Admin as Dashboard Admin
participant Route as POST /refund
participant Prisma as Prisma DB
participant Bulldozer as Bulldozer Ledger
participant Stripe as Stripe
Admin->>Route: amount_usd, revoke_product, end_subscription, invoice_id?
Route->>Route: Validate inputs and coupling rules
Route->>Prisma: Find subscription or purchase (legacy-refundedAt backstop)
Route->>Bulldozer: Read prior refund summary (LIKE pattern on txnId)
Bulldozer-->>Route: refundedStripeUnits, productRevoked
Route->>Route: Check remaining cap and revoke guards
alt "amount > 0 and live mode"
Route->>Stripe: Create refund with deterministic idempotency key
Stripe-->>Route: OK
end
alt revoke_product
Route->>Stripe: Cancel subscription (idempotent)
Route->>Prisma: "Update subscription endedAt=now"
Route->>Bulldozer: Write subscription state
else end_subscription only
Route->>Stripe: "Update cancel_at_period_end=true"
Route->>Prisma: Update subscription cancelAtPeriodEnd
Route->>Bulldozer: Write subscription state
end
Route->>Bulldozer: "Write refund row (type=refund, entries=[money-transfer?, product-revocation?])"
Route-->>Admin: "success=true, refund_transaction_id"
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx:396-397
**Invoice ownership validated via Stripe ID, which is null for test-mode subs**
The check `found.stripeSubscriptionId !== subscription.stripeSubscriptionId` evaluates to `null !== null = false` when both the invoice and the subscription are test-mode (i.e. `stripeSubscriptionId` is `null` for both). In that scenario any `SubscriptionInvoice` row in the same tenancy with a null `stripeSubscriptionId` would pass the guard, even if it belongs to a different subscription — allowing `isSubscriptionCreationInvoice` from the wrong invoice to influence the `revoke_product` gate and the `sourceTxnId` on `subscription_B` when the caller supplies `invoiceId` from `subscription_A`.
Today test-mode subscriptions don't create `SubscriptionInvoice` rows (as noted in the inline comment `"test-mode sub has no invoice"`), so the null-equals-null branch is unreachable in practice. But the correctness of this gate then relies on that data-model invariant being stable. A direct FK comparison — `found.subscriptionId !== subscription.id` — is the semantically correct check and doesn't depend on the Stripe ID being populated.
Reviews (2): Last reviewed commit: "payments: address third-pass refund revi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/dashboard/src/components/data-table/transaction-table.tsx (1)
239-240: ⚡ Quick winFail loudly if the USD invariant is broken.
Returning
canSubmit: falsewith no error leaves the dialog dead with no explanation. IfSUPPORTED_CURRENCIESever stops containing USD, this should throw an explicit invariant error or at least surface an actionable alert instead of silently disabling the refund flow.As per coding guidelines "Code defensively. Prefer
?? throwErr(...)over non-null assertions, with good error messages explicitly stating the assumption that must've been violated for the error to be thrown" and "When building frontend code, always carefully deal with loading and error states. Be very explicit with these... and make sure errors are NEVER just silently swallowed".🤖 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/dashboard/src/components/data-table/transaction-table.tsx` around lines 239 - 240, The current check returns { canSubmit: false, error: null } when USD_CURRENCY (derived from SUPPORTED_CURRENCIES) or target is missing, which silently disables the dialog; instead, make the USD invariant fail loudly: replace the silent return by throwing or returning an explicit error via a helper (e.g., using the project's throwErr/Invariant helper) when USD_CURRENCY is missing and keep a clear actionable error when target is missing; locate the guard around USD_CURRENCY and target in the function that computes canSubmit (references: USD_CURRENCY, SUPPORTED_CURRENCIES, target, canSubmit) and ensure you surface a descriptive message like "Invariant violated: USD must be in SUPPORTED_CURRENCIES for refunds" rather than silently returning null error.
🤖 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/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx`:
- Around line 67-80: getTotalUsdStripeUnits incorrectly uses the product's USD
price (resolveSelectedPriceFromProduct / moneyAmountToStripeUnits) rather than
the actual charge currency; change the flow to read the original charge/invoice
currency (from the Invoice or PaymentIntent currency field) and either reject
refunds when that currency is not USD or compute refund units using the original
charge currency's moneyAmountToStripeUnits conversion, and update
getTotalUsdStripeUnits (or rename to reflect currency) to accept the charge
currency and amount source instead of product USD. Separately, remove
priorRefundedStripeUnits from the idempotency key calculation in
makeStripeIdempotencyKey and build the key from immutable request-specific
values (e.g., original charge id, refund amount, currency, and a client-supplied
refund reference or timestamp) so retries do not change the idempotency key
after persistence.
- Around line 97-105: makeStripeIdempotencyKey currently builds the key using
priorRefundedStripeUnits which can change after a successful-but-response-lost
refund, causing retries to generate a different idempotency key and duplicate
refunds; instead generate and persist a stable idempotency token (e.g. uuid) on
the refund request before calling Stripe, reuse that token for all retry
attempts in makeStripeIdempotencyKey (or replace its usage), and make the
ledger/DB write idempotent by recording and checking that persisted token
(abort/return existing result if token already has a completed refund) so
retries reuse the same Stripe idempotency key; apply the same pattern to the
subscription and one-time purchase refund flows referenced in the codebase (the
other call sites noted in the review).
In `@apps/backend/src/app/api/latest/internal/payments/transactions/route.tsx`:
- Around line 594-600: In the loop over entries handling entry.type ===
"product-revocation", don't pass the loop index i to addLink for legacy refunds;
instead read the original source index from the entry's adjustedEntryIndex field
(e.g., const adjustedEntryIndex = Reflect.get(entry, "adjustedEntryIndex")),
validate it's a non-negative number, and pass that into addLink (fall back to i
only if adjustedEntryIndex is missing/invalid). Keep the existing checks for
adjustedTransactionId and use adjustedEntryIndex when calling addLink to
preserve back-compat.
In `@apps/dashboard/src/components/data-table/transaction-table.tsx`:
- Around line 270-273: The seedFromTransaction function currently preloads
setAmountUsd with chargedAmountUsd which can be larger than the remaining
refundable balance after a partial refund; change it to detect when the
transaction has been adjusted (e.g., transaction.adjusted_by or similar adjusted
flag is non-empty) and in that case call setAmountUsd('0') instead of
chargedAmountUsd, otherwise keep the existing chargedAmountUsd ?? '0' behavior;
update the logic in seedFromTransaction (referencing seedFromTransaction,
chargedAmountUsd, transaction.adjusted_by, and setAmountUsd) so reopened dialogs
default to 0 for follow-up partial refunds.
In
`@apps/e2e/tests/backend/endpoints/api/v1/internal/transactions-refund.test.ts`:
- Around line 240-283: The test "supports multiple partial refunds capped at
remaining amount" uses inconsistent cent notation for the third refund: change
the amount_usd value passed to niceBackendFetch for refund3 from the decimal
string "0.01" to the cent integer string "1" so it matches the other calls
(e.g., "2000", "3000") and keeps createLiveModeOneTimePurchaseTransaction /
refund assertions correct.
In `@packages/stack-shared/src/interface/admin-interface.ts`:
- Around line 920-921: The response handling in admin-interface.ts currently
maps json.refund_transaction_id directly to refundTransactionId, which can be
undefined for malformed responses; update the mapping to validate and fail fast
by using the nullish coalescing-with-throw pattern (e.g.,
json.refund_transaction_id ?? throwErr(...)) so that refundTransactionId is
guaranteed, and use a clear error message referencing refund_transaction_id when
calling throwErr; ensure you apply this around the response.json() handling that
returns { success: json.success, refundTransactionId: ... }.
---
Nitpick comments:
In `@apps/dashboard/src/components/data-table/transaction-table.tsx`:
- Around line 239-240: The current check returns { canSubmit: false, error: null
} when USD_CURRENCY (derived from SUPPORTED_CURRENCIES) or target is missing,
which silently disables the dialog; instead, make the USD invariant fail loudly:
replace the silent return by throwing or returning an explicit error via a
helper (e.g., using the project's throwErr/Invariant helper) when USD_CURRENCY
is missing and keep a clear actionable error when target is missing; locate the
guard around USD_CURRENCY and target in the function that computes canSubmit
(references: USD_CURRENCY, SUPPORTED_CURRENCIES, target, canSubmit) and ensure
you surface a descriptive message like "Invariant violated: USD must be in
SUPPORTED_CURRENCIES for refunds" rather than silently returning null error.
🪄 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: c5f08f0b-dc78-4949-80ed-0e020866491a
📒 Files selected for processing (12)
apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsxapps/backend/src/app/api/latest/internal/payments/transactions/route.tsxapps/backend/src/app/api/latest/internal/payments/transactions/transaction-builder.tsapps/backend/src/lib/payments/refund-txn-id.tsapps/backend/src/lib/payments/schema/phase-1/transactions.tsapps/dashboard/src/components/data-table/transaction-table.tsxapps/e2e/tests/backend/endpoints/api/v1/internal/transactions-refund.test.tspackages/stack-shared/src/interface/admin-interface.tspackages/stack-shared/src/interface/crud/transactions.tspackages/stack-shared/src/known-errors.tsxpackages/template/src/lib/stack-app/apps/implementations/admin-app-impl.tspackages/template/src/lib/stack-app/apps/interfaces/admin-app.ts
- Wrap refund dialog submit in runAsynchronouslyWithAlert so backend errors surface to the admin instead of becoming unhandled rejections. - Tailor the "refund must do something" validation message to OTP vs subscription so OTP admins aren't told to use a checkbox the UI hides. - Seed the refund dialog amount to 0 when the source transaction has already been adjusted, avoiding a stale default that exceeds the cap. - Use adjustedEntryIndex (not the loop index) when materialising legacy refund -> source links in the transactions listing.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Collaborator
|
@greptileai please rereview comprehensively |
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.
Summary
{ amount_usd, revoke_product, end_subscription? }shape; refund state is now derived from bulldozer ledger rows (refund:<sourceTxnId>:<uuid>) instead of the legacyrefundedAtcolumn, enabling multiple partial refunds up to the remaining cap.invoice_idfor refunding any subscription invoice (start or renewal), Stripe idempotency keys derived from(tenancyId, sourceTxnId, amount, prior_refunded)so retries dedupe but intentional partials don't collide, and a legacy backstop that rejects pre-reworkrefundedAtpurchases.type: "refund"withadjusted_bylinkage handling both new and legacy formats.Implements
STA2-52 — Build in refund logic for payments
Documented limitations (planned follow-up work)
These are called out in code comments and intentionally deferred to a follow-up PR:
BEGIN/COMMITprevents an outer Prisma tx from scoping the writes, so two concurrent refunds can both pass the cap check. Needs a bulldozer-aware mutex or pending-refund-intent pattern. In practice refunds are admin-only and rare, so the race window is small.(tenancyId, sourceTxnId, amount, priorRefunded), so a retry after Stripe-success → DB-fail self-heals (Stripe dedupes; the next attempt writes the bulldozer row). The hole is the reverse direction: if the bulldozer row commits but the response is lost, a retry sees a higherpriorRefundedand generates a fresh key — Stripe would issue a second real refund. No out-of-band reconciliation today.invoice_idpath. Refund actions are only enabled onpurchaserows and the submit call never passesinvoice_id, so admins refunding a renewal must use the API directly. Follow-up: enable the action onsubscription-renewalrows and threadinvoice_idthrough.Architectural note
active-subscription-endanditem-quantity-expireentries are not emitted on the refund row itself. They're produced by the derived sub-end transaction (transactions.ts:158-228) once Prismasubscription.endedAtis updated, keeping theexpiresWhen/when-repeatedsemantics in one place. This is the main structural divergence from the ticket's literal entry recipe.Review follow-ups addressed in this PR
First-pass review:
SubscriptionAlreadyRefunded/OneTimePurchaseAlreadyRefundedare once again thrown by the legacy-refundedAtbackstop, andTestModePurchaseNonRefundableis thrown when an admin sendsamount_usd > 0against a test-mode purchase. Callers catching by error code keep working through the rework.(tenancyId, sourceTxnId, amount, priorRefunded)key and its self-healing behaviour on the Stripe-success → DB-fail retry path (see Documented limitations above for the remaining hole).subscription_create+subscription_cycleinvoices), refunds the renewal invoice viainvoice_id, and asserts the resultingrefund_transaction_idstarts withrefund:sub-renewal:and is linked back viaadjusted_byon the renewal row (not the start row). Plus negative cases: cross-subscriptioninvoice_id→ 404,invoice_idon a one-time purchase → SchemaError.Second-pass review:
subscription_already_canceled, notsubscription_canceled— the previous catch would have re-thrown.amount=0, revoke=false, end=trueand the sub is alreadycancelAtPeriodEndorendedAt, throw SchemaError. OtherwisereadPriorRefundSummarydoesn't see end-only events and the call would be a forever-no-op accumulating empty refund rows.revoke_product=truewith renewalinvoice_idrejected: the product grant lives on the sub-start txn, not on renewal txns — a renewal-scoped revocation would write a back-reference to a non-existent entry. Forces admin to revoke against the start invoice (or the default no-invoice_idcall).idmatches the linkage: the listing route now returns the full refund txnId asidfortype: "refund"rows so it matchesadjusted_by.transaction_id— the dashboard can join source rows to their refund rows.Third-pass review:
ActionDialog'sonOpenChange, which doesn't fire on the open transition for a controlled dialog. As a result the dialog opened with the initialuseStatedefaults (amountUsd = '0'), and an admin submitting unchanged on a paid purchase would revoke/end at $0 instead of refunding the charged amount. The seed now runs in the menuonClickbeforesetIsDialogOpen(true).SUBSCRIPTION_START_PRODUCT_GRANT_ENTRY_INDEXcorrected from 1 → 0: the constant is persisted asadjustedEntryIndexon product-revocation entries and copied through verbatim bymapLedgerEntry. That mapper drops the hiddenactive-subscription-startentry, so the public-API layout puts the product grant at index 0. The prior value of1pointed at the money-transfer entry (or out of range on test-mode subs) through the public listing.amountTotalcap gated behind a USD pre-flight:SubscriptionInvoicedoesn't persist invoice currency, and the previous code tookinvoice.amountTotalas USD cents directly. NowgetTotalUsdStripeUnits(which throws on non-USD pricing) is always called first;amountTotalis only preferred as the actual cap after that pre-flight succeeds.Test plan
pnpm typecheck— 28/28 passpnpm lint— 28/28 passpnpm test run apps/e2e/tests/backend/endpoints/api/v1/internal/transactions-refund.test.ts— 19/19 pass (was 14/14 on the original PR; +3 forinvoice_idpath: renewal refund happy path, unrelatedinvoice_idrejection,invoice_idon OTP rejection; +2 for second-pass: end-only replay rejection, revoke+renewal rejection)/api/latest/internal/payments/transactions/refund— unknown purchase → 404, no-op → 400, negative → 400, sub-revoke-without-end → 400amount_usd = "0". Re-test before un-drafting: open the refund dialog from the menu, confirm the amount field pre-fills with the charged amount, exercise validation (negative / exceeds-cap / no-op), and submit both an end-subscription-only sub refund and a money+revoke OTP refund; verify bulldozer rows and PrismacancelAtPeriodEndupdates.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests