feat: implemented dynamic import optimization for app store services#23371
feat: implemented dynamic import optimization for app store services#23371Vansh5632 wants to merge 2 commits intocalcom:mainfrom
Conversation
|
@Vansh5632 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces generated, lazily-loaded service maps for calendar, video, and payment integrations (CalendarServiceMap, VideoAdapterMap, PaymentServiceMap) and replaces prior appStore-based dynamic lookups across consumers. An E2E guard returns empty maps when NEXT_PUBLIC_IS_E2E is truthy. Utility predicates filter which apps are included in each map during generation. Consumers in app-store, lib, and trpc now resolve integrations via these maps. Constants for location types are extracted to a separate module to reduce circular dependencies, with re-exports maintained. turbo.json adjusts a pipeline dependency from db-migrate to db-deploy. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ 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
|
github-actions
bot
added
$2K
app-store
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (20)
packages/app-store/constants.ts (1)
5-7: LGTM: constants extracted to break cyclesThe move is clean and minimizes risk. Consider adding an exported union type for discoverability.
Suggested follow-up:
export const DailyLocationType = "integrations:daily"; export const MeetLocationType = "integrations:google:meet"; export const OrganizerDefaultConferencingAppType = "conferencing"; + +export type LocationTypeConstant = + | typeof DailyLocationType + | typeof MeetLocationType + | typeof OrganizerDefaultConferencingAppType;packages/app-store/locations.ts (1)
61-62: Re-export with deprecation note for clearer migration pathPreserve BC but signal developers to migrate to the new source.
Apply:
-// Re-export for backwards compatibility -export { DailyLocationType, MeetLocationType, OrganizerDefaultConferencingAppType }; +/** @deprecated Import from "./constants" instead. These are re-exported for backwards compatibility. */ +export { + DailyLocationType, + MeetLocationType, + OrganizerDefaultConferencingAppType, +};packages/app-store/calendar.services.generated.ts (1)
18-18: Key normalization mismatch for 'ics-feedcalendar'The consumer normalizes by removing underscores only. If a credential type comes as
ics_feedcalendar, it becomesicsfeedcalendarand won’t match the hyphenated key. Either normalize keys in the generator (replace-and_) or expand normalization logic in the consumer.I can patch the generator to emit keys normalized as
slug.replace(/[-_]/g, "")and add a compatibility alias map for known legacy keys. Want me to open a follow-up PR?packages/app-store/_utils/getCalendar.ts (2)
49-56: Harden type guard and logging; keep logs low-costUse the widened
calendarAppobject and ensure type guard runs on the unwrapped module. Minor nit: avoid logging full classes in prod logs.Apply:
- if (!isCalendarService(calendarApp)) { + if (!isCalendarService(calendarApp)) { log.warn(`calendar of type ${calendarType} is not implemented`); return null; } - log.info("Got calendarApp", calendarApp.lib.CalendarService); + log.debug("Resolved CalendarService for type:", calendarType); const CalendarService = calendarApp.lib.CalendarService; return new CalendarService(credential);
39-41: Normalization may still miss edge casesConsider centralizing normalization (single helper) shared by all service lookups (calendar/video/payment) so we don’t drift in the future.
Happy to extract a
normalizeAppType(type: string)util and wire it across consumers and generators.packages/app-store-cli/src/build.ts (3)
424-427: Prefer explicit filename→guard mapping; the current includes-based detection is fragileString-includes can misfire if filenames change or contain similar substrings. Use an explicit map keyed by filename.
- const filesToGenerate: [string, string[]][] = [ + const filesToGenerate: [fileName: string, content: string[]][] = [ ["apps.metadata.generated.ts", metadataOutput], ["apps.server.generated.ts", serverOutput], ["apps.browser.generated.tsx", browserOutput], ["apps.schemas.generated.ts", schemasOutput], ["apps.keys-schemas.generated.ts", appKeysSchemasOutput], ["bookerApps.metadata.generated.ts", bookerMetadataOutput], ["crm.apps.generated.ts", crmOutput], ["calendar.services.generated.ts", calendarOutput], ["video.adapters.generated.ts", videoOutput], ["payment.services.generated.ts", paymentOutput], ]; - filesToGenerate.forEach(([fileName, output]) => { - const objectName = fileName.includes("calendar.services") - ? "CalendarServiceMap" - : fileName.includes("video.adapters") - ? "VideoAdapterMap" - : fileName.includes("payment.services") - ? "PaymentServiceMap" - : ""; + const guardByFile = new Map<string, "CalendarServiceMap" | "VideoAdapterMap" | "PaymentServiceMap">([ + ["calendar.services.generated.ts", "CalendarServiceMap"], + ["video.adapters.generated.ts", "VideoAdapterMap"], + ["payment.services.generated.ts", "PaymentServiceMap"], + ]); + filesToGenerate.forEach(([fileName, output]) => { + const objectName = guardByFile.get(fileName); const rawOutput = `${banner}${output.join("\n")}`; - const finalOutput = objectName ? addE2EGuard(rawOutput, objectName) : rawOutput; + const finalOutput = objectName ? addE2EGuard(rawOutput, objectName) : rawOutput; fs.writeFileSync(`${APP_STORE_PATH}/${fileName}`, formatOutput(finalOutput)); });Also applies to: 429-439
363-377: Refactor Suggestion: Deduplicate Service‐Map GeneratorsI ran the verification script you provided and confirmed that every app in the calendar, conferencing (video), and payment categories has either an
index.tsorindex.tsxentry point as assumed by the existing generators. It’s therefore safe to factor out the repeated logic without breaking imports.• File:
packages/app-store-cli/src/build.ts
– Original blocks at lines 363–377, 379–393, and 395–409Proposed helper and diff
+ const pushServiceMap = ( + out: string[], + objectName: "CalendarServiceMap" | "VideoAdapterMap" | "PaymentServiceMap", + filter: (app: App) => boolean + ) => { + out.push( + ...getExportedObject( + objectName, + { + importConfig: { fileToBeImported: "index.ts", importName: "" }, + lazyImport: true, + }, + filter + ) + ); + }; - // Generate Calendar Service Map - const calendarOutput: string[] = []; - calendarOutput.push( - ...getExportedObject( - "CalendarServiceMap", - { - importConfig: { - fileToBeImported: "index.ts", - importName: "", - }, - lazyImport: true, - }, - isCalendarApp - ) - ); + const calendarOutput: string[] = []; + pushServiceMap(calendarOutput, "CalendarServiceMap", isCalendarApp); - // Generate Video Adapter Map - const videoOutput: string[] = []; - videoOutput.push( - ...getExportedObject( - "VideoAdapterMap", - { - importConfig: { - fileToBeImported: "index.ts", - importName: "", - }, - lazyImport: true, - }, - isVideoApp - ) - ); + const videoOutput: string[] = []; + pushServiceMap(videoOutput, "VideoAdapterMap", isVideoApp); - // Generate Payment Service Map - const paymentOutput: string[] = []; - paymentOutput.push( - ...getExportedObject( - "PaymentServiceMap", - { - importConfig: { - fileToBeImported: "index.ts", - importName: "", - }, - lazyImport: true, - }, - isPaymentApp - ) - ); + const paymentOutput: string[] = []; + pushServiceMap(paymentOutput, "PaymentServiceMap", isPaymentApp);Apply the same transformation to the identical blocks at lines 379–393 and 395–409 to keep all three service-map generators in sync.
484-494: Centralize and unify category-based app predicatesTo prevent drift as categories evolve and keep all category checks in one place, let’s consolidate the four existing predicates into a single
categoryPredicatesobject. Based on the audit you ran, each of these four category strings appears exactly in yourconfig.jsonfiles—except thatweather_in_your_calendaris tagged asotherrather thancalendar, so please confirm whether that’s intentional.Locations to update:
packages/app-store-cli/src/build.ts, around lines 478–496Suggested diff:
-function isCrmApp(app: App) { - return !!app.categories?.includes("crm"); -} -function isCalendarApp(app: App) { - return !!app.categories?.includes("calendar"); -} -function isVideoApp(app: App) { - return !!app.categories?.includes("conferencing"); -} -function isPaymentApp(app: App) { - return !!app.categories?.includes("payment"); -} +const categoryPredicates = { + crm: (a: App) => !!a.categories?.includes("crm"), + calendar: (a: App) => !!a.categories?.includes("calendar"), + video: (a: App) => !!a.categories?.includes("conferencing"), + payment: (a: App) => !!a.categories?.includes("payment"), +} as const; + +const { + crm: isCrmApp, + calendar: isCalendarApp, + video: isVideoApp, + payment: isPaymentApp, +} = categoryPredicates;Next steps:
- Verify that every app you expect to match CRM, Calendar, Video, or Payment has the corresponding category in its
config.json.- Adjust any outliers (e.g.
weather_in_your_calendar) or update predicates if the intended mapping differs.packages/app-store/payment.services.generated.ts (1)
5-14: Type the map and preserve literal keys for safer indexingTyping the export improves DX at call sites and keeps
keyof typeof PaymentServiceMapprecise. Since this file is generated, change the generator to emit the typed/as-const form shown below.Apply this diff (via generator):
-// Return empty map during E2E tests to prevent module loading issues -export const PaymentServiceMap = process.env.NEXT_PUBLIC_IS_E2E - ? {} - : { +// Return empty map during E2E tests to prevent module loading issues +export const PaymentServiceMap = + process.env.NEXT_PUBLIC_IS_E2E + ? ({} as const) + : ({ alby: import("./alby/index"), hitpay: import("./hitpay/index"), "mock-payment-app": import("./mock-payment-app/index"), paypal: import("./paypal/index"), stripepayment: import("./stripepayment/index"), - }; + } as const);This keeps keys literal and values readonly Promises, enabling
keyof typeof PaymentServiceMapto be a strict union of provider keys.packages/app-store/video.adapters.generated.ts (1)
5-38: Apply typed/as-const pattern for VideoAdapterMapAll adapter import paths have been verified to correspond to existing directories, so this change can be applied safely. Mirroring the pattern used in payments will:
- Preserve the literal key types on
VideoAdapterMap- Improve indexing safety and autocomplete
File: packages/app-store/video.adapters.generated.ts
Lines: 5–38Suggested diff:
-export const VideoAdapterMap = process.env.NEXT_PUBLIC_IS_E2E - ? {} - : { +export const VideoAdapterMap = + process.env.NEXT_PUBLIC_IS_E2E + ? ({} as const) + : ({ campfire: import("./campfire/index"), dailyvideo: import("./dailyvideo/index"), demodesk: import("./demodesk/index"), dialpad: import("./dialpad/index"), discord: import("./discord/index"), eightxeight: import("./eightxeight/index"), "element-call": import("./element-call/index"), facetime: import("./facetime/index"), googlevideo: import("./googlevideo/index"), "horizon-workrooms": import("./horizon-workrooms/index"), huddle01video: import("./huddle01video/index"), jelly: import("./jelly/index"), jitsivideo: import("./jitsivideo/index"), mirotalk: import("./mirotalk/index"), nextcloudtalk: import("./nextcloudtalk/index"), office365video: import("./office365video/index"), ping: import("./ping/index"), riverside: import("./riverside/index"), roam: import("./roam/index"), salesroom: import("./salesroom/index"), shimmervideo: import("./shimmervideo/index"), sirius_video: import("./sirius_video/index"), skype: import("./skype/index"), sylapsvideo: import("./sylapsvideo/index"), tandemvideo: import("./tandemvideo/index"), "event-type-location-video-static": import("./templates/event-type-location-video-static/index"), webex: import("./webex/index"), whereby: import("./whereby/index"), zoomvideo: import("./zoomvideo/index"), - }; + } as const);packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
118-126: Minor: align with typed/as-const maps to drop the cast, and include more context in the errorIf the generator emits
as const, thekeyofcast can remain, but you can also provide a clearer error message including the provider for easier debugging.- const normalizedDirName = paymentCredential.app.dirName as keyof typeof PaymentServiceMap; - const paymentAppPromise = PaymentServiceMap[normalizedDirName]; + const normalizedDirName = paymentCredential.app.dirName as keyof typeof PaymentServiceMap; + const paymentAppPromise = PaymentServiceMap[normalizedDirName]; if (!paymentAppPromise) { - throw new TRPCError({ code: "BAD_REQUEST", message: "Payment service not found" }); + throw new TRPCError({ + code: "BAD_REQUEST", + message: `Payment service not found for provider "${normalizedDirName}"`, + }); }packages/trpc/server/routers/viewer/payments.tsx (5)
111-119: Harden dynamic import shape handling and typingThe lookup + guard pattern is solid. Two small robustness tweaks:
- Some modules may surface the payload under a default export. Consider checking
paymentApp?.lib ?? paymentApp?.default?.libto avoid shape drift breakage when individual apps change their export surface.- Avoid the broad cast to
PaymentApp | nullby narrowing the module to a local type that models{ lib?: { PaymentService?: unknown } }and refine via in-operator checks.Apply:
- const paymentApp = (await paymentAppPromise) as PaymentApp | null; + const paymentModule = await paymentAppPromise; + const lib = (paymentModule as any)?.lib ?? (paymentModule as any)?.default?.lib; + if (!lib || !("PaymentService" in lib)) { + throw new TRPCError({ code: "BAD_REQUEST", message: "Payment service not found" }); + } + const PaymentService = (lib as PaymentApp["lib"]).PaymentService;…and then drop the separate reassignment to
PaymentServicebelow accordingly.
29-46: Prisma: prefer select over include to minimize payloadsSwitching to
selecttrims unnecessary columns and aligns with the repo guideline to avoidinclude. Also explicitly select scalar fields used later (id,title,startTime,endTime,eventTypeId).- 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 } }, + }, + });
59-76: Build attendees list concurrently and fix namingYou're awaiting
getTranslationinside the loop, then wrapping non-promises inPromise.all. Create the list with a singlemap + Promise.allto parallelize and rename the variable for clarity.- 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", + }, + })) + );
151-155: Use attendeesList, not attendeesListPromisesMinor clarity fix; after the refactor above, the first attendee should come from
attendeesList[0].- await sendNoShowFeeChargedEmail( - attendeesListPromises[0], + await sendNoShowFeeChargedEmail( + attendeesList[0], evt, booking?.eventType?.metadata as EventTypeMetadata );
159-163: Do not surface raw error details to clientsInterpolating
errinto the TRPCError message may leak internal details. Log the error server-side and return a generic message to the client.- throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `Error processing payment with error ${err}`, - }); + ctx.logger?.error({ err }, "Error processing payment"); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Error processing payment", + });packages/lib/videoClient.ts (4)
23-27: Local interface provides helpful type contextDefining
VideoApphere improves readability. If a shared type for the generated module shape exists later, you can replace this with an import to avoid drift, but this is fine for now.
48-50: Be resilient to default exports from adapter modulesDepending on how a specific app exports its module,
libmay hang off a default export. Prefer a small normalization before dereferencing.- if ("lib" in app && app.lib && "VideoApiAdapter" in app.lib) { - const makeVideoApiAdapter = (app.lib as { VideoApiAdapter: VideoApiAdapterFactory }).VideoApiAdapter; + const lib = (app as any)?.lib ?? (app as any)?.default?.lib; + if (lib && "VideoApiAdapter" in lib) { + const makeVideoApiAdapter = (lib as { VideoApiAdapter: VideoApiAdapterFactory }).VideoApiAdapter; const videoAdapter = makeVideoApiAdapter(cred); videoAdapters.push(videoAdapter);
40-41: Comment is now inaccurateThe generated map currently includes a static template adapter:
"event-type-location-video-static". Update the comment or remove it to avoid confusion.- // Static Link Video Apps don't exist in packages/app-store/video.adapters.generated.ts and they aren't needed there anyway. + // Some static-link-only video apps may not need dynamic adapters; resolve only when the key exists.
246-252: Use a single logger consistently instead of console.errorThere are several
console.errorcalls; preferlog.errorfor consistent formatting and routing.- console.error("Error: Cal video provider is not installed."); + log.error("Error: Cal video provider is not installed.");Repeat for the similar blocks in the other helpers.
Also applies to: 272-278, 296-302, 320-326, 344-350, 380-386, 404-410
📜 Review details
Configuration used: CodeRabbit UI
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)
packages/app-store-cli/src/build.ts(4 hunks)packages/app-store/_utils/getCalendar.ts(2 hunks)packages/app-store/calendar.services.generated.ts(1 hunks)packages/app-store/constants.ts(1 hunks)packages/app-store/locations.ts(2 hunks)packages/app-store/payment.services.generated.ts(1 hunks)packages/app-store/video.adapters.generated.ts(1 hunks)packages/lib/getConnectedApps.ts(1 hunks)packages/lib/videoClient.ts(2 hunks)packages/trpc/server/routers/viewer/payments.tsx(2 hunks)packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts(2 hunks)turbo.json(1 hunks)
🧰 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/app-store/constants.tspackages/app-store/payment.services.generated.tspackages/lib/videoClient.tspackages/app-store/calendar.services.generated.tspackages/lib/getConnectedApps.tspackages/app-store/video.adapters.generated.tspackages/app-store-cli/src/build.tspackages/trpc/server/routers/viewer/payments/chargeCard.handler.tspackages/app-store/_utils/getCalendar.tspackages/app-store/locations.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/app-store/constants.tspackages/app-store/payment.services.generated.tspackages/lib/videoClient.tspackages/app-store/calendar.services.generated.tspackages/lib/getConnectedApps.tspackages/app-store/video.adapters.generated.tspackages/app-store-cli/src/build.tspackages/trpc/server/routers/viewer/payments.tsxpackages/trpc/server/routers/viewer/payments/chargeCard.handler.tspackages/app-store/_utils/getCalendar.tspackages/app-store/locations.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 (1)
📚 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:
packages/app-store/calendar.services.generated.tspackages/app-store/_utils/getCalendar.ts
🧬 Code graph analysis (6)
packages/app-store/constants.ts (1)
packages/app-store/locations.ts (3)
DailyLocationType(62-62)MeetLocationType(62-62)OrganizerDefaultConferencingAppType(62-62)
packages/lib/videoClient.ts (2)
packages/lib/piiFreeData.ts (1)
getPiiFreeCredential(61-72)packages/app-store/video.adapters.generated.ts (1)
VideoAdapterMap(6-38)
packages/lib/getConnectedApps.ts (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(6-14)
packages/trpc/server/routers/viewer/payments.tsx (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(6-14)
packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(6-14)
packages/app-store/_utils/getCalendar.ts (1)
packages/app-store/calendar.services.generated.ts (1)
CalendarServiceMap(6-23)
🪛 Biome (2.1.2)
packages/lib/getConnectedApps.ts
[error] 192-192: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (7)
turbo.json (1)
13-16: Double-check@calcom/prisma#db-seeddependency orderingIt looks like in turbo.json we have:
@calcom/prisma#db-up(no deps)@calcom/prisma#db-migrate→ depends on@calcom/prisma#db-up@calcom/prisma#db-seed→ depends on@calcom/prisma#db-migrate- a standalone
db-deploytask (with migration inputs)- no
@calcom/prisma#db-deploydefined (yet@calcom/prisma#dxreferences it)If your migration flow has shifted to the
db-deploystep, seeding afterdb-migratecould run too early. Please confirm whether you intended:
- to rename/alias the standalone
db-deployunder@calcom/prisma(so@calcom/prisma#dxisn’t broken), and- to change
@calcom/prisma#db-seedto depend ondb-deploy(instead of@calcom/prisma#db-migrate)Proposed diff (adjust naming as needed):
- "@calcom/prisma#db-seed": { - "cache": false, - "dependsOn": ["@calcom/prisma#db-migrate"] - }, + "@calcom/prisma#db-seed": { + "cache": false, + "dependsOn": ["db-deploy"] + },Please verify the intended pipeline ordering to prevent stale migrations or CI race conditions.
packages/app-store/locations.ts (1)
10-10: LGTM: import from constants removes circular depsImporting the constants here avoids prior cycles while keeping usages intact.
packages/app-store/_utils/getCalendar.ts (1)
5-5: Import path switch is correct; keep it stableSwitching to the generated map is the right direction for lazy-loading.
packages/lib/getConnectedApps.ts (1)
185-201: No unsafe optional chaining – guard logic is already sufficientThe existing code’s guard
if (paymentApp && "lib" in paymentApp && paymentApp?.lib && "PaymentService" in paymentApp?.lib) { … }ensures that the right-hand side of the
inoperator is only ever evaluated whenpaymentApp.libis a non-nullish object, so it cannot throw. The extra?.is harmless but redundant given the preceding&&checks.Optional suggestion (non-critical):
- If you’re iterating over many apps in a single
Promise.all, you can hoist
const { PaymentServiceMap } = await import("@calcom/app-store/payment.services.generated");
outside the loop to avoid repeated dynamic-import awaits.Likely an incorrect or invalid review comment.
packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
1-1: Import switch to generated map looks goodThe new source aligns with lazy-loading and keeps the legacy appStore out of the hot path.
packages/trpc/server/routers/viewer/payments.tsx (1)
3-3: LGTM on switching to generated PaymentServiceMapGood move to lazy-load payment integrations via the generated map. This aligns with the PR objective to reduce initial load and breaks circular deps.
packages/lib/videoClient.ts (1)
4-7: LGTM on switching to generated VideoAdapterMap and centralized constantsGood adoption of the generated adapter map and use of DailyLocationType from the constants module. Improves modularity and helps with bundle splitting.
| const addE2EGuard = (source: string, objectName: string) => { | ||
| if (["CalendarServiceMap", "VideoAdapterMap", "PaymentServiceMap"].includes(objectName)) { | ||
| return source | ||
| .replace( | ||
| `export const ${objectName} = {`, | ||
| `// Return empty map during E2E tests to prevent module loading issues | ||
| export const ${objectName} = process.env.NEXT_PUBLIC_IS_E2E | ||
| ? {} | ||
| : {` | ||
| ) | ||
| .replace(`};`, ` };`); | ||
| } | ||
| return source; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make E2E guard injection robust; avoid brittle string replacements
Using two plain string replacements can silently fail if formatting changes, or capture the wrong closing brace when additional objects exist. Replace with a single, anchored, multiline regex that wraps the exact export block.
Apply this diff:
-const addE2EGuard = (source: string, objectName: string) => {
- if (["CalendarServiceMap", "VideoAdapterMap", "PaymentServiceMap"].includes(objectName)) {
- return source
- .replace(
- `export const ${objectName} = {`,
- `// Return empty map during E2E tests to prevent module loading issues
-export const ${objectName} = process.env.NEXT_PUBLIC_IS_E2E
- ? {}
- : {`
- )
- .replace(`};`, ` };`);
- }
- return source;
-};
+const addE2EGuard = (source: string, objectName: string) => {
+ if (!["CalendarServiceMap", "VideoAdapterMap", "PaymentServiceMap"].includes(objectName)) return source;
+ const pattern = new RegExp(
+ String.raw`export\s+const\s+${objectName}\s*=\s*{([\\s\\S]*?)};`,
+ "m"
+ );
+ return source.replace(
+ pattern,
+ (_m, inner) =>
+ `// Return empty map during E2E tests to prevent module loading issues
+export const ${objectName} =
+ process.env.NEXT_PUBLIC_IS_E2E ? ({} as const) : ({${inner}} as const);`
+ );
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const addE2EGuard = (source: string, objectName: string) => { | |
| if (["CalendarServiceMap", "VideoAdapterMap", "PaymentServiceMap"].includes(objectName)) { | |
| return source | |
| .replace( | |
| `export const ${objectName} = {`, | |
| `// Return empty map during E2E tests to prevent module loading issues | |
| export const ${objectName} = process.env.NEXT_PUBLIC_IS_E2E | |
| ? {} | |
| : {` | |
| ) | |
| .replace(`};`, ` };`); | |
| } | |
| return source; | |
| }; | |
| const addE2EGuard = (source: string, objectName: string) => { | |
| if (!["CalendarServiceMap", "VideoAdapterMap", "PaymentServiceMap"].includes(objectName)) return source; | |
| const pattern = new RegExp( | |
| String.raw`export\s+const\s+${objectName}\s*=\s*{([\\s\\S]*?)};`, | |
| "m" | |
| ); | |
| return source.replace( | |
| pattern, | |
| (_m, inner) => | |
| `// Return empty map during E2E tests to prevent module loading issues | |
| export const ${objectName} = | |
| process.env.NEXT_PUBLIC_IS_E2E ? ({} as const) : ({${inner}} as const);` | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In packages/app-store-cli/src/build.ts around lines 24 to 37, the current
addE2EGuard uses two brittle string replacements that can break with formatting
changes or extra braces; replace that logic with a single anchored multiline
regular expression that matches the exact "export const <objectName> = { ... }"
block (using a non-greedy, dotall/multiline pattern anchored to the export line
and its corresponding closing brace) and replace it with the guarded version
that injects the NEXT_PUBLIC_IS_E2E check, ensuring only the intended export
block is wrapped and spacing/indentation are preserved.
| const normalizedType = calendarType.split("_").join("") as keyof typeof CalendarServiceMap; | ||
| const calendarAppPromise = CalendarServiceMap[normalizedType]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Call loader functions and support default exports; current code awaits already-started imports
After switching the map to loader functions, update the consumer accordingly. Also normalize both underscores and hyphens to avoid key mismatches.
Apply:
- const normalizedType = calendarType.split("_").join("") as keyof typeof CalendarServiceMap;
- const calendarAppPromise = CalendarServiceMap[normalizedType];
+ // Normalize by removing underscores and hyphens to match generator keys
+ const normalizedType = calendarType.replace(/[_-]/g, "");
+ const loader =
+ // try normalized first
+ CalendarServiceMap[normalizedType] ||
+ // fallback: try raw and underscore-only normalized for legacy keys
+ CalendarServiceMap[calendarType as keyof typeof CalendarServiceMap] ||
+ CalendarServiceMap[calendarType.replace(/_/g, "") as keyof typeof CalendarServiceMap];
- if (!calendarAppPromise) {
+ if (!loader) {
log.warn(`calendar of type ${calendarType} is not implemented`);
return null;
}
- const calendarApp = await calendarAppPromise;
+ const mod = await loader();
+ const calendarApp: unknown = (mod as any)?.default ?? mod;Also applies to: 47-47
| export const CalendarServiceMap = process.env.NEXT_PUBLIC_IS_E2E | ||
| ? {} | ||
| : { |
There was a problem hiding this comment.
E2E guard will be truthy even when set to "false"
Checking presence instead of value means NEXT_PUBLIC_IS_E2E="false" yields an empty map. Compare explicitly to "true".
Apply:
-export const CalendarServiceMap = process.env.NEXT_PUBLIC_IS_E2E
+export const CalendarServiceMap = process.env.NEXT_PUBLIC_IS_E2E === "true"
? {}
: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const CalendarServiceMap = process.env.NEXT_PUBLIC_IS_E2E | |
| ? {} | |
| : { | |
| export const CalendarServiceMap = process.env.NEXT_PUBLIC_IS_E2E === "true" | |
| ? {} | |
| : { |
🤖 Prompt for AI Agents
In packages/app-store/calendar.services.generated.ts around lines 6 to 8, the
environment guard uses a truthy check (process.env.NEXT_PUBLIC_IS_E2E) which
treats the string "false" as truthy; change the condition to an explicit string
comparison against "true" (process.env.NEXT_PUBLIC_IS_E2E === "true") so the map
is only empty when the env var is exactly "true".
| amie: import("./amie/index"), | ||
| applecalendar: import("./applecalendar/index"), | ||
| caldavcalendar: import("./caldavcalendar/index"), | ||
| cron: import("./cron/index"), | ||
| exchange2013calendar: import("./exchange2013calendar/index"), | ||
| exchange2016calendar: import("./exchange2016calendar/index"), | ||
| exchangecalendar: import("./exchangecalendar/index"), | ||
| feishucalendar: import("./feishucalendar/index"), | ||
| googlecalendar: import("./googlecalendar/index"), | ||
| "ics-feedcalendar": import("./ics-feedcalendar/index"), | ||
| larkcalendar: import("./larkcalendar/index"), | ||
| office365calendar: import("./office365calendar/index"), | ||
| vimcal: import("./vimcal/index"), | ||
| zohocalendar: import("./zohocalendar/index"), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dynamic imports are eagerly executed; this defeats lazy loading
Using import("./x") at object construction time kicks off all imports immediately when this module is evaluated. Store loader functions () => import("./x") instead. This aligns with the PR objective to reduce initial load.
Apply (generator should emit loaders and a type):
-// Return empty map during E2E tests to prevent module loading issues
-export const CalendarServiceMap = process.env.NEXT_PUBLIC_IS_E2E === "true"
- ? {}
- : {
- amie: import("./amie/index"),
- applecalendar: import("./applecalendar/index"),
- caldavcalendar: import("./caldavcalendar/index"),
- cron: import("./cron/index"),
- exchange2013calendar: import("./exchange2013calendar/index"),
- exchange2016calendar: import("./exchange2016calendar/index"),
- exchangecalendar: import("./exchangecalendar/index"),
- feishucalendar: import("./feishucalendar/index"),
- googlecalendar: import("./googlecalendar/index"),
- "ics-feedcalendar": import("./ics-feedcalendar/index"),
- larkcalendar: import("./larkcalendar/index"),
- office365calendar: import("./office365calendar/index"),
- vimcal: import("./vimcal/index"),
- zohocalendar: import("./zohocalendar/index"),
- };
+type CalendarLoader = () => Promise<unknown>;
+// Return empty map during E2E tests to prevent module loading issues
+export const CalendarServiceMap: Record<string, CalendarLoader> =
+ process.env.NEXT_PUBLIC_IS_E2E === "true"
+ ? {}
+ : {
+ amie: () => import("./amie/index"),
+ applecalendar: () => import("./applecalendar/index"),
+ caldavcalendar: () => import("./caldavcalendar/index"),
+ cron: () => import("./cron/index"),
+ exchange2013calendar: () => import("./exchange2013calendar/index"),
+ exchange2016calendar: () => import("./exchange2016calendar/index"),
+ exchangecalendar: () => import("./exchangecalendar/index"),
+ feishucalendar: () => import("./feishucalendar/index"),
+ googlecalendar: () => import("./googlecalendar/index"),
+ "ics-feedcalendar": () => import("./ics-feedcalendar/index"),
+ larkcalendar: () => import("./larkcalendar/index"),
+ office365calendar: () => import("./office365calendar/index"),
+ vimcal: () => import("./vimcal/index"),
+ zohocalendar: () => import("./zohocalendar/index"),
+ };Follow-up: Update the consumer in getCalendar.ts to call the loader function (see my comment there).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| amie: import("./amie/index"), | |
| applecalendar: import("./applecalendar/index"), | |
| caldavcalendar: import("./caldavcalendar/index"), | |
| cron: import("./cron/index"), | |
| exchange2013calendar: import("./exchange2013calendar/index"), | |
| exchange2016calendar: import("./exchange2016calendar/index"), | |
| exchangecalendar: import("./exchangecalendar/index"), | |
| feishucalendar: import("./feishucalendar/index"), | |
| googlecalendar: import("./googlecalendar/index"), | |
| "ics-feedcalendar": import("./ics-feedcalendar/index"), | |
| larkcalendar: import("./larkcalendar/index"), | |
| office365calendar: import("./office365calendar/index"), | |
| vimcal: import("./vimcal/index"), | |
| zohocalendar: import("./zohocalendar/index"), | |
| }; | |
| type CalendarLoader = () => Promise<unknown>; | |
| // Return empty map during E2E tests to prevent module loading issues | |
| export const CalendarServiceMap: Record<string, CalendarLoader> = | |
| process.env.NEXT_PUBLIC_IS_E2E === "true" | |
| ? {} | |
| : { | |
| amie: () => import("./amie/index"), | |
| applecalendar: () => import("./applecalendar/index"), | |
| caldavcalendar: () => import("./caldavcalendar/index"), | |
| cron: () => import("./cron/index"), | |
| exchange2013calendar: () => import("./exchange2013calendar/index"), | |
| exchange2016calendar: () => import("./exchange2016calendar/index"), | |
| exchangecalendar: () => import("./exchangecalendar/index"), | |
| feishucalendar: () => import("./feishucalendar/index"), | |
| googlecalendar: () => import("./googlecalendar/index"), | |
| "ics-feedcalendar": () => import("./ics-feedcalendar/index"), | |
| larkcalendar: () => import("./larkcalendar/index"), | |
| office365calendar: () => import("./office365calendar/index"), | |
| vimcal: () => import("./vimcal/index"), | |
| zohocalendar: () => import("./zohocalendar/index"), | |
| }; |
| const normalizedAppName = appName as keyof typeof VideoAdapterMap; | ||
| const appPromise = VideoAdapterMap[normalizedAppName]; | ||
|
|
||
| // Static Link Video Apps don't exist in packages/app-store/video.adapters.generated.ts and they aren't needed there anyway. | ||
| const app = appPromise ? ((await appPromise) as VideoApp | null) : null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adapter key normalization will fail for several apps (hyphens/underscores mismatch)
appName = cred.type.split("_").join("") yields keys like elementcall or siriusvideo, but the generated map contains mixed keys such as element-call, sirius_video, and event-type-location-video-static. This will cause lookups to miss and silently fall back (e.g., log error and default to Cal Video), breaking those integrations.
Apply a robust resolver that tries multiple candidates derived from both cred.type and cred.appId, then falls back cleanly:
interface VideoApp {
lib: {
VideoApiAdapter: VideoApiAdapterFactory;
};
}
+// Resolve a key in VideoAdapterMap from various credential identifiers
+const resolveVideoMapKey = (cred: CredentialPayload): keyof typeof VideoAdapterMap | null => {
+ const candidates = [
+ cred.type, // e.g. "sirius_video"
+ cred.type?.replace(/_/g, ""), // "siriusvideo"
+ cred.type?.replace(/_/g, "-"), // "sirius-video"
+ cred.appId, // e.g. "element-call"
+ cred.appId?.replace(/-/g, ""), // "elementcall"
+ cred.appId?.replace(/_/g, "-"), // normalize to hyphens
+ ].filter(Boolean) as string[];
+ for (const key of candidates) {
+ if (key in VideoAdapterMap) return key as keyof typeof VideoAdapterMap;
+ }
+ return null;
+};
// factory
const getVideoAdapters = async (withCredentials: CredentialPayload[]): Promise<VideoApiAdapter[]> => {
const videoAdapters: VideoApiAdapter[] = [];
for (const cred of withCredentials) {
- const appName = cred.type.split("_").join(""); // Transform `zoom_video` to `zoomvideo`;
- log.silly("Getting video adapter for", safeStringify({ appName, cred: getPiiFreeCredential(cred) }));
-
- const normalizedAppName = appName as keyof typeof VideoAdapterMap;
- const appPromise = VideoAdapterMap[normalizedAppName];
+ const key = resolveVideoMapKey(cred);
+ log.silly(
+ "Getting video adapter for",
+ safeStringify({ candidatesFrom: { type: cred.type, appId: cred.appId }, resolvedKey: key, cred: getPiiFreeCredential(cred) })
+ );
+ const appPromise = key ? VideoAdapterMap[key] : undefined;
- // Static Link Video Apps don't exist in packages/app-store/video.adapters.generated.ts and they aren't needed there anyway.
- const app = appPromise ? ((await appPromise) as VideoApp | null) : null;
+ // Adapter module is loaded lazily only if we resolved a key above.
+ const app = appPromise ? ((await appPromise) as VideoApp | null) : null;
if (!app) {
- log.error(`Couldn't get adapter for ${appName}`);
+ log.error(
+ `Couldn't resolve video adapter`,
+ safeStringify({ type: cred.type, appId: cred.appId })
+ );
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const normalizedAppName = appName as keyof typeof VideoAdapterMap; | |
| const appPromise = VideoAdapterMap[normalizedAppName]; | |
| // Static Link Video Apps don't exist in packages/app-store/video.adapters.generated.ts and they aren't needed there anyway. | |
| const app = appPromise ? ((await appPromise) as VideoApp | null) : null; | |
| interface VideoApp { | |
| lib: { | |
| VideoApiAdapter: VideoApiAdapterFactory; | |
| }; | |
| } | |
| // Resolve a key in VideoAdapterMap from various credential identifiers | |
| const resolveVideoMapKey = (cred: CredentialPayload): keyof typeof VideoAdapterMap | null => { | |
| const candidates = [ | |
| cred.type, // e.g. "sirius_video" | |
| cred.type?.replace(/_/g, ""), // "siriusvideo" | |
| cred.type?.replace(/_/g, "-"), // "sirius-video" | |
| cred.appId, // e.g. "element-call" | |
| cred.appId?.replace(/-/g, ""), // "elementcall" | |
| cred.appId?.replace(/_/g, "-"), // normalize to hyphens | |
| ].filter(Boolean) as string[]; | |
| for (const key of candidates) { | |
| if (key in VideoAdapterMap) return key as keyof typeof VideoAdapterMap; | |
| } | |
| return null; | |
| }; | |
| // factory | |
| const getVideoAdapters = async (withCredentials: CredentialPayload[]): Promise<VideoApiAdapter[]> => { | |
| const videoAdapters: VideoApiAdapter[] = []; | |
| for (const cred of withCredentials) { | |
| const key = resolveVideoMapKey(cred); | |
| log.silly( | |
| "Getting video adapter for", | |
| safeStringify({ | |
| candidatesFrom: { type: cred.type, appId: cred.appId }, | |
| resolvedKey: key, | |
| cred: getPiiFreeCredential(cred), | |
| }) | |
| ); | |
| const appPromise = key ? VideoAdapterMap[key] : undefined; | |
| // Adapter module is loaded lazily only if we resolved a key above. | |
| const app = appPromise ? ((await appPromise) as VideoApp | null) : null; | |
| if (!app) { | |
| log.error( | |
| `Couldn't resolve video adapter`, | |
| safeStringify({ type: cred.type, appId: cred.appId }) | |
| ); | |
| continue; | |
| } | |
| // ...rest of adapter instantiation... | |
| } | |
| return videoAdapters; | |
| }; |
turbo.json
Outdated
| "cache": false, | ||
| "dependsOn": ["@calcom/prisma#db-migrate"] | ||
| "dependsOn": ["@calcom/prisma#db-deploy"] | ||
| }, |
There was a problem hiding this comment.
Broken pipeline dependency: task '@calcom/prisma#db-deploy' is not defined
The dependency was changed to "@calcom/prisma#db-deploy", but there is no such namespaced task in this file. Only a top-level "db-deploy" exists (Lines 140-143). Turbo will fail to resolve this dependency.
Apply one of these fixes (Option A preferred):
- "dependsOn": ["@calcom/prisma#db-deploy"]
+ "dependsOn": ["db-deploy"]Or define the missing namespaced task:
+ "@calcom/prisma#db-deploy": {
+ "cache": false,
+ "inputs": ["./schema.prisma", "./migrations/**/*.sql"]
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "cache": false, | |
| "dependsOn": ["@calcom/prisma#db-migrate"] | |
| "dependsOn": ["@calcom/prisma#db-deploy"] | |
| }, | |
| "cache": false, | |
| "dependsOn": ["db-deploy"] | |
| }, |
🤖 Prompt for AI Agents
In turbo.json around lines 21-23, the task dependency "
@calcom/prisma#db-deploy" is invalid because there is no namespaced task
defined; only a top-level "db-deploy" exists later (lines ~140-143). Fix by
either updating the dependsOn entry to reference the existing top-level task
"db-deploy" (preferred), or add a namespaced task definition for @calcom/prisma
with a "db-deploy" pipeline entry so the namespaced reference resolves; ensure
the string exactly matches the task key and run turbo lint/graph to verify
resolution.
|
@Vansh5632 This is looking quite good because we know that the video adapters and payment apps are going to help a lot when separated. My PR #22450 was just merged and caused a lot of conflicts here since it build the calendars map. Can you take a look and fix those and then I'll review here? Thanks! |
|
okay i will do this and make the necessary changes |
|
@Vansh5632 if you get around to this soon, you can just focus on the video adapters. I have #23408 landing soon |
|
@keithwillcode now i will focus on the working of the video adapters |
- Optimize video adapter loading with dynamic imports for better performance - Generate VideoAdapterMap for conditional loading of video adapters - Update videoClient.ts to use dynamic import pattern with proper error handling - Add comprehensive mock system for video adapters in test environment - Support for 11 video adapters: dailyvideo, huddle01video, jelly, jitsivideo, nextcloudtalk, office365video, shimmervideo, sylapsvideo, tandemvideo, webex, zoomvideo - Maintain backward compatibility while improving application performance - Use NEXT_PUBLIC_IS_E2E for E2E test compatibility instead of PLAYWRIGHT env var
Vansh5632
force-pushed
the
feat/dynamic-imports-optimization
branch
from
458df9a to
b48445c
Compare
|
FYI I took the full bounty off of this since other PRs for calendar, analytics and payment maps have been merged. If this merges, I will add a tip directly here. |
|
Hey @Vansh5632 I was able to get the video adapters ready as well #23435. Even with these changes, we still see slow compile/load times locally so the bounty is still up for grabs! But closing this PR for now. |
|
/tip 100 @Vansh5632 |
|
@Vansh5632: You've been awarded a $100 by Cal.com, Inc.! 👉 Complete your Algora onboarding to collect the tip. |
|
🎉🎈 @Vansh5632 has been awarded $100 by Cal.com, Inc.! 🎈🎊 |
2 participants
What does this PR do?
Add dynamic import service maps for calendars, videos, and payments
Generate CalendarServiceMap, VideoAdapterMap, and PaymentServiceMap
Update core loading functions to use lazy loading
Optimize bundle size and improve page load performance
Add constants file for service configuration
Fixes Local dev crazy slow #23104 (GitHub issue number)
Fixes CAL-6255 (Linear issue number - should be visible at the bottom of the GitHub issue description)
Video Demo (if applicable):
Screencast.from.2025-08-27.00-22-33.mp4
Image Demo (if applicable):
before:
after:
/claim #23104