chore(app-store): optimize local development loading performance#23339
chore(app-store): optimize local development loading performance#23339BenraouaneSoufiane wants to merge 0 commit intocalcom:mainfrom
Conversation
|
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, per-category lazy-load maps for App Store modules and refactors consumers to use them. New scripts generate factory maps for calendar, payment, CRM, and conferencing adapters and add a CI check to ban barrel imports. Code changes replace appStore-based dynamic lookups with category-specific registries (CALENDAR_SERVICES, PAYMENT_APPS, VIDEO_ADAPTERS), adding guards for missing services. A safety wrapper is added for broken video integration email notifications. package.json gains scripts to generate maps and to run the banned-imports check. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ 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
|
Someone is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/25/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (08/25/25)1 label was added to this PR based on Keith Williams's automation. |
![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 (3)
packages/lib/payment/handlePayment.ts (1)
72-92: Bug: optional-chaining misuse can throw at runtime (apps?.[id].price)Expressions like
apps?.[paymentAppCredentials.appId].pricedereference.priceeven whenapps?.[id]isundefined, causing a TypeError. This can break payment flows when metadata is missing.Apply this diff to safely access metadata:
- const apps = eventTypeAppMetadataOptionalSchema.parse(selectedEventType?.metadata?.apps); - const paymentOption = apps?.[paymentAppCredentials.appId].paymentOption || "ON_BOOKING"; + const apps = eventTypeAppMetadataOptionalSchema.parse(selectedEventType?.metadata?.apps); + const appCfg = apps?.[paymentAppCredentials.appId]; + const paymentOption = appCfg?.paymentOption ?? "ON_BOOKING"; @@ - paymentData = await paymentInstance.collectCard( - { - amount: apps?.[paymentAppCredentials.appId].price, - currency: apps?.[paymentAppCredentials.appId].currency, - }, + paymentData = await paymentInstance.collectCard( + { + amount: appCfg?.price, + currency: appCfg?.currency, }, @@ - paymentData = await paymentInstance.create( - { - amount: apps?.[paymentAppCredentials.appId].price, - currency: apps?.[paymentAppCredentials.appId].currency, - }, + paymentData = await paymentInstance.create( + { + amount: appCfg?.price, + currency: appCfg?.currency, + },Optionally validate
amount/currencyand surface a clear error if missing.packages/lib/videoClient.ts (2)
139-143: PII risk: logging raw CalendarEvent. Use PII-safe helpers.This path logs the full calEvent on errors. Replace with getPiiFreeCalendarEvent to avoid leaking personal data to logs.
- ? await firstVideoAdapter?.updateMeeting(bookingRef, calEvent).catch(async (e) => { - await sendBrokenIntegrationEmail(calEvent, "video"); - log.error("updateMeeting failed", e, calEvent); + ? await firstVideoAdapter?.updateMeeting(bookingRef, calEvent).catch(async (e) => { + await sendBrokenIntegrationEmail(calEvent, "video"); + log.error( + "updateMeeting failed", + safeStringify(e), + safeStringify({ calEvent: getPiiFreeCalendarEvent(calEvent) }) + ); success = false; return undefined; })
148-151: PII risk: credential and calEvent logged unsanitized.Ensure both credential and calEvent are sanitized when logging structured payloads.
- log.error( - "updateMeeting failed", - safeStringify({ bookingRef, canCallUpdateMeeting, calEvent, credential }) - ); + log.error( + "updateMeeting failed", + safeStringify({ + bookingRef, + canCallUpdateMeeting, + calEvent: getPiiFreeCalendarEvent(calEvent), + credential: getPiiFreeCredential(credential), + }) + );
🧹 Nitpick comments (22)
scripts/check-banned-imports.mjs (1)
9-14: Optional: include match count and hint for remediationSmall DX improvement: print the number of matches found and a short hint (e.g., “replace with generated maps, run pnpm app-store:gen”). Not critical.
Apply this diff:
-if (out.trim()) { - console.error("Forbidden imports found:\n" + out); +if (out.trim()) { + const lines = out.trim().split("\n"); + console.error(`Forbidden imports found (${lines.length}):\n` + out + "\n\nHint: replace barrel imports with generated maps (e.g., PAYMENT_APPS, CALENDAR_SERVICES)."); process.exit(1); } else {packages/app-store/calendar.services.generated.ts (1)
1-17: LGTM for generated registry; consider exporting a slug type for stronger typing downstreamThe map looks correct and uses named exports (good for tree-shaking). As a nice-to-have, export a
CalendarServiceSlugtype to improve consumer typing without duplicating keys.Apply this diff (and mirror in the generator):
} as const; +export type CalendarServiceSlug = keyof typeof CALENDAR_SERVICES;packages/app-store/payment.apps.generated.ts (1)
1-10: LGTM; optional: exposePaymentAppSlugfor consumersMap entries and import paths look right. To make key checks type-safe in callers (e.g., handlePayment), export a slug type.
Apply this diff (and mirror in the generator):
} as const; +export type PaymentAppSlug = keyof typeof PAYMENT_APPS;packages/lib/payment/handlePayment.ts (3)
10-16: Tighten the type guard for PaymentApp; check constructor explicitly
"PaymentService" in x.libpasses even when the value isundefined. Tighten the guard to assert it’s a function/class and simplify the later null-check.Apply this diff:
-const isPaymentApp = (x: unknown): x is PaymentApp => - !!x && - typeof x === "object" && - "lib" in x && - typeof x.lib === "object" && - !!x.lib && - "PaymentService" in x.lib; +const isPaymentApp = (x: unknown): x is PaymentApp => + !!x && + typeof x === "object" && + x !== null && + typeof (x as any).lib === "object" && + (x as any).lib !== null && + typeof (x as any).lib?.PaymentService === "function"; @@ - const PaymentService = paymentApp.lib?.PaymentService; - if (!PaymentService) { - console.warn("PaymentService is not defined in paymentApp.lib"); + const PaymentService = (paymentApp as PaymentApp).lib?.PaymentService; + if (typeof PaymentService !== "function") { + console.warn("PaymentService is not defined or not a constructor in paymentApp.lib"); return null; }Also applies to: 65-69
18-19: Prefer hasOwnProperty overinto avoid prototype pollution edge casesNot critical here, but
inchecks the prototype chain. UsehasOwnPropertyfor precise membership.Apply this diff:
-const isKeyOf = <T extends object>(obj: T, key: unknown): key is keyof T => - typeof key === "string" && key in obj; +const isKeyOf = <T extends object>(obj: T, key: unknown): key is keyof T => + typeof key === "string" && Object.prototype.hasOwnProperty.call(obj, key);
105-115: Optional: include more context in errors and rethrow with cause
afterPaymentfailures are swallowed with a log. Consider including booking ID and app key for observability, or rethrow with a wrapped error if callers should handle it.Apply this diff:
- try { - await paymentInstance.afterPayment(evt, booking, paymentData, selectedEventType?.metadata); - } catch (e) { - console.error(e); - } + try { + await paymentInstance.afterPayment(evt, booking, paymentData, selectedEventType?.metadata); + } catch (e) { + console.error(`afterPayment failed for bookingId=${booking.id}, appId=${paymentAppCredentials.appId}`, e); + }packages/app-store/crm.services.generated.ts (1)
1-10: LGTM; optional: export aCrmServiceSlugaliasMap shape and paths look correct. As with other generated maps, exporting a slug type improves consumer ergonomics.
Apply this diff (and mirror in the generator):
} as const; +export type CrmServiceSlug = keyof typeof CRM_SERVICES;packages/app-store/_utils/getCalendar.ts (5)
29-33: Guard against unexpected missing/empty credential.type.Types likely guarantee a string, but if type is ever null/empty, calendarType.split("_") will throw. Add a defensive early return.
if (!credential || !credential.key) return null; let { type: calendarType } = credential; + if (!calendarType) { + log.warn("Missing credential.type for calendar credential id=%s", credential?.id ?? "unknown"); + return null; + }Also applies to: 38-39
39-39: Avoidany; type the factory to improve DX and checks.Typing the factory reduces accidental misuse and enables smarter intellisense.
- const modFactory = (CALENDAR_SERVICES as Record<string, any>)[slug]; + type CalendarModule = { lib?: { CalendarService?: CalendarClass } }; + const modFactory = normalizedMap[slug] as (() => Promise<CalendarModule>) | undefined;
42-44: Improve warning context.Including the original credential.type and credential id makes debugging much easier.
- if (!calendarApp?.lib?.CalendarService) { - log.warn(`calendar of type ${slug} is not implemented`); + if (!calendarApp?.lib?.CalendarService) { + log.warn( + `Calendar not implemented`, + { slug, originalType: calendarType, credentialId: credential?.id } + ); return null; }
18-25: Use the type predicate for narrowing (or remove it).
isCalendarServiceis defined but unused. Use it to make the intent explicit.- if (!calendarApp?.lib?.CalendarService) { + if (!isCalendarService(calendarApp)) { log.warn( - `calendar of type ${slug} is not implemented`); + `Calendar not implemented`, + { slug, originalType: calendarType, credentialId: credential?.id } + ); return null; } - const { CalendarService } = calendarApp.lib; + const { CalendarService } = calendarApp.lib;Also applies to: 42-45
40-41: Consider catching dynamic import failures to downgrade to a warn.If the import throws (missing file, build artifact mismatch), this currently bubbles and throws. Optional: catch, log, and return null to keep UX consistent with "not implemented".
- const calendarApp = modFactory ? await modFactory() : null; + const calendarApp = modFactory + ? await modFactory().catch((e) => { + log.warn("Failed to load calendar module", { slug, error: String(e) }); + return null; + }) + : null;packages/app-store/conferencing.videoAdapters.generated.ts (1)
3-15: Emit a minimal type contract from the generator for better safety.Without a shape, downstreams must cast to
any. Consider making the generator appendsatisfies Record<string, () => Promise<unknown>>to the object literal. This preserves readonly keys, catches accidental non-function values, and doesn’t force consumers to import heavy types.If implemented in the generator, this file would end as:
export const VIDEO_ADAPTERS = { dailyvideo: () => import("./dailyvideo/index"), ... zoomvideo: () => import("./zoomvideo/index"), -} as const; +} as const satisfies Record<string, () => Promise<unknown>>;packages/lib/videoClient.ts (3)
27-33: Normalize keys consistently and improve typing to avoidany.The same underscore-stripping shows up in multiple places across the codebase. Centralizing normalization reduces drift. Also, you can avoid
anywhen indexing VIDEO_ADAPTERS.- const appName = cred.type.split("_").join(""); // Transform `zoom_video` to `zoomvideo`; + const normalize = (s: string) => s.replace(/[_-]/g, "").toLowerCase(); + const appName = normalize(cred.type); // `zoom_video` -> `zoomvideo` log.silly("Getting video adapter for", safeStringify({ appName, cred: getPiiFreeCredential(cred) })); - const modFactory = (VIDEO_ADAPTERS as Record<string, any>)[appName]; + const normalizedMap = Object.fromEntries( + Object.entries(VIDEO_ADAPTERS).map(([k, v]) => [normalize(k), v]) + ) as Record<string, () => Promise<unknown>>; + const modFactory = normalizedMap[appName];Optionally extract
normalizeinto a shared util (e.g.,@calcom/app-store/normalizeKey) and reuse in calendar/payment/CRM to keep behavior aligned.
32-33: Optional: parallelize module loading when multiple credentials are present.If users have multiple video creds, sequential awaits add latency. You can stage imports with Promise.all.
- const app = modFactory ? await modFactory() : null; + const app = modFactory ? await modFactory() : null;Outside this hunk, a more impactful change:
// Replace the for-of + await with: const normalize = (s: string) => s.replace(/[_-]/g, "").toLowerCase(); const normalizedMap = Object.fromEntries(Object.entries(VIDEO_ADAPTERS).map(([k, v]) => [normalize(k), v])) as Record<string, () => Promise<unknown>>; const apps = await Promise.all( withCredentials.map((cred) => { const appName = normalize(cred.type); const factory = normalizedMap[appName]; return factory ? factory().then((m) => [cred, m] as const).catch(() => [cred, null] as const) : Promise.resolve([cred, null] as const); }) ); // then iterate apps to build adapters
51-54: Filter out undefined availability entries.Current reduce may accumulate undefined entries. Filter them out to keep consumers simpler.
- Promise.all((await getVideoAdapters(withCredentials)).map((c) => c?.getAvailability())).then((results) => - results.reduce((acc, availability) => acc.concat(availability), [] as (EventBusyDate | undefined)[]) - ); + Promise.all((await getVideoAdapters(withCredentials)).map((c) => c?.getAvailability())) + .then((results) => results.flat().filter(Boolean) as EventBusyDate[]);scripts/generate-app-store-maps.mjs (5)
45-49: Deterministic output: sort slugs to stabilize diffs across OS/filesystems.Directory order varies; sorting prevents noisy PR churn.
function listAppSlugs() { - return fs.readdirSync(APP_STORE) + return fs.readdirSync(APP_STORE) .filter((n) => !n.startsWith(".")) - .filter((n) => isDir(path.join(APP_STORE, n))); + .filter((n) => isDir(path.join(APP_STORE, n))) + .sort(); }
51-58: Signature detection is brittle (TS-only, case-sensitive, and path separators).
- Some packages may ship JS (".js") signatures in OSS/EE splits.
- Case differences (e.g., "videoApiAdapter.ts") can slip in.
- Joining a slashy relSig with path.join on Windows yields mixed separators.
Make detection more forgiving.
-function findSlugsWithSignature(slugs, relSig) { - return slugs.filter((slug) => - exists(path.join(APP_STORE, slug, relSig)) && - (exists(path.join(APP_STORE, slug, "index.ts")) - || exists(path.join(APP_STORE, slug, "index.tsx")) - || exists(path.join(APP_STORE, slug, "index.js"))) - ); -} +function findSlugsWithSignature(slugs, relSig) { + const sigParts = relSig.split("/"); // normalize separators + const candidates = [ + path.join(APP_STORE, "$SLUG$", ...sigParts), + path.join(APP_STORE, "$SLUG$", ...sigParts).replace(/\.ts$/i, ".js"), + path.join(APP_STORE, "$SLUG$", ...sigParts.map((p) => p.replace(/ApiAdapter\.ts$/i, "apiadapter.ts"))), + ]; + const hasIndex = (slug) => + ["index.ts", "index.tsx", "index.js", "index.mjs", "index.cjs"].some((f) => + exists(path.join(APP_STORE, slug, f)) + ); + return slugs.filter((slug) => candidates.some((c) => exists(c.replace("$SLUG$", slug))) && hasIndex(slug)); +}
60-67: Emit a minimal type contract to improve consumer typing.Have the generator append
satisfies Record<string, () => Promise<unknown>>so consuming code avoidsanycasts while not importing heavy types.- `export const ${exportName} = {`, + `export const ${exportName} = {`, ...slugs.map((slug) => ` "${slug}": () => import("./${slug}/index"),`), - `} as const;`, + `} as const satisfies Record<string, () => Promise<unknown>>;`,
74-84: Stabilize per-category ordering and log empties with context.Sorting matches makes output stable; logging counts per category helps CI debugging.
for (const cat of CATEGORIES) { - const matches = findSlugsWithSignature(slugs, cat.signature); - writeFactoryMap({ exportName: cat.exportName, outFile: cat.outFile, slugs: matches }); + const matches = findSlugsWithSignature(slugs, cat.signature).sort(); + writeFactoryMap({ exportName: cat.exportName, outFile: cat.outFile, slugs: matches }); }
11-36: Consider generating normalized alias keys to remove runtime normalization costs.To avoid repeating normalization logic in consumers (strip [_-], lowercase), the generator could add alias keys. Example: for "googlecalendar", also generate "google_calendar" and "google-calendar" (or a separate alias map) pointing to the same thunk.
Happy to draft a small enhancement to emit aliases per slug in this script if you want to go this route.
package.json (1)
60-61: Ensure generated maps exist in all environments (predev/postinstall hooks).If a dev pulls fresh without generated files, imports will fail. Run the generator in postinstall and before dev/type-check to avoid footguns.
Apply this diff to append the generator to postinstall:
- "postinstall": "husky install && turbo run post-install", + "postinstall": "husky install && turbo run post-install && node scripts/generate-app-store-maps.mjs",Optionally, make dev more robust:
- "predev": "echo 'Checking env files'", + "predev": "node scripts/generate-app-store-maps.mjs && echo 'Checking env files'",And have CI enforce banned imports during type-check:
- "type-check:ci": "turbo run type-check:ci --log-prefix=none", + "type-check:ci": "turbo run type-check:ci --log-prefix=none && node scripts/check-banned-imports.mjs",
📜 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 (10)
package.json(1 hunks)packages/app-store/_utils/getCalendar.ts(2 hunks)packages/app-store/calendar.services.generated.ts(1 hunks)packages/app-store/conferencing.videoAdapters.generated.ts(1 hunks)packages/app-store/crm.services.generated.ts(1 hunks)packages/app-store/payment.apps.generated.ts(1 hunks)packages/lib/payment/handlePayment.ts(2 hunks)packages/lib/videoClient.ts(2 hunks)scripts/check-banned-imports.mjs(1 hunks)scripts/generate-app-store-maps.mjs(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/payment.apps.generated.tspackages/app-store/calendar.services.generated.tspackages/lib/payment/handlePayment.tspackages/lib/videoClient.tspackages/app-store/crm.services.generated.tspackages/app-store/conferencing.videoAdapters.generated.tspackages/app-store/_utils/getCalendar.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/payment.apps.generated.tspackages/app-store/calendar.services.generated.tspackages/lib/payment/handlePayment.tspackages/lib/videoClient.tspackages/app-store/crm.services.generated.tspackages/app-store/conferencing.videoAdapters.generated.tspackages/app-store/_utils/getCalendar.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/app-store/payment.apps.generated.tspackages/app-store/calendar.services.generated.tspackages/lib/payment/handlePayment.tspackages/lib/videoClient.tspackages/app-store/crm.services.generated.tspackages/app-store/conferencing.videoAdapters.generated.tspackages/app-store/_utils/getCalendar.ts
🧠 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.ts
🧬 Code graph analysis (3)
packages/lib/payment/handlePayment.ts (1)
packages/app-store/payment.apps.generated.ts (1)
PAYMENT_APPS(3-10)
packages/lib/videoClient.ts (1)
packages/app-store/conferencing.videoAdapters.generated.ts (1)
VIDEO_ADAPTERS(3-15)
packages/app-store/_utils/getCalendar.ts (1)
packages/app-store/calendar.services.generated.ts (1)
CALENDAR_SERVICES(3-17)
🔇 Additional comments (3)
scripts/check-banned-imports.mjs (1)
1-14: LGTM overallNice lightweight guard for CI. With the regex fix above, this becomes reliable and future-proof.
packages/app-store/conferencing.videoAdapters.generated.ts (1)
1-15: Auto-generated map looks correct and aligns with consumers.Keys match the adapters referenced in videoClient, thunks are lazy, and the export is named (good for tree-shaking).
package.json (1)
85-88: New scripts LGTM.The generator and banned-imports checker are wired into scripts. Consider ensuring they run in CI/local flows automatically (see next comment).
| const slug = calendarType.split("_").join(""); // e.g., "google_calendar" -> "googlecalendar" | ||
| const modFactory = (CALENDAR_SERVICES as Record<string, any>)[slug]; | ||
| const calendarApp = modFactory ? await modFactory() : null; | ||
|
|
There was a problem hiding this comment.
Slug normalization is brittle; breaks hyphenated keys like "ics-feedcalendar".
Some entries in CALENDAR_SERVICES use hyphens (e.g., "ics-feedcalendar"), but here we only strip underscores. For a credential type like "ics_feedcalendar", slug becomes "icsfeedcalendar" and won't match the generated key, causing a false "not implemented" and feature breakage.
Harden lookup by normalizing both sides (strip underscores and hyphens; lowercase) before indexing.
Apply this diff to make the lookup robust without touching the generator:
- const slug = calendarType.split("_").join(""); // e.g., "google_calendar" -> "googlecalendar"
- const modFactory = (CALENDAR_SERVICES as Record<string, any>)[slug];
- const calendarApp = modFactory ? await modFactory() : null;
+ const normalize = (s: string) => s.replace(/[_-]/g, "").toLowerCase();
+ const normalizedMap = Object.fromEntries(
+ Object.entries(CALENDAR_SERVICES).map(([k, v]) => [normalize(k), v])
+ ) as Record<string, () => Promise<unknown>>;
+ const slug = normalize(calendarType); // e.g., "google_calendar" -> "googlecalendar"
+ const modFactory = normalizedMap[slug];
+ const calendarApp = modFactory ? await modFactory() : null;📝 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 slug = calendarType.split("_").join(""); // e.g., "google_calendar" -> "googlecalendar" | |
| const modFactory = (CALENDAR_SERVICES as Record<string, any>)[slug]; | |
| const calendarApp = modFactory ? await modFactory() : null; | |
| const normalize = (s: string) => s.replace(/[_-]/g, "").toLowerCase(); | |
| const normalizedMap = Object.fromEntries( | |
| Object.entries(CALENDAR_SERVICES).map(([k, v]) => [normalize(k), v]) | |
| ) as Record<string, () => Promise<unknown>>; | |
| const slug = normalize(calendarType); // e.g., "google_calendar" -> "googlecalendar" | |
| const modFactory = normalizedMap[slug]; | |
| const calendarApp = modFactory ? await modFactory() : null; |
🤖 Prompt for AI Agents
In packages/app-store/_utils/getCalendar.ts around lines 38 to 41, the slug
normalization only strips underscores and fails to match CALENDAR_SERVICES keys
that contain hyphens; update the lookup to normalize both sides by lowercasing
and removing underscores and hyphens: compute normalizedSlug =
calendarType.toLowerCase().replace(/[_-]/g, "") and then find the service by
iterating CALENDAR_SERVICES keys, normalizing each key the same way and using
the first match to get the factory; call the factory if found, otherwise return
null. Ensure you do not change the generator or CALENDAR_SERVICES itself.
| if (!isKeyOf(PAYMENT_APPS as any, key)) { | ||
| console.warn(`key: ${key} is not a valid key in PAYMENT_APPS`); | ||
| return null; | ||
| } | ||
| const paymentApp = await appStore[key]?.(); | ||
| const modFactory = (PAYMENT_APPS as Record<string, any>)[key]; | ||
| const paymentApp = modFactory ? await modFactory() : null; | ||
| if (!isPaymentApp(paymentApp)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen key narrowing and handle dynamic import failures
Type-narrow key against PAYMENT_APPS without any casts, and guard the dynamic import with try/catch to avoid unhandled rejections if the module fails to load.
Apply this diff:
- if (!isKeyOf(PAYMENT_APPS as any, key)) {
+ if (!key || !isKeyOf(PAYMENT_APPS, key)) {
console.warn(`key: ${key} is not a valid key in PAYMENT_APPS`);
return null;
}
- const modFactory = (PAYMENT_APPS as Record<string, any>)[key];
- const paymentApp = modFactory ? await modFactory() : null;
+ const modFactory = PAYMENT_APPS[key];
+ let paymentApp: unknown = null;
+ try {
+ paymentApp = await modFactory();
+ } catch (e) {
+ console.error(`Failed to load payment app module for key ${key}`, e);
+ return null;
+ }📝 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.
| if (!isKeyOf(PAYMENT_APPS as any, key)) { | |
| console.warn(`key: ${key} is not a valid key in PAYMENT_APPS`); | |
| return null; | |
| } | |
| const paymentApp = await appStore[key]?.(); | |
| const modFactory = (PAYMENT_APPS as Record<string, any>)[key]; | |
| const paymentApp = modFactory ? await modFactory() : null; | |
| if (!isPaymentApp(paymentApp)) { | |
| if (!key || !isKeyOf(PAYMENT_APPS, key)) { | |
| console.warn(`key: ${key} is not a valid key in PAYMENT_APPS`); | |
| return null; | |
| } | |
| const modFactory = PAYMENT_APPS[key]; | |
| let paymentApp: unknown = null; | |
| try { | |
| paymentApp = await modFactory(); | |
| } catch (e) { | |
| console.error(`Failed to load payment app module for key ${key}`, e); | |
| return null; | |
| } |
scripts/check-banned-imports.mjs
Outdated
| const forbidden = [ | ||
| 'from "@calcom/app-store"', | ||
| "from 'packages/app-store/index'", | ||
| ]; | ||
| const out = execSync('git grep -nE "(from \\\\\"@calcom/app-store\\\\\\\"|from \\\\\"packages/app-store/index\\\\\\\")" || true', { stdio: "pipe" }).toString(); |
There was a problem hiding this comment.
Fix over-escaped regex and cover single quotes, dynamic import(), and require()
The current pattern only catches double-quoted ESM from imports and misses:
- single-quoted imports (common in this repo),
import(...)dynamic imports,require(...),- plus the
forbiddenarray is unused.
This risks letting barrel imports slip through CI unnoticed. Replace the command with a clearer, PCRE-based pattern and scope search to JS/TS files.
Apply this diff:
-const forbidden = [
- 'from "@calcom/app-store"',
- "from 'packages/app-store/index'",
-];
-const out = execSync('git grep -nE "(from \\\\\"@calcom/app-store\\\\\\\"|from \\\\\"packages/app-store/index\\\\\\\")" || true', { stdio: "pipe" }).toString();
+const pattern = [
+ String.raw`\bfrom\s+['"]@calcom/app-store['"]`,
+ String.raw`\bfrom\s+['"]packages\/app-store\/index['"]`,
+ String.raw`\bimport\(\s*['"]@calcom/app-store['"]\s*\)`,
+ String.raw`\bimport\(\s*['"]packages\/app-store\/index['"]\s*\)`,
+ String.raw`\brequire\(\s*['"]@calcom/app-store['"]\s*\)`,
+ String.raw`\brequire\(\s*['"]packages\/app-store\/index['"]\s*\)`,
+].join("|");
+const out = execSync(`git grep -nP '${pattern}' -- ':(glob)**/*.{ts,tsx,js,jsx}' || true`, { stdio: "pipe" }).toString();📝 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 forbidden = [ | |
| 'from "@calcom/app-store"', | |
| "from 'packages/app-store/index'", | |
| ]; | |
| const out = execSync('git grep -nE "(from \\\\\"@calcom/app-store\\\\\\\"|from \\\\\"packages/app-store/index\\\\\\\")" || true', { stdio: "pipe" }).toString(); | |
| const pattern = [ | |
| String.raw`\bfrom\s+['"]@calcom/app-store['"]`, | |
| String.raw`\bfrom\s+['"]packages\/app-store\/index['"]`, | |
| String.raw`\bimport\(\s*['"]@calcom/app-store['"]\s*\)`, | |
| String.raw`\bimport\(\s*['"]packages\/app-store\/index['"]\s*\)`, | |
| String.raw`\brequire\(\s*['"]@calcom/app-store['"]\s*\)`, | |
| String.raw`\brequire\(\s*['"]packages\/app-store\/index['"]\s*\)`, | |
| ].join("|"); | |
| const out = execSync( | |
| `git grep -nP '${pattern}' -- ':(glob)**/*.{ts,tsx,js,jsx}' || true`, | |
| { stdio: "pipe" } | |
| ).toString(); |
🤖 Prompt for AI Agents
In scripts/check-banned-imports.mjs around lines 3 to 7, the regex is
over-escaped, only matches double-quoted static `from` imports, and the
`forbidden` array is unused; update the script to (1) remove or use the
`forbidden` array and not leave it unused, (2) replace the execSync git-grep
command with a PCRE pattern that matches both single and double quotes and also
matches dynamic imports and CommonJS requires (e.g. patterns for
from\s+["']@calcom/app-store["'] | import\(\s*["']@calcom/app-store["']\s*\) |
require\(\s*["']@calcom/app-store["']\s*\)), (3) scope the git-grep to only
search JS/TS files (e.g. -- '*.js' -- '*.ts' -- '*.tsx' etc.), and (4) simplify
escaping so the pattern is passed correctly to git grep -P; ensure stdout is
captured and the command falls back to true when no matches.
joeauyeung
left a comment
joeauyeung
left a comment
There was a problem hiding this comment.
Hey @BenraouaneSoufiane thanks for your contributions. Thanks for expanding this approach. Heads up in #22450 is already handling calendar apps so we only need to expand this for other app categories.
Have you considered the build script in under the packages/app-store-cli to generate the app maps? Can we have all the logic under there and follow the naming convention that we have now?
Once this PR is ready, can you also add the "ready for E2E" label to your PR as well to ensure all tests are passing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/lib/videoClient.ts (1)
128-131: Use of safe wrapper is correct here once recursion is fixed.After fixing safeNotifyBrokenVideoIntegration, this await is appropriate to ensure consistent sequencing and logging.
🧹 Nitpick comments (2)
packages/lib/videoClient.ts (2)
43-43: Tighten typing when indexing VIDEO_ADAPTERS.Avoid
anyand add a type guard so TS can help catch key mismatches.Apply this diff:
- const modFactory = (VIDEO_ADAPTERS as Record<string, any>)[appName]; + const modFactory = + appName in VIDEO_ADAPTERS + ? (VIDEO_ADAPTERS as Record<string, () => Promise<unknown>>)[appName] + : undefined;
160-163: Same note as in createMeeting: rely on the fixed safeNotify wrapper; also sanitize subsequent logs.After you fix the wrapper, consider sanitizing the next log line (Line 164) to avoid leaking PII by logging the raw calEvent.
A change outside this range would look like:
log.error( "updateMeeting failed", safeStringify(e), safeStringify({ calEvent: getPiiFreeCalendarEvent(calEvent) }) );
📜 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 (1)
packages/lib/videoClient.ts(4 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/lib/videoClient.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/lib/videoClient.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/lib/videoClient.ts
🧬 Code graph analysis (1)
packages/lib/videoClient.ts (3)
packages/types/Calendar.d.ts (1)
CalendarEvent(163-226)packages/types/VideoApiAdapter.d.ts (1)
VideoApiAdapter(19-49)packages/app-store/conferencing.videoAdapters.generated.ts (1)
VIDEO_ADAPTERS(3-15)
⏰ 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). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
🔇 Additional comments (1)
packages/lib/videoClient.ts (1)
5-5: Good move: swap to generated VIDEO_ADAPTERS map.This aligns with the PR goal to avoid the app-store barrel and reduces unnecessary async indirection in hot paths.
packages/lib/videoClient.ts
Outdated
| // Swallow email-template errors so they never break booking flows. | ||
| // (E.g., if the broken-integration email template expects fields that are missing) | ||
| const safeNotifyBrokenVideoIntegration = async (calEvent: CalendarEvent, logCtx?: unknown) => { | ||
| try { | ||
| await safeNotifyBrokenVideoIntegration(calEvent, { | ||
| where: "videoClient", | ||
| action: "send_broken_integration_email", | ||
| }); | ||
| } catch (err) { | ||
| log.error("sendBrokenIntegrationEmail failed", safeStringify(err), safeStringify(logCtx)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix infinite recursion in safeNotifyBrokenVideoIntegration.
The function calls itself, resulting in unbounded recursion and eventual stack overflow. It should call the real mailer (e.g., sendBrokenIntegrationEmail) instead.
Apply this diff to call the underlying notifier via dynamic import and keep failures non-blocking:
const safeNotifyBrokenVideoIntegration = async (calEvent: CalendarEvent, logCtx?: unknown) => {
try {
- await safeNotifyBrokenVideoIntegration(calEvent, {
- where: "videoClient",
- action: "send_broken_integration_email",
- });
+ // Late-load to keep emails out of the hot path bundles.
+ const { sendBrokenIntegrationEmail } = await import(
+ /* webpackChunkName: "broken-integration-email" */ "@calcom/lib/emails/sendBrokenIntegrationEmail"
+ );
+ await sendBrokenIntegrationEmail(calEvent, logCtx);
} catch (err) {
log.error("sendBrokenIntegrationEmail failed", safeStringify(err), safeStringify(logCtx));
}
};Run this to confirm the correct import path for the mailer and adjust the string above if needed:
🏁 Script executed:
#!/bin/bash
# Find the definition(s) and usages of sendBrokenIntegrationEmail to determine the canonical import path.
rg -nP --hidden --glob '!**/node_modules/**' -C2 '\bsendBrokenIntegrationEmail\b'Length of output: 1675
Replace recursive self-call with dynamic import of sendBrokenIntegrationEmail
This change fixes the infinite recursion in safeNotifyBrokenVideoIntegration by late-loading the real notifier and calling it with the proper signature.
• File: packages/lib/videoClient.ts
• Lines: 25–36
const safeNotifyBrokenVideoIntegration = async (calEvent: CalendarEvent, logCtx?: unknown) => {
try {
- await safeNotifyBrokenVideoIntegration(calEvent, {
- where: "videoClient",
- action: "send_broken_integration_email",
- });
+ // Late-load to keep email logic out of the hot path.
+ const { sendBrokenIntegrationEmail } = await import(
+ /* webpackChunkName: "broken-integration-email" */ "@calcom/lib/emails/email-manager"
+ );
+ // Pass the “video” integration type per its signature.
+ await sendBrokenIntegrationEmail(calEvent, "video");
} catch (err) {
log.error("sendBrokenIntegrationEmail failed", safeStringify(err), safeStringify(logCtx));
}
};📝 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.
| // Swallow email-template errors so they never break booking flows. | |
| // (E.g., if the broken-integration email template expects fields that are missing) | |
| const safeNotifyBrokenVideoIntegration = async (calEvent: CalendarEvent, logCtx?: unknown) => { | |
| try { | |
| await safeNotifyBrokenVideoIntegration(calEvent, { | |
| where: "videoClient", | |
| action: "send_broken_integration_email", | |
| }); | |
| } catch (err) { | |
| log.error("sendBrokenIntegrationEmail failed", safeStringify(err), safeStringify(logCtx)); | |
| } | |
| }; | |
| // Swallow email-template errors so they never break booking flows. | |
| // (E.g., if the broken-integration email template expects fields that are missing) | |
| const safeNotifyBrokenVideoIntegration = async (calEvent: CalendarEvent, logCtx?: unknown) => { | |
| try { | |
| // Late-load to keep email logic out of the hot path. | |
| const { sendBrokenIntegrationEmail } = await import( | |
| /* webpackChunkName: "broken-integration-email" */ "@calcom/lib/emails/email-manager" | |
| ); | |
| // Pass the “video” integration type per its signature. | |
| await sendBrokenIntegrationEmail(calEvent, "video"); | |
| } catch (err) { | |
| log.error("sendBrokenIntegrationEmail failed", safeStringify(err), safeStringify(logCtx)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In packages/lib/videoClient.ts around lines 25–36, the function currently calls
itself causing infinite recursion; replace the self-call with a dynamic import
of the module that exports sendBrokenIntegrationEmail (use the correct relative
path in your repo), then invoke the imported sendBrokenIntegrationEmail with the
original CalendarEvent and the metadata object ({ where: "videoClient", action:
"send_broken_integration_email" }) and pass through the log context so errors
are caught and logged as before.
packages/lib/videoClient.ts
Outdated
| let app = modFactory ? await modFactory() : null; | ||
| if (!app) { | ||
| const appImportFn = (appStore as Record<string, any>)[appName]; | ||
| app = appImportFn ? await appImportFn() : null; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid bundling @calcom/app-store in this hot path; lazy-load the fallback.
A static import of the app-store barrel undermines the PR’s performance goal by pulling in a large module even when VIDEO_ADAPTERS covers the adapter. Only load it if the generated map has no entry.
Apply this diff to lazy-load the fallback:
- let app = modFactory ? await modFactory() : null;
- if (!app) {
- const appImportFn = (appStore as Record<string, any>)[appName];
- app = appImportFn ? await appImportFn() : null;
- }
+ let app = modFactory ? await modFactory() : null;
+ if (!app) {
+ // Late-load legacy barrel only when strictly necessary.
+ const { default: legacyAppStore } = await import("@calcom/app-store");
+ const appImportFn = (legacyAppStore as Record<string, () => Promise<unknown>>)[appName];
+ app = appImportFn ? await appImportFn() : null;
+ }And remove the now-unneeded top-level import to prevent it from being bundled (outside the selected range):
- import appStore from "@calcom/app-store";
+ // Avoid static import of "@calcom/app-store"; it is lazy-loaded on demand.Use this to verify there are no remaining static imports of the app-store barrel in hot paths:
🏁 Script executed:
#!/bin/bash
rg -nP --glob '!**/node_modules/**' -C2 'from\s+["\']@calcom/app-store["\']'Length of output: 116
🏁 Script executed:
#!/bin/bash
# Re-run search for static imports of "@calcom/app-store" with corrected quoting
rg -nP --glob '!**/node_modules/**' -C2 "from\\s+[\\"']@calcom/app-store[\\"']"Length of output: 116
🏁 Script executed:
#!/bin/bash
# Check for ES module static imports of "@calcom/app-store"
rg -nP --glob '!**/node_modules/**' -C2 'import\s+.*@calcom/app-store'
# Check for CommonJS requires of "@calcom/app-store"
rg -nP --glob '!**/node_modules/**' -C2 'require\(['"'"']@calcom/app-store['"'"']\)'Length of output: 188486
Avoid bundling @calcom/app-store in videoClient.ts hot path
The static import of the entire app-store barrel in packages/lib/videoClient.ts forces a large module into your main bundle even when a specialized adapter exists. To meet the PR’s performance goals, move that import into the fallback branch so it’s only loaded when absolutely necessary.
• In packages/lib/videoClient.ts (line 4), remove:
- import appStore from "@calcom/app-store";• In the fallback path (around lines 45–49), replace use of the static appStore object with a dynamic import:
- let app = modFactory ? await modFactory() : null;
- if (!app) {
- const appImportFn = (appStore as Record<string, any>)[appName];
- app = appImportFn ? await appImportFn() : null;
- }
+ let app = modFactory ? await modFactory() : null;
+ if (!app) {
+ // Late‐load legacy app‐store barrel only when the generated map has no entry
+ const { default: legacyAppStore } = await import("@calcom/app-store");
+ const appImportFn = (legacyAppStore as Record<string, () => Promise<unknown>>)[appName];
+ app = appImportFn ? await appImportFn() : null;
+ }This ensures the bulk of @calcom/app-store is only fetched on demand when VIDEO_ADAPTERS doesn’t cover a given app.
📝 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.
| let app = modFactory ? await modFactory() : null; | |
| if (!app) { | |
| const appImportFn = (appStore as Record<string, any>)[appName]; | |
| app = appImportFn ? await appImportFn() : null; | |
| } | |
| // --- at the top of packages/lib/videoClient.ts, remove the static import --- | |
| // import appStore from "@calcom/app-store"; | |
| // ... other imports ... | |
| // --- later, around lines 45–49, replace the existing fallback with this --- | |
| let app = modFactory ? await modFactory() : null; | |
| if (!app) { | |
| // Late‐load legacy app‐store barrel only when the generated map has no entry | |
| const { default: legacyAppStore } = await import("@calcom/app-store"); | |
| const appImportFn = (legacyAppStore as Record<string, () => Promise<unknown>>)[appName]; | |
| app = appImportFn ? await appImportFn() : null; | |
| } |
This comment was marked as resolved.
This comment was marked as resolved.
|
Hey @laz-001 I was looking through PRs linked to the original issue under the "Development" tab on the right hand side. I see the PR you shared wasn't linked to the issue through the development so that's why I missed it but I see @keithwillcode is looking at it. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/features/bookings/lib/handleNewBooking.ts (2)
125-127: Don’t duplicate error-code strings; reuse centralErrorCodeenum.These constants duplicate values already exposed via
ErrorCode. Prefer the centralized enum to avoid drift and keep behavior consistent across surfaces.Apply this diff to remove the redundant constants (the helper below will be updated to use
ErrorCode.*):-const ERR_NO_AVAILABLE_USERS = "no_available_users_found_error"; -const ERR_FIXED_HOSTS_UNAVAILABLE = "fixed_hosts_unavailable_for_booking";
2447-2469: Tighten types and switch toSchedulingType/ErrorCodeinpickNoAvailabilityErrorConfirmed that
ErrorCode.FixedHostsUnavailableForBookingandErrorCode.NoAvailableUsersFoundexist with the expected string values, so you can safely replace the oldERR_*constants.Key changes:
- Define a minimal
EventTypeLikeinterface instead of usingany- Use the
SchedulingTypeenum (falling back to a case-insensitive check) rather than raw string literals- Refine “fixed hosts” detection to include:
- All collective events
- Any host explicitly marked with
isFixed- Legacy
fixedHostsarrays- Return errors via the
ErrorCodeenum instead ofERR_NO_AVAILABLE_USERS/ERR_FIXED_HOSTS_UNAVAILABLESuggested diff:
-type pickNoAvailabilityError(opts: { - eventType?: any; // tolerate unknown shape to keep this change simple - isTeamEvent?: boolean; -}): Error { +type EventTypeLike = { + schedulingType?: SchedulingType | string; + hosts?: Array<{ isFixed?: boolean }>; + fixedHosts?: unknown[]; +}; + +function pickNoAvailabilityError(opts: { + eventType?: EventTypeLike; + isTeamEvent?: boolean; +}): Error { const et = opts.eventType ?? {}; - const schedulingType = et?.schedulingType as string | undefined; - const fixedHostsCount = Array.isArray(et?.hosts) - ? et.hosts.length - : Array.isArray(et?.fixedHosts) - ? et.fixedHosts.length - : 0; - - const isCollective = schedulingType?.toUpperCase?.() === "COLLECTIVE"; - const looksFixedHosts = (fixedHostsCount ?? 0) > 0; - - if ((opts.isTeamEvent && isCollective && looksFixedHosts) || (isCollective && looksFixedHosts)) { - return new Error(ERR_FIXED_HOSTS_UNAVAILABLE); - } - return new Error(ERR_NO_AVAILABLE_USERS); + // Use the enum if possible, otherwise fall back to case-insensitive string match + const schedulingType = et.schedulingType as SchedulingType | string | undefined; + const isCollective = + schedulingType === SchedulingType.COLLECTIVE || + String(schedulingType ?? "").toUpperCase() === "COLLECTIVE"; + + // Collective events count as “fixed hosts”, as do any hosts marked `isFixed` or a nonempty legacy array + const hasFixedHosts = + isCollective || + (Array.isArray(et.hosts) && et.hosts.some((h) => Boolean(h?.isFixed))) || + (Array.isArray(et.fixedHosts) && et.fixedHosts.length > 0); + + if (opts.isTeamEvent && hasFixedHosts) { + return new Error(ErrorCode.FixedHostsUnavailableForBooking); + } + if (isCollective && hasFixedHosts) { + return new Error(ErrorCode.FixedHostsUnavailableForBooking); + } + return new Error(ErrorCode.NoAvailableUsersFound); +}Optional: add a few unit tests covering combinations such as:
{ schedulingType: COLLECTIVE, hosts: [{}] }→FixedHostsUnavailableForBooking{ schedulingType: ROUND_ROBIN, hosts: [{ isFixed: true }], isTeamEvent: true }→FixedHostsUnavailableForBooking{ schedulingType: ROUND_ROBIN, hosts: [] }→NoAvailableUsersFound
📜 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 (1)
packages/features/bookings/lib/handleNewBooking.ts(3 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/features/bookings/lib/handleNewBooking.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/bookings/lib/handleNewBooking.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/bookings/lib/handleNewBooking.ts
⏰ 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: Type check / check-types
- GitHub Check: Tests / Unit
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/features/bookings/lib/handleNewBooking.ts (1)
866-869: Prior undefined identifier is now resolved — thanks; finish cleanupThe earlier compile error about
isTeamEventbeing undefined is resolved by adding a localisTeamEvent. After the helper signature simplification above, you can remove that local and the placeholder comments here.
🧹 Nitpick comments (2)
packages/features/bookings/lib/handleNewBooking.ts (2)
830-834: Avoid redefining “isTeamEvent”; reuse existingisTeamEventTypeor drop itYou compute a new
isTeamEventhere, butisTeamEventTypeis already computed on Line 599. Two slightly different definitions for “team event” can diverge and cause inconsistent error selection paths. With the simplified helper below, you can drop this entirely.Apply this diff to remove the redundant local:
- const isTeamEvent = - Boolean(eventType?.teamId) || - (Array.isArray(eventType?.hosts) && eventType.hosts.length > 1) || - eventType?.schedulingType?.toUpperCase?.() === "COLLECTIVE";
866-869: Remove placeholder comments and simplify helper invocationThe inline comments are placeholders and should not ship. After simplifying the helper signature, just pass
eventType.Apply this diff:
- throw pickNoAvailabilityError({ - eventType, // whatever variable in scope holds the event type/config - isTeamEvent, // if you have such a boolean; otherwise you can omit it - }); + throw pickNoAvailabilityError({ eventType });
📜 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 (1)
packages/features/bookings/lib/handleNewBooking.ts(4 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/features/bookings/lib/handleNewBooking.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/bookings/lib/handleNewBooking.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/bookings/lib/handleNewBooking.ts
🧠 Learnings (1)
📚 Learning: 2025-08-21T13:44:06.784Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.784Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
packages/features/bookings/lib/handleNewBooking.ts (1)
125-127: It looks like we still need to confirm ifpickNoAvailabilityErroris referenced anywhere in the codebase. Once we know whether that helper is actually in use, we can finalize whether it needs to be retained or removed alongside the string literals.Please run the following and share the output:
#!/bin/bash set -euo pipefail # Verify whether the pickNoAvailabilityError helper is referenced anywhere rg -nP '\bpickNoAvailabilityError\s*\(' -n -C2 --glob '!**/dist/**'
I tested this yesterday... #23254 (comment) |
|
Moved back to draft since we now have merge conflicts here due to #22450 merging today. |
BenraouaneSoufiane
force-pushed
the
fix-lazy-local-dev
branch
from
fa75e8c to
6932498
Compare
4 participants
What does this PR do?
PR Body / Description
Context
Historically, parts of the codebase relied on the dynamic
appStorebarrel imports (lazy-loaded via async functions). This created unnecessary indirection, made typing harder, and added overhead.Changes
This PR updates those references to use the new generated service maps directly, aligning with other parts of the repo that already follow this pattern:
Video conferencing
appStoreinvideoClient.tswithVIDEO_ADAPTERS.Calendar services
appStoreingetCalendar.tswithCALENDAR_SERVICES.await calendarAppImportFn()to direct module access.Payment providers
appStoreinhandlePayment.tswithPAYMENT_APPS.isKeyOf) to targetPAYMENT_APPS.CRM services
getCrm.ts, it already uses the generatedCrmServiceMap.Benefits
await appStore[...]?.()).Visual Demo
Video Demo:
https://youtu.be/SQrH-S5bivM
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Algora.io
/claim #23104