perf: optimize payment app imports to avoid loading entire app store #23408
Conversation
- Add PaymentServiceMap generation to app-store-cli build process - Generate payment.services.generated.ts with lazy imports for 6 payment services - Update handlePayment.ts, deletePayment.ts, handlePaymentRefund.ts to use PaymentServiceMap - Update getConnectedApps.ts and tRPC payment routers to use PaymentServiceMap - Follow same pattern as analytics optimization in PR #23372 - Reduces bundle size by avoiding import of 100+ apps when only payment functionality needed Co-Authored-By: [email protected] <[email protected]>
- Remove obsolete appStoreMock line from bookingScenario.ts since handlePayment now uses PaymentServiceMap - Update setupVitest.ts to import prismaMock from correct PrismockClient instance - Add PaymentServiceMap mock following PR #22450 pattern for calendar services - Ensure MockPaymentService uses consistent externalId across test files - Fix webhook handler to return 200 status by ensuring payment records are found correctly Co-Authored-By: [email protected] <[email protected]>
…vi.spyOn() calls - Remove global prismaMock import from setupVitest.ts that was causing 'is not a spy' errors - Update MockPaymentService to import prismaMock locally to maintain payment test functionality - Fixes organization and outOfOffice tests while preserving payment service optimization Co-Authored-By: [email protected] <[email protected]>
- Payment services map now always includes all payment apps regardless of E2E environment - Ensures payment functionality is consistently available across all environments - Addresses CI failures caused by conditional payment service loading Co-Authored-By: [email protected] <[email protected]>
- Update app-store-cli to import directly from lib/PaymentService.ts files - Modify all payment handlers to access PaymentService directly - Update test mocks to match new direct import structure - Remove .lib property access pattern across payment system - Maintain backward compatibility while improving import efficiency Co-Authored-By: [email protected] <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughReplaces runtime resolution of payment providers by generating PaymentServiceMap (payment.services.generated.ts) in the app-store build and switching code to lazy-import PaymentService implementations from that map (replacing previous appStore indexing) across payment flows (handlePayment, handlePaymentRefund, deletePayment) and TRPC viewer/chargeCard handlers. Adds Vitest setup (setupVitest.ts) that registers a unified MockPaymentService and updates payment-provider mocks and generated PaymentServiceMap entries for tests; introduces a deterministic MOCK_PAYMENT_UID/externalId and removes auto-wiring of MockPaymentService into appStore in bookingScenario test utilities. Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
E2E results are ready! |
keithwillcode
changed the title
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/lib/payment/handlePayment.ts (2)
70-79: Fix collectCard signature mismatch
packages/lib/payment/handlePayment.ts:70-79 passes 5 args to collectCard, but IAbstractPaymentService.collectCard and all concrete implementations accept only (payment, bookingId). Remove or adapt the extra parameters—or extend the interface and update every provider—to restore type safety.
81-95: Aligncreatesignature across interface and all providers.
UpdateIAbstractPaymentService.createin packages/types/PaymentService.d.ts and everyPaymentServiceclass under packages/app-store/*/lib/PaymentService.ts to accept the same 10 parameters passed in handlePayment.ts.packages/trpc/server/routers/viewer/payments.tsx (1)
28-45: Use Prismaselectinstead ofincludeand only fetch needed fields.Reduces over-fetching and aligns with repo guidelines.
Apply:
- const booking = await prisma.booking.findUniqueOrThrow({ - where: { id: input.bookingId }, - include: { - payment: true, - user: { - select: { email: true, locale: true, name: true, timeZone: true }, - }, - attendees: true, - eventType: true, - }, - }); + const booking = await prisma.booking.findUniqueOrThrow({ + where: { id: input.bookingId }, + select: { + id: true, + title: true, + startTime: true, + endTime: true, + eventTypeId: true, + payment: { + select: { + id: true, + amount: true, + currency: true, + paymentOption: true, + appId: true, + success: true, + }, + }, + user: { select: { email: true, locale: true, name: true, timeZone: true } }, + attendees: { select: { name: true, email: true, timeZone: true, locale: true } }, + eventType: { select: { slug: true, customReplyToEmail: true, metadata: true } }, + }, + });packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (3)
28-38: Use Prismaselect(noinclude) and only fetch needed fields.
Complies with repo guidelines and avoids overfetching. Also make payment ordering deterministic if multiple rows exist.Apply:
- const booking = await prisma.booking.findUnique({ - where: { - id: input.bookingId, - }, - include: { - payment: true, - user: true, - attendees: true, - eventType: true, - }, - }); + const booking = await prisma.booking.findUnique({ + where: { id: input.bookingId }, + select: { + id: true, + title: true, + startTime: true, + endTime: true, + userPrimaryEmail: true, + payment: { + select: { + id: true, + appId: true, + success: true, + amount: true, + currency: true, + paymentOption: true, + data: true, + }, + orderBy: { id: "asc" }, + }, + user: { + select: { email: true, name: true, timeZone: true, locale: true }, + }, + attendees: { + select: { name: true, email: true, timeZone: true, locale: true }, + }, + eventType: { + select: { title: true, hideOrganizerEmail: true, metadata: true, teamId: true }, + }, + }, + });
44-49: Guard against missing payment records before indexing.
Indexing booking.payment[0] can throw when no record exists.Apply:
- if (booking.payment[0].success) { + if (!booking.payment?.length) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `No payment record found for booking ${booking.id}`, + }); + } + if (booking.payment[0].success) {
148-152: Bug: referencingattendeesListPromises[0]; should useattendeesList[0].
The current name suggests promises but holds plain objects; use the resolved list.Apply:
- await sendNoShowFeeChargedEmail( - attendeesListPromises[0], + await sendNoShowFeeChargedEmail( + attendeesList[0], evt, booking?.eventType?.metadata as EventTypeMetadata );
🧹 Nitpick comments (17)
packages/app-store-cli/src/build.ts (1)
422-427: Nit: importName is ignored when lazyImport=true.Passing
importName: "PaymentService"here is unused in the lazy path; consider removing to avoid confusion.packages/lib/getConnectedApps.ts (1)
186-194: Guard dynamic import, add default-export fallback, and avoid surfacing errors to UI.If a payment provider still defaults its export or throws on import, this could break the Apps page. Wrap in try/catch and fall back to
default.Apply this diff:
- const paymentAppImportFn = PaymentServiceMap[app.dirName as keyof typeof PaymentServiceMap]; - if (paymentAppImportFn) { - const paymentApp = await paymentAppImportFn; - if (paymentApp && "PaymentService" in paymentApp && paymentApp?.PaymentService) { - const PaymentService = paymentApp.PaymentService; - const paymentInstance = new PaymentService(credential); - isSetupAlready = paymentInstance.isSetupAlready(); - } - } + const importPromise = PaymentServiceMap[app.dirName as keyof typeof PaymentServiceMap]; + if (importPromise) { + try { + const paymentApp = await importPromise; + const Service = (paymentApp as any)?.PaymentService ?? (paymentApp as any)?.default; + if (Service) { + const paymentInstance = new Service(credential); + isSetupAlready = paymentInstance.isSetupAlready(); + } + } catch (e) { + // ignore; leave isSetupAlready as undefined + } + }apps/web/test/utils/bookingScenario/MockPaymentService.ts (1)
53-57: Remove the extra verification read or guard it behind a debug flag.Redundant DB read slows tests and adds noise. Keep logs minimal in hot test paths.
Apply:
- const verifyPayment = await prismaMock.payment.findFirst({ - where: { externalId: paymentCreateData.externalId }, - }); - logger.silly("Verified payment exists", JSON.stringify({ verifyPayment })); + // Optional: enable only when diagnosing flaky tests + // if (process.env.DEBUG_MOCK_PAYMENTS) { + // const verifyPayment = await prismaMock.payment.findFirst({ + // where: { externalId: paymentCreateData.externalId }, + // }); + // logger.silly("Verified payment exists", JSON.stringify({ verifyPayment })); + // }packages/app-store/payment.services.generated.ts (1)
5-12: Type the generated map to ensure module shape and catch missing exports at build time.Have the generator emit a typed object so consumers don’t need ad-hoc guards.
Consider emitting:
-export const PaymentServiceMap = { +export const PaymentServiceMap = { alby: import("./alby/lib/PaymentService"), btcpayserver: import("./btcpayserver/lib/PaymentService"), hitpay: import("./hitpay/lib/PaymentService"), "mock-payment-app": import("./mock-payment-app/lib/PaymentService"), paypal: import("./paypal/lib/PaymentService"), stripepayment: import("./stripepayment/lib/PaymentService"), -}; +} satisfies Record<string, Promise<{ PaymentService: new (...args: any[]) => unknown }>>;Note: update the CLI generator, not this file, to keep it auto-managed.
packages/lib/payment/handlePaymentRefund.ts (2)
18-24: Use the shared logger instead ofconsole.warnfor consistency and log routing.Apply:
+import logger from "@calcom/lib/logger"; @@ - if (!paymentAppImportFn) { - console.warn(`payment app not implemented for key: ${key}`); + if (!paymentAppImportFn) { + logger.warn(`payment app not implemented for key: ${key}`); return false; }
25-31: Harden module validation with the same type guard used elsewhere.Avoid duplicating ad-hoc checks; align with
isPaymentServicepattern from handlePayment.Apply:
+ const isPaymentService = (x: unknown): x is { PaymentService: any } => + !!x && typeof x === "object" && "PaymentService" in x && typeof (x as any).PaymentService === "function"; const paymentAppModule = await paymentAppImportFn; - if (!paymentAppModule?.PaymentService) { - console.warn(`payment App service not found for key: ${key}`); - return false; - } + if (!isPaymentService(paymentAppModule)) { + logger.warn(`payment App service not found for key: ${key}`); + return false; + }packages/lib/payment/handlePayment.ts (2)
51-55: Preferlogger.warnoverconsole.warn.Apply:
- if (!paymentAppImportFn) { - console.warn(`payment app not implemented for key: ${key}`); + if (!paymentAppImportFn) { + logger.warn(`payment app not implemented for key: ${key}`); return null; }
57-63: Unify module validation with the guard; use structured logging.Apply:
- if (!isPaymentService(paymentAppModule)) { - console.warn(`payment App service not found for key: ${key}`); + if (!isPaymentService(paymentAppModule)) { + logger.warn(`payment App service not found for key: ${key}`); return null; }packages/trpc/server/routers/viewer/payments.tsx (4)
96-104: Also narrow the credential query to required fields; avoid fetching secrets broadly.Fetch only
keyandapp.dirNameto minimize exposure.Apply:
- const paymentCredential = await prisma.credential.findFirst({ - where: { userId: ctx.user.id, appId: payment.appId }, - include: { app: true }, - }); + const paymentCredential = await prisma.credential.findFirst({ + where: { userId: ctx.user.id, appId: payment.appId }, + select: { + key: true, + app: { select: { dirName: true } }, + }, + });
116-122: Extract a shared type guard for module validation to reduce duplication.Mirror the
isPaymentServicehelper used in lib. Keeps checks uniform across call sites.Apply:
- const paymentApp = await paymentAppImportFn; - if (!(paymentApp && "PaymentService" in paymentApp && paymentApp?.PaymentService)) { + const paymentApp = await paymentAppImportFn; + const isPaymentService = (x: unknown): x is { PaymentService: any } => + !!x && typeof x === "object" && "PaymentService" in x && typeof (x as any).PaymentService === "function"; + if (!isPaymentService(paymentApp)) { throw new TRPCError({ code: "BAD_REQUEST", message: "Payment service not found" }); }
58-75: Minor cleanup: drop “Promises” naming and use the resolved list at send time.The items are not Promises; use
attendeesListfor clarity.Apply:
- const attendeesListPromises = []; - for (const attendee of booking.attendees) { - const attendeeObject = { - name: attendee.name, - email: attendee.email, - timeZone: attendee.timeZone, - language: { - translate: await getTranslation(attendee.locale ?? "en", "common"), - locale: attendee.locale ?? "en", - }, - }; - attendeesListPromises.push(attendeeObject); - } - const attendeesList = await Promise.all(attendeesListPromises); + const attendeesList = []; + for (const attendee of booking.attendees) { + attendeesList.push({ + name: attendee.name, + email: attendee.email, + timeZone: attendee.timeZone, + language: { + translate: await getTranslation(attendee.locale ?? "en", "common"), + locale: attendee.locale ?? "en", + }, + }); + } @@ - await sendNoShowFeeChargedEmail( - attendeesListPromises[0], + await sendNoShowFeeChargedEmail( + attendeesList[0], evt, booking?.eventType?.metadata as EventTypeMetadata );Also applies to: 148-152
47-55: Guard against missingpayment.
booking.payment[0]can be undefined; add an early check with a clear error.Apply:
- const payment = booking.payment[0]; + const payment = booking.payment[0]; + if (!payment) { + throw new TRPCError({ code: "BAD_REQUEST", message: "No payment found for booking" }); + }packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (2)
53-70: Build attendees list concurrently and remove misleading “Promises” naming.
Current code awaits per-iteration then wraps plain values in Promise.all. Simplify and parallelize lookups.Apply:
- const attendeesListPromises = []; - - for (const attendee of booking.attendees) { - const attendeeObject = { - name: attendee.name, - email: attendee.email, - timeZone: attendee.timeZone, - language: { - translate: await getTranslation(attendee.locale ?? "en", "common"), - locale: attendee.locale ?? "en", - }, - }; - - attendeesListPromises.push(attendeeObject); - } - - const attendeesList = await Promise.all(attendeesListPromises); + const attendeesList = await Promise.all( + booking.attendees.map(async (attendee) => ({ + name: attendee.name, + email: attendee.email, + timeZone: attendee.timeZone, + language: { + translate: await getTranslation(attendee.locale ?? "en", "common"), + locale: attendee.locale ?? "en", + }, + })) + );
128-131: Minor: rename variable; it’s a Promise, not a function.
Improves readability.Apply:
- const paymentAppImportFn = PaymentServiceMap[key as keyof typeof PaymentServiceMap]; + const paymentAppModulePromise = PaymentServiceMap[key as keyof typeof PaymentServiceMap]; if (!paymentAppImportFn) { throw new TRPCError({ code: "BAD_REQUEST", message: "Payment app not implemented" }); } - - const paymentApp = await paymentAppImportFn; + const paymentApp = await paymentAppModulePromise; if (!paymentApp?.PaymentService) { throw new TRPCError({ code: "BAD_REQUEST", message: "Payment service not found" }); }Also applies to: 134-139
setupVitest.ts (3)
60-66: Nit: unused privatecredentials.
If not needed, drop it; otherwise markreadonlyto clarify intent.- constructor(credentials?: any) { - this.credentials = credentials; - } - - private credentials: any; + constructor(private readonly credentials?: any) {}
103-107: Return a Payment-like object fromchargeCardto match handler expectations.
Handler returnspaymentData; tests may rely on fields like amount/currency.Apply:
- async chargeCard() { - return { success: true }; - } + async chargeCard(payment: any) { + return { ...payment, success: true }; + }
129-152: Redundant provider index mocks.
The runtime resolves via PaymentServiceMap; these mocks are unnecessary unless other tests import provider indexes directly.Would you like me to remove these to reduce setup noise?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
apps/web/test/utils/bookingScenario/MockPaymentService.ts(3 hunks)apps/web/test/utils/bookingScenario/bookingScenario.ts(1 hunks)packages/app-store-cli/src/build.ts(2 hunks)packages/app-store/payment.services.generated.ts(1 hunks)packages/lib/getConnectedApps.ts(2 hunks)packages/lib/payment/deletePayment.ts(2 hunks)packages/lib/payment/handlePayment.ts(2 hunks)packages/lib/payment/handlePaymentRefund.ts(2 hunks)packages/trpc/server/routers/viewer/payments.tsx(2 hunks)packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts(3 hunks)setupVitest.ts(2 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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:
apps/web/test/utils/bookingScenario/bookingScenario.tsvitest.config.tspackages/lib/payment/deletePayment.tspackages/app-store/payment.services.generated.tspackages/lib/payment/handlePaymentRefund.tsapps/web/test/utils/bookingScenario/MockPaymentService.tssetupVitest.tspackages/trpc/server/routers/viewer/payments/chargeCard.handler.tspackages/lib/getConnectedApps.tspackages/app-store-cli/src/build.tspackages/lib/payment/handlePayment.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:
apps/web/test/utils/bookingScenario/bookingScenario.tsvitest.config.tspackages/lib/payment/deletePayment.tspackages/app-store/payment.services.generated.tspackages/lib/payment/handlePaymentRefund.tsapps/web/test/utils/bookingScenario/MockPaymentService.tssetupVitest.tspackages/trpc/server/routers/viewer/payments/chargeCard.handler.tspackages/lib/getConnectedApps.tspackages/trpc/server/routers/viewer/payments.tsxpackages/app-store-cli/src/build.tspackages/lib/payment/handlePayment.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:
apps/web/test/utils/bookingScenario/bookingScenario.tsvitest.config.tspackages/lib/payment/deletePayment.tspackages/app-store/payment.services.generated.tspackages/lib/payment/handlePaymentRefund.tsapps/web/test/utils/bookingScenario/MockPaymentService.tssetupVitest.tspackages/trpc/server/routers/viewer/payments/chargeCard.handler.tspackages/lib/getConnectedApps.tspackages/trpc/server/routers/viewer/payments.tsxpackages/app-store-cli/src/build.tspackages/lib/payment/handlePayment.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:
apps/web/test/utils/bookingScenario/MockPaymentService.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/trpc/server/routers/viewer/payments.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
apps/web/test/utils/bookingScenario/bookingScenario.ts
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
setupVitest.ts
🧬 Code graph analysis (8)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
apps/web/test/utils/bookingScenario/MockPaymentService.ts (1)
getMockPaymentService(11-92)
packages/lib/payment/deletePayment.ts (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)
packages/lib/payment/handlePaymentRefund.ts (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)
setupVitest.ts (1)
packages/emails/email-manager.ts (1)
sendAwaitingPaymentEmailAndSMS(556-571)
packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (2)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)packages/app-store/stripepayment/lib/PaymentService.ts (1)
PaymentService(36-432)
packages/lib/getConnectedApps.ts (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)
packages/trpc/server/routers/viewer/payments.tsx (5)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)packages/app-store/alby/lib/PaymentService.ts (1)
PaymentService(17-141)packages/app-store/mock-payment-app/lib/PaymentService.ts (1)
PaymentService(7-141)packages/app-store/paypal/lib/PaymentService.ts (1)
PaymentService(23-200)packages/app-store/hitpay/lib/PaymentService.ts (1)
PaymentService(22-242)
packages/lib/payment/handlePayment.ts (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)
⏰ 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). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Type check / check-types
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (14)
packages/app-store-cli/src/build.ts (1)
451-451: LGTM: emits payment.services.generated.ts.Generation wiring looks correct and consistent with other generated files.
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
2108-2114: LGTM: mockPaymentApp now sources IDs from getMockPaymentService.Side-effects via appStore mocks are gone, aligning with PaymentServiceMap-based tests.
vitest.config.ts (1)
6-7: LGTM: setupVitest loaded before tests.Ensures mocks (including PaymentServiceMap) are in place.
packages/lib/getConnectedApps.ts (1)
148-156: Good: avoids leaking credential.key to callers.The destructuring pattern
key: _2ensures thecredential.keynever escapes. Complies with our guideline.apps/web/test/utils/bookingScenario/MockPaymentService.ts (3)
15-16: Deterministic IDs: good call for stable tests.Fixed IDs remove flakiness from UUID randomness and simplify assertions.
48-50: Awaiting the prisma mock create is correct.Prevents racing the subsequent assertions/logs against unresolved promises.
39-39: Verify booking tests after changingMockPaymentService.successdefault
Tests using thebookingScenarioutilities may assume payments succeed by default; search for imports ofMockPaymentServiceor usages ofbookingScenarioin apps/web/test and update fixtures/assertions to match the newsuccess: falsebehavior.packages/trpc/server/routers/viewer/payments.tsx (1)
110-114: Map-based resolver looks good.Keyed by
dirNameand early-fails when missing, which prevents accidental fall-through.packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (3)
1-1: Good move to map-based dynamic imports.
Switching to PaymentServiceMap aligns payments with calendar/analytics and improves tree-shaking.
13-13: Type usage is appropriate.
Narrowing to IAbstractPaymentService keeps the handler decoupled from app-specific implementations.
142-142: API alignment looks good.
Passing booking.id to chargeCard matches provider signature updates (e.g., Stripe).setupVitest.ts (3)
5-5: Type-only import is correct.
Keeps runtime clean and avoids bundling types.
58-59: Expose MOCK_PAYMENT_UID to tests if it’s meant to be global.
Currently module-scoped. If tests need it, attach to global.Apply:
const MOCK_PAYMENT_UID = "MOCK_PAYMENT_UID"; +(globalThis as any).MOCK_PAYMENT_UID = MOCK_PAYMENT_UID;
153-162: PaymentServiceMap mock aligns with new resolution path.
Good coverage across providers with a unified MockPaymentService.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
- Remove booking.id parameter from chargeCard calls in chargeCard.handler.ts and payments.tsx - Addresses GitHub feedback to investigate chargeCard signature changes in separate PR - Keeps all other direct PaymentService import refactor changes intact Co-Authored-By: [email protected] <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/trpc/server/routers/viewer/payments.tsx (2)
58-75: Build attendees list concurrently; current code awaits inside loop and misuses Promise.all on plain objects.
This reduces latency and fixes naming mismatch at send email.- const attendeesListPromises = []; - - for (const attendee of booking.attendees) { - const attendeeObject = { - name: attendee.name, - email: attendee.email, - timeZone: attendee.timeZone, - language: { - translate: await getTranslation(attendee.locale ?? "en", "common"), - locale: attendee.locale ?? "en", - }, - }; - - attendeesListPromises.push(attendeeObject); - } - - const attendeesList = await Promise.all(attendeesListPromises); + const attendeesList = await Promise.all( + booking.attendees.map(async (attendee) => ({ + name: attendee.name, + email: attendee.email, + timeZone: attendee.timeZone, + language: { + translate: await getTranslation(attendee.locale ?? "en", "common"), + locale: attendee.locale ?? "en", + }, + })) + ); @@ - await sendNoShowFeeChargedEmail( - attendeesListPromises[0], + await sendNoShowFeeChargedEmail( + attendeesList[0], evt, booking?.eventType?.metadata as EventTypeMetadata );Also applies to: 148-152
96-104: Use Prismaselect(notinclude) and fetch only fields needed.
Pull justkeyandapp.dirNameper guidelines; prevents accidental data exposure and shrinks payload.- const paymentCredential = await prisma.credential.findFirst({ + const paymentCredential = await prisma.credential.findFirst({ where: { userId: ctx.user.id, appId: payment.appId, }, - include: { - app: true, - }, + select: { + key: true, + app: { select: { dirName: true } }, + }, });Note: Constructor for PaymentService should only need
{ key }. If more fields are required, explicitly add them toselect.packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
53-70: Parallelize attendee translations and fix naming mismatch.
Avoid sequential awaits; use the computed list when emailing.- const attendeesListPromises = []; - - for (const attendee of booking.attendees) { - const attendeeObject = { - name: attendee.name, - email: attendee.email, - timeZone: attendee.timeZone, - language: { - translate: await getTranslation(attendee.locale ?? "en", "common"), - locale: attendee.locale ?? "en", - }, - }; - - attendeesListPromises.push(attendeeObject); - } - - const attendeesList = await Promise.all(attendeesListPromises); + const attendeesList = await Promise.all( + booking.attendees.map(async (attendee) => ({ + name: attendee.name, + email: attendee.email, + timeZone: attendee.timeZone, + language: { + translate: await getTranslation(attendee.locale ?? "en", "common"), + locale: attendee.locale ?? "en", + }, + })) + ); @@ - await sendNoShowFeeChargedEmail( - attendeesListPromises[0], + await sendNoShowFeeChargedEmail( + attendeesList[0], evt, booking?.eventType?.metadata as EventTypeMetadata );Also applies to: 148-152
♻️ Duplicate comments (2)
packages/trpc/server/routers/viewer/payments.tsx (1)
125-125: ConfirmchargeCardparameter shape across providers.
Current call passes the full Payment row; several implementations/interfaces may diverge.#!/bin/bash # Verify interface and implementations for chargeCard rg -nP -C2 'interface\s+IAbstractPaymentService\b.*chargeCard\s*\(' packages -g '!**/dist/**' --type=ts rg -nP -C2 '\bclass\s+PaymentService\b.*chargeCard\s*\(' packages/app-store -g '!**/dist/**' --type=tspackages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
142-142: Re-validatechargeCardsignature compatibility.
Call site passes a fullPaymentobject; interface/implementations may vary. Keep this as-is if reverting bookingId is intentional, but align providers ASAP to avoid runtime errors.#!/bin/bash # Inspect call sites and interfaces for consistency rg -nP -C2 '\bchargeCard\s*\(' packages -g '!**/dist/**' --type=ts rg -nP -C2 'IAbstractPaymentService\b' packages -g '!**/dist/**' --type=ts
🧹 Nitpick comments (3)
packages/trpc/server/routers/viewer/payments.tsx (2)
116-121: Tighten type guard and naming.
Name suggests function but it's a module; also guard can be simpler.- if (!(paymentApp && "PaymentService" in paymentApp && paymentApp?.PaymentService)) { + if (!paymentApp?.PaymentService) { throw new TRPCError({ code: "BAD_REQUEST", message: "Payment service not found" }); } - const PaymentService = paymentApp.PaymentService; + const { PaymentService } = paymentApp;
110-114: Localize error messages returned to clients.
These TRPCError messages are user-facing; consider wrapping with a translator (like tOrganizer) for consistency with other paths.Also applies to: 118-118
packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
124-126: Consider localizing early TRPC errors.
For consistency with the catch-path using tOrganizer, wrap these messages or return error codes only.Also applies to: 131-136
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/trpc/server/routers/viewer/payments.tsx(2 hunks)packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/trpc/server/routers/viewer/payments.tsx
**/*.{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/trpc/server/routers/viewer/payments.tsxpackages/trpc/server/routers/viewer/payments/chargeCard.handler.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/trpc/server/routers/viewer/payments.tsxpackages/trpc/server/routers/viewer/payments/chargeCard.handler.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/trpc/server/routers/viewer/payments/chargeCard.handler.ts
🧬 Code graph analysis (2)
packages/trpc/server/routers/viewer/payments.tsx (2)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)packages/app-store/alby/lib/PaymentService.ts (1)
PaymentService(17-141)
packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/payments.tsx (1)
3-3: Import switch to PaymentServiceMap looks good.
The new source aligns with the PR direction.packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
1-1: Import switch to PaymentServiceMap looks good.
Matches PR intent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/trpc/server/routers/viewer/payments.tsx (2)
125-125: Revert passing booking.id to chargeCard (maintainer requested).Aligns with “revisit in a separate PR.”
- const paymentData = await paymentInstance.chargeCard(payment, booking.id); + const paymentData = await paymentInstance.chargeCard(payment);
110-117: Not actually lazy-loading; entries are Promises executed at module load.Emit loader functions in PaymentServiceMap and invoke them here to defer imports.
- const key = paymentCredential?.app?.dirName; - const paymentAppImportFn = PaymentServiceMap[key as keyof typeof PaymentServiceMap]; - if (!paymentAppImportFn) { + const key = paymentCredential?.app?.dirName; + const loadPaymentApp = PaymentServiceMap[key as keyof typeof PaymentServiceMap]; + if (!loadPaymentApp) { throw new TRPCError({ code: "BAD_REQUEST", message: "Payment app not implemented" }); } - - const paymentApp = await paymentAppImportFn; + const paymentApp = await loadPaymentApp();Generator change required: make values
() => import("./<provider>/lib/PaymentService")instead ofimport(...).
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/payments.tsx (1)
1-1: Rename file to .ts (no JSX).Prevents frontend-only lint rules (e.g., t() enforcement) from applying to this server route.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/trpc/server/routers/viewer/payments.tsx(2 hunks)packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/trpc/server/routers/viewer/payments.tsx
**/*.{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/trpc/server/routers/viewer/payments.tsx
**/*.{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/trpc/server/routers/viewer/payments.tsx
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/payments.tsx (5)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)packages/app-store/alby/lib/PaymentService.ts (1)
PaymentService(17-141)packages/app-store/mock-payment-app/lib/PaymentService.ts (1)
PaymentService(7-141)packages/app-store/hitpay/lib/PaymentService.ts (1)
PaymentService(22-242)packages/app-store/paypal/lib/PaymentService.ts (1)
PaymentService(23-200)
🔇 Additional comments (1)
packages/trpc/server/routers/viewer/payments.tsx (1)
121-121: LGTM: Solid PaymentService presence check.The guard against missing PaymentService is correct.
keithwillcode
deleted the
devin/direct-payment-service-imports-1756339115
branch
What does this PR do?
This PR optimizes payment app imports by creating a dedicated
PaymentServiceMapthat only loads payment apps with PaymentService implementations, avoiding the need to load the entire app store when only payment functionality is needed. This follows the same optimization pattern established in PR #23372 for analytics services.Key changes:
PaymentServiceMapgeneration to the app-store-cli build processpayment.services.generated.tswith lazy imports for 6 payment services (alby, btcpayserver, hitpay, mock-payment-app, paypal, stripepayment)PaymentServiceMapinstead of importing the fullappStorePerformance benefits:
How should this be tested?
Critical testing areas:
PaymentServiceMaplogicyarn app-store:buildgeneratespayment.services.generated.tscorrectlyTest configuration:
Expected behavior:
Checklist
Link to Devin run: https://app.devin.ai/sessions/6ff5d1bb0e98478180209c3a930013c2
Requested by: @keithwillcode
chargeCardmethod signature was updated to includebooking.id- confirm this matches all PaymentService implementationsPaymentServiceMapcontains all expected payment services after runningyarn app-store:build