Conversation
WalkthroughThe cancelPhoneNumberSubscription flow in packages/features/calAIPhone/providers/retellAI/services/BillingService.ts now imports zod and defines a Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
keithwillcode
added
core
dosubot
bot
added
billing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (1)
142-151: Tighten error typing and add idempotency key to the Stripe cancel call.
- Use
unknown+ a small type guard to avoidanyin the catch and make thecodecheck robust.- Add an
idempotencyKeyto the cancel request to protect against network retries or concurrent attempts.- } catch (error: any) { - if (error.code === 'resource_missing') { + } catch (error: unknown) { + const code = + typeof error === "object" && error !== null && "code" in error + ? // eslint-disable-next-line @typescript-eslint/no-explicit-any + (error as any).code + : undefined; + if (code === "resource_missing") { needsStripeCancellation = false; this.logger.warn("Subscription not found in Stripe:", { subscriptionId: phoneNumber.stripeSubscriptionId, phoneNumberId, }); } else { throw error; } } if (needsStripeCancellation) { - await stripe.subscriptions.cancel(phoneNumber.stripeSubscriptionId); + await stripe.subscriptions.cancel( + phoneNumber.stripeSubscriptionId, + undefined, + { idempotencyKey: `cancel:${phoneNumber.stripeSubscriptionId}` } + ); }Also applies to: 154-156
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts (4)
25-26: Remove duplicate default mock for subscriptions.retrieve.
retrieveis mocked both in the module factory and again inbeforeEach. Keep one (preferbeforeEach) to avoid confusing test setup.vi.mock("@calcom/features/ee/payments/server/stripe", () => ({ default: { checkout: { sessions: { create: vi.fn().mockResolvedValue({ url: "https://checkout.stripe.com/session-123", }), }, }, subscriptions: { cancel: vi.fn().mockResolvedValue({}), - retrieve: vi.fn().mockResolvedValue({ status: "active" }), }, }, })); // ... beforeEach(async () => { vi.clearAllMocks(); mocks = setupBasicMocks(); const stripe = (await import("@calcom/features/ee/payments/server/stripe")).default; stripe.checkout.sessions.create.mockResolvedValue({ url: "https://checkout.stripe.com/session-123", }); stripe.subscriptions.cancel.mockResolvedValue({}); stripe.subscriptions.retrieve.mockResolvedValue({ status: "active" });Also applies to: 51-52
260-291: Assert Retell deletion also runs in “already canceled in Stripe” path.The service still deletes the phone number from Retell on idempotent paths. Add an assertion to guard this behavior.
expect(mocks.mockPhoneNumberRepository.updateSubscriptionStatus).toHaveBeenCalledWith({ id: 1, subscriptionStatus: PhoneNumberSubscriptionStatus.CANCELLED, disconnectOutboundAgent: true, }); + expect(mocks.mockRetellRepository.deletePhoneNumber).toHaveBeenCalledWith("+1234567890");
293-324: Also assert Retell deletion when Stripe returns resource_missing.Strengthens coverage of the idempotent “not found” path.
expect(mocks.mockPhoneNumberRepository.updateSubscriptionStatus).toHaveBeenCalledWith({ id: 1, subscriptionStatus: PhoneNumberSubscriptionStatus.CANCELLED, disconnectOutboundAgent: true, }); + expect(mocks.mockRetellRepository.deletePhoneNumber).toHaveBeenCalledWith("+1234567890");
326-348: Ensure Retell deletion is not attempted on retrieve failure.On non-resource_missing retrieve errors, the service throws before DB/Retell effects. Assert Retell delete wasn’t called.
// Should NOT attempt to cancel due to error expect(stripe.subscriptions.cancel).not.toHaveBeenCalled(); // Should NOT update database due to error expect(mocks.mockPhoneNumberRepository.updateSubscriptionStatus).not.toHaveBeenCalled(); + // Should NOT attempt Retell deletion + expect(mocks.mockRetellRepository.deletePhoneNumber).not.toHaveBeenCalled();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts(1 hunks)packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.tspackages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.tspackages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.tspackages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts:13-24
Timestamp: 2025-08-21T16:34:10.839Z
Learning: In calcom/cal.com PR #22995, the deletePhoneNumber function in packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts is only used for imported phone numbers that don't have active Stripe subscriptions. Purchased phone numbers with subscriptions use a separate cancellation flow first (per maintainer Udit-takkar).
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
📚 Learning: 2025-08-21T16:34:10.839Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts:13-24
Timestamp: 2025-08-21T16:34:10.839Z
Learning: In calcom/cal.com PR #22995, the deletePhoneNumber function in packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts is only used for imported phone numbers that don't have active Stripe subscriptions. Purchased phone numbers with subscriptions use a separate cancellation flow first (per maintainer Udit-takkar).
Applied to files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
🧬 Code graph analysis (2)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (2)
packages/app-store/_utils/stripe.ts (1)
stripe(70-72)apps/api/v2/src/lib/logger.bridge.ts (1)
error(77-79)
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts (1)
packages/features/calAIPhone/providers/retellAI/services/__tests__/test-utils.ts (2)
createMockPhoneNumberRecord(66-79)TestError(194-199)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (1)
132-156: Good guard: prevent redundant/erroneous Stripe cancellation.Pre-retrieving the subscription and skipping cancel on "canceled" or "resource_missing" aligns with the PR goal and makes the flow idempotent-enough for common retries.
Please confirm Stripe errors surface
code: "resource_missing"from the wrapper at @calcom/features/ee/payments/server/stripe in your env (API version 2020-08-27 snippet suggests yes). If that wrapper normalizes errors differently, we should adjust the check.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
| await stripe.subscriptions.cancel(phoneNumber.stripeSubscriptionId); | ||
| try { | ||
| await stripe.subscriptions.cancel(phoneNumber.stripeSubscriptionId); | ||
| } catch (error: any) { |
There was a problem hiding this comment.
} catch (error: unknown) {
if (error instanceof Error && error.code === "resource_missing" /* ideally this is a enum of sorts? */) {
..
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (1)
143-157: Use a resilient guard for Stripe “resource_missing”; log useful fieldsReplace the zod-parse branch with a simple predicate that checks
code(andraw.code). Also surfacerequest_log_urlwhen present to speed up Stripe dashboard triage.- try { - await stripe.subscriptions.cancel(phoneNumber.stripeSubscriptionId); - } catch (error) { - const parsedError = stripeResourceMissingErrorSchema.safeParse(error); - - if (parsedError.success) { - this.logger.info("Subscription not found in Stripe (already cancelled or deleted):", { - subscriptionId: phoneNumber.stripeSubscriptionId, - phoneNumberId, - stripeMessage: parsedError.data.message, - }); - } else { - throw error; - } - } + try { + await stripe.subscriptions.cancel(phoneNumber.stripeSubscriptionId); + } catch (error) { + if (isResourceMissingStripeError(error)) { + const err: any = error; + this.logger.info("Subscription not found in Stripe (already cancelled or deleted):", { + subscriptionId: phoneNumber.stripeSubscriptionId, + phoneNumberId, + stripeMessage: err?.message, + stripeRequestLogUrl: err?.request_log_url ?? err?.raw?.request_log_url, + }); + } else { + throw error; + } + }Also consider hoisting
"resource_missing"into a central enum/const to avoid stringly‑typed checks across the codebase.
🧹 Nitpick comments (1)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (1)
1-1: Avoid pulling zod for single error-shape checkIf the above guard is adopted, drop the
zodimport to keep the service lean.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts:13-24
Timestamp: 2025-08-21T16:34:10.839Z
Learning: In calcom/cal.com PR #22995, the deletePhoneNumber function in packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts is only used for imported phone numbers that don't have active Stripe subscriptions. Purchased phone numbers with subscriptions use a separate cancellation flow first (per maintainer Udit-takkar).
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts:118-143
Timestamp: 2025-08-08T09:29:11.681Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (TypeScript), the AIPhoneServiceAgentListItem is required to include user.email in listAgents responses (per maintainer Udit-takkar). Future reviews should not flag this as unnecessary PII unless requirements change.
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
📚 Learning: 2025-08-21T16:34:10.839Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts:13-24
Timestamp: 2025-08-21T16:34:10.839Z
Learning: In calcom/cal.com PR #22995, the deletePhoneNumber function in packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts is only used for imported phone numbers that don't have active Stripe subscriptions. Purchased phone numbers with subscriptions use a separate cancellation flow first (per maintainer Udit-takkar).
Applied to files:
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
🧬 Code graph analysis (1)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (1)
packages/app-store/_utils/stripe.ts (1)
stripe(70-72)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
Show resolved
Hide resolved
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
Outdated
Show resolved
Hide resolved
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
Looks good to me, significantly better handling than before and should fix 500 error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts (2)
257-293: Also assert Retell deletion on Stripe 404 path.We’re treating Stripe
resource_missingas success; the test should verify we still delete the Retell phone number. Add an assertion to prevent regressions.Apply this diff:
expect(mocks.mockPhoneNumberRepository.updateSubscriptionStatus).toHaveBeenCalledWith({ id: 1, subscriptionStatus: PhoneNumberSubscriptionStatus.CANCELLED, disconnectOutboundAgent: true, }); + expect(mocks.mockRetellRepository.deletePhoneNumber).toHaveBeenCalledWith("+1234567890");Nit: include optional request_log_url in the mocked error for future‑proofing.
Schema allows it; adding reduces brittleness if validation tightens later.
stripe.subscriptions.cancel.mockRejectedValue({ type: "invalid_request_error", code: "resource_missing", message: "No such subscription: 'sub_123'", param: "id", doc_url: "https://stripe.com/docs/error-codes/resource-missing", + request_log_url: "https://dashboard.stripe.com/test/logs/req_123", });
295-315: Assert Retell deletion is not attempted on non‑resource_missing failures.We already assert no DB update; also guard that Retell deletion isn’t called when cancellation fails.
Apply this diff:
await expect(service.cancelPhoneNumberSubscription(validCancelData)).rejects.toThrow( "Failed to cancel subscription" ); // Should attempt to cancel expect(stripe.subscriptions.cancel).toHaveBeenCalledWith("sub_123"); // Should NOT update database due to error expect(mocks.mockPhoneNumberRepository.updateSubscriptionStatus).not.toHaveBeenCalled(); + // Should NOT delete Retell number on failure + expect(mocks.mockRetellRepository.deletePhoneNumber).not.toHaveBeenCalled();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts(3 hunks)packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/calAIPhone/providers/retellAI/services/BillingService.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts:13-24
Timestamp: 2025-08-21T16:34:10.839Z
Learning: In calcom/cal.com PR #22995, the deletePhoneNumber function in packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts is only used for imported phone numbers that don't have active Stripe subscriptions. Purchased phone numbers with subscriptions use a separate cancellation flow first (per maintainer Udit-takkar).
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts:118-143
Timestamp: 2025-08-08T09:29:11.681Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (TypeScript), the AIPhoneServiceAgentListItem is required to include user.email in listAgents responses (per maintainer Udit-takkar). Future reviews should not flag this as unnecessary PII unless requirements change.
🧬 Code graph analysis (1)
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts (1)
packages/features/calAIPhone/providers/retellAI/services/__tests__/test-utils.ts (2)
createMockPhoneNumberRecord(66-79)TestError(194-199)
E2E results are ready! |
3 participants
What does this PR do?
Don't delete from stripe if it doesn't exist
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?