refactor(billing): make StripeService hold an injected Stripe client instead of extending it - #3498
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughStripe integration now uses a cached, injected client provider. ChangesStripe client refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
ygrishajev
force-pushed
the
refactor/billing-stripe-named-primitives
branch
from
July 21, 2026 15:24
01c0fbc to
4c4db87
Compare
ygrishajev
force-pushed
the
refactor/billing-stripe-client-composition
branch
2 times, most recently
from
July 21, 2026 15:31
707bed7 to
01ba888
Compare
Base automatically changed from
refactor/billing-stripe-named-primitives
to
main
July 21, 2026 15:41
ygrishajev
force-pushed
the
refactor/billing-stripe-client-composition
branch
from
July 21, 2026 15:49
01ba888 to
fda0c54
Compare
Contributor
Author
|
@coderabbitai review the PR |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Repository guidance ==='
fd -HI '^CLAUDE\.md$' . -x sh -c 'echo "--- $1"; cat "$1"' sh {} || true
echo '=== PR changed files and stat ==='
git diff --stat HEAD^ HEAD
git diff --name-status HEAD^ HEAD
echo '=== TypeScript outlines for changed files ==='
git diff --name-only --diff-filter=ACMR HEAD^ HEAD | while IFS= read -r file; do
case "$file" in
*.ts|*.tsx)
echo "--- $file"
ast-grep outline "$file" --items all || true
;;
esac
done
echo '=== Full PR diff ==='
git diff --find-renames --find-copies --unified=80 HEAD^ HEADLength of output: 170119 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Billing provider bootstrap/import paths ==='
rg -n -C 3 --type ts '(?:import\s+["'\'']`@src/billing/providers`|import\s+["'\''].*billing/providers|from\s+["'\'']`@src/billing/providers`)' apps/api
echo '=== StripeService construction, resolution, mocks, and imports ==='
rg -n -C 3 --type ts '(?:new\s+StripeService\s*\(|container\.resolve\s*\(\s*StripeService|mock\s*<\s*StripeService\s*>|StripeService)' apps/api \
-g '!apps/api/src/billing/services/stripe/stripe.service.spec.ts'
echo '=== References to former StripeService SDK resources ==='
rg -n -C 2 --type ts '(?:StripeService|stripeService)\.(?:customers|paymentIntents|paymentMethods|charges|coupons|promotionCodes|prices|invoices|invoiceItems|refunds|setupIntents|webhooks)\b' apps/api || true
echo '=== Provider and service constructor ==='
cat -n apps/api/src/billing/providers/stripe-client.provider.ts
sed -n '45,125p' apps/api/src/billing/services/stripe/stripe.service.ts
echo '=== All injected Stripe client calls in service ==='
rg -n -C 1 --type ts 'this\.stripe\.' apps/api/src/billing/services/stripe/stripe.service.ts
echo '=== API provider index imports ==='
fd -HI 'index.ts' apps/api/src -p | while IFS= read -r file; do
if rg -q '`@src/billing/providers`|billing/providers' "$file"; then
echo "--- $file"
cat -n "$file"
fi
doneLength of output: 55341 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== API entry points and provider registration imports ==='
fd -HI -t f -e ts . apps/api/src | while IFS= read -r file; do
if rg -q 'providers/index|`@src/`.*/providers["'\'']|import\s+["'\'']`@src/`.*/providers' "$file"; then
echo "--- $file"
rg -n -C 2 'providers/index|`@src/`.*/providers|import\s+["'\'']`@src/`.*/providers' "$file"
fi
done
echo '=== Comparable tsyringe SDK/client providers ==='
fd -HI -t f -e ts '.*provider\.ts$' apps/api/src | while IFS= read -r file; do
if rg -q 'instancePerContainerCachingFactory|useFactory.*new |InjectionToken<' "$file"; then
echo "--- $file"
cat -n "$file"
fi
done
echo '=== Entire-repository inheritance and raw SDK resource checks ==='
rg -n -C 2 --glob '*.{ts,tsx,js,mjs,cjs}' '(?:extends\s+Stripe\b|new\s+StripeService\s*\()' . || true
rg -n -C 2 --glob '*.{ts,tsx,js,mjs,cjs}' '(?:StripeService|stripeService)\.(?:customers|paymentIntents|paymentMethods|charges|coupons|promotionCodes|prices|invoices|invoiceItems|refunds|setupIntents|webhooks)\b' . || true
echo '=== Exact non-test implementation patch ==='
git diff --unified=12 HEAD^ HEAD -- \
apps/api/src/billing/providers/index.ts \
apps/api/src/billing/providers/stripe-client.provider.ts \
apps/api/src/billing/services/stripe/stripe.service.tsLength of output: 73730 @ygrishajev: Reviewed the DI refactor. I found no blocking correctness, security, or concurrency issues. The Stripe client provider follows existing per-container DI conventions, all SDK calls now consistently use the injected client, and no remaining callers rely on the removed inherited Stripe SDK surface. Static review only; I did not execute tests in this environment. ✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3498 +/- ##
==========================================
- Coverage 73.47% 72.42% -1.06%
==========================================
Files 1132 1043 -89
Lines 29527 27181 -2346
Branches 7410 6950 -460
==========================================
- Hits 21696 19686 -2010
+ Misses 6900 6593 -307
+ Partials 931 902 -29
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/billing/services/stripe/stripe.service.ts (2)
187-192: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not call Stripe inside
@WithTransaction().This method performs a Stripe request while the database transaction is open, then updates Stripe again before the transaction commits. A database rollback cannot undo the remote mutation, and slow Stripe calls hold database locks. Move remote calls outside the transaction and define explicit compensation/order for local and remote state.
As per path instructions, this
apps/apibilling path must not perform external API calls inside a database transaction.🤖 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/billing/services/stripe/stripe.service.ts` around lines 187 - 192, Refactor markPaymentMethodAsDefault so no Stripe operation occurs within the `@WithTransaction` transaction: move stripe.paymentMethods.retrieve and the subsequent remote update outside the transactional method, and define an explicit ordering/compensation flow that keeps local and Stripe default-payment-method state consistent if either operation fails. Keep the repository update transactional and ensure the apps/api billing path performs all external API calls outside database transactions.Source: Path instructions
681-688: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDelete draft invoices on rollback.
If the failure happens beforefinalizeInvoice,invoiceis still draft andvoidInvoicewill fail, leaving it behind. UsedeleteInvoicefor drafts andvoidInvoiceonly after finalization. apps/api/src/billing/services/stripe/stripe.service.ts:681-688🤖 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/billing/services/stripe/stripe.service.ts` around lines 681 - 688, Update the rollback handling in the surrounding Stripe invoice flow to use deleteInvoice when the invoice remains a draft before finalizeInvoice, and use voidInvoice only for finalized invoices. Preserve the existing isInvoiceRolledBack result handling and failure fallback while selecting the operation based on the invoice’s finalization state.
🧹 Nitpick comments (1)
apps/api/src/billing/providers/stripe-client.provider.ts (1)
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove bootstrap registration to the application provider registry.
This module mutates the global tsyringe container at import time, but lives under
src/billing/providers/. Keep the token/factory here and register the provider fromapps/api/src/providers/.As per path instructions, tsyringe bootstrap side effects must be registered in
src/providers/.🤖 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/billing/providers/stripe-client.provider.ts` around lines 9 - 17, Remove the import-time container registration from the Stripe client provider module while preserving its STRIPE_CLIENT token and instancePerContainerCachingFactory definition. Add that provider registration to the application provider registry under src/providers/, ensuring bootstrap side effects occur there and the existing BillingConfigService-based Stripe construction remains unchanged.Source: Path instructions
🤖 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/billing/services/stripe/stripe.service.spec.ts`:
- Around line 418-425: Replace the `as unknown as Stripe.Response<...>` casts
used to construct Stripe charge-list test responses throughout the relevant
tests and `setup()` with the repository’s `mock<T>()` helper, using the
appropriate response type. Preserve each mock’s existing data and behavior,
including `data` and `has_more` values, and follow the existing `mock<T>()`
patterns in this spec.
- Around line 2009-2012: Replace the hardcoded "sk_test_123" argument in the
Stripe constructor within the test setup with a dynamically generated
alphanumeric value, matching the faker.string.alphanumeric(32) pattern already
used for the webhook secret in this spec. Keep the existing Stripe configuration
and StripeService construction unchanged.
- Around line 2093-2096: Update the default mock for paymentIntents.retrieve in
the test setup to resolve paymentIntentToReturn, matching the
paymentIntents.create mock and honoring params.paymentIntent overrides.
In `@apps/api/src/billing/services/stripe/stripe.service.ts`:
- Around line 649-655: Update the invoice item creation in the Stripe billing
flow to use the coupon’s currency, falling back to the existing STRIPE_CURRENCY
constant instead of hardcoding "usd". Keep the invoice item currency consistent
with the currency stored for the transaction.
---
Outside diff comments:
In `@apps/api/src/billing/services/stripe/stripe.service.ts`:
- Around line 187-192: Refactor markPaymentMethodAsDefault so no Stripe
operation occurs within the `@WithTransaction` transaction: move
stripe.paymentMethods.retrieve and the subsequent remote update outside the
transactional method, and define an explicit ordering/compensation flow that
keeps local and Stripe default-payment-method state consistent if either
operation fails. Keep the repository update transactional and ensure the
apps/api billing path performs all external API calls outside database
transactions.
- Around line 681-688: Update the rollback handling in the surrounding Stripe
invoice flow to use deleteInvoice when the invoice remains a draft before
finalizeInvoice, and use voidInvoice only for finalized invoices. Preserve the
existing isInvoiceRolledBack result handling and failure fallback while
selecting the operation based on the invoice’s finalization state.
---
Nitpick comments:
In `@apps/api/src/billing/providers/stripe-client.provider.ts`:
- Around line 9-17: Remove the import-time container registration from the
Stripe client provider module while preserving its STRIPE_CLIENT token and
instancePerContainerCachingFactory definition. Add that provider registration to
the application provider registry under src/providers/, ensuring bootstrap side
effects occur there and the existing BillingConfigService-based Stripe
construction remains unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1c60084-5c75-467f-b952-36f13d4d8401
📒 Files selected for processing (4)
apps/api/src/billing/providers/index.tsapps/api/src/billing/providers/stripe-client.provider.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/services/stripe/stripe.service.ts
ygrishajev
force-pushed
the
refactor/billing-stripe-client-composition
branch
from
July 21, 2026 16:15
fda0c54 to
ee8b77d
Compare
…instead of extending it StripeService no longer extends the Stripe SDK. It receives a Stripe client through a new STRIPE_CLIENT provider and reaches the SDK via this.stripe; external callers already use named methods (prior slice), so the SDK is now fully encapsulated. The service's own spec injects and mocks the client directly. Behavior-preserving.
ygrishajev
force-pushed
the
refactor/billing-stripe-client-composition
branch
from
July 21, 2026 16:16
ee8b77d to
bbf62b6
Compare
baktun14
approved these changes
Jul 21, 2026
This was referenced Jul 22, 2026
Merged
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.
Why
StripeServiceextends the Stripe SDK — it literally is aStripeinstance. So its public surface is the entire SDK, it can never be a real abstraction, and it can't be unit-tested in isolation (its own tests have to reach intoservice.paymentIntents,service.charges, … the inherited SDK resources).The prior PR (#3497) routed every external caller through named methods, so nothing outside the service touches the raw SDK anymore. This PR takes the next step: make the SDK an injected dependency instead of a base class. That encapsulates it (only our named operations are public) and makes the service properly testable — its spec now injects and mocks the client directly. It's also the prerequisite for the layered
StripeServicesplit that follows.Closes CON-720 · Part of CON-718. Stacked on #3497 — review/merge after it.
What
apps/api— behavior-preserving, no new runtime behavior, no migrations.STRIPE_CLIENTDI provider (a factory that builds theStripeclient from the billing config) and injects it intoStripeService, replacing the internalsuper(...)construction.StripeServiceno longerextends Stripe; every internal SDK call moves fromthis.<resource>tothis.stripe.<resource>.Stripeclient and mocks it (viavi.spyOn(stripe.<resource>, …)) instead of reaching through the service instance. Same assertions, same coverage — only the mock target changed.Verification
extends Stripecaused no fallout in the manymock<StripeService>()consumers).stripe.servicespec preserves every test (proved 0 removed vs base);stripe.controllerspec unaffected.tsc: no new errors in the changed files (pre-existing apps/api baseline unchanged).Summary by CodeRabbit