feat(payments): collect 0.9% platform fee on every stripe money movement#1378
feat(payments): collect 0.9% platform fee on every stripe money movement#1378mantrakp04 wants to merge 10 commits intodevfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR introduces platform fee configuration for Stripe operations (90 bps for non-internal projects), centralizes refund parameter construction with an invariant-enforcing helper, and extracts shared E2E payment test utilities into a reusable module. Three independent feature additions across backend payment flows and test infrastructure. ChangesRefund Flow Improvements
Platform Fee Configuration and Integration
E2E Payment Test Helpers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The PR spans three independent feature areas (refund helpers, platform fee infrastructure, and E2E test extraction) with consistent patterns across routes. New module (
🚥 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)
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 |
Greptile SummaryThis PR adds a 0.9% platform fee to all non-internal Stripe money movements by attaching Note: The PR description still references Confidence Score: 5/5Safe to merge — no P0 or P1 findings; the fee application logic is correct, well-tested, and consistently applied across all charge paths. All three charge paths (OTP, subscription create, subscription switch) correctly attach fee params; refund path unconditionally sets refund_application_fee: false which is safe even for pre-fee or internal-project payments. Integer BPS math avoids IEEE-754 accumulation. In-file unit tests pin all edge cases. Only finding is P2: the PR description is stale relative to the scope reduction in 02a18c0. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "chore: document float division" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
mantrakp04
force-pushed
the
payment-subscription-handling-rework
branch
from
f8533a8 to
7e6492c
Compare
mantrakp04
changed the title
|
@greptile-ai review |
|
@greptile-ai review |
nams1570
force-pushed
the
fix/platform-fee-reconciliation-double-debit
branch
from
e95fc3f to
0c9059c
Compare
|
@greptileai refresh and do a deep re review |
|
@greptile-ai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsx (1)
96-110: TODO is well-documented but may need tracking.The comment correctly identifies uncertainty about whether Stripe's immediately-generated proration invoice inherits
application_fee_percent. This is a potential revenue gap on mid-cycle plan switches until verified or webhook-based stamping is implemented.Consider opening an issue to track this follow-up verification against a real Connect account.
Would you like me to open an issue to track verifying the proration invoice fee behavior?
🤖 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/payments/purchases/purchase-session/route.tsx` around lines 96 - 110, Create a tracking issue for the TODO about Stripe proration invoice behavior: document the uncertainty, include the relevant code reference (the TODO comment around getApplicationFeePercentOrUndefined and the applicationFeePercent variable in route.tsx), add steps to verify against a real Connect account and to implement webhook-based stamping of invoice.application_fee_amount if needed, and link the Stripe docs mentioned (connect/subscriptions and billing/subscriptions/prorations) plus any test/QA steps and acceptance criteria for closing the issue.
🤖 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/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsx`:
- Around line 96-110: Create a tracking issue for the TODO about Stripe
proration invoice behavior: document the uncertainty, include the relevant code
reference (the TODO comment around getApplicationFeePercentOrUndefined and the
applicationFeePercent variable in route.tsx), add steps to verify against a real
Connect account and to implement webhook-based stamping of
invoice.application_fee_amount if needed, and link the Stripe docs mentioned
(connect/subscriptions and billing/subscriptions/prorations) plus any test/QA
steps and acceptance criteria for closing the issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2a08623-9fd7-48d9-a518-01a29ff2d5b9
📒 Files selected for processing (6)
apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsxapps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/switch/route.tsapps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsxapps/backend/src/lib/payments/platform-fees.tsapps/e2e/tests/backend/endpoints/api/v1/internal/transactions-refund.test.tsapps/e2e/tests/backend/helpers/payments.ts
|
@greptile-ai review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/e2e/tests/backend/helpers/payments.ts (2)
124-124: ⚡ Quick winAdd a comment explaining the
anyon thetxcallback parameter.Using
anywithout justification violates the project guideline. The transactions body is an untyped API response here, which is a valid reason — just make it explicit.♻️ Proposed refactor
- const purchaseTransaction = transactionsRes.body.transactions.find((tx: any) => tx.type === "purchase"); + // `any` here because `transactionsRes.body` is an untyped raw HTTP response shape + // eslint-disable-next-line `@typescript-eslint/no-explicit-any` + const purchaseTransaction = transactionsRes.body.transactions.find((tx: any) => tx.type === "purchase");As per coding guidelines, "Try to avoid the
anytype. Whenever you need to useany, leave a comment explaining why you're using it."🤖 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/e2e/tests/backend/helpers/payments.ts` at line 124, The callback in transactionsRes.body.transactions.find currently uses an untyped parameter (tx: any); add a concise inline comment explaining why any is used (e.g., "transactionsRes.body is an untyped API response from the test backend, so we use any here to avoid coupling tests to response types") so it complies with the guideline; locate the find call where purchaseTransaction is defined and add that explanatory comment next to the (tx: any) parameter or immediately above the line.
46-49: ⚡ Quick winReplace
as stringtype cast with a defensive guard.
as stringbypasses the type system; afterexpect(code).toBeDefined()vitest narrows the runtime value but TypeScript still seesstring | undefined. Use the project'sthrowErrpattern instead.♻️ Proposed refactor
- const codeMatch = (res.body.url as string).match(/\/purchase\/([a-z0-9-_]+)/); - const code = codeMatch ? codeMatch[1] : undefined; - expect(code).toBeDefined(); - return code as string; + const codeMatch = (res.body.url as string).match(/\/purchase\/([a-z0-9_-]+)/); + const code = codeMatch?.[1] ?? throwErr("No purchase code found in URL: " + res.body.url); + return code;As per coding guidelines, "Do NOT use
as/any/type casts or anything else like that to bypass the type system" and "Prefer?? throwErr(...)over non-null assertions, with good error messages explicitly stating the assumption that must've been violated."🤖 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/e2e/tests/backend/helpers/payments.ts` around lines 46 - 49, The code currently casts the extracted purchase code with "as string"; instead replace the cast with the project's defensive throwErr pattern: locate the extraction using codeMatch and the code variable in the helper (the lines that compute code from codeMatch and return it) and change the return to use the non-null coalescing guard—e.g. derive code via codeMatch?.[1] or keep code variable, then return code ?? throwErr("Expected purchase code in redirect URL from payment helper")—removing the "as string" cast and keeping the expect assertion as-is.
🤖 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/lib/payments/platform-fees.ts`:
- Around line 3-7: Update the top block comment that currently claims "there is
no separate refund-leg collection" to accurately describe the implemented flow:
state that refunds keep the charge-leg fee via refund_application_fee: false and
that the refund-leg fee is collected separately by performing an inverse Connect
transfer in the refund route (the code that constructs/dispatches the inverse
transfer). Replace the incorrect sentence with a concise note referencing the
refund route's inverse Connect transfer behavior so future readers understand
both the charge-leg retention and the separate refund-leg collection.
In `@apps/e2e/tests/backend/helpers/payments.ts`:
- Around line 86-87: The eventId and paymentIntentId prefixes are incorrect for
a purchase flow: update the constants eventId and paymentIntentId (currently
`evt_otp_refund_${idSuffix}` and `pi_otp_refund_${idSuffix}`) to use
purchase/charge semantics (e.g., `evt_otp_purchase_${idSuffix}` and
`pi_otp_purchase_${idSuffix}` or `evt_otp_charge_${idSuffix}` /
`pi_otp_charge_${idSuffix}`) so the generated `payment_intent.succeeded` test
event and logs accurately reflect a one-time purchase.
---
Nitpick comments:
In `@apps/e2e/tests/backend/helpers/payments.ts`:
- Line 124: The callback in transactionsRes.body.transactions.find currently
uses an untyped parameter (tx: any); add a concise inline comment explaining why
any is used (e.g., "transactionsRes.body is an untyped API response from the
test backend, so we use any here to avoid coupling tests to response types") so
it complies with the guideline; locate the find call where purchaseTransaction
is defined and add that explanatory comment next to the (tx: any) parameter or
immediately above the line.
- Around line 46-49: The code currently casts the extracted purchase code with
"as string"; instead replace the cast with the project's defensive throwErr
pattern: locate the extraction using codeMatch and the code variable in the
helper (the lines that compute code from codeMatch and return it) and change the
return to use the non-null coalescing guard—e.g. derive code via codeMatch?.[1]
or keep code variable, then return code ?? throwErr("Expected purchase code in
redirect URL from payment helper")—removing the "as string" cast and keeping the
expect assertion as-is.
🪄 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: 66e567a4-b7de-492d-8aa0-60f18e25b3c4
📒 Files selected for processing (3)
apps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsxapps/backend/src/lib/payments/platform-fees.tsapps/e2e/tests/backend/helpers/payments.ts
✅ Files skipped from review due to trivial changes (1)
- apps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsx
|
@greptileai refresh summary and rereview please |
nams1570
left a comment
nams1570
left a comment
There was a problem hiding this comment.
Should be good now, worth noting that we charge on otps and subscription changes but not on refunds. We just dont refund the app fees on refunds.
|
@greptileai please rereview and update summary |
|
@greptileai rereview and refresh summary. Removal of sub product was intentional under YAGNI, don't see how that is an issue |
Charges the platform 0.9% on both legs of each transaction on non-internal projects. The charge leg rides along via Stripe's native `application_fee_amount` / `application_fee_percent` params on the PaymentIntent / Subscription. The refund leg cannot use Stripe's built-in fee handling (Stripe's default is to reverse our charge-leg fee on refund, netting us zero) so we disable that with `refund_application_fee: false` and collect the refund-leg fee via an inverse Connect transfer. The inverse-transfer path is fire-and-forget from the refund route so a fee-collection failure never blocks the end-customer's refund. Every attempt writes a durable `PlatformFeeEvent` ledger row first, upserting on `(sourceType, sourceId)` so replayed refunds cannot double-record. On retry beyond Stripe's 24h idempotency-key window we reconcile via a content-addressed `transfer_group` lookup so we don't double-debit the merchant. FAILED rows keep a Sentry-captured error string so ops can see what's stuck via the admin `/platform-fees` endpoint. Merchant Connect accounts now onboard with `debit_negative_balances: true` so Stripe can ACH-debit the merchant's linked bank when their balance goes negative from settlement events (payouts, chargebacks). Inverse transfers themselves still hard-fail on insufficient balance and land in the ledger as FAILED for manual reconciliation.
…tion finds existing transfer
The retry-reconciliation block in `collectInverseFeeInner` wraps two
distinct Stripe/DB calls in one try/catch. When Stripe's `transfers.list`
finds a pre-existing transfer for this `transfer_group` but the
subsequent `platformFeeEvent.update` throws (transient DB error, etc.),
the shared catch swallowed the error and fell through to
`transfers.create`. That's fine in-window (the idempotency key dedupes),
but after the 24h idempotency-key window expires the next retry would
create a second transfer on the merchant's account — double-debit.
Split into two try blocks with distinct error semantics:
(a) `transfers.list` fails — safe to fall through. No transfer
is known to exist yet, and the idempotency key still dedupes
the near-term retry path.
(b) `transfers.list` succeeds and returns a pre-existing transfer,
but the ledger update then fails — bail with FAILED status and
return. Creating another transfer here would double-debit once
the idempotency key expires. The Sentry report and ledger row
both capture the pre-existing transfer id so ops can reconcile
manually.
Refs:
https://docs.stripe.com/api/idempotent_requests (24h key pruning)
https://docs.stripe.com/api/transfers/list (transfer_group filter)
…ingify dbErr in Sentry
- schema.prisma: use dbgenerated("gen_random_uuid()") to match migration's DEFAULT and fix prisma migrate diff drift in CI
- platform-fees.ts: serialize dbErr to string in the case-(b) reconciliation captureError so the primary double-debit signal is readable in Sentry
Rather than maintaining complex refund cut logic, lets not charge on refunds. Let's keep the logic where we dont refund the app fee.
OTPs require application_fee_amounts so we calc them using a helper. However this means that for really small purchases, we charge nothing. We explicitly document this
nams1570
force-pushed
the
fix/platform-fee-reconciliation-double-debit
branch
from
7583fdb to
c4e201b
Compare
|
@greptileai re review and refresh summary |
2 participants
Summary
Charges the platform 0.9% on both legs of each transaction on non-internal projects.
Test plan
Refs
Summary by CodeRabbit
New Features
Improvements