feat(billing): add server-side transaction resolution to simplify client polling#2791
Merged
ygrishajev merged 1 commit intomainfrom Feb 20, 2026
Merged
feat(billing): add server-side transaction resolution to simplify client polling#2791ygrishajev merged 1 commit intomainfrom
ygrishajev merged 1 commit intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughController and service changes propagate Stripe transaction metadata (transactionId and transactionStatus) through payment and coupon flows; a new resolveTransaction flow polls transactions with backoff/timeout when Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as StripeController
participant Service as StripeService
participant Repo as TransactionRepository
participant StripeAPI as Stripe API
participant Executor as resolveTransactionExecutor
Client->>Controller: applyCoupon(code, awaitResolved=true)
Controller->>Service: applyCoupon(user, code, awaitResolved)
Service->>Repo: create transaction record -> returns transactionId
Service->>StripeAPI: call Stripe (apply coupon / create intent)
StripeAPI-->>Service: immediate result (may be pending)
alt awaitResolved = true
Service->>Executor: resolveTransaction(transactionId)
Executor->>Repo: fetch transaction status (poll)
Repo-->>Executor: current status
loop until terminal or timeout
alt not terminal
Executor->>Executor: wait / backoff / retry
Executor->>Repo: fetch transaction status
Repo-->>Executor: current status
else terminal
Executor-->>Service: final status
end
end
end
Service-->>Controller: { ..., transactionId, transactionStatus }
Controller-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
ygrishajev
force-pushed
the
feature/billing
branch
2 times, most recently
from
7f03110 to
5224610
Compare
![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: 1
🧹 Nitpick comments (2)
apps/api/src/billing/services/stripe/stripe.service.ts (1)
489-513: ThelastTransactionclosure pattern is subtle but correct.The implementation captures
lastTransactionin the outer scope and updates it inside the executor callback (line 500). This allows returning the last known state on timeout. However, this pattern could be confusing to future maintainers.Consider adding a brief comment explaining this intentional side-effect:
Suggested clarification
async resolveTransaction(transactionId: string): Promise<StripeTransactionOutput> { await wait(4_000); + // Track last fetched transaction to return on timeout (updated inside executor) let lastTransaction: StripeTransactionOutput | undefined; try { lastTransaction = await this.resolveTransactionExecutor.execute(async () => { const transaction = await this.stripeTransactionRepository.findById(transactionId); assert(transaction, 404, "Transaction not found"); + // Side-effect: update outer variable for timeout fallback lastTransaction = transaction; return transaction; }); } catch (error) { if (!(error instanceof TaskCancelledError)) { throw error; } + // On timeout, fall through to return lastTransaction }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/billing/services/stripe/stripe.service.ts` around lines 489 - 513, In resolveTransaction, the outer-scoped variable lastTransaction is intentionally mutated inside the resolveTransactionExecutor.execute callback to allow returning the last known state on TaskCancelledError/timeouts; add a concise inline comment above the declaration of lastTransaction (and/or above the execute call) explaining that this closure-side effect is deliberate: it captures the last fetched transaction inside the executor so that, if the task is cancelled, the method can assert and return the most recent transaction instead of undefined; reference resolveTransaction, lastTransaction, and resolveTransactionExecutor.execute in the comment for clarity.apps/api/src/billing/controllers/stripe/stripe.controller.ts (1)
117-122: Consider documenting the potential request duration forawaitResolved.When
awaitResolvedis true, the request will block for at least 4 seconds (initial delay) and potentially up to 64+ seconds (4s delay + 60s timeout). This could cause HTTP timeouts on some clients or load balancers.Consider:
- Documenting this behavior in API docs/schema
- Ensuring downstream infrastructure (load balancers, proxies) supports long-running requests for these endpoints
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/billing/controllers/stripe/stripe.controller.ts` around lines 117 - 122, The endpoint's awaitResolved flag causes the controller to call this.stripe.resolveTransaction which blocks for an initial ~4s and can extend to ~64s+ (4s delay + 60s timeout); update the API schema/docs for the parameter awaitResolved and the relevant Stripe controller method to state the expected blocking duration and timeout behavior, and add a developer note advising clients and infra (load balancers/proxies) must support long-running requests or use an asynchronous pattern (webhooks/polling); additionally, confirm any public API contract/code comments around resolveTransaction reflect these timing constraints so callers are aware.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/billing/services/stripe/stripe.service.spec.ts`:
- Around line 1416-1463: Replace all `as any` casts in the resolveTransaction
tests by constructing properly typed Transaction objects (import or define the
Transaction/StripeTransaction type used by stripeTransactionRepository and use
those types for pendingTransaction, succeededTransaction, terminalTransaction,
failedTransaction) and pass them to
stripeTransactionRepository.findById.mockResolvedValue / mockResolvedValueOnce;
in the "throws 404 when transaction is not found" test, assert the rejection is
a 404 by expecting the specific error shape or instance (e.g., await
expect(...).rejects.toMatchObject({ status: 404 }) or
expect(...).rejects.toThrowError(/404|Not Found/) depending on the error type
thrown by resolveTransaction) instead of the generic rejects.toThrow(), and
update imports/types as needed to compile.
---
Nitpick comments:
In `@apps/api/src/billing/controllers/stripe/stripe.controller.ts`:
- Around line 117-122: The endpoint's awaitResolved flag causes the controller
to call this.stripe.resolveTransaction which blocks for an initial ~4s and can
extend to ~64s+ (4s delay + 60s timeout); update the API schema/docs for the
parameter awaitResolved and the relevant Stripe controller method to state the
expected blocking duration and timeout behavior, and add a developer note
advising clients and infra (load balancers/proxies) must support long-running
requests or use an asynchronous pattern (webhooks/polling); additionally,
confirm any public API contract/code comments around resolveTransaction reflect
these timing constraints so callers are aware.
In `@apps/api/src/billing/services/stripe/stripe.service.ts`:
- Around line 489-513: In resolveTransaction, the outer-scoped variable
lastTransaction is intentionally mutated inside the
resolveTransactionExecutor.execute callback to allow returning the last known
state on TaskCancelledError/timeouts; add a concise inline comment above the
declaration of lastTransaction (and/or above the execute call) explaining that
this closure-side effect is deliberate: it captures the last fetched transaction
inside the executor so that, if the task is cancelled, the method can assert and
return the most recent transaction instead of undefined; reference
resolveTransaction, lastTransaction, and resolveTransactionExecutor.execute in
the comment for clarity.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2791 +/- ##
==========================================
- Coverage 53.64% 52.91% -0.73%
==========================================
Files 1019 984 -35
Lines 23591 22783 -808
Branches 5754 5662 -92
==========================================
- Hits 12655 12056 -599
+ Misses 9531 9333 -198
+ Partials 1405 1394 -11
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ygrishajev
force-pushed
the
feature/billing
branch
from
5224610 to
48c03a0
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/billing/services/stripe/stripe.service.spec.ts (1)
1471-1472: Pre-existinganytypes in setup function.The setup function contains
anytypes (paymentMethodValidation?: any[]at line 1471 andlet lastUser: anyat line 1509) which violate coding guidelines. While these are pre-existing and not introduced by this PR, consider addressing them in a follow-up.As per coding guidelines: "Never use type
anyor cast to typeany. Always define the proper TypeScript types."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/billing/services/stripe/stripe.service.spec.ts` around lines 1471 - 1472, The tests use the unsafe type annotations paymentMethodValidation?: any[] in the setup function and let lastUser: any; replace these with concrete types: define or import the appropriate types (e.g., PaymentMethodValidation[], Stripe.PaymentMethod[], User, Partial<User> or TestUser) and annotate paymentMethodValidation and lastUser accordingly; update the setup function signature and the declaration of lastUser to use those types and adjust any usages in stripe.service.spec.ts to satisfy the stronger types (create minimal test interfaces if needed and import existing domain types instead of using any).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/billing/services/stripe/stripe.service.spec.ts`:
- Around line 1471-1472: The tests use the unsafe type annotations
paymentMethodValidation?: any[] in the setup function and let lastUser: any;
replace these with concrete types: define or import the appropriate types (e.g.,
PaymentMethodValidation[], Stripe.PaymentMethod[], User, Partial<User> or
TestUser) and annotate paymentMethodValidation and lastUser accordingly; update
the setup function signature and the declaration of lastUser to use those types
and adjust any usages in stripe.service.spec.ts to satisfy the stronger types
(create minimal test interfaces if needed and import existing domain types
instead of using any).
…ent polling Introduce `awaitResolved` flag on payment and coupon endpoints so the client can rely on the server to poll for terminal transaction status instead of implementing its own balance-based polling logic.
ygrishajev
force-pushed
the
feature/billing
branch
from
48c03a0 to
0e3a003
Compare
baktun14
approved these changes
Feb 19, 2026
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
Currently clients poll for balance changes to determine when a payment or coupon has been processed. Comparing balances is unreliable — concurrent transactions or timing issues can cause incorrect results. By exposing transaction status and allowing the server to poll for terminal state, the client can rely on a definitive status rather than balance diffs.
What
awaitResolvedflag to payment and coupon request schemasresolveTransactionmethod using cockatiel retry + timeout to poll DB for terminal transaction status (4s initial delay, 500ms interval, 60s timeout)transactionIdandtransactionStatusin payment and coupon responsesresolveTransactionSummary by CodeRabbit
New Features
Tests