payments: rework refund flow to three-knob API#1429
Open
BilalG1 wants to merge 5 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
📝 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: 3/5The backend refund logic is sound and well-tested, but the dashboard's async submit handler drops backend errors silently — an admin triggering a backend rejection (cap exceeded, already revoked, network error) will see no feedback. The backend changes are carefully written with documented caveats, good test coverage (+5 tests), and correct idempotency key design. The dashboard regression — removing runAsynchronouslyWithAlert from the ActionDialog submit handler while ActionDialog itself has no try/catch — means any error thrown by app.refundTransaction() is an unhandled promise rejection, silently lost from the UI. This is on the primary refund action path in the dashboard. apps/dashboard/src/components/data-table/transaction-table.tsx — async submit handler missing error handling wrapper Important Files Changed
Sequence DiagramsequenceDiagram
participant Admin
participant Dashboard
participant RefundRoute as POST /refund
participant Prisma
participant Bulldozer
participant Stripe
Admin->>Dashboard: Click Refund (amount, revoke, end-sub)
Dashboard->>RefundRoute: POST body with type/id/amount_usd/revoke_product
RefundRoute->>Prisma: Look up subscription or OTP record
Prisma-->>RefundRoute: record (legacy refundedAt backstop check)
RefundRoute->>Bulldozer: readPriorRefundSummary via LIKE pattern on txnId
Bulldozer-->>RefundRoute: refundedStripeUnits + productRevoked flags
RefundRoute->>RefundRoute: Cap check and schema validation
alt live mode and amount greater than zero
RefundRoute->>Stripe: refunds.create with SHA-256 idempotency key
Stripe-->>RefundRoute: refund confirmed
end
alt revoke_product is true
RefundRoute->>Stripe: subscriptions.cancel (idempotent)
RefundRoute->>Prisma: subscription.update status and endedAt
RefundRoute->>Bulldozer: bulldozerWriteSubscription
else end_subscription is true
RefundRoute->>Stripe: subscriptions.update cancel_at_period_end
RefundRoute->>Prisma: subscription.update cancelAtPeriodEnd
RefundRoute->>Bulldozer: bulldozerWriteSubscription
end
RefundRoute->>Bulldozer: bulldozerWriteManualTransaction for refund row
RefundRoute-->>Dashboard: success + refund_transaction_id
Dashboard-->>Admin: Dialog closes
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/dashboard/src/components/data-table/transaction-table.tsx:287-297
The async `onClick` handler calls `app.refundTransaction()` without wrapping it in `runAsynchronouslyWithAlert`. `ActionDialog` calls `await okButton.onClick?.()` directly without a try/catch (see `action-dialog.tsx:130-131`), so any backend error (network failure, `SchemaError`, `SubscriptionAlreadyRefunded`, etc.) becomes an unhandled promise rejection — the dialog may close silently with no feedback to the admin.
```suggestion
onClick: async () => {
if (!validation.canSubmit) {
return "prevent-close";
}
await runAsynchronouslyWithAlert(async () => {
await app.refundTransaction({
...target,
amountUsd: amountUsd as MoneyAmount,
revokeProduct,
...(isSubscription ? { endSubscription } : {}),
});
});
},
```
### Issue 2 of 2
apps/dashboard/src/components/data-table/transaction-table.tsx:258-260
The error message mentions "end subscription" as an option even for OTP refunds, but the "End subscription" checkbox is not rendered for OTP purchases. An admin who unchecks "Revoke product" and clears the amount on an OTP row sees an instruction to do something the UI doesn't expose.
```suggestion
if (refundUnits === 0 && !revokeProduct && !endSubscription) {
return {
canSubmit: false,
error: isSubscription
? "Refund must do something: enter an amount, revoke product, or end subscription."
: "Refund must do something: enter an amount or revoke product.",
};
}
```
Reviews (1): Last reviewed commit: "payments: fix refund dialog defaults, su..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
287
to
297
| onClick: async () => { | ||
| if (chargedAmountUsd && !refundValidation.canSubmit) { | ||
| if (!validation.canSubmit) { | ||
| return "prevent-close"; | ||
| } | ||
| await app.refundTransaction({ | ||
| ...target, | ||
| refundEntries: refundValidation.refundEntries ?? throwErr("Refund entries missing for refund"), | ||
| amountUsd: amountUsd as MoneyAmount, | ||
| revokeProduct, | ||
| ...(isSubscription ? { endSubscription } : {}), | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
onClick handler calls app.refundTransaction() without wrapping it in runAsynchronouslyWithAlert. ActionDialog calls await okButton.onClick?.() directly without a try/catch (see action-dialog.tsx:130-131), so any backend error (network failure, SchemaError, SubscriptionAlreadyRefunded, etc.) becomes an unhandled promise rejection — the dialog may close silently with no feedback to the admin.
Suggested change
| onClick: async () => { | |
| if (chargedAmountUsd && !refundValidation.canSubmit) { | |
| if (!validation.canSubmit) { | |
| return "prevent-close"; | |
| } | |
| await app.refundTransaction({ | |
| ...target, | |
| refundEntries: refundValidation.refundEntries ?? throwErr("Refund entries missing for refund"), | |
| amountUsd: amountUsd as MoneyAmount, | |
| revokeProduct, | |
| ...(isSubscription ? { endSubscription } : {}), | |
| }); | |
| }, | |
| onClick: async () => { | |
| if (!validation.canSubmit) { | |
| return "prevent-close"; | |
| } | |
| await runAsynchronouslyWithAlert(async () => { | |
| await app.refundTransaction({ | |
| ...target, | |
| amountUsd: amountUsd as MoneyAmount, | |
| revokeProduct, | |
| ...(isSubscription ? { endSubscription } : {}), | |
| }); | |
| }); | |
| }, |
Rule Used: Use runAsynchronouslyWithAlert from `@stackframe... (source)
Learned From
stack-auth/stack-auth#943
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/dashboard/src/components/data-table/transaction-table.tsx
Line: 287-297
Comment:
The async `onClick` handler calls `app.refundTransaction()` without wrapping it in `runAsynchronouslyWithAlert`. `ActionDialog` calls `await okButton.onClick?.()` directly without a try/catch (see `action-dialog.tsx:130-131`), so any backend error (network failure, `SchemaError`, `SubscriptionAlreadyRefunded`, etc.) becomes an unhandled promise rejection — the dialog may close silently with no feedback to the admin.
```suggestion
onClick: async () => {
if (!validation.canSubmit) {
return "prevent-close";
}
await runAsynchronouslyWithAlert(async () => {
await app.refundTransaction({
...target,
amountUsd: amountUsd as MoneyAmount,
revokeProduct,
...(isSubscription ? { endSubscription } : {}),
});
});
},
```
**Rule Used:** Use `runAsynchronouslyWithAlert` from `@stackframe... ([source](https://app.greptile.com/review/custom-context?memory=5e671275-7493-402a-93a8-969537ec4d63))
**Learned From**
[stack-auth/stack-auth#943](https://github.com/stack-auth/stack-auth/pull/943)
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+258
to
260
| if (refundUnits === 0 && !revokeProduct && !endSubscription) { | ||
| return { canSubmit: false, error: "Refund must do something: enter an amount, revoke product, or end subscription." }; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| if (refundUnits === 0 && !revokeProduct && !endSubscription) { | |
| return { canSubmit: false, error: "Refund must do something: enter an amount, revoke product, or end subscription." }; | |
| } | |
| if (refundUnits === 0 && !revokeProduct && !endSubscription) { | |
| return { | |
| canSubmit: false, | |
| error: isSubscription | |
| ? "Refund must do something: enter an amount, revoke product, or end subscription." | |
| : "Refund must do something: enter an amount or revoke product.", | |
| }; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/dashboard/src/components/data-table/transaction-table.tsx
Line: 258-260
Comment:
The error message mentions "end subscription" as an option even for OTP refunds, but the "End subscription" checkbox is not rendered for OTP purchases. An admin who unchecks "Revoke product" and clears the amount on an OTP row sees an instruction to do something the UI doesn't expose.
```suggestion
if (refundUnits === 0 && !revokeProduct && !endSubscription) {
return {
canSubmit: false,
error: isSubscription
? "Refund must do something: enter an amount, revoke product, or end subscription."
: "Refund must do something: enter an amount or revoke product.",
};
}
```
How can I resolve this? If you propose a fix, please make it concise.![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
Comment on lines
+97
to
105
| function makeStripeIdempotencyKey(args: { | ||
| tenancyId: string, | ||
| sourceTxnId: string, | ||
| amountStripeUnits: number, | ||
| priorRefundedStripeUnits: number, | ||
| }): string { | ||
| const fingerprint = `${args.tenancyId}:${args.sourceTxnId}:${args.amountStripeUnits}:${args.priorRefundedStripeUnits}`; | ||
| return `refund:${createHash("sha256").update(fingerprint).digest("hex").slice(0, 32)}`; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Stripe refund creation, if a retry uses a different idempotency key from the original successful request, will Stripe treat it as a brand-new refund?
💡 Result:
Yes—if the retry uses a different idempotency key, Stripe will treat it as a new request (i.e., it will not return the cached result for the original retry), so it can create a new refund if the underlying request would succeed. Stripe’s idempotency behavior is keyed on the specific idempotency key value: Stripe saves the status code/body for the first request made for any given idempotency key, and subsequent requests with the same key return the same result. If you change the idempotency key, you’re no longer reusing the same cached result, so Stripe won’t deduplicate based on the original successful request’s key.[1][2] Practical implication for refunds: creating a refund is a “create new refund” operation on a charge/payment_intent, and if the charge isn’t already fully refunded, a new (non-idempotently deduped) call can create another refund. Stripe notes that once entirely refunded, a charge can’t be refunded again, and calling it again will raise an error.[3] Therefore: using a different idempotency key on retry does not guarantee “same refund” behavior; it can result in another refund attempt unless the API rejects it due to refund state (e.g., already fully refunded) or you otherwise guard against it.[1][3]
Citations:
- 1: https://stripe.com/docs/api/idempotent%5Frequests
- 2: https://docs.stripe.com/api/idempotent_requests
- 3: https://docs.stripe.com/api/refunds/object
🏁 Script executed:
# Check if the file exists and get context around the mentioned lines
fd -t f "route.tsx" | grep -i "refund"Repository: hexclave/stack-auth
Length of output: 143
🏁 Script executed:
# List files in the api path to understand structure
git ls-files "apps/backend/src/app/api/latest/internal/payments/transactions/" | head -20Repository: hexclave/stack-auth
Length of output: 302
🏁 Script executed:
# Read the refund route file to understand the context
wc -l apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsxRepository: hexclave/stack-auth
Length of output: 147
🏁 Script executed:
# Get the full file content
cat -n apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsxRepository: hexclave/stack-auth
Length of output: 37892
Use a response-loss-safe idempotency token.
The Stripe idempotency key includes priorRefundedStripeUnits, which changes as soon as the refund row commits. If a successful Stripe refund's response is lost and the handler retries, prior.refundedStripeUnits will now include the refunded amount, generating a fresh idempotency key. Stripe will treat this as a new request and create a second real refund.
The fix requires a stable retry token persisted before the Stripe call and reused across retries, with the ledger write made idempotent on that token. The developers acknowledge this gap in lines 330–341 as an open hole without out-of-band reconciliation.
Also applies to: lines 485–490 (subscription), 672–677 (one-time purchase).
🤖 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/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx`
around lines 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).
Comment on lines
+594
to
+600
| for (let i = 0; i < entries.length; i++) { | ||
| const entry = entries[i]; | ||
| if (!isRecord(entry)) continue; | ||
| if (entry.type !== "product-revocation") continue; | ||
| const adjustedTxnId = Reflect.get(entry, "adjustedTransactionId"); | ||
| if (typeof adjustedTxnId !== "string" || adjustedTxnId.length === 0) continue; | ||
| addLink(adjustedTxnId, refundTxnId, i); |
Contributor
There was a problem hiding this comment.
Use adjustedEntryIndex for legacy refund links.
i is the entry's position inside the refund row, not the original source entry index. That changes adjusted_by.entry_index for legacy refunds and breaks the back-compat this comment promises.
💡 Proposed fix
for (let i = 0; i < entries.length; i++) {
const entry = entries[i];
if (!isRecord(entry)) continue;
if (entry.type !== "product-revocation") continue;
const adjustedTxnId = Reflect.get(entry, "adjustedTransactionId");
+ const adjustedEntryIndex = Reflect.get(entry, "adjustedEntryIndex");
if (typeof adjustedTxnId !== "string" || adjustedTxnId.length === 0) continue;
- addLink(adjustedTxnId, refundTxnId, i);
+ if (typeof adjustedEntryIndex !== "number" || !Number.isInteger(adjustedEntryIndex) || adjustedEntryIndex < 0) continue;
+ addLink(adjustedTxnId, refundTxnId, adjustedEntryIndex);
}📝 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.
Suggested change
| for (let i = 0; i < entries.length; i++) { | |
| const entry = entries[i]; | |
| if (!isRecord(entry)) continue; | |
| if (entry.type !== "product-revocation") continue; | |
| const adjustedTxnId = Reflect.get(entry, "adjustedTransactionId"); | |
| if (typeof adjustedTxnId !== "string" || adjustedTxnId.length === 0) continue; | |
| addLink(adjustedTxnId, refundTxnId, i); | |
| for (let i = 0; i < entries.length; i++) { | |
| const entry = entries[i]; | |
| if (!isRecord(entry)) continue; | |
| if (entry.type !== "product-revocation") continue; | |
| const adjustedTxnId = Reflect.get(entry, "adjustedTransactionId"); | |
| const adjustedEntryIndex = Reflect.get(entry, "adjustedEntryIndex"); | |
| if (typeof adjustedTxnId !== "string" || adjustedTxnId.length === 0) continue; | |
| if (typeof adjustedEntryIndex !== "number" || !Number.isInteger(adjustedEntryIndex) || adjustedEntryIndex < 0) continue; | |
| addLink(adjustedTxnId, refundTxnId, adjustedEntryIndex); |
🤖 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/backend/src/app/api/latest/internal/payments/transactions/route.tsx`
around lines 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.
Comment on lines
+270
to
+273
| const seedFromTransaction = () => { | ||
| setAmountUsd(chargedAmountUsd ?? '0'); | ||
| setRevokeProduct(true); | ||
| setEndSubscription(isSubscription); |
Contributor
There was a problem hiding this comment.
Don't reuse the original charge as the default for follow-up partial refunds.
After a partial money refund, chargedAmountUsd is larger than the remaining refundable balance, so reopening the dialog and submitting unchanged now fails on the repeat-partial-refund path. If this payload doesn’t expose the remaining amount, defaulting to 0 once transaction.adjusted_by is non-empty is safer than preloading a potentially invalid value.
Suggested fix
const seedFromTransaction = () => {
- setAmountUsd(chargedAmountUsd ?? '0');
+ setAmountUsd(transaction.adjusted_by.length > 0 ? '0' : (chargedAmountUsd ?? '0'));
setRevokeProduct(true);
setEndSubscription(isSubscription);
};📝 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.
Suggested change
| const seedFromTransaction = () => { | |
| setAmountUsd(chargedAmountUsd ?? '0'); | |
| setRevokeProduct(true); | |
| setEndSubscription(isSubscription); | |
| const seedFromTransaction = () => { | |
| setAmountUsd(transaction.adjusted_by.length > 0 ? '0' : (chargedAmountUsd ?? '0')); | |
| setRevokeProduct(true); | |
| setEndSubscription(isSubscription); | |
| }; |
🤖 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
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.
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
Tests