Conversation
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
keithwillcode
added
core
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
billing
Udit-takkar
changed the title
![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 (2)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (2)
15-22: Update error schema to match actual Stripe error structure.The current schema expects both a top-level
codefield and nestedraw.codefield, but based on the test file (lines 276-286), the actual Stripe error structure only has the nestedraw.codefield.const stripeErrorSchema = z.object({ type: z.string(), raw: z.object({ code: z.literal("resource_missing"), type: z.literal("invalid_request_error"), }), - code: z.literal("resource_missing"), });
168-173: Fix typo in comment.There's a typo in the comment that should be corrected for clarity.
- // Disconnnect agent after cancelling from stripe + // Disconnect agent after cancelling from stripe
📜 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 (3)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts(2 hunks)packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts(4 hunks)packages/features/ee/billing/api/webhook/_customer.subscription.deleted.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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.tspackages/features/ee/billing/api/webhook/_customer.subscription.deleted.tspackages/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/__tests__/BillingService.test.tspackages/features/ee/billing/api/webhook/_customer.subscription.deleted.tspackages/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/__tests__/BillingService.test.tspackages/features/ee/billing/api/webhook/_customer.subscription.deleted.tspackages/features/calAIPhone/providers/retellAI/services/BillingService.ts
**/*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
🧠 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/__tests__/BillingService.test.tspackages/features/ee/billing/api/webhook/_customer.subscription.deleted.tspackages/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/__tests__/BillingService.test.tspackages/features/ee/billing/api/webhook/_customer.subscription.deleted.tspackages/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)
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
packages/features/ee/billing/api/webhook/_customer.subscription.deleted.ts (1)
75-78: LGTM! Good defensive check to prevent duplicate processing.The early return when the subscription is already cancelled prevents unnecessary processing and potential side effects. The inclusion of the
skipped: trueflag provides clear indication that the webhook was intentionally bypassed.packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (2)
143-147: LGTM! Optimistic cancellation improves user experience.The pre-cancellation update ensures the subscription appears cancelled immediately, providing better UX even if the Stripe API call takes time or fails in a recoverable way.
152-166: LGTM! Robust error handling with proper rollback.The error handling correctly distinguishes between resource-missing errors (which are treated as already cancelled) and other errors (which trigger a rollback to ACTIVE status). The logging provides good visibility into the error handling flow.
packages/features/calAIPhone/providers/retellAI/services/__tests__/BillingService.test.ts (4)
165-176: LGTM! Tests correctly validate the two-phase cancellation flow.The test expectations accurately reflect the new behavior where the subscription is first marked as cancelled with
disconnectOutboundAgent: false, then updated again withdisconnectOutboundAgent: true.
276-286: LGTM! Error structure matches expected Stripe API format.The test error structure correctly represents the actual Stripe API error format for resource_missing errors, providing comprehensive coverage of the error handling path.
297-309: LGTM! Comprehensive validation of resource_missing error handling.The test correctly validates that resource_missing errors are handled gracefully with two sequential database updates, ensuring the subscription ends up in the CANCELLED state with the agent disconnected.
329-339: LGTM! Proper validation of error rollback behavior.The test correctly validates that non-resource_missing errors trigger a rollback to ACTIVE status after the initial optimistic cancellation, ensuring data consistency when Stripe operations fail.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (5)
15-19: Broaden Stripe error parsing; current schema is too narrow.Stripe errors may expose
codeat the top level or underraw.code. Restricting toraw.codecan misclassifyresource_missing.Apply:
-const stripeErrorSchema = z.object({ - raw: z.object({ - code: z.string(), - }), -}); +const stripeErrorSchema = z.object({ + code: z.string().optional(), + raw: z + .object({ + code: z.string().optional(), + }) + .optional(), +});
149-151: Use a unified error code extraction (top‑level or raw) and add idempotency.Make detection resilient:
- const parsedError = stripeErrorSchema.safeParse(error); - if (parsedError.success && parsedError.data.raw.code === "resource_missing") { + const parsedError = stripeErrorSchema.safeParse(error); + const errCode = parsedError.success + ? parsedError.data.code ?? parsedError.data.raw?.code + : undefined; + if (errCode === "resource_missing") {Additionally, consider passing an idempotency key when canceling (outside this hunk):
await stripe.subscriptions.cancel( phoneNumber.stripeSubscriptionId, undefined, { idempotencyKey: `ph-sub-cancel:${phoneNumber.stripeSubscriptionId}` } );
151-155: Log the actual Stripe error message for diagnosability.Replace the fixed string with the message when available:
- stripeMessage: "Subscription resource not found", + stripeMessage: + (error as any)?.raw?.message ?? (error as any)?.message ?? "Subscription resource not found",
157-160: Explicitly restore disconnect flag on revert.If you retain the optimistic pre‑write, ensure the revert also restores
disconnectOutboundAgent:- await this.phoneNumberRepository.updateSubscriptionStatus({ - id: phoneNumberId, - subscriptionStatus: PhoneNumberSubscriptionStatus.ACTIVE, - }); + await this.phoneNumberRepository.updateSubscriptionStatus({ + id: phoneNumberId, + subscriptionStatus: PhoneNumberSubscriptionStatus.ACTIVE, + disconnectOutboundAgent: false, + });Please confirm
updateSubscriptionStatusperforms partial updates; if it replaces the whole record, this becomes mandatory.
165-165: Fix typo in comment.- // Disconnnect agent after cancelling from stripe + // Disconnect agent after cancelling from Stripe
📜 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(2 hunks)packages/features/ee/billing/api/webhook/_customer.subscription.deleted.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/ee/billing/api/webhook/_customer.subscription.deleted.ts
🧰 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#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 (1)
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)
🔇 Additional comments (1)
packages/features/calAIPhone/providers/retellAI/services/BillingService.ts (1)
139-145: ```shell
#!/usr/bin/env bashSearch repository for relevant occurrences and print the BillingService file for inspection.
Safe: only reads files, no builds or executions.
set -uo pipefail
echo "PWD: $(pwd)"
echoif command -v rg >/dev/null 2>&1; then
RG=rg
else
RG=""
fiecho "=== Searching for target file path ==="
if [ -n "$RG" ]; then
$RG -n --hidden --no-ignore-vcs "packages/features/calAIPhone/providers/retellAI/services/BillingService.ts" || true
else
find . -type f -path "*packages/features/calAIPhone/providers/retellAI/services/BillingService.ts" -print || true
fi
echoecho "=== Searching for updateSubscriptionStatus occurrences ==="
if [ -n "$RG" ]; then
$RG -n --hidden --no-ignore-vcs -S "updateSubscriptionStatus" || true
else
grep -RIn --line-number "updateSubscriptionStatus" packages || true
fi
echoecho "=== Searching for PhoneNumberSubscriptionStatus occurrences ==="
if [ -n "$RG" ]; then
$RG -n --hidden --no-ignore-vcs -S "PhoneNumberSubscriptionStatus" || true
else
grep -RIn --line-number "PhoneNumberSubscriptionStatus" packages || true
fi
echoecho "=== Searching for phoneNumberRepository occurrences ==="
if [ -n "$RG" ]; then
$RG -n --hidden --no-ignore-vcs -S "phoneNumberRepository" || true
else
grep -RIn --line-number "phoneNumberRepository" packages || true
fi
echoecho "=== Searching for disconnectOutboundAgent occurrences ==="
if [ -n "$RG" ]; then
$RG -n --hidden --no-ignore-vcs -S "disconnectOutboundAgent" || true
else
grep -RIn --line-number "disconnectOutboundAgent" packages || true
fi
echoecho "=== Searching for 'cancel' (case-insensitive) inside calAIPhone package ==="
if [ -n "$RG" ]; then
$RG -n --hidden --no-ignore-vcs -S -i "cancel" packages/features/calAIPhone || true
else
grep -RIn --line-number -i "cancel" packages/features/calAIPhone || true
fi
echoecho "=== Searching for 'stripe' occurrences in repository ==="
if [ -n "$RG" ]; then
$RG -n --hidden --no-ignore-vcs -S "stripe" || true
else
grep -RIn --line-number "stripe" || true
fi
echoTARGET="packages/features/calAIPhone/providers/retellAI/services/BillingService.ts"
if [ -f "$TARGET" ]; then
echo "=== Showing $TARGET (1..400) ==="
sed -n '1,400p' "$TARGET" || true
echo
echo "=== Showing $TARGET (401..800) ==="
sed -n '401,800p' "$TARGET" || true
else
echo "File $TARGET not found."
fi</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
E2E results are ready! |
* fix: phone number billing bug * fix: phone number billing bug * fix: use raw code * fix: use zod * chore: update test * fix: double cancel * fix: double cancel * fix: double cancel * fix: chore update * fix: schema
3 participants
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?