perf: improve /apps/installed/calendar page load speed.#23198
perf: improve /apps/installed/calendar page load speed.#23198zhyd1997 wants to merge 30 commits intocalcom:mainfrom
/apps/installed/calendar page load speed.#23198Conversation
|
@zhyd1997 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a cached dynamic-import helper and per-domain loader registries (calendarLoaders, videoLoaders, paymentLoaders, analyticsLoaders). Generates and normalizes calendar and video metadata (calendarApps.metadata.generated.ts, videoApps.metadata.generated.ts, calendarAppsMetaData, videoAppsMetaData) and introduces prepareAppsWithCredentials and sanitizeAppsMetadata. Exposes getCalendarApps and getVideoApps helpers and wires callers (EventManager, CalendarManager, videoClient, TRPC payment handlers, etc.) to use the new loaders and helpers. Adds test mocks for the new loaders and updates test helpers accordingly. Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
dosubot
bot
added
the
performance
|
Related Documentation No published documentation to review for changes on this repository. How did I do? Any feedback? |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/20/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (08/20/25)1 label was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (08/21/25)1 reviewer was added to this PR based on Keith Williams's automation. |
|
cc @keithwillcode for review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/payments.tsx (1)
124-161: Don’t leak raw errors to clientsReturning interpolated errors can expose internals/PII. Log server-side and return a generic message.
Apply this diff:
} catch (err) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `Error processing payment with error ${err}`, - }); + // Consider logging the full error server-side (e.g., with logger.error) + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Error processing payment.", + }); }
🧹 Nitpick comments (16)
tests/libs/__mocks__/app-store.ts (1)
4-5: Type-only import is fine here; no runtime cost.Answering the FIXME: importing the “large type” via
import type * as appStorewon’t impact runtime since it erases at compile time. Safe for tests. If you begin mocking loader maps (e.g., paymentLoaders) elsewhere due to this PR, consider adding a dedicated mock for them too.packages/app-store/_utils/createCachedImport.ts (1)
1-10: Avoid duplicate concurrent imports by caching the in-flight Promise.Current logic can trigger multiple parallel imports if called concurrently before
cachedModuleis set. Cache the in-flight Promise and reset on failure.Apply this diff:
-export function createCachedImport<T>(importFunc: () => Promise<T>): () => Promise<T> { - let cachedModule: T | undefined; - - return async () => { - if (!cachedModule) { - cachedModule = await importFunc(); - } - return cachedModule; - }; -} +export function createCachedImport<T>(importFunc: () => Promise<T>): () => Promise<T> { + let cachedModule: T | undefined; + let inFlight: Promise<T> | null = null; + + return async () => { + if (cachedModule) return cachedModule; + if (!inFlight) { + inFlight = importFunc() + .then((mod) => { + cachedModule = mod; + return mod; + }) + .catch((err) => { + inFlight = null; // allow retry after failure + throw err; + }); + } + return inFlight; + }; +}packages/app-store-cli/src/build.ts (1)
349-367: Calendar metadata generation wiring LGTM; consider watching _metadata.ts for live rebuilds.This mirrors the booker metadata block and filters via
isCalendarApp; good. However, the watcher below only reacts toconfig.jsonchanges. Since_metadata.tsis also a valid source, edits there won’t regenerate outputs in watch mode.Suggested watcher addition (outside this hunk):
// in the chokidar "change" handler .on("change", (filePath) => { if (filePath.endsWith("config.json") || filePath.endsWith("_metadata.ts")) { console.log("Config/metadata file changed"); debouncedGenerateFiles(); } })If you want, I can open a follow-up PR to add this change.
packages/app-store/_utils/getAnalytics.ts (2)
34-39: Avoid unsafe type assertion; guard the key and improve warning context.Casting to AnalyticsLoadersKey can mask typos and lead to undefined loader calls. Prefer a runtime key guard and include the resolved name in the warning.
Apply this diff:
- const analyticsAppImportFn = analyticsLoaders[analyticsName as AnalyticsLoadersKey]; + const analyticsAppImportFn = + analyticsName in analyticsLoaders + ? analyticsLoaders[analyticsName as AnalyticsLoadersKey] + : undefined; @@ - if (!analyticsAppImportFn) { - log.warn(`analytics app not implemented`); + if (!analyticsAppImportFn) { + log.warn(`analytics app not implemented: ${analyticsName}`); return null; }
41-46: Wrap dynamic import in try/catch to avoid bubbling transient import errors.A failed import (network/fs/transpile) will currently throw. Safely catch and log the analyticsName so the caller gets null instead of an unhandled rejection.
Suggested pattern:
- const analyticsApp = await analyticsAppImportFn(); + let analyticsApp: unknown; + try { + analyticsApp = await analyticsAppImportFn(); + } catch (e) { + log.warn(`failed to import analytics app`, { analyticsName, error: (e as Error)?.message }); + return null; + }packages/app-store/_utils/getCalendar.ts (1)
48-48: Drop redundant optional chain and add try/catch around import.You already guard when the loader is missing; optional chaining here hides type issues and swallows the signal. Prefer a try/catch to capture import errors and keep returning null.
- const calendarApp = await calendarAppImportFn?.(); + let calendarApp: unknown; + try { + calendarApp = await calendarAppImportFn(); + } catch (e) { + log.warn(`failed to import calendar app for type ${calendarType}`, { + error: (e as Error)?.message, + }); + return null; + }packages/lib/payment/handlePaymentRefund.ts (2)
23-25: Improve warning context.Interpolating the whole paymentApp object yields “[object Object]”. Log the dirName instead for actionable telemetry.
- console.warn(`payment App service of type ${paymentApp} is not implemented`); + console.warn(`payment App service not implemented for dirName: ${paymentAppCredentials?.app?.dirName ?? "<unknown>"}`);
19-31: Optional: standardize refund error handling
We’ve confirmed that across allPaymentServiceimplementations,refund()returns aPromise<Payment>, so any exception will bubble up. If you’d prefer to handle failed refunds gracefully:
- Wrap the
refundcall in atry/catchblock.- Log or transform the error.
- Return a sentinel (e.g.
false) instead of throwing.- Update the function signature to reflect
Promise<Payment | false>(orPromise<boolean>if you only care about success/failure) and adjust callers accordingly.Locations to update:
packages/lib/payment/handlePaymentRefund.ts:28–32Example refactor:
- const refund = await paymentInstance.refund(paymentId); - return refund; + try { + const refund = await paymentInstance.refund(paymentId); + return refund; + } catch (error) { + console.error('Refund failed for paymentId', paymentId, error); + return false; + }packages/app-store/_utils/calendars/calendarLoaders.ts (1)
3-16: Solid centralization; consider stabilizing the map shape.The loader registry is straightforward and will cache modules per createCachedImport. Recommend marking the object as const to preserve literal keys at the type level and prevent accidental mutation.
-export const calendarLoaders = { +export const calendarLoaders = { applecalendar: createCachedImport(() => import("../../applecalendar")), caldavcalendar: createCachedImport(() => import("../../caldavcalendar")), googlecalendar: createCachedImport(() => import("../../googlecalendar")), "ics-feedcalendar": createCachedImport(() => import("../../ics-feedcalendar")), larkcalendar: createCachedImport(() => import("../../larkcalendar")), office365calendar: createCachedImport(() => import("../../office365calendar")), exchange2013calendar: createCachedImport(() => import("../../exchange2013calendar")), exchange2016calendar: createCachedImport(() => import("../../exchange2016calendar")), exchangecalendar: createCachedImport(() => import("../../exchangecalendar")), zohocalendar: createCachedImport(() => import("../../zohocalendar")), -}; +} as const;Note: With as const, CalendarLoaderKey remains keyof typeof calendarLoaders as desired.
packages/app-store/_utils/analytics/analyticsLoaders.ts (1)
3-8: Looks good; add as const to preserve literal key types.Minor nit to further tighten typing and prevent accidental writes.
-export const analyticsLoaders = { +export const analyticsLoaders = { dub: createCachedImport(() => import("../../dub")), plausible: createCachedImport(() => import("../../plausible")), -}; +} as const;packages/trpc/server/routers/viewer/payments.tsx (4)
112-115: Avoid ts-expect-error; add a safe, typed loader lookup with guardRelying on @ts-expect-error hides real issues and can mask unsupported apps at runtime. Guard the key and drop the suppression.
Apply this diff:
- // @ts-expect-error FIXME - const paymentApp = (await paymentLoaders[ - paymentCredential?.app?.dirName as PaymentLoaderKey - ]?.()) as PaymentApp | null; + const dirName = paymentCredential.app.dirName as string; + if (!(dirName in paymentLoaders)) { + throw new TRPCError({ code: "BAD_REQUEST", message: "Unsupported payment app" }); + } + const paymentApp = (await paymentLoaders[ + dirName as keyof typeof paymentLoaders + ]()) as PaymentApp | null;
60-77: Parallelize attendee translations and fix variable misnamingTranslations are currently awaited sequentially inside the loop and the array is misnamed “attendeesListPromises” while holding objects. Use Promise.all over mapped promises and pass attendeesList[0] to the email.
Apply this diff:
- 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", + }, + })) + );And:
- await sendNoShowFeeChargedEmail( - attendeesListPromises[0], - evt, - booking?.eventType?.metadata as EventTypeMetadata - ); + await sendNoShowFeeChargedEmail( + attendeesList[0], + evt, + booking?.eventType?.metadata as EventTypeMetadata + );Also applies to: 148-153
23-26: Stricter input validation for bookingIdMinor improvement: ensure bookingId is a positive integer.
Apply this diff:
- z.object({ - bookingId: z.number(), - }) + z.object({ + bookingId: z.number().int().positive(), + })
30-47: Prefer Prisma select over include to avoid overfetchingPer repo guidelines, use select to fetch only needed fields. This reduces payload and CPU for serialization.
Apply this diff:
- 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, + appId: true, + amount: true, + currency: true, + paymentOption: true, + success: true, + }, + }, + user: { + select: { + email: true, + locale: true, + name: true, + timeZone: true, + }, + }, + attendees: { + select: { + name: true, + email: true, + timeZone: true, + locale: true, + }, + }, + eventType: { + select: { + slug: true, + customReplyToEmail: true, + metadata: true, + }, + }, + }, + });packages/app-store/_utils/payments/paymentLoaders.ts (1)
11-14: Remove ts-expect-error; define mock loader without mutating the inferred typeAvoid mutating the typed object with an undeclared key. Provide a stub when the mock is disabled so the shape stays consistent and no suppression is needed.
Apply this diff:
-export const paymentLoaders = { +export const paymentLoaders = { alby: createCachedImport(() => import("../../alby")), paypal: createCachedImport(() => import("../../paypal")), stripepayment: createCachedImport(() => import("../../stripepayment")), hitpay: createCachedImport(() => import("../../hitpay")), btcpayserver: createCachedImport(() => import("../../btcpayserver")), -}; - -if (process.env.MOCK_PAYMENT_APP_ENABLED !== undefined) { - // @ts-expect-error FIXME - paymentLoaders["mock-payment-app"] = createCachedImport(() => import("../../mock-payment-app/index")); -} + // Keep the key present to satisfy types; gate actual import behind flag + "mock-payment-app": + process.env.MOCK_PAYMENT_APP_ENABLED !== undefined + ? createCachedImport(() => import("../../mock-payment-app/index")) + : async () => { + throw new Error("Mock payment app not enabled"); + }, +} as const; -export type PaymentLoaderKey = keyof typeof paymentLoaders | "mock-payment-app"; +export type PaymentLoaderKey = keyof typeof paymentLoaders;This preserves typings, removes the suppression, and prevents accidental undefined lookups at runtime.
Also applies to: 16-16
packages/app-store/_utils/calendars/getCalendarApps.ts (1)
51-57: Optional: Pre-index credentials to reduce repeated filtersYou filter the full credentials array for each app (O(apps * credentials)). If the list grows, precompute a Map<appId, Credential[]>.
I can provide a concrete refactor if desired; it’s a small change that builds a lookup map once and then does O(1) accesses inside the reduce.
📜 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 (19)
packages/app-store-cli/src/build.ts(4 hunks)packages/app-store/_utils/analytics/analyticsLoaders.ts(1 hunks)packages/app-store/_utils/calendars/calendarLoaders.ts(1 hunks)packages/app-store/_utils/calendars/getCalendarApps.ts(1 hunks)packages/app-store/_utils/createCachedImport.ts(1 hunks)packages/app-store/_utils/getAnalytics.ts(2 hunks)packages/app-store/_utils/getCalendar.ts(2 hunks)packages/app-store/_utils/payments/paymentLoaders.ts(1 hunks)packages/app-store/_utils/videos/videoLoaders.ts(1 hunks)packages/app-store/calendarApps.metadata.generated.ts(1 hunks)packages/lib/CalendarManager.ts(2 hunks)packages/lib/getConnectedApps.ts(2 hunks)packages/lib/payment/deletePayment.ts(2 hunks)packages/lib/payment/handlePayment.ts(2 hunks)packages/lib/payment/handlePaymentRefund.ts(2 hunks)packages/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)tests/libs/__mocks__/app-store.ts(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/lib/CalendarManager.tstests/libs/__mocks__/app-store.tspackages/lib/videoClient.tspackages/lib/getConnectedApps.tspackages/app-store/_utils/videos/videoLoaders.tspackages/app-store/_utils/getAnalytics.tspackages/app-store/calendarApps.metadata.generated.tspackages/app-store/_utils/createCachedImport.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/lib/payment/handlePayment.tspackages/app-store/_utils/payments/paymentLoaders.tspackages/trpc/server/routers/viewer/payments/chargeCard.handler.tspackages/app-store/_utils/analytics/analyticsLoaders.tspackages/lib/payment/deletePayment.tspackages/app-store/_utils/calendars/calendarLoaders.tspackages/lib/payment/handlePaymentRefund.tspackages/app-store/_utils/getCalendar.tspackages/app-store-cli/src/build.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/CalendarManager.tstests/libs/__mocks__/app-store.tspackages/lib/videoClient.tspackages/lib/getConnectedApps.tspackages/app-store/_utils/videos/videoLoaders.tspackages/app-store/_utils/getAnalytics.tspackages/app-store/calendarApps.metadata.generated.tspackages/app-store/_utils/createCachedImport.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/lib/payment/handlePayment.tspackages/trpc/server/routers/viewer/payments.tsxpackages/app-store/_utils/payments/paymentLoaders.tspackages/trpc/server/routers/viewer/payments/chargeCard.handler.tspackages/app-store/_utils/analytics/analyticsLoaders.tspackages/lib/payment/deletePayment.tspackages/app-store/_utils/calendars/calendarLoaders.tspackages/lib/payment/handlePaymentRefund.tspackages/app-store/_utils/getCalendar.tspackages/app-store-cli/src/build.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/lib/CalendarManager.tspackages/app-store/_utils/getCalendar.ts
🧬 Code Graph Analysis (16)
packages/lib/CalendarManager.ts (1)
packages/app-store/_utils/calendars/getCalendarApps.ts (1)
getCalendarApps(51-111)
packages/lib/videoClient.ts (1)
packages/app-store/_utils/videos/videoLoaders.ts (2)
videoLoaders(3-15)VideoLoaderKey(17-17)
packages/lib/getConnectedApps.ts (1)
packages/app-store/_utils/payments/paymentLoaders.ts (2)
paymentLoaders(3-9)PaymentLoaderKey(16-16)
packages/app-store/_utils/videos/videoLoaders.ts (1)
packages/app-store/_utils/createCachedImport.ts (1)
createCachedImport(1-10)
packages/app-store/_utils/getAnalytics.ts (1)
packages/app-store/_utils/analytics/analyticsLoaders.ts (2)
analyticsLoaders(3-6)AnalyticsLoadersKey(8-8)
packages/app-store/calendarApps.metadata.generated.ts (1)
packages/app-store/_utils/calendars/getCalendarApps.ts (1)
calendarAppsMetadata(30-30)
packages/app-store/_utils/calendars/getCalendarApps.ts (3)
packages/app-store/locations.ts (1)
EventLocationType(58-58)packages/app-store/calendarApps.metadata.generated.ts (1)
calendarAppsMetadata(20-35)packages/lib/piiFreeData.ts (1)
getPiiFreeCredential(61-72)
packages/lib/payment/handlePayment.ts (1)
packages/app-store/_utils/payments/paymentLoaders.ts (1)
paymentLoaders(3-9)
packages/trpc/server/routers/viewer/payments.tsx (1)
packages/app-store/_utils/payments/paymentLoaders.ts (2)
paymentLoaders(3-9)PaymentLoaderKey(16-16)
packages/app-store/_utils/payments/paymentLoaders.ts (1)
packages/app-store/_utils/createCachedImport.ts (1)
createCachedImport(1-10)
packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
packages/app-store/_utils/payments/paymentLoaders.ts (2)
paymentLoaders(3-9)PaymentLoaderKey(16-16)
packages/app-store/_utils/analytics/analyticsLoaders.ts (1)
packages/app-store/_utils/createCachedImport.ts (1)
createCachedImport(1-10)
packages/lib/payment/deletePayment.ts (1)
packages/app-store/_utils/payments/paymentLoaders.ts (2)
paymentLoaders(3-9)PaymentLoaderKey(16-16)
packages/app-store/_utils/calendars/calendarLoaders.ts (1)
packages/app-store/_utils/createCachedImport.ts (1)
createCachedImport(1-10)
packages/lib/payment/handlePaymentRefund.ts (1)
packages/app-store/_utils/payments/paymentLoaders.ts (2)
paymentLoaders(3-9)PaymentLoaderKey(16-16)
packages/app-store/_utils/getCalendar.ts (1)
packages/app-store/_utils/calendars/calendarLoaders.ts (2)
CalendarLoaderKey(16-16)calendarLoaders(3-14)
⏰ 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 (18)
packages/app-store-cli/src/build.ts (3)
41-41: Calendar metadata output collection added — looks consistent with existing patterns.
382-382: New generated file added to outputs.Good inclusion; aligns with the new calendar app flow and consumers in calendar utilities.
431-433: Predicate aligns with category filtering elsewhere.
isCalendarAppmirrorsisCrmAppand is straightforward.packages/lib/payment/handlePayment.ts (1)
3-3: Switch to paymentLoaders aligns with the lazy-loader architecture.Importing from
@calcom/app-store/_utils/payments/paymentLoadersmatches the new pattern; reduces eager imports and improves perf.packages/lib/videoClient.ts (2)
4-5: LGTM! Clean import organization for the new video loader system.The imports properly use named imports for the videoLoaders and type-only imports for VideoLoaderKey, following TypeScript best practices.
31-31: VideoLoaderKey coverage verifiedAll credential types ending in
_videoor_conferencingcorrectly map—viasplit("_").join("")—to keys invideoLoaders. No integrations are missing loader entries.• daily_video → dailyvideo
• google_video → googlevideo
• huddle01_video → huddle01video
• shimmer_video → shimmervideo
• jitsi_video → jitsivideo
• webex_video → webexvideo
• sylaps_video → sylapsvideo
• office365_video → office365video
• tandem_video → tandemvideo
• zoom_video → zoomvideo
• jelly_conferencing → jellyconferencingpackages/lib/CalendarManager.ts (2)
4-4: LGTM! Clean import for the new calendar app utilities.The import properly uses the new getCalendarApps utility from the calendar-specific loader system.
33-33: Verify getCalendarApps matches legacy getApps behaviorI wasn’t able to locate the original
getAppsimplementation in the repo. Please manually confirm that switching to
getCalendarApps(credentials, true)preserves:
- The same
filterOnCredentialslogic (skipping non-global apps with no credentials)- Credential handling (including the deprecated
credentialfield alongsidecredentials)- The returned object shape (all fields previously returned by
getApps)Key locations to review:
packages/lib/CalendarManager.ts(import changed fromgetApps→getCalendarApps)packages/app-store/_utils/calendars/getCalendarApps.ts(filter andreducedArray.pushstructure)packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts (1)
1-2: LGTM! Proper import structure for the new payment loader system.The imports correctly bring in both the paymentLoaders map and the PaymentLoaderKey type, following the established pattern across the codebase.
packages/lib/getConnectedApps.ts (1)
4-5: LGTM! Consistent import pattern for payment loaders.The imports follow the same pattern established in other files, using named imports for paymentLoaders and type-only imports for PaymentLoaderKey.
packages/lib/payment/deletePayment.ts (1)
3-4: LGTM! Consistent import pattern maintained.The imports follow the established pattern used across all payment-related files in this PR.
packages/app-store/_utils/getAnalytics.ts (1)
5-6: Good move: switch to loader registry (cacheable, tree-shakeable).Replacing appStore lookup with analyticsLoaders aligns with the new lazy-loading pattern and should reduce initial bundle/SSR cost. No issues here.
packages/app-store/_utils/getCalendar.ts (1)
5-6: Loader registry import looks correct.Imports line up with the new calendarLoaders module and typed key. Good.
packages/lib/payment/handlePaymentRefund.ts (1)
3-4: Aligns with loader pattern.Switch to paymentLoaders is consistent with the new lazy-loading approach. Good step towards decoupling and better SSR performance.
packages/trpc/server/routers/viewer/payments.tsx (2)
3-4: Good shift to lazy loaders for payment adaptersSwitching to paymentLoaders aligns this module with the new lazy-load pattern and reduces cold-start overhead. Nice.
49-56: Guard against missing payment recordIf booking.payment is empty, accessing payment.success will throw.
Apply this diff to avoid a runtime crash:
const payment = booking.payment[0]; + if (!payment) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "No payment record found for this booking.", + }); + } + if (payment.success) { throw new TRPCError({ code: "BAD_REQUEST", message: `The no show fee for ${booking.id} has already been charged.`, }); }If this case is impossible by design, please confirm and we can skip this guard.
packages/app-store/_utils/videos/videoLoaders.ts (1)
3-15: Solid addition; consistent with the new loader patternThe loader map and cached dynamic imports look good and will minimize initial bundle cost.
packages/app-store/_utils/calendars/getCalendarApps.ts (1)
59-83: No client-side exposure of injectedkey
After tracinggetCalendarAppsusages:
- It’s only consumed by
getCalendarCredentialsinpackages/lib/CalendarManager.ts.- All returned objects go through
getConnectedCalendars, which strips out full credential objects viacleanIntegrationKeysand only exposescredentialId(never the rawkey).- Logging calls use
getPiiFreeCredential, which redacts sensitive fields.- A grep across API and TRPC routes found no code that returns or serializes
credential.key.This synthetic credential’s
keynever reaches clients.
| const calendarLoaderKey = calendarType.split("_").join("") as CalendarLoaderKey; | ||
| const calendarAppImportFn = calendarLoaders[calendarLoaderKey]; | ||
|
|
There was a problem hiding this comment.
Bug: loader key derivation breaks for “ics-feedcalendar”.
calendarLoaders defines the ICS loader key as "ics-feedcalendar" (with a hyphen), but the current derivation removes underscores only, producing "icsfeedcalendar" which won’t match, causing ICS Feed calendars to fail to load.
Apply this minimal fix to handle the special case and harden the lookup:
- const calendarLoaderKey = calendarType.split("_").join("") as CalendarLoaderKey;
- const calendarAppImportFn = calendarLoaders[calendarLoaderKey];
+ // Derive loader key from type; handle special cases like ICS Feed
+ const baseKey = calendarType.split("_").join("");
+ const normalizedKey =
+ baseKey === "icsfeedcalendar" ? ("ics-feedcalendar" as const) : (baseKey as const);
+ const calendarAppImportFn =
+ normalizedKey in calendarLoaders
+ ? calendarLoaders[normalizedKey as CalendarLoaderKey]
+ : undefined;Optionally, extract a reusable normalizer to avoid future key drift:
function toCalendarLoaderKey(t: string): CalendarLoaderKey | null {
const base = t.replace(/_/g, "");
const special: Record<string, CalendarLoaderKey> = { icsfeedcalendar: "ics-feedcalendar" };
const resolved = (special[base] ?? base) as CalendarLoaderKey;
return resolved in calendarLoaders ? resolved : null;
}🤖 Prompt for AI Agents
In packages/app-store/_utils/getCalendar.ts around lines 40-42, the loader key
derivation currently strips only underscores and yields "icsfeedcalendar" which
doesn't match the defined "ics-feedcalendar" key; implement a small normalizer
function that removes underscores, maps the special-case base "icsfeedcalendar"
to "ics-feedcalendar", then check that the resolved key exists in
calendarLoaders before using it (return null or throw if not found). Replace the
inline split/join with a call to this normalizer to produce a valid
CalendarLoaderKey and use the normalized value for calendarLoaders lookup.
| /** | ||
| This file is autogenerated using the command `yarn app-store:build --watch`. | ||
| Don't modify this file manually. | ||
| **/ | ||
| import amie_config_json from "./amie/config.json"; | ||
| import { metadata as applecalendar__metadata_ts } from "./applecalendar/_metadata"; | ||
| import { metadata as caldavcalendar__metadata_ts } from "./caldavcalendar/_metadata"; | ||
| import cron_config_json from "./cron/config.json"; | ||
| import { metadata as exchange2013calendar__metadata_ts } from "./exchange2013calendar/_metadata"; | ||
| import { metadata as exchange2016calendar__metadata_ts } from "./exchange2016calendar/_metadata"; | ||
| import exchangecalendar_config_json from "./exchangecalendar/config.json"; | ||
| import { metadata as feishucalendar__metadata_ts } from "./feishucalendar/_metadata"; | ||
| import { metadata as googlecalendar__metadata_ts } from "./googlecalendar/_metadata"; | ||
| import ics_feedcalendar_config_json from "./ics-feedcalendar/config.json"; | ||
| import { metadata as larkcalendar__metadata_ts } from "./larkcalendar/_metadata"; | ||
| import { metadata as office365calendar__metadata_ts } from "./office365calendar/_metadata"; | ||
| import vimcal_config_json from "./vimcal/config.json"; | ||
| import zohocalendar_config_json from "./zohocalendar/config.json"; | ||
|
|
||
| export const calendarAppsMetadata = { | ||
| amie: amie_config_json, | ||
| applecalendar: applecalendar__metadata_ts, | ||
| caldavcalendar: caldavcalendar__metadata_ts, | ||
| cron: cron_config_json, | ||
| exchange2013calendar: exchange2013calendar__metadata_ts, | ||
| exchange2016calendar: exchange2016calendar__metadata_ts, | ||
| exchangecalendar: exchangecalendar_config_json, | ||
| feishucalendar: feishucalendar__metadata_ts, | ||
| googlecalendar: googlecalendar__metadata_ts, | ||
| "ics-feedcalendar": ics_feedcalendar_config_json, | ||
| larkcalendar: larkcalendar__metadata_ts, | ||
| office365calendar: office365calendar__metadata_ts, | ||
| vimcal: vimcal_config_json, | ||
| zohocalendar: zohocalendar_config_json, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Generated mapping looks correct; verify there’s no competing stub export.
The entries and imports are coherent. Ensure that getCalendarApps.ts (or any other module) no longer exports a stub calendarAppsMetadata = {} as CalendarAppsMetaData; which would diverge from this source of truth.
Run this to confirm there are no non-generated declarations:
🏁 Script executed:
#!/bin/bash
# Find other declarations of calendarAppsMetadata outside the generated file
rg -nP -C2 'export\s+const\s+calendarAppsMetadata\b' \
-g '!**/calendarApps.metadata.generated.ts' --type=tsLength of output: 636
Remove stub export of calendarAppsMetadata in getCalendarApps.ts and import the generated metadata instead
The file packages/app-store/_utils/calendars/getCalendarApps.ts still defines and exports a stubbed calendarAppsMetadata, which conflicts with the source of truth in calendarApps.metadata.generated.ts. Replace the manual stub with an import from the generated file:
• File: packages/app-store/_utils/calendars/getCalendarApps.ts
Lines: ~30–32
Suggested diff:
- export const calendarAppsMetadata = {} as CalendarAppsMetaData;
- for (const [key, value] of Object.entries(rawCalendarAppsMetadata)) {
- calendarAppsMetadata[key as keyof typeof calendarAppsMetadata] = getNormalizedAppMetadata(value);
- }
+ import { calendarAppsMetadata } from '../../calendarApps.metadata.generated';This ensures all consumers use the single, autogenerated mapping.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/app-store/_utils/calendars/getCalendarApps.ts around lines 30 to 32,
remove the manually defined/exported stubbed calendarAppsMetadata and replace it
by importing the autogenerated calendarAppsMetadata from
packages/app-store/calendarApps.metadata.generated.ts; update the top of the
file to add the import (e.g. import { calendarAppsMetadata } from
"packages/app-store/calendarApps.metadata.generated";) and delete the local
stub/export so all consumers use the single autogenerated mapping.
| // @ts-expect-error FIXME | ||
| const paymentApp = (await paymentLoaders[ | ||
| paymentAppCredentials?.app?.dirName as PaymentLoaderKey | ||
| ]?.()) as PaymentApp; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same TypeScript error pattern - consider consolidating the fix.
This is the third occurrence of the same @ts-expect-error pattern. Consider creating a helper function to handle payment app loading consistently across the codebase.
- // @ts-expect-error FIXME
- const paymentApp = (await paymentLoaders[
- paymentAppCredentials?.app?.dirName as PaymentLoaderKey
- ]?.()) as PaymentApp;
+ const dirName = paymentAppCredentials?.app?.dirName as PaymentLoaderKey;
+ const loader = paymentLoaders[dirName];
+ if (!loader) {
+ console.warn(`Payment app loader not found for ${dirName}`);
+ return false;
+ }
+ const paymentApp = (await loader()) as PaymentApp;Consider creating a shared utility function for payment app loading:
// packages/lib/payment/loadPaymentApp.ts
export async function loadPaymentApp(dirName: string): Promise<PaymentApp | null> {
const loader = paymentLoaders[dirName as PaymentLoaderKey];
return loader ? (await loader()) as PaymentApp : null;
}🤖 Prompt for AI Agents
In packages/lib/payment/deletePayment.ts around lines 19 to 22, the code uses a
repeated // @ts-expect-error pattern when invoking a dynamic payment loader;
consolidate this by creating a shared utility (e.g.,
packages/lib/payment/loadPaymentApp.ts) that accepts the dirName string, looks
up the loader with a PaymentLoaderKey cast, returns null if not found, and
otherwise awaits and returns the typed PaymentApp; then replace this direct
loader invocation with a call to that helper, add proper null checking and error
handling where the helper is used, and remove the inline @ts-expect-error
comments.
| if (!isKeyOf(paymentLoaders, key)) { | ||
| console.warn(`key: ${key} is not a valid key in appStore`); | ||
| return null; | ||
| } | ||
| const paymentApp = await appStore[key]?.(); | ||
| const paymentApp = await paymentLoaders[key]?.(); | ||
| if (!isPaymentApp(paymentApp)) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix log message and consider tightening types and tests for the new loaders.
- The warning still mentions “appStore”. Update message to reflect
paymentLoaders. - Optional: use
PaymentLoaderKeyto narrowkeyat compile-time. - Tests that previously mocked
@calcom/app-storemay need to mock@calcom/app-store/_utils/payments/paymentLoadersnow.
Apply this diff for the log message:
- if (!isKeyOf(paymentLoaders, key)) {
- console.warn(`key: ${key} is not a valid key in appStore`);
+ if (!isKeyOf(paymentLoaders, key)) {
+ console.warn(`key: ${key} is not a valid key in paymentLoaders`);
return null;
}To mock the new loaders in Vitest (outside this file), you can add in your test setup:
vi.mock("@calcom/app-store/_utils/payments/paymentLoaders", () => ({
paymentLoaders: {
stripepayment: async () => ({ lib: { PaymentService: /* your mock */ {} as any } }),
// ...other loaders as needed
},
}));Run this to find tests relying on the old app-store mock:
🏁 Script executed:
#!/bin/bash
# Search for tests mocking "@calcom/app-store"
rg -nP -C2 'vi\.mock\(\s*"@calcom/app-store"\s*,' --type=ts --type=tsxLength of output: 100
🏁 Script executed:
#!/bin/bash
# Search for tests mocking "@calcom/app-store" in .ts and .tsx files
rg -nP -C2 'vi\.mock\(\s*"@calcom/app-store"' --glob '*.ts' --glob '*.tsx'Length of output: 379
Fix warning message and update tests for paymentLoaders
- Update the log in
packages/lib/payment/handlePayment.tsto referencepaymentLoadersinstead ofappStore. - Add a mock for the new module path in your Vitest setup so tests continue to pass.
- Optional: tighten the
keyparameter to thePaymentLoaderKeytype for compile-time safety.
Diff for the runtime code:
--- packages/lib/payment/handlePayment.ts
@@ lines 55-60
- if (!isKeyOf(paymentLoaders, key)) {
- console.warn(`key: ${key} is not a valid key in appStore`);
+ if (!isKeyOf(paymentLoaders, key)) {
+ console.warn(`key: ${key} is not a valid key in paymentLoaders`);
return null;
}Example mock for Vitest (e.g. in tests/libs/__mocks__/paymentLoaders.ts or your global setup file):
vi.mock("@calcom/app-store/_utils/payments/paymentLoaders", () => ({
paymentLoaders: {
stripepayment: async () => ({
lib: { PaymentService: {} as any },
}),
// …other loaders as needed
},
}));You’ll also want to remove or update the old mock in tests/libs/__mocks__/app-store.ts so it no longer intercepts the new loader import.
🤖 Prompt for AI Agents
In packages/lib/payment/handlePayment.ts around lines 55 to 60, the runtime
warning references the wrong module name ("appStore"); change the console warn
to reference paymentLoaders instead, and tighten the key parameter to
PaymentLoaderKey if possible. Update tests by adding a Vitest mock for the new
module path "@calcom/app-store/_utils/payments/paymentLoaders" which exports
paymentLoaders with the required loader entries, and remove or update the old
mock in tests/libs/__mocks__/app-store.ts so it no longer intercepts the loader
import; ensure the mock returns the same shape used by handlePayment to keep
tests passing.
| // @ts-expect-error FIXME | ||
| const paymentApp = (await paymentLoaders[ | ||
| paymentAppCredentials?.app?.dirName as PaymentLoaderKey | ||
| ]?.()) as PaymentApp; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove ts-expect-error; add typed guard and clearer error when loader is missing.
The current assertion and // @ts-expect-error suppress real issues and can mask misconfigured app dirNames. Add a runtime key guard and avoid calling an undefined loader.
Apply this refactor:
- // @ts-expect-error FIXME
- const paymentApp = (await paymentLoaders[
- paymentAppCredentials?.app?.dirName as PaymentLoaderKey
- ]?.()) as PaymentApp;
+ const dirName = paymentAppCredentials?.app?.dirName;
+ if (!dirName || !(dirName in paymentLoaders)) {
+ console.warn(`payment loader not found for dirName: ${dirName ?? "<missing>"}`);
+ return false;
+ }
+ const loader = paymentLoaders[dirName as PaymentLoaderKey];
+ const paymentApp = (await loader?.()) as PaymentApp | undefined;📝 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.
| // @ts-expect-error FIXME | |
| const paymentApp = (await paymentLoaders[ | |
| paymentAppCredentials?.app?.dirName as PaymentLoaderKey | |
| ]?.()) as PaymentApp; | |
| const dirName = paymentAppCredentials?.app?.dirName; | |
| if (!dirName || !(dirName in paymentLoaders)) { | |
| console.warn(`payment loader not found for dirName: ${dirName ?? "<missing>"}`); | |
| return false; | |
| } | |
| const loader = paymentLoaders[dirName as PaymentLoaderKey]; | |
| const paymentApp = (await loader?.()) as PaymentApp | undefined; |
🤖 Prompt for AI Agents
In packages/lib/payment/handlePaymentRefund.ts around lines 19 to 22, remove the
// @ts-expect-error and the unchecked assertion; instead validate that
paymentAppCredentials?.app?.dirName is a valid PaymentLoaderKey present in
paymentLoaders before calling it, e.g. narrow the key with a typed guard (typeof
check or an Array.includes of Object.keys(paymentLoaders) cast to
PaymentLoaderKey), throw a clear runtime Error if the loader is missing
(including the problematic dirName and context), then call the loader and type
its result as PaymentApp only after the guard ensures it exists.
💡 Verification agent
🧩 Analysis chain
Verify loader coverage for payment app dirNames.
Ensure every payment app dirName in app.json is present in paymentLoaders to avoid runtime misses.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List loader keys
PAYMENT_LOADERS_FILE="$(fd -a -t f 'paymentLoaders.ts' packages | head -n1)"
echo "Loader file: $PAYMENT_LOADERS_FILE"
echo "Loader keys:"
awk '
/export const paymentLoaders/ { inObj=1; next }
inObj && /};/ { inObj=0 }
inObj && match($0, /^\s*([a-z0-9-]+):\s*createCachedImport/, m) { print m[1] }
' "$PAYMENT_LOADERS_FILE" | sort | tee /tmp/loader_keys.txt
# Extract payment app dirNames from app.json metadata
echo "Payment app dirNames from app.json:"
fd -a -t f app.json packages/app-store | xargs -I {} sh -c '
if jq -e --arg cat "payment" '"'"'.categories | index($cat)'"'"' {} >/dev/null 2>&1; then
jq -r ".dirName" {}
fi
' | sort | tee /tmp/dirnames.txt
echo
echo "DirNames missing in loaders:"
comm -23 /tmp/dirnames.txt /tmp/loader_keys.txt || trueLength of output: 751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the paymentLoaders file
PAYMENT_LOADERS_FILE=$(fd -t f 'paymentLoaders.ts' packages | head -n1)
echo "Loader file: $PAYMENT_LOADERS_FILE"
# Extract loader keys from the paymentLoaders object
awk '/export const paymentLoaders/,/};/' "$PAYMENT_LOADERS_FILE" \
| grep -oP "^\s*['\"]?\K[a-z0-9-]+(?=['\"]?:\s*createCachedImport)" \
| sort > /tmp/loader_keys.txt
echo "Loader keys:"
cat /tmp/loader_keys.txt
# Extract all payment app dirNames from app.json files
echo "Payment app dirNames from app.json:"
find packages/app-store -type f -name app.json -print0 \
| xargs -0 jq -r 'select(.categories | index("payment")) | .dirName' \
| sort > /tmp/dirnames.txt
cat /tmp/dirnames.txt
# Show any dirNames missing in the loaders
echo
echo "DirNames missing in loaders:"
comm -23 /tmp/dirnames.txt /tmp/loader_keys.txt || echo "None missing"Length of output: 932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All JSON files in packages/app-store:"
fd -t f -e json packages/app-store || true
echo
echo "JSON files containing \"categories\":"
rg -n '"categories"' --glob 'packages/app-store/**/*.json' || true
echo
echo "JSON files containing \"dirName\":"
rg -n '"dirName"' --glob 'packages/app-store/**/*.json' || trueLength of output: 8420
Add missing loader for mock-payment-app
The paymentLoaders.ts registry currently includes:
- alby
- btcpayserver
- hitpay
- paypal
- stripepayment
…but the only payment‐category apps in packages/app-store/**/config.json are:
- alby
- btcpayserver
- hitpay
- paypal
- mock-payment-app
mock-payment-app is not registered, so paymentLoaders['mock-payment-app'] will be undefined at runtime.
• File to update:
packages/app-store/_utils/payments/paymentLoaders.ts
• Action: add an entry for mock-payment-app, e.g.:
export const paymentLoaders = {
alby: createCachedImport(() => import(/* … */)),
btcpayserver: createCachedImport(() => import(/* … */)),
hitpay: createCachedImport(() => import(/* … */)),
paypal: createCachedImport(() => import(/* … */)),
stripepayment: createCachedImport(() => import(/* … */)),
+ 'mock-payment-app': createCachedImport(() => import(/* path to mock-payment-app payment adapter */)),
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/app-store/_utils/payments/paymentLoaders.ts (and note related runtime
use in packages/lib/payment/handlePaymentRefund.ts lines ~19-22): the registry
is missing an entry for "mock-payment-app" which causes
paymentLoaders['mock-payment-app'] to be undefined at runtime; add a loader
entry keyed exactly "mock-payment-app" following the existing pattern (import or
require the mock-payment-app loader module and add it to the exported
paymentLoaders object), ensuring the key matches the app config dirname and the
loader returns the same PaymentApp shape as the other entries.
| // @ts-expect-error FIXME | ||
| const paymentApp = (await paymentLoaders[ | ||
| paymentCredential?.app?.dirName as PaymentLoaderKey | ||
| ]?.()) as PaymentApp; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the TypeScript error and improve type safety.
The @ts-expect-error comment indicates a type mismatch that should be resolved rather than suppressed. The issue likely stems from the optional chaining and type casting.
- // @ts-expect-error FIXME
- const paymentApp = (await paymentLoaders[
- paymentCredential?.app?.dirName as PaymentLoaderKey
- ]?.()) as PaymentApp;
+ const dirName = paymentCredential?.app?.dirName as PaymentLoaderKey;
+ const loader = paymentLoaders[dirName];
+ if (!loader) {
+ throw new TRPCError({ code: "BAD_REQUEST", message: "Payment app loader not found" });
+ }
+ const paymentApp = (await loader()) as PaymentApp;📝 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.
| // @ts-expect-error FIXME | |
| const paymentApp = (await paymentLoaders[ | |
| paymentCredential?.app?.dirName as PaymentLoaderKey | |
| ]?.()) as PaymentApp; | |
| const dirName = paymentCredential?.app?.dirName as PaymentLoaderKey; | |
| const loader = paymentLoaders[dirName]; | |
| if (!loader) { | |
| throw new TRPCError({ code: "BAD_REQUEST", message: "Payment app loader not found" }); | |
| } | |
| const paymentApp = (await loader()) as PaymentApp; |
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts around
lines 129-132, remove the @ts-expect-error and fix the type mismatch by
narrowing and guarding the loader key before calling it: extract
paymentCredential?.app?.dirName into a local const, add a runtime/type-guard
that asserts this value is a valid PaymentLoaderKey (or throw/return early if
missing), look up the loader from paymentLoaders with that guarded key, ensure
the loader exists, then await loader() and cast the result to PaymentApp;
alternatively, update the paymentLoaders type to include an index signature
mapping PaymentLoaderKey to a loader function so the lookup is statically
safe—either approach removes the need for @ts-expect-error and makes the call
type-safe.
kart1ka
left a comment
kart1ka
left a comment
There was a problem hiding this comment.
A test seems to be failing. Can you please fix that?
Thanks, will do! might need a little time |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/app-store/_utils/calendars/getCalendarApps.ts (1)
83-99: Redact credential.key from returned data and fix nullable credential typeThis function returns credentials including sensitive
key. If any consumer exposes this via tRPC/API (e.g., /apps/installed/calendar), secrets could leak. Sanitize before returning and makecredentialnullable when no creds exist.- const credential: (typeof appCredentials)[number] | null = appCredentials[0] || null; - - reducedArray.push({ + // Remove sensitive fields like `key` before returning + const sanitizedCredentials = appCredentials.map(({ key, ...rest }) => rest as CredentialDataWithTeamName); + const credential: CredentialDataWithTeamName | null = sanitizedCredentials[0] || null; + + reducedArray.push({ ...appMeta, /** * @deprecated use `credentials` */ credential, - credentials: appCredentials, + credentials: sanitizedCredentials, /** Option to display in `location` field while editing event types */ locationOption, }); - return reducedArray; - }, [] as (App & { credential: CredentialDataWithTeamName; credentials: CredentialDataWithTeamName[]; locationOption: LocationOption | null })[]); + return reducedArray; + }, [] as (App & { + credential: CredentialDataWithTeamName | null; + credentials: CredentialDataWithTeamName[]; + locationOption: LocationOption | null; + })[]);
🧹 Nitpick comments (3)
packages/app-store/_utils/calendars/calendarAppsMetaData.ts (1)
11-14: Minor: build via Object.fromEntries to avoid intermediate mutation and tighten typingFunctionally equivalent but a bit cleaner and immutable.
-export const calendarAppsMetaData = {} as CalendarAppsMetaData; -for (const [key, value] of Object.entries(rawCalendarAppsMetadata)) { - calendarAppsMetaData[key as keyof typeof calendarAppsMetaData] = getNormalizedAppMetadata(value); -} +export const calendarAppsMetaData = Object.fromEntries( + Object.entries(rawCalendarAppsMetadata).map(([key, value]) => [key, getNormalizedAppMetadata(value)]) +) as CalendarAppsMetaData;packages/app-store/_utils/calendars/getCalendarApps.ts (1)
40-46: Optional: pre-index credentials by appId to reduce filter scansCurrent approach is O(A*C). For large orgs, pre-bucket by appId to avoid repeated scans.
If interested, I can provide a small refactor that builds a Map<string, CredentialDataWithTeamName[]> once and then looks up per app.
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
1958-1965: ts-expect-error annotations are acceptable; ensure upstream mock provides adapter shapeThe adapter surface is intentionally patched for tests. With the fixed
videoAppsMockexport shape (in tests/libs/mocks/videoApps.ts), this will work as intended.If you want to avoid
@ts-expect-error, we can declare a minimal interface for the mockedVideoApiAdapterin test types.
📜 Review details
Configuration used: .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 (5)
apps/web/test/utils/bookingScenario/bookingScenario.ts(7 hunks)packages/app-store/_utils/calendars/calendarAppsMetaData.ts(1 hunks)packages/app-store/_utils/calendars/getCalendarApps.ts(1 hunks)tests/libs/__mocks__/calendarApps.ts(1 hunks)tests/libs/__mocks__/videoApps.ts(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:
tests/libs/__mocks__/videoApps.tstests/libs/__mocks__/calendarApps.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/app-store/_utils/calendars/calendarAppsMetaData.tsapps/web/test/utils/bookingScenario/bookingScenario.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:
tests/libs/__mocks__/videoApps.tstests/libs/__mocks__/calendarApps.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/app-store/_utils/calendars/calendarAppsMetaData.tsapps/web/test/utils/bookingScenario/bookingScenario.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:
tests/libs/__mocks__/videoApps.tstests/libs/__mocks__/calendarApps.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/app-store/_utils/calendars/calendarAppsMetaData.tsapps/web/test/utils/bookingScenario/bookingScenario.ts
🧠 Learnings (1)
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : Ensure the `credential.key` field is never returned from tRPC endpoints or APIs
Applied to files:
packages/app-store/_utils/calendars/getCalendarApps.ts
🧬 Code graph analysis (1)
tests/libs/__mocks__/videoApps.ts (1)
packages/app-store/_utils/videos/videoLoaders.ts (1)
videoLoaders(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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
tests/libs/__mocks__/videoApps.ts (1)
8-10: OK to reset in beforeEach; keep as-isResetting the deep mock per test is good practice and avoids cross-test pollution.
tests/libs/__mocks__/calendarApps.ts (1)
8-10: Good: reset mock per testThis ensures isolated scenarios.
apps/web/test/utils/bookingScenario/bookingScenario.ts (4)
2-6: Good migration to calendarAppsMetaData and new loader mocksImports now align with calendar-specific metadata and loader-based mocks for calendar/video. This keeps tests consistent with the new app-store loader architecture.
Also applies to: 16-16
1721-1774: Type-safety improvement on mockCalendar’s metadataLookupKey is spot onUsing
keyof typeof calendarAppsMetaDataprevents drift with real metadata keys. Logging and resolution viacalendarAppsMetaDataandcalendarAppsMockare correct.
1156-1169: Note: getMockedCredential still sources from appStoreMetadataThis helper continues to use
appStoreMetadata. That’s fine since it’s used across non-calendar mocks, but be aware it now diverges from the calendar-specific metadata source used elsewhere in this file.Consider whether you want a calendar-specific helper mirroring
calendarAppsMetaDatafor consistency.
957-988: Scenario orchestrator looks consistent after metadata/mocks shiftNo issues noted with DB seeding and workflow wiring after the imports switch.
test: mock default export instead of named export.
…for type safety
dosubot
bot
added
the
app-store
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
1845-1847: Bug: returning id from normalizedCalendarData.update.uid (mixing uid/id)In
updateEvent, the returnedidis sourced fromnormalizedCalendarData.update?.uid. That’s inconsistent with the create path (uses.create.id) and with the declaredupdateshape that already has anidfield. This can break assertions that compareidacross create/update flows.Apply this fix to prefer
update.id, falling back toupdate.uidfor backward compatibility:- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - id: normalizedCalendarData.update?.uid || "FALLBACK_MOCK_ID", + id: + normalizedCalendarData.update?.id ?? + normalizedCalendarData.update?.uid ?? + "FALLBACK_MOCK_ID",
🧹 Nitpick comments (17)
packages/app-store/_utils/videos/videoAppsMetaData.ts (3)
11-14: Avoid mutation and type assertions; build the map in a single typed passCurrent approach mutates a casted empty object and relies on type assertions. Build the map functionally to improve type safety and readability.
Apply this diff:
-export const videoAppsMetaData = {} as VideoAppsMetaData; -for (const [key, value] of Object.entries(rawVideoAppsMetadata)) { - videoAppsMetaData[key as keyof typeof videoAppsMetaData] = getNormalizedAppMetadata(value); -} +export const videoAppsMetaData: VideoAppsMetaData = Object.entries(rawVideoAppsMetadata).reduce( + (acc, [key, value]) => { + acc[key as keyof RawVideoAppsMetaData] = getNormalizedAppMetadata(value); + return acc; + }, + {} as VideoAppsMetaData +);
6-9: Leverage “satisfies” to lock the shape without over-assertingIf getNormalizedAppMetadata guarantees dirName is a string, you can encode this with “satisfies” to validate the final object without hiding errors behind “as”.
Example (no behavior change, improves type checking):
type VideoAppsMetaData = { [key in keyof RawVideoAppsMetaData]: Omit<AppMeta, "dirName"> & { dirName: string }; };Then ensure construction uses a value that satisfies VideoAppsMetaData (see previous comment). This helps catch discrepancies if the generator changes in the future.
1-14: Name consistency: MetaData vs. MetadataElsewhere the generated symbol/file is videoAppsMetadata (lowercase “d”). Consider renaming this file/symbol to videoAppsMetadata for consistency and easier discoverability.
packages/app-store/videoApps.metadata.generated.ts (1)
1-4: Autogenerated: no manual edits — ensure the builder watches _metadata.ts tooFile looks good as generated output. Recommendation: in the builder, also trigger regeneration on changes to per-app _metadata.ts, not just config.json, to avoid stale outputs during local dev.
I’ve proposed a builder watch tweak in build.ts in a separate comment.
packages/app-store-cli/src/build.ts (1)
424-429: Watch _metadata.ts changes to keep generated outputs freshCurrently regeneration only triggers on config.json changes. Apps using _metadata.ts won’t refresh in watch mode.
Apply this diff:
- .on("change", (filePath) => { - if (filePath.endsWith("config.json")) { - console.log("Config file changed"); - debouncedGenerateFiles(); - } - }) + .on("change", (filePath) => { + if (filePath.endsWith("config.json") || filePath.endsWith("_metadata.ts")) { + console.log(`${path.basename(filePath)} changed`); + debouncedGenerateFiles(); + } + })apps/api/v2/src/modules/conferencing/services/conferencing.service.ts (2)
97-99: Use Array.find for clarity and to avoid scanning the full listfilter()[0] is less clear and needlessly processes all items.
Apply this diff:
- const foundApp = getVideoApps(credentials, true).filter((app) => app.slug === appSlug)[0]; + const foundApp = getVideoApps(credentials, true).find((app) => app.slug === appSlug);
101-105: Add a defensive check that credential existsAfter aligning types in getVideoApps (credential | null), we should assert presence here to keep type safety and avoid accidental undefined access.
Apply this diff:
- if (!foundApp || !appLocation) { + if (!foundApp || !appLocation || !foundApp.credential) { throw new BadRequestException(`${appSlug} not connected.`); }packages/lib/server/getDefaultLocations.ts (2)
23-27: Prefer.findover.filter(...)[0]for clarity and micro-optimization
.findexpresses intent (single match) and avoids creating a temporary array.- const foundApp = getVideoApps(credentials, true).filter( - (app) => app.slug === defaultConferencingData.appSlug - )[0]; // There is only one possible install here so index [0] is the one we are looking for ; + const foundApp = getVideoApps(credentials, true).find( + (app) => app.slug === defaultConferencingData.appSlug + ); // There is only one possible install here
23-28: Avoid returning a stale link when the app slug is missing or not installedIf
foundAppis undefined, we currently default the type toDailyLocationTypebut still returndefaultConferencingData.appLink. That can mismatch the location type and the link. Consider falling back to Daily without a link when the app isn’t found.- const locationType = foundApp?.locationOption?.value ?? DailyLocationType; // Default to Daily if no location type is found - return [{ type: locationType, link: defaultConferencingData.appLink }]; + if (!foundApp) { + // Slug not found or not installed: safe fallback to Daily without a stale link + return [{ type: DailyLocationType }]; + } + const locationType = foundApp.locationOption?.value ?? DailyLocationType; + return [{ type: locationType, link: defaultConferencingData.appLink }];packages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.ts (1)
28-31: Use.findinstead of.filter(...)[0]Conveys uniqueness and avoids an intermediate array.
- const foundApp = getVideoApps(credentials, true).filter((app) => app.slug === input.appSlug)[0]; + const foundApp = getVideoApps(credentials, true).find((app) => app.slug === input.appSlug);packages/lib/EventManager.ts (2)
154-166: Combine sorts into a single comparator to make ordering deterministic and avoid double traversalRelying on sort stability across engines is usually okay in modern V8, but a single comparator is clearer and future-proof.
- .sort(latestCredentialFirst) - // Keep Delegation Credentials first so because those credentials never expire and are preferred. - // Also, those credentials have consistent permission for all the members avoiding the scenario where user doesn't give all permissions - .sort(delegatedCredentialFirst); + // Delegation credentials first, then newest credential ID + .sort((a, b) => { + const byDelegation = delegatedCredentialFirst(a, b); + return byDelegation !== 0 ? byDelegation : latestCredentialFirst(a, b); + });
150-177: Minor perf nit: avoid flattening all app credentials when only CRM is neededWe still call
getApps(...).flatMap(... )to buildappCredentialsjust to filter CRM/other_calendar later. If there’s no immediate need for per-app credential flattening outside CRM, consider deriving CRM credentials directly fromuser.credentialsand mappingappNamevia a slug→name map. This reduces array allocations on hot paths.- const appCredentials = getApps(user.credentials, true).flatMap((app) => - app.credentials.map((creds) => ({ ...creds, appName: app.name })) - ); + const allApps = getApps(user.credentials, true); + const appNameBySlug = new Map(allApps.map((app) => [app.slug, app.name])); @@ - this.crmCredentials = appCredentials.filter( - (cred) => cred.type.endsWith("_crm") || cred.type.endsWith("_other_calendar") - ); + this.crmCredentials = user.credentials + .filter((cred) => cred.type.endsWith("_crm") || cred.type.endsWith("_other_calendar")) + .map((cred) => ({ ...cred, appName: appNameBySlug.get(cred.appId || "") || "" }));apps/web/test/utils/bookingScenario/bookingScenario.ts (5)
1772-1775: Indexing mocks: prefer explicit type cast helper to avoid brittle indexing
calendarAppsMock.default[appStoreLookupKey as keyof typeof calendarAppsMock.default]relies on a wide index signature and then feature-detectsmockResolvedValue. Consider a tiny helper that narrows the loader tovi.Mockand fails loudly if not a mock function. This reduces the need for scattered ts-ignore/expect-error and yields cleaner diagnostics.function getCalendarLoaderMock<K extends keyof typeof calendarAppsMock.default>( key: K ): vi.Mock { const loader = calendarAppsMock.default[key]; if (typeof loader !== "function" || !("mockResolvedValue" in loader)) { throw new Error(`calendarAppsMock.default["${String(key)}"] is not a vi mock function`); } return loader as unknown as vi.Mock; } // usage: const appLoaderMock = getCalendarLoaderMock(appStoreLookupKey as keyof typeof calendarAppsMock.default); appLoaderMock.mockResolvedValue({ /* ... */ });
1959-1966: Replace ts-expect-error with explicit types to stabilize mocksTwo
// @ts-expect-error FIXMEcomments leak here forvideoAppsMockandVideoApiAdapter. Prefer minimal type assertions or a helper similar to the calendar one to avoid suppressing type checking. This keeps the mocks change-proof.type VideoLoader = vi.Mock<Promise<{ lib: { VideoApiAdapter: (cred: unknown) => unknown } }>>; const getVideoLoaderMock = (key: keyof typeof videoAppsMock.default) => videoAppsMock.default[key] as unknown as VideoLoader; // ... getVideoLoaderMock(appStoreLookupKey).mockImplementation(() => Promise.resolve({ lib: { VideoApiAdapter: (credential: unknown) => ({ /* ... */ }), }, }));
2069-2083: Payment mock: simplify Promise and avoid repeated ts-expect-error with a typed helperGood move to route payments through
paymentAppsMock. You can tighten this by:
- using
Promise.resolve(...)to reduce noise,- centralizing the type cast to a helper and dropping
ts-expect-error.- paymentAppsMock.default[appStoreLookupKey as keyof typeof paymentAppsMock.default].mockImplementation( - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error FIXME - () => { - return new Promise((resolve) => { - resolve({ - lib: { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error FIXME - PaymentService: MockPaymentService, - }, - }); - }); - } - ); + type PaymentLoader = vi.Mock<Promise<{ lib: { PaymentService: unknown } }>>; + const getPaymentLoaderMock = (k: keyof typeof paymentAppsMock.default) => + paymentAppsMock.default[k] as unknown as PaymentLoader; + getPaymentLoaderMock(appStoreLookupKey).mockImplementation(() => + Promise.resolve({ lib: { PaymentService: MockPaymentService } }) + );
2099-2113: Unify to ts-expect-error and streamline implementationThis block still uses
// @ts-ignorewhile nearby code uses// @ts-expect-error. Preferts-expect-errorwith a short justification so future type changes won’t hide errors. Also, usePromise.resolve(...)for brevity.- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore - videoAppsMock.default[appStoreLookupKey].mockImplementation(() => { - return new Promise((resolve) => { - resolve({ + // @ts-expect-error videoAppsMock default registry is a vi.Mock map at test-time + videoAppsMock.default[appStoreLookupKey].mockImplementation(() => { + return Promise.resolve({ lib: { VideoApiAdapter: () => ({ createMeeting: () => { throw new MockError("Error creating Video meeting"); }, }), }, - }); - }); + }); });
17-17: Add a runtime check for missing metadata entriesAlthough the generated metadata file (
packages/app-store/calendarApps.metadata.generated.ts) exists andcalendarAppsMetaDatais populated at build time, it’s still possible for tests to run against an empty object if the raw metadata import isn’t bundled into the test build. Without a guard, looking up a missing key will returnundefined, and subsequent property reads (e.g.app.type) will throw a less-informative “cannot read property ‘type’ of undefined” error.Please add a defensive assertion immediately after you pull the entry from
calendarAppsMetaData:import { calendarAppsMetaData } from "@calcom/app-store/_utils/calendars/calendarAppsMetaData"; @@ - const app = calendarAppsMetaData[metadataLookupKey as keyof typeof calendarAppsMetaData]; + const app = calendarAppsMetaData[metadataLookupKey as keyof typeof calendarAppsMetaData]; + if (!app) { + throw new Error( + `Missing calendarAppsMetaData entry for "${String(metadataLookupKey)}". ` + + `Make sure calendar-apps metadata is generated and included in this test build.` + ); + }This will immediately surface a clear, actionable error when metadata isn’t present, rather than an obscure property-access failure later.
📜 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 (24)
apps/api/v2/src/modules/conferencing/services/conferencing.service.ts(3 hunks)apps/web/test/utils/bookingScenario/bookingScenario.ts(9 hunks)packages/app-store-cli/src/build.ts(4 hunks)packages/app-store/_utils/calendars/calendarLoaders.ts(1 hunks)packages/app-store/_utils/getCalendar.ts(2 hunks)packages/app-store/_utils/payments/paymentLoaders.ts(1 hunks)packages/app-store/_utils/videos/getVideoApps.ts(1 hunks)packages/app-store/_utils/videos/videoAppsMetaData.ts(1 hunks)packages/app-store/_utils/videos/videoLoaders.ts(1 hunks)packages/app-store/videoApps.metadata.generated.ts(1 hunks)packages/lib/CalendarManager.ts(2 hunks)packages/lib/EventManager.ts(3 hunks)packages/lib/getConnectedApps.ts(2 hunks)packages/lib/payment/deletePayment.ts(2 hunks)packages/lib/payment/handlePayment.ts(2 hunks)packages/lib/payment/handlePaymentRefund.ts(2 hunks)packages/lib/server/getDefaultLocations.ts(2 hunks)packages/lib/videoClient.ts(2 hunks)packages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.ts(2 hunks)packages/trpc/server/routers/viewer/payments.tsx(2 hunks)packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts(2 hunks)tests/libs/__mocks__/calendarApps.ts(1 hunks)tests/libs/__mocks__/paymentApps.ts(1 hunks)tests/libs/__mocks__/videoApps.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/lib/CalendarManager.ts
- packages/app-store/_utils/calendars/calendarLoaders.ts
- packages/lib/payment/deletePayment.ts
- packages/app-store/_utils/payments/paymentLoaders.ts
- packages/lib/videoClient.ts
- tests/libs/mocks/videoApps.ts
- packages/lib/payment/handlePaymentRefund.ts
- packages/trpc/server/routers/viewer/payments.tsx
- packages/app-store/_utils/videos/videoLoaders.ts
- tests/libs/mocks/calendarApps.ts
- packages/lib/payment/handlePayment.ts
- packages/lib/getConnectedApps.ts
- packages/app-store/_utils/getCalendar.ts
- packages/trpc/server/routers/viewer/payments/chargeCard.handler.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/app-store/_utils/videos/videoAppsMetaData.tspackages/app-store/videoApps.metadata.generated.tsapps/api/v2/src/modules/conferencing/services/conferencing.service.tspackages/lib/server/getDefaultLocations.tspackages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.tstests/libs/__mocks__/paymentApps.tspackages/app-store/_utils/videos/getVideoApps.tspackages/lib/EventManager.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/app-store-cli/src/build.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/_utils/videos/videoAppsMetaData.tspackages/app-store/videoApps.metadata.generated.tsapps/api/v2/src/modules/conferencing/services/conferencing.service.tspackages/lib/server/getDefaultLocations.tspackages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.tstests/libs/__mocks__/paymentApps.tspackages/app-store/_utils/videos/getVideoApps.tspackages/lib/EventManager.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/app-store-cli/src/build.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/_utils/videos/videoAppsMetaData.tspackages/app-store/videoApps.metadata.generated.tsapps/api/v2/src/modules/conferencing/services/conferencing.service.tspackages/lib/server/getDefaultLocations.tspackages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.tstests/libs/__mocks__/paymentApps.tspackages/app-store/_utils/videos/getVideoApps.tspackages/lib/EventManager.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/app-store-cli/src/build.ts
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
apps/api/v2/src/modules/conferencing/services/conferencing.service.ts
🧬 Code graph analysis (6)
apps/api/v2/src/modules/conferencing/services/conferencing.service.ts (1)
packages/app-store/_utils/videos/getVideoApps.ts (1)
getVideoApps(40-100)
packages/lib/server/getDefaultLocations.ts (1)
packages/app-store/_utils/videos/getVideoApps.ts (1)
getVideoApps(40-100)
packages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.ts (1)
packages/app-store/_utils/videos/getVideoApps.ts (1)
getVideoApps(40-100)
packages/app-store/_utils/videos/getVideoApps.ts (3)
packages/app-store/locations.ts (1)
EventLocationType(58-58)packages/app-store/_utils/videos/videoAppsMetaData.ts (1)
videoAppsMetaData(11-11)packages/lib/piiFreeData.ts (1)
getPiiFreeCredential(61-72)
packages/lib/EventManager.ts (2)
packages/app-store/_utils/calendars/getCalendarApps.ts (1)
getCalendarApps(40-100)packages/app-store/_utils/videos/getVideoApps.ts (1)
getVideoApps(40-100)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
packages/app-store/_utils/calendars/calendarAppsMetaData.ts (1)
calendarAppsMetaData(11-11)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
packages/app-store-cli/src/build.ts (2)
350-388: Calendar/video metadata generation: nice addition, aligns with goalsThe two new generated collections (calendarAppsMetadata, videoAppsMetadata) are wired correctly through getExportedObject and filtered by isCalendarApp/isVideoApp. This should help trim import graphs and improve build times on the installed apps pages.
457-459: Category predicate covers both “video” and “conferencing”Good call including “conferencing” for legacy categorization.
packages/app-store/_utils/videos/getVideoApps.ts (1)
40-100: No credential.key exposed in API responsesI’ve verified all
getVideoAppscall sites and confirmed that none of the API or tRPC handlers return the raw credentials (includingkey) to clients:
- In
packages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.ts, the output is limited to the selected app slug and location, not the credential payload.- In
packages/lib/server/getDefaultLocations.tsand the NestJS service atapps/api/v2/src/modules/conferencing/services/conferencing.service.ts, we only derive and return location data.- All other usages (e.g., in
packages/lib/EventManager.ts) occur within backend-only libraries that decrypt or process the key internally.No further sanitization is required at these call sites.
apps/api/v2/src/modules/conferencing/services/conferencing.service.ts (2)
17-17: Switch to getVideoApps: aligns with new metadata/loader flowImport change is correct and aligns this service with the new video loader/metadata pipeline.
107-114: Delete handler import integration looks goodhandleDeleteCredential usage is straightforward; once the credential nullability guard is in place, this path remains safe.
packages/lib/server/getDefaultLocations.ts (1)
2-2: Good swap to category-specific loader (video) — aligns with new loader architectureUsing getVideoApps here is correct and keeps this module scoped to video apps only. It also benefits from global-app credential injection and avoids scanning unrelated app categories.
packages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.ts (1)
3-3: Right import — use video-only catalog for conferencing defaultsSwitching to getVideoApps keeps this handler focused on video apps and leverages global-app credential injection. Nice.
packages/lib/EventManager.ts (2)
7-10: Scoped imports to calendar/video loaders are the right directionAdopting getCalendarApps and getVideoApps reduces cross-category work and keeps credentials derivation aligned with the new metadata/loader split.
168-174: Video credentials derivation looks good; filter and sort capture the right setUsing getVideoApps + suffix filters is consistent with how video types are named, and newest-first sorting matches “latest connection wins”.
tests/libs/__mocks__/paymentApps.ts (2)
1-1: Nice: global reset and explicit failure fallbackResetting the deep mock in
beforeEachand throwing loudly viafallbackMockImplementationare good testing ergonomics. This will make missing mocks fail fast.Also applies to: 8-10, 20-20
12-20: Mock shape matches the realpaymentLoadersexport
I’ve confirmed that the real module atpackages/app-store/_utils/payments/paymentLoaders.ts:
- Exports a default object whose keys are exactly
alby,paypal,stripepayment,hitpay,btcpayserver(and, whenMOCK_PAYMENT_APP_ENABLEDis set, adds"mock-payment-app").- Each key’s value is a loader function (returned by
createCachedImport), so under a deep mock each becomes ajest.Mockand supports.mockImplementation(...).Since
mockDeep<typeof paymentLoaders>correctly mirrors that shape, no changes to the mock are needed.apps/web/test/utils/bookingScenario/bookingScenario.ts (4)
1723-1723: Improved typing for mockCalendar keySwitching to
keyof typeof calendarAppsMetaDatastrengthens call-site safety. Good change.
1906-1918: Helper overload updates look goodThe new signatures keyed by
keyof typeof calendarAppsMetaDataalign withmockCalendarand improve type safety across helpers.
1766-1766: Good: targeted log contextAdding the app key to the log line helps chase down the specific mocked adapter in test traces.
2-6: Mocks imports verified – please confirm Vitest path resolutionAll three mock files exist and your relative imports resolve correctly from apps/web/test/utils/bookingScenario:
- tests/libs/mocks/calendarApps.ts
- tests/libs/mocks/videoApps.ts
- tests/libs/mocks/paymentApps.ts
Please verify that your Vitest configuration (e.g.
resolve.alias,root, ortest.include) supports these long relative paths when tests are executed from different working directories. Alternatively, consider adding a path alias (for example,@tests→<root>/tests) in your Vitest config to simplify and stabilize mock imports.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/app-store/_utils/sanitizeAppsMetadata.ts (1)
3-19: Avoid mutation + ts-ignore; create a cleaned copy insteadDeleting magic keys with
ts-ignoreis brittle and mutates imported metadata. Build a sanitized shallow copy using destructuring to keep typing sound and avoid side effects.Apply:
export const sanitizeAppsMetadata = <T extends object>(appsMetaData: T) => { const appsMap = Object.keys(appsMetaData).reduce((store, key) => { - const metadata = appsMetaData[key as keyof T] as AppMeta; - - store[key] = metadata; - - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore - delete store[key]["/*"]; - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore - delete store[key]["__createdUsingCli"]; - return store; + const metadata = appsMetaData[key as keyof T] as AppMeta & { + ["/*"]?: unknown; + __createdUsingCli?: unknown; + }; + // Create a cleaned copy without internal markers + const { ["/*"]: _omitInternal, __createdUsingCli: _omitCli, ...cleaned } = metadata; + store[key] = cleaned; + return store; }, {} as Record<string, AppMeta>); return Object.values(appsMap); };
🧹 Nitpick comments (5)
packages/app-store/_utils/prepareAppsWithCredentials.ts (1)
36-52: Nit: Sentinel id0for synthetic credentialsUsing
id: 0risks colliding with real records if any system treats0as “valid” rather than “absent”. Consider a negative sentinel or a branded type to avoid ambiguity.packages/app-store/_utils/sanitizeAppsMetadata.ts (1)
1-1: Optional: make return type explicitFor readability and editor inference, consider annotating the return type as
AppMeta[].packages/app-store/utils.ts (3)
22-35: Avoid mutatingALL_APPS_MAP; use sanitized copiesThis block deletes fields from imported metadata with
ts-ignore, mutating shared objects. Prefer deriving this data fromALL_APPS(already sanitized) or clone first.For example:
// Derive by slug from sanitized array const ALL_APPS_BY_SLUG = new Map(ALL_APPS.map((a) => [a.slug, a] as const)); export function getAppName(name: string): string | null { return ALL_APPS_BY_SLUG.get(name)?.name ?? null; } export function getAppType(name: string): string { const type = ALL_APPS_BY_SLUG.get(name)?.type; if (!type) return "Unknown"; if (type.endsWith("_calendar")) return "Calendar"; if (type.endsWith("_payment")) return "Payment"; return "Unknown"; }
122-123: Prefer named exports over default for utilitiesDefault exports make refactors harder and hurt tree-shaking. Consider
export { getApps }and update imports.
36-40: Type duplication: reuse sharedCredentialDataWithTeamName
CredentialDataWithTeamNameis exported from./_utils/prepareAppsWithCredentials. Import that type here instead of re-declaring to avoid drift.
📜 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 (5)
packages/app-store/_utils/calendars/getCalendarApps.ts(1 hunks)packages/app-store/_utils/prepareAppsWithCredentials.ts(1 hunks)packages/app-store/_utils/sanitizeAppsMetadata.ts(1 hunks)packages/app-store/_utils/videos/getVideoApps.ts(1 hunks)packages/app-store/utils.ts(2 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/_utils/videos/getVideoApps.tspackages/app-store/_utils/sanitizeAppsMetadata.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/app-store/utils.tspackages/app-store/_utils/prepareAppsWithCredentials.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/_utils/videos/getVideoApps.tspackages/app-store/_utils/sanitizeAppsMetadata.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/app-store/utils.tspackages/app-store/_utils/prepareAppsWithCredentials.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/_utils/videos/getVideoApps.tspackages/app-store/_utils/sanitizeAppsMetadata.tspackages/app-store/_utils/calendars/getCalendarApps.tspackages/app-store/utils.tspackages/app-store/_utils/prepareAppsWithCredentials.ts
🧠 Learnings (1)
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : Ensure the `credential.key` field is never returned from tRPC endpoints or APIs
Applied to files:
packages/app-store/_utils/videos/getVideoApps.tspackages/app-store/_utils/calendars/getCalendarApps.ts
🧬 Code graph analysis (4)
packages/app-store/_utils/videos/getVideoApps.ts (3)
packages/app-store/_utils/sanitizeAppsMetadata.ts (1)
sanitizeAppsMetadata(3-19)packages/app-store/_utils/videos/videoAppsMetaData.ts (1)
videoAppsMetaData(11-11)packages/app-store/_utils/prepareAppsWithCredentials.ts (2)
CredentialDataWithTeamName(16-20)prepareAppsWithCredentials(22-84)
packages/app-store/_utils/calendars/getCalendarApps.ts (3)
packages/app-store/_utils/sanitizeAppsMetadata.ts (1)
sanitizeAppsMetadata(3-19)packages/app-store/_utils/calendars/calendarAppsMetaData.ts (1)
calendarAppsMetaData(11-11)packages/app-store/_utils/prepareAppsWithCredentials.ts (2)
CredentialDataWithTeamName(16-20)prepareAppsWithCredentials(22-84)
packages/app-store/utils.ts (2)
packages/app-store/_utils/sanitizeAppsMetadata.ts (1)
sanitizeAppsMetadata(3-19)packages/app-store/_utils/prepareAppsWithCredentials.ts (2)
CredentialDataWithTeamName(16-20)prepareAppsWithCredentials(22-84)
packages/app-store/_utils/prepareAppsWithCredentials.ts (2)
packages/app-store/locations.ts (1)
EventLocationType(61-61)packages/lib/piiFreeData.ts (1)
getPiiFreeCredential(61-72)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
🔇 Additional comments (2)
packages/app-store/utils.ts (2)
10-12: Good move delegating to shared utilitiesSwitching to
prepareAppsWithCredentialsandsanitizeAppsMetadataremoves duplication and centralizes behavior.
42-42: Nice: centralized metadata sanitationUsing
sanitizeAppsMetadataforALL_APPSkeeps internal markers out of public consumption.
| export const getCalendarApps = (credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) => { | ||
| return prepareAppsWithCredentials(ALL_CALENDAR_APPS, credentials, filterOnCredentials); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redact credential.key before returning calendar apps
Aligns with security guidance and avoids leaking secrets to the UI.
Apply:
-export const getCalendarApps = (credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) => {
- return prepareAppsWithCredentials(ALL_CALENDAR_APPS, credentials, filterOnCredentials);
-};
+export const getCalendarApps = (
+ credentials: CredentialDataWithTeamName[],
+ filterOnCredentials?: boolean
+) => {
+ const apps = prepareAppsWithCredentials(ALL_CALENDAR_APPS, credentials, filterOnCredentials);
+ return apps.map(({ credentials: creds, ...app }) => {
+ const sanitizedCredentials: Omit<CredentialDataWithTeamName, "key">[] = creds.map(
+ ({ key, ...rest }) => rest
+ );
+ return {
+ ...app,
+ credentials: sanitizedCredentials,
+ credential: sanitizedCredentials[0] ?? 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.
| export const getCalendarApps = (credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) => { | |
| return prepareAppsWithCredentials(ALL_CALENDAR_APPS, credentials, filterOnCredentials); | |
| }; | |
| export const getCalendarApps = ( | |
| credentials: CredentialDataWithTeamName[], | |
| filterOnCredentials?: boolean | |
| ) => { | |
| const apps = prepareAppsWithCredentials( | |
| ALL_CALENDAR_APPS, | |
| credentials, | |
| filterOnCredentials | |
| ); | |
| return apps.map(({ credentials: creds, ...app }) => { | |
| const sanitizedCredentials: Omit<CredentialDataWithTeamName, "key">[] = | |
| creds.map(({ key, ...rest }) => rest); | |
| return { | |
| ...app, | |
| credentials: sanitizedCredentials, | |
| credential: sanitizedCredentials[0] ?? null, | |
| }; | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In packages/app-store/_utils/calendars/getCalendarApps.ts around lines 8 to 10,
the function currently returns apps with credential.key values that could leak
secrets to the UI; before returning, remove or redact credential.key for every
credential attached to the prepared apps (e.g., map over the result of
prepareAppsWithCredentials and replace credential.key with a masked string or
delete the key property), ensuring the returned objects preserve other
credential metadata but never include the raw secret.
| export const prepareAppsWithCredentials = ( | ||
| apps: App[], | ||
| credentials: CredentialDataWithTeamName[], | ||
| filterOnCredentials?: boolean | ||
| ) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm public exposure and redact credential.key before returning outside server boundaries
This helper returns credentials including key. If any consumer sends this result through tRPC/API (e.g., powering /apps/installed/* pages), it would leak secrets. Either sanitize here or at the module boundary (e.g., in getCalendarApps/getVideoApps/getApps).
Run to locate potential API exposures:
If exposure is confirmed, I can provide a minimal patch to sanitize at the call sites or here with a feature flag (e.g., sanitizeSecrets = true).
🏁 Script executed:
#!/bin/bash
# Find callers of these helpers in routers/APIs and inspect returned fields
rg -nP -C3 --type=ts '\b(getApps|getCalendarApps|getVideoApps)\s*\('
# Heuristic: look for tRPC/router handlers returning "credentials" or "credential"
rg -nP -C3 --type=ts '(createTRPCRouter|publicProcedure|privateProcedure|router\(|NextResponse\.json|res\.json)\b'
# Check for accidental exposure of credential.key
rg -nP -C2 --type=ts '\bcredential\.key\b|credentials?:\s*\[?[^{\]]*key'Length of output: 182599
🏁 Script executed:
#!/bin/bash
# Display the implementation of prepareAppsWithCredentials to inspect credential handling
sed -n '1,200p' packages/app-store/_utils/prepareAppsWithCredentials.tsLength of output: 2716
Redact credential.key in prepareAppsWithCredentials before exposing to clients
prepareAppsWithCredentials currently embeds each credential’s raw key in both the credential and credentials arrays. Any consumer (e.g. via getApps → front‐end pages or API routers) will inherit those secrets unless explicitly stripped.
Please update as follows:
• File: packages/app-store/_utils/prepareAppsWithCredentials.ts
– After building appCredentials, map each through getPiiFreeCredential (or explicitly delete the key field) before pushing.
– Example patch:
…
const credential: CredentialDataWithTeamName | null = appCredentials[0] || null;
- reducedArray.push({
- ...appMeta,
- credential,
- credentials: appCredentials,
+ // remove `key` from any outgoing credential
+ const safeCredentials = appCredentials.map(getPiiFreeCredential);
+ const safeCredential = safeCredentials[0] || null;
+ reducedArray.push({
+ ...appMeta,
+ credential: safeCredential,
+ credentials: safeCredentials,
locationOption,
});• Verify all call sites (e.g. any tRPC procedures or REST handlers invoking getApps/getCalendarApps/getVideoApps) do not forward the raw key. Either sanitize at prepareAppsWithCredentials or at those boundaries.
• Optionally, consider renaming the deprecated credential field or splitting into a “public” DTO to avoid accidental leaks.
📝 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 prepareAppsWithCredentials = ( | |
| apps: App[], | |
| credentials: CredentialDataWithTeamName[], | |
| filterOnCredentials?: boolean | |
| ) => { | |
| const credential: CredentialDataWithTeamName | null = appCredentials[0] || null; | |
| // remove `key` from any outgoing credential | |
| const safeCredentials = appCredentials.map(getPiiFreeCredential); | |
| const safeCredential = safeCredentials[0] || null; | |
| reducedArray.push({ | |
| ...appMeta, | |
| - credential, | |
| credential: safeCredential, | |
| credentials: safeCredentials, | |
| locationOption, | |
| }); |
🤖 Prompt for AI Agents
In packages/app-store/_utils/prepareAppsWithCredentials.ts around lines 22–26,
the function currently includes raw credential.key values in returned objects;
update the code so that before pushing appCredentials into the
credential/credentials arrays you map each credential through
getPiiFreeCredential (or explicitly delete the key property) to remove the
secret, ensuring no raw keys are exposed to clients; additionally audit and
confirm all call sites (tRPC procedures, REST handlers like
getApps/getCalendarApps/getVideoApps) do not forward raw credential.key values
and consider renaming or splitting the deprecated credential field into a
sanitized public DTO to prevent accidental leaks.
| if (appMeta.isGlobal) { | ||
| const credential = { | ||
| id: 0, | ||
| type: appMeta.type, | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| key: appMeta.key!, | ||
| userId: 0, | ||
| user: { email: "" }, | ||
| teamId: null, | ||
| appId: appMeta.slug, | ||
| invalid: false, | ||
| delegatedTo: null, | ||
| delegatedToId: null, | ||
| delegationCredentialId: null, | ||
| team: { | ||
| name: "Global", | ||
| }, | ||
| }; | ||
| logger.debug( | ||
| `${appMeta.type} is a global app, injecting credential`, | ||
| safeStringify(getPiiFreeCredential(credential)) | ||
| ); | ||
| appCredentials.push(credential); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent duplicate global credentials and guard missing key
Only inject the synthetic global credential when none exists for this app, and guard against a missing appMeta.key to avoid runtime assertion with !.
Apply:
- /** If the app is a globally installed one, let's inject it's key */
- if (appMeta.isGlobal) {
- const credential = {
- id: 0,
- type: appMeta.type,
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
- key: appMeta.key!,
- userId: 0,
- user: { email: "" },
- teamId: null,
- appId: appMeta.slug,
- invalid: false,
- delegatedTo: null,
- delegatedToId: null,
- delegationCredentialId: null,
- team: {
- name: "Global",
- },
- };
- logger.debug(
- `${appMeta.type} is a global app, injecting credential`,
- safeStringify(getPiiFreeCredential(credential))
- );
- appCredentials.push(credential);
- }
+ /** If the app is a globally installed one, inject its key once */
+ if (
+ appMeta.isGlobal &&
+ !appCredentials.some((c) => c.userId === 0 && c.appId === appMeta.slug)
+ ) {
+ if (!appMeta.key) {
+ logger.warn(
+ `${appMeta.type} marked as global but has no key; skipping credential injection for ${appMeta.slug}`
+ );
+ } else {
+ const credential = {
+ id: 0,
+ type: appMeta.type,
+ key: appMeta.key,
+ userId: 0,
+ user: { email: "" },
+ teamId: null,
+ appId: appMeta.slug,
+ invalid: false,
+ delegatedTo: null,
+ delegatedToId: null,
+ delegationCredentialId: null,
+ team: {
+ name: "Global",
+ },
+ };
+ logger.debug(
+ `${appMeta.type} is a global app, injecting credential`,
+ safeStringify(getPiiFreeCredential(credential))
+ );
+ appCredentials.push(credential);
+ }
+ }📝 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 (appMeta.isGlobal) { | |
| const credential = { | |
| id: 0, | |
| type: appMeta.type, | |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
| key: appMeta.key!, | |
| userId: 0, | |
| user: { email: "" }, | |
| teamId: null, | |
| appId: appMeta.slug, | |
| invalid: false, | |
| delegatedTo: null, | |
| delegatedToId: null, | |
| delegationCredentialId: null, | |
| team: { | |
| name: "Global", | |
| }, | |
| }; | |
| logger.debug( | |
| `${appMeta.type} is a global app, injecting credential`, | |
| safeStringify(getPiiFreeCredential(credential)) | |
| ); | |
| appCredentials.push(credential); | |
| } | |
| /** If the app is a globally installed one, inject its key once */ | |
| if ( | |
| appMeta.isGlobal && | |
| !appCredentials.some((c) => c.userId === 0 && c.appId === appMeta.slug) | |
| ) { | |
| if (!appMeta.key) { | |
| logger.warn( | |
| `${appMeta.type} marked as global but has no key; skipping credential injection for ${appMeta.slug}` | |
| ); | |
| } else { | |
| const credential = { | |
| id: 0, | |
| type: appMeta.type, | |
| key: appMeta.key, | |
| userId: 0, | |
| user: { email: "" }, | |
| teamId: null, | |
| appId: appMeta.slug, | |
| invalid: false, | |
| delegatedTo: null, | |
| delegatedToId: null, | |
| delegationCredentialId: null, | |
| team: { | |
| name: "Global", | |
| }, | |
| }; | |
| logger.debug( | |
| `${appMeta.type} is a global app, injecting credential`, | |
| safeStringify(getPiiFreeCredential(credential)) | |
| ); | |
| appCredentials.push(credential); | |
| } | |
| } |
| const credential: (typeof appCredentials)[number] | null = appCredentials[0] || null; | ||
|
|
||
| reducedArray.push({ | ||
| ...appMeta, | ||
| /** | ||
| * @deprecated use `credentials` | ||
| */ | ||
| credential, | ||
| credentials: appCredentials, | ||
| /** Option to display in `location` field while editing event types */ | ||
| locationOption, | ||
| }); | ||
|
|
||
| return reducedArray; | ||
| }, [] as (App & { credential: CredentialDataWithTeamName; credentials: CredentialDataWithTeamName[]; locationOption: LocationOption | null })[]); | ||
| }; |
There was a problem hiding this comment.
Type unsoundness: credential can be null but return type forces non-null
credential is computed with a || null fallback, yet the accumulator type requires a non-null CredentialDataWithTeamName. This can mask errors and cause downstream NPEs.
Apply:
- }, [] as (App & { credential: CredentialDataWithTeamName; credentials: CredentialDataWithTeamName[]; locationOption: LocationOption | null })[]);
+ }, [] as (App & {
+ credential: CredentialDataWithTeamName | null;
+ credentials: CredentialDataWithTeamName[];
+ locationOption: LocationOption | null;
+ })[]);Optionally, make the function’s return type explicit so callers get correct typing:
-export const prepareAppsWithCredentials = (
+export const prepareAppsWithCredentials: (
+ apps: App[],
+ credentials: CredentialDataWithTeamName[],
+ filterOnCredentials?: boolean
+) => Array<App & {
+ credential: CredentialDataWithTeamName | null;
+ credentials: CredentialDataWithTeamName[];
+ locationOption: LocationOption | null;
+}> = (
apps: App[],
credentials: CredentialDataWithTeamName[],
filterOnCredentials?: boolean
) => {📝 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 credential: (typeof appCredentials)[number] | null = appCredentials[0] || null; | |
| reducedArray.push({ | |
| ...appMeta, | |
| /** | |
| * @deprecated use `credentials` | |
| */ | |
| credential, | |
| credentials: appCredentials, | |
| /** Option to display in `location` field while editing event types */ | |
| locationOption, | |
| }); | |
| return reducedArray; | |
| }, [] as (App & { credential: CredentialDataWithTeamName; credentials: CredentialDataWithTeamName[]; locationOption: LocationOption | null })[]); | |
| }; | |
| const credential: (typeof appCredentials)[number] | null = appCredentials[0] || null; | |
| reducedArray.push({ | |
| ...appMeta, | |
| /** | |
| * @deprecated use `credentials` | |
| */ | |
| credential, | |
| credentials: appCredentials, | |
| /** Option to display in `location` field while editing event types */ | |
| locationOption, | |
| }); | |
| return reducedArray; | |
| }, [] as (App & { | |
| credential: CredentialDataWithTeamName | null; | |
| credentials: CredentialDataWithTeamName[]; | |
| locationOption: LocationOption | null; | |
| })[]); | |
| }; |
🤖 Prompt for AI Agents
In packages/app-store/_utils/prepareAppsWithCredentials.ts around lines 69–84,
the code sets credential = appCredentials[0] || null but the accumulator's
return type forces credential to be non-null; update the accumulator return type
to allow credential to be CredentialDataWithTeamName | null (and keep
credentials: CredentialDataWithTeamName[]), or alternatively ensure credential
is always non-null (e.g., filter out entries with empty appCredentials before
reduce or provide a safe default), then make the function's return type explicit
to reflect this change so callers receive correct typing and avoid masked NPEs.
| export const getVideoApps = (credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) => { | ||
| return prepareAppsWithCredentials(ALL_VIDEO_APPS, credentials, filterOnCredentials); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redact credential.key before returning video apps
Per our guideline, never return credential.key to clients. Sanitize the credentials array and make the single credential nullable.
Apply:
-export const getVideoApps = (credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) => {
- return prepareAppsWithCredentials(ALL_VIDEO_APPS, credentials, filterOnCredentials);
-};
+export const getVideoApps = (
+ credentials: CredentialDataWithTeamName[],
+ filterOnCredentials?: boolean
+) => {
+ const apps = prepareAppsWithCredentials(ALL_VIDEO_APPS, credentials, filterOnCredentials);
+ return apps.map(({ credentials: creds, ...app }) => {
+ const sanitizedCredentials: Omit<CredentialDataWithTeamName, "key">[] = creds.map(
+ ({ key, ...rest }) => rest
+ );
+ return {
+ ...app,
+ credentials: sanitizedCredentials,
+ credential: sanitizedCredentials[0] ?? 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.
| export const getVideoApps = (credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) => { | |
| return prepareAppsWithCredentials(ALL_VIDEO_APPS, credentials, filterOnCredentials); | |
| }; | |
| export const getVideoApps = ( | |
| credentials: CredentialDataWithTeamName[], | |
| filterOnCredentials?: boolean | |
| ) => { | |
| const apps = prepareAppsWithCredentials(ALL_VIDEO_APPS, credentials, filterOnCredentials); | |
| return apps.map(({ credentials: creds, ...app }) => { | |
| const sanitizedCredentials: Omit<CredentialDataWithTeamName, "key">[] = creds.map( | |
| ({ key, ...rest }) => rest | |
| ); | |
| return { | |
| ...app, | |
| credentials: sanitizedCredentials, | |
| credential: sanitizedCredentials[0] ?? null, | |
| }; | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In packages/app-store/_utils/videos/getVideoApps.ts around lines 8 to 10, the
function currently forwards credentials (including credential.key) to clients;
sanitize the credentials before returning: map the credentials array to remove
or null out the key field on each CredentialDataWithTeamName (e.g., copy all
fields except set key = null or delete key) and update the signature/usage so
any single credential passed into prepareAppsWithCredentials is nullable; ensure
prepareAppsWithCredentials is called with the sanitized array (and the
possibly-null single credential) so no credential.key values are returned to
clients.
|
|
||
| return reducedArray; | ||
| }, [] as (App & { credential: CredentialDataWithTeamName; credentials: CredentialDataWithTeamName[]; locationOption: LocationOption | null })[]); | ||
| const apps = prepareAppsWithCredentials(ALL_APPS, credentials, filterOnCredentials); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redact credential.key before returning from getApps
This function often feeds UI via tRPC. Sanitize secrets prior to returning to avoid leaking key.
Apply:
-function getApps(credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) {
- const apps = prepareAppsWithCredentials(ALL_APPS, credentials, filterOnCredentials);
-
- return apps;
-}
+function getApps(credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) {
+ const prepared = prepareAppsWithCredentials(ALL_APPS, credentials, filterOnCredentials);
+ const apps = prepared.map(({ credentials: creds, ...app }) => {
+ const sanitizedCredentials: Omit<CredentialDataWithTeamName, "key">[] = creds.map(
+ ({ key, ...rest }) => rest
+ );
+ return {
+ ...app,
+ credentials: sanitizedCredentials,
+ credential: sanitizedCredentials[0] ?? null,
+ };
+ });
+ return apps;
+}📝 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 apps = prepareAppsWithCredentials(ALL_APPS, credentials, filterOnCredentials); | |
| function getApps(credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean) { | |
| const prepared = prepareAppsWithCredentials(ALL_APPS, credentials, filterOnCredentials); | |
| const apps = prepared.map(({ credentials: creds, ...app }) => { | |
| const sanitizedCredentials: Omit<CredentialDataWithTeamName, "key">[] = creds.map( | |
| ({ key, ...rest }) => rest | |
| ); | |
| return { | |
| ...app, | |
| credentials: sanitizedCredentials, | |
| credential: sanitizedCredentials[0] ?? null, | |
| }; | |
| }); | |
| return apps; | |
| } |
🤖 Prompt for AI Agents
In packages/app-store/utils.ts around line 49,
prepareAppsWithCredentials(ALL_APPS, credentials, filterOnCredentials) returns
app objects that include credential.key which must be sanitized before exposing
to the UI; modify the code after that call to produce a shallow/deep copy of the
apps and remove or replace credential.key (e.g., set to undefined or a fixed
"[REDACTED]" string) for each credential entry so secrets are not returned,
ensuring you do not mutate the original credentials array (use map or
structuredClone) and preserve all other fields and types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/libs/__mocks__/mockDeepHelper.ts (1)
1-10: Make return type explicit and tighten mock typing for better DXAnnotating the helper’s return type helps downstream usage (e.g.,
mockReset) with full IntelliSense and safer refactors.Apply this diff:
-import { mockDeep } from "vitest-mock-extended"; +import { mockDeep, type DeepMockProxy } from "vitest-mock-extended"; -export const mockDeepHelper = <T>(mockFnName = "") => +export const mockDeepHelper = <T>(mockFnName = ""): DeepMockProxy<T> => mockDeep<T>({ fallbackMockImplementation: () => { throw new Error( `Unimplemented ${mockFnName}. You seem to have not mocked the app that you are trying to use` ); }, });tests/libs/__mocks__/videoLoadersMock.ts (1)
8-16: Harden ESM default interop in vi.mock factory and provide a named exportReturning the mock object directly can stumble on ESM/CJS default interop. Declare
__esModule: trueand return an object with adefaultexport. Also export a named symbol for consistency with our guidelines on named exports.Apply this diff:
-const videoLoadersMock = mockDeepHelper<typeof videoLoaders>("videoLoadersMock"); +const videoLoadersMock = mockDeepHelper<typeof videoLoaders>("videoLoadersMock"); -vi.mock("@calcom/app-store/_utils/videos/videoLoaders", () => videoLoadersMock); +vi.mock("@calcom/app-store/_utils/videos/videoLoaders", () => ({ + __esModule: true, + default: videoLoadersMock.default, +})); export default videoLoadersMock; +export { videoLoadersMock };tests/libs/__mocks__/paymentLoadersMock.ts (1)
8-16: Mirror the ESM mock shape and add a named exportSame rationale as video loaders: ensure
__esModule: trueand expose a named export for consistency.-const paymentLoadersMock = mockDeepHelper<typeof paymentLoaders>("paymentLoadersMock"); +const paymentLoadersMock = mockDeepHelper<typeof paymentLoaders>("paymentLoadersMock"); -vi.mock("@calcom/app-store/_utils/payments/paymentLoaders", () => paymentLoadersMock); +vi.mock("@calcom/app-store/_utils/payments/paymentLoaders", () => ({ + __esModule: true, + default: paymentLoadersMock.default, +})); export default paymentLoadersMock; +export { paymentLoadersMock };tests/libs/__mocks__/calendarLoadersMock.ts (1)
8-16: Align calendar loaders mock with robust ESM interop and add named exportKeep the mock shape explicit and export a named symbol to ease future refactors.
-const calendarLoadersMock = mockDeepHelper<typeof calendarLoaders>("calendarLoadersMock"); +const calendarLoadersMock = mockDeepHelper<typeof calendarLoaders>("calendarLoadersMock"); -vi.mock("@calcom/app-store/_utils/calendars/calendarLoaders", () => calendarLoadersMock); +vi.mock("@calcom/app-store/_utils/calendars/calendarLoaders", () => ({ + __esModule: true, + default: calendarLoadersMock.default, +})); export default calendarLoadersMock; +export { calendarLoadersMock };apps/web/test/utils/bookingScenario/bookingScenario.ts (4)
1767-1775: Make failure modes explicit and fix logging labelCurrently, if
calendarAppsMetaData[key]or the loader mock is missing, the code silently skips mocking and tests may fail deeper in the stack. Fail fast with actionable messages. Also, the log label still says “calendarAppsMock”.- log.silly(`Mocking ${appStoreLookupKey} on calendarAppsMock`); + log.silly(`Mocking ${appStoreLookupKey} on calendarLoadersMock`); - const app = calendarAppsMetaData[metadataLookupKey as keyof typeof calendarAppsMetaData]; + const app = calendarAppsMetaData[metadataLookupKey as keyof typeof calendarAppsMetaData]; + if (!app) { + throw new Error( + `calendarAppsMetaData["${String( + metadataLookupKey + )}"] not found. Ensure the calendar metadata is generated/available for this key.` + ); + } - const appMock = calendarLoadersMock.default[appStoreLookupKey as keyof typeof calendarLoadersMock.default]; + const appMock = + calendarLoadersMock.default[appStoreLookupKey as keyof typeof calendarLoadersMock.default]; + if (!appMock) { + throw new Error( + `calendarLoadersMock.default["${String( + appStoreLookupKey + )}"] is missing. Did you import tests/libs/__mocks__/calendarLoadersMock and set up the loader key?` + ); + }
1959-2020: Guard missing video loader keys and simplify the mock wiringAdd an explicit check so tests fail fast if the loader key isn’t present. Also, slightly simplify the promise creation.
- videoLoadersMock.default[appStoreLookupKey as keyof typeof videoLoadersMock.default].mockImplementation( - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error FIXME - () => { - return new Promise((resolve) => { - resolve({ - lib: { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error FIXME - VideoApiAdapter: (credential) => { + const videoLoader = + videoLoadersMock.default[appStoreLookupKey as keyof typeof videoLoadersMock.default]; + if (!videoLoader) { + throw new Error( + `videoLoadersMock.default["${String( + appStoreLookupKey + )}"] is missing. Ensure the video loader key is mocked before use.` + ); + } + videoLoader.mockImplementation( + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-expect-error FIXME + async () => { + return { + lib: { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-expect-error FIXME + VideoApiAdapter: (credential) => { return { // eslint-disable-next-line @typescript-eslint/no-explicit-any createMeeting: (...rest: any[]) => { if (creationCrash) { throw new Error("MockVideoApiAdapter.createMeeting fake error"); } createMeetingCalls.push({ credential, args: rest, }); return Promise.resolve({ type: appStoreMetadata[metadataLookupKey as keyof typeof appStoreMetadata].type, ...videoMeetingData, }); }, // eslint-disable-next-line @typescript-eslint/no-explicit-any updateMeeting: async (...rest: any[]) => { if (updationCrash) { throw new Error("MockVideoApiAdapter.updateMeeting fake error"); } const [bookingRef, calEvent] = rest; updateMeetingCalls.push({ credential, args: rest, }); if (!bookingRef.type) { throw new Error("bookingRef.type is not defined"); } if (!calEvent.organizer) { throw new Error("calEvent.organizer is not defined"); } log.silly("MockVideoApiAdapter.updateMeeting", JSON.stringify({ bookingRef, calEvent })); return Promise.resolve({ type: appStoreMetadata[metadataLookupKey as keyof typeof appStoreMetadata].type, ...videoMeetingData, }); }, // eslint-disable-next-line @typescript-eslint/no-explicit-any deleteMeeting: async (...rest: any[]) => { log.silly("MockVideoApiAdapter.deleteMeeting", JSON.stringify(rest)); deleteMeetingCalls.push({ credential, args: rest, }); }, }; - }, - }, - }); - }); - } + }, + }, + }; + } );
2071-2085: Add a guard for missing payment loader keys and simplify promise styleFailing early makes test failures clearer; using
asynccleans up the promise ceremony.- paymentLoadersMock.default[appStoreLookupKey as keyof typeof paymentLoadersMock.default].mockImplementation( - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error FIXME - () => { - return new Promise((resolve) => { - resolve({ - lib: { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error FIXME - PaymentService: MockPaymentService, - }, - }); - }); - } - ); + const paymentLoader = + paymentLoadersMock.default[appStoreLookupKey as keyof typeof paymentLoadersMock.default]; + if (!paymentLoader) { + throw new Error( + `paymentLoadersMock.default["${String( + appStoreLookupKey + )}"] is missing. Ensure the payment loader key is mocked before use.` + ); + } + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-expect-error FIXME + paymentLoader.mockImplementation(async () => { + return { + lib: { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-expect-error FIXME + PaymentService: MockPaymentService, + }, + }; + });
2103-2115: Prefer ts-expect-error over ts-ignore for intentional type breaks
@ts-expect-errorensures we notice if the error stops being necessary in future refactors.- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-expect-error - dynamic key on deep mock videoLoadersMock.default[appStoreLookupKey].mockImplementation(() => {
📜 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 (6)
apps/web/test/utils/bookingScenario/bookingScenario.ts(9 hunks)tests/libs/__mocks__/app-store.ts(1 hunks)tests/libs/__mocks__/calendarLoadersMock.ts(1 hunks)tests/libs/__mocks__/mockDeepHelper.ts(1 hunks)tests/libs/__mocks__/paymentLoadersMock.ts(1 hunks)tests/libs/__mocks__/videoLoadersMock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/libs/mocks/app-store.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
tests/libs/__mocks__/videoLoadersMock.tstests/libs/__mocks__/mockDeepHelper.tstests/libs/__mocks__/calendarLoadersMock.tstests/libs/__mocks__/paymentLoadersMock.tsapps/web/test/utils/bookingScenario/bookingScenario.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:
tests/libs/__mocks__/videoLoadersMock.tstests/libs/__mocks__/mockDeepHelper.tstests/libs/__mocks__/calendarLoadersMock.tstests/libs/__mocks__/paymentLoadersMock.tsapps/web/test/utils/bookingScenario/bookingScenario.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:
tests/libs/__mocks__/videoLoadersMock.tstests/libs/__mocks__/mockDeepHelper.tstests/libs/__mocks__/calendarLoadersMock.tstests/libs/__mocks__/paymentLoadersMock.tsapps/web/test/utils/bookingScenario/bookingScenario.ts
🧬 Code graph analysis (4)
tests/libs/__mocks__/videoLoadersMock.ts (1)
tests/libs/__mocks__/mockDeepHelper.ts (1)
mockDeepHelper(3-10)
tests/libs/__mocks__/calendarLoadersMock.ts (1)
tests/libs/__mocks__/mockDeepHelper.ts (1)
mockDeepHelper(3-10)
tests/libs/__mocks__/paymentLoadersMock.ts (1)
tests/libs/__mocks__/mockDeepHelper.ts (1)
mockDeepHelper(3-10)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
packages/app-store/_utils/calendars/calendarAppsMetaData.ts (1)
calendarAppsMetaData(11-11)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
apps/web/test/utils/bookingScenario/bookingScenario.ts (5)
2-6: Switching to loader-specific mocks looks goodMoving from the monolithic
appStoreMockto targetedcalendarLoadersMock,videoLoadersMock, andpaymentLoadersMockimproves isolation and speeds up test setup. No issues here.
1723-1723: Type tightening on mockCalendar’s key is a good changeUsing
keyof typeof calendarAppsMetaDataensures compile-time alignment between metadata and mock usage.
1906-1917: Helper retype to calendar metadata keys looks goodThe API remains the same but is now safer against mismatched keys. No changes requested.
1920-1926: Crash helpers read well and mirror the success pathClear, minimal helpers to force failure paths in tests. Good to keep them symmetric with
mockCalendar.
17-17: calendarAppsMetaData contains the expected calendar keysI’ve verified that the auto-generated metadata includes all the required calendar entries, so
calendarAppsMetaDatawill be populated correctly and won’t causemockCalendarto crash:• In
packages/app-store/calendarApps.metadata.generated.ts, the exportedcalendarAppsMetadataobject contains
– applecalendar
– googlecalendar
– office365calendar
(alongside all other calendar keys)
as shown by the generated file’s mapping.
• The runtime loader (calendarAppsMetaData.ts) simply normalizes and re-exports each of these entries, so tests importingcalendarAppsMetaDatawill have all needed keys.No changes are necessary here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/lib/setupPaymentService.ts (1)
1-16: Undefined identifiers (app, credential), unsafe optional chaining, and ts-expect-error — replace with typed, guarded loader flowThe current implementation won’t compile/run: app and credential are not defined in scope; ts-expect-error hides a typing issue; optional chaining on the call site is unnecessary; isSetupAlready likely needs await. This also mirrors a previously raised concern about error suppression. Fix by accepting explicit inputs, guarding the loader/module shape, and removing console logging.
Apply this diff:
-import paymentLoaders, { type PaymentLoaderKey } from "@calcom/app-store/_utils/payments/paymentLoaders"; -import type { PaymentApp } from "@calcom/types/PaymentService"; - -export const setupPaymentService = async () => { - console.log("Checking payment setup for app", app.name); - // @ts-expect-error FIXME - const paymentApp = (await paymentLoaders[app.dirName as PaymentLoaderKey]?.()) as PaymentApp | null; - if (paymentApp && "lib" in paymentApp && paymentApp?.lib && "PaymentService" in paymentApp?.lib) { - const PaymentService = paymentApp.lib.PaymentService; - const paymentInstance = new PaymentService(credential); - - const isPaymentSetupAlready = paymentInstance.isSetupAlready(); - - return isPaymentSetupAlready; - } -}; +import paymentLoaders, { type PaymentLoaderKey } from "@calcom/app-store/_utils/payments/paymentLoaders"; + +type PaymentServiceCtor = new (credential: unknown) => { + isSetupAlready?: () => unknown | Promise<unknown>; +}; + +function hasPaymentService(mod: unknown): mod is { lib: { PaymentService: PaymentServiceCtor } } { + return ( + !!mod && + typeof mod === "object" && + "lib" in (mod as any) && + (mod as any).lib && + typeof (mod as any).lib === "object" && + "PaymentService" in (mod as any).lib && + typeof (mod as any).lib.PaymentService === "function" + ); +} + +export const setupPaymentService = async ({ + dirName, + credential, +}: { + dirName: string; + credential: unknown; +}): Promise<boolean | undefined> => { + const key = dirName as PaymentLoaderKey; + const loader = paymentLoaders[key]; + if (!loader) return undefined; + + const mod = await loader(); + if (!hasPaymentService(mod)) return undefined; + + try { + const PaymentService = mod.lib.PaymentService; + const instance = new PaymentService(credential); + const result = instance.isSetupAlready?.(); + const resolved = await Promise.resolve(result); + return typeof resolved === "boolean" ? resolved : undefined; + } catch { + // Swallow and return undefined; caller treats undefined as "not required/unknown" + return undefined; + } +};
- Removes console.log to avoid noisy logs on hot paths.
- Eliminates ts-expect-error by narrowing via a type guard.
- Replaces optional chaining call with an explicit loader existence check (fixes Biome noUnsafeOptionalChaining).
- Awaits isSetupAlready() and normalizes sync/async returns.
If you want, I can add unit tests covering:
- no loader found → undefined
- loader without PaymentService → undefined
- PaymentService.isSetupAlready sync/async → true/false
- constructor or method throws → undefined
#!/bin/bash # Sanity-check Biome warnings on this file after the refactor rg -n "ts-expect-error|\\?\\." packages/lib/setupPaymentService.ts -n -C0
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/lib/getConnectedApps.ts(2 hunks)packages/lib/setupPaymentService.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/setupPaymentService.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/setupPaymentService.tspackages/lib/getConnectedApps.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/setupPaymentService.tspackages/lib/getConnectedApps.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/setupPaymentService.tspackages/lib/getConnectedApps.ts
🧬 Code graph analysis (2)
packages/lib/setupPaymentService.ts (1)
packages/app-store/_utils/payments/paymentLoaders.ts (1)
PaymentLoaderKey(16-16)
packages/lib/getConnectedApps.ts (1)
packages/lib/setupPaymentService.ts (1)
setupPaymentService(4-16)
🪛 Biome (2.1.2)
packages/lib/setupPaymentService.ts
[error] 8-8: 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 180000ms. 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 (1)
packages/lib/getConnectedApps.ts (1)
16-16: Named import looks goodUsing a named export for setupPaymentService aligns with our tree-shaking guideline and avoids default exports.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/lib/setupPaymentService.ts (1)
16-18: Remove@ts-expect-errorand safely resolve the loaderAvoid suppressing type errors. Narrow the loader first and only call it if present. This removes the suppression and makes the control flow explicit.
Apply this diff:
- // @ts-expect-error FIXME - const paymentApp = (await paymentLoaders[app.dirName as PaymentLoaderKey]?.()) as PaymentApp | null; + const loader = paymentLoaders[app.dirName as PaymentLoaderKey]; + const paymentApp = loader ? ((await loader()) as PaymentApp | null) : null;
🧹 Nitpick comments (6)
packages/lib/setupPaymentService.ts (5)
9-15: Add explicit return type to the exported APIMake the return type explicit to prevent accidental widening in future edits and to improve DX.
-export const setupPaymentService = async ({ +export const setupPaymentService = async ({ app, credential, }: { app: Omit<EnabledApp, "credentials" | "key" | "credential">; credential: CredentialDataWithTeamName; -}) => { +}): Promise<boolean | undefined> => {
1-1: Prefer named exports over default exports for better tree-shakingIf feasible, refactor
@calcom/app-store/_utils/payments/paymentLoadersto exportpaymentLoadersas a named export and import it accordingly. This aligns with our guideline to avoid default exports for shared modules.Example:
// In paymentLoaders.ts export const paymentLoaders = { /* ... */ }; // Here import { paymentLoaders } from "@calcom/app-store/_utils/payments/paymentLoaders";
7-8: Optional: Introduce a small type guard to encapsulate shape checksEncapsulating the runtime checks into a reusable guard will reduce repetition and silence lints without resorting to
any.Proposed addition (place near the top of the file):
+type PaymentServiceCtor = new (c: CredentialDataWithTeamName) => { + isSetupAlready(): boolean | Promise<boolean>; +}; + +function hasPaymentService(x: unknown): x is { lib: { PaymentService: PaymentServiceCtor } } { + if (!x || typeof x !== "object") return false; + const lib = (x as any).lib; + return lib && typeof lib === "object" && typeof lib.PaymentService === "function"; +}Then the main check becomes:
if (paymentApp && hasPaymentService(paymentApp)) { /* ... */ }I can wire this in for you if you prefer.
22-24: Consistency: await the service callEven if
isSetupAlready()can be sync, awaiting it makes the return consistentlyPromise<boolean | undefined>and prevents leaking a rejected promise upwards.- const isPaymentSetupAlready = paymentInstance.isSetupAlready(); + const isPaymentSetupAlready = await paymentInstance.isSetupAlready();
9-26: Add tests for loader missing, invalid module shape, and happy-pathThis module is a new seam. Please add unit tests that cover:
- No loader for
app.dirName→ returnsundefined- Loader resolves to object without
lib.PaymentService→ returnsundefined- Happy path:
PaymentService#isSetupAlready()returns boolean and promise variantsI can generate Jest/Vitest tests using your existing
tests/libs/__mocks__/paymentLoadersMock.ts. Want me to draft them?packages/lib/getConnectedApps.ts (1)
183-187: GuardsetupPaymentServiceagainst failures in getConnectedApps.tsThe only invocation of
setupPaymentServiceis inpackages/lib/getConnectedApps.tsat line 186. Wrapping this call in atry/catchensures that if the dynamic import or service initialization fails, it degrades gracefully without preventing other apps from rendering.• File:
packages/lib/getConnectedApps.ts
Line: 186Suggested diff:
let isSetupAlready = undefined; if (credential && app.categories.includes("payment")) { - isSetupAlready = await setupPaymentService({ app, credential }); + try { + isSetupAlready = await setupPaymentService({ app, credential }); + } catch (err) { + // Optionally add debug logging here + isSetupAlready = undefined; + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/lib/getConnectedApps.ts(2 hunks)packages/lib/setupPaymentService.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/getConnectedApps.tspackages/lib/setupPaymentService.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/getConnectedApps.tspackages/lib/setupPaymentService.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/getConnectedApps.tspackages/lib/setupPaymentService.ts
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/setupPaymentService.ts
🧬 Code graph analysis (1)
packages/lib/setupPaymentService.ts (1)
packages/app-store/_utils/payments/paymentLoaders.ts (1)
PaymentLoaderKey(16-16)
🪛 Biome (2.1.2)
packages/lib/setupPaymentService.ts
[error] 18-18: 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 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
packages/lib/getConnectedApps.ts (2)
16-16: LGTM: Scoped import ofsetupPaymentServiceImporting only the new helper keeps this module lean and aligns with the refactor toward loaders.
149-156: PII check: credential.key not leakedSanitization by omitting
credentials,credential, andkeyfromappbefore returning is preserved. Good.
| if (paymentApp && "lib" in paymentApp && paymentApp?.lib && "PaymentService" in paymentApp?.lib) { | ||
| const PaymentService = paymentApp.lib.PaymentService; | ||
| const paymentInstance = new PaymentService(credential); | ||
|
|
||
| const isPaymentSetupAlready = paymentInstance.isSetupAlready(); | ||
|
|
||
| return isPaymentSetupAlready; | ||
| } |
There was a problem hiding this comment.
Fix unsafe optional chaining with in operator and strengthen runtime guards
Biome is right: using optional chaining with in is unsafe. Also, check that PaymentService is a constructor and await isSetupAlready() to normalize return type and avoid unhandled rejections.
Apply this diff:
- if (paymentApp && "lib" in paymentApp && paymentApp?.lib && "PaymentService" in paymentApp?.lib) {
- const PaymentService = paymentApp.lib.PaymentService;
- const paymentInstance = new PaymentService(credential);
-
- const isPaymentSetupAlready = paymentInstance.isSetupAlready();
-
- return isPaymentSetupAlready;
- }
+ if (
+ paymentApp &&
+ typeof paymentApp === "object" &&
+ "lib" in paymentApp &&
+ paymentApp.lib &&
+ typeof (paymentApp as any).lib === "object" &&
+ "PaymentService" in (paymentApp as any).lib &&
+ typeof (paymentApp as any).lib.PaymentService === "function"
+ ) {
+ const PaymentService = (paymentApp as any).lib.PaymentService as new (
+ credential: CredentialDataWithTeamName
+ ) => { isSetupAlready(): boolean | Promise<boolean> };
+ const paymentInstance = new PaymentService(credential);
+ const isPaymentSetupAlready = await paymentInstance.isSetupAlready();
+ return isPaymentSetupAlready;
+ }📝 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 (paymentApp && "lib" in paymentApp && paymentApp?.lib && "PaymentService" in paymentApp?.lib) { | |
| const PaymentService = paymentApp.lib.PaymentService; | |
| const paymentInstance = new PaymentService(credential); | |
| const isPaymentSetupAlready = paymentInstance.isSetupAlready(); | |
| return isPaymentSetupAlready; | |
| } | |
| if ( | |
| paymentApp && | |
| typeof paymentApp === "object" && | |
| "lib" in paymentApp && | |
| paymentApp.lib && | |
| typeof (paymentApp as any).lib === "object" && | |
| "PaymentService" in (paymentApp as any).lib && | |
| typeof (paymentApp as any).lib.PaymentService === "function" | |
| ) { | |
| const PaymentService = (paymentApp as any).lib.PaymentService as new ( | |
| credential: CredentialDataWithTeamName | |
| ) => { isSetupAlready(): boolean | Promise<boolean> }; | |
| const paymentInstance = new PaymentService(credential); | |
| const isPaymentSetupAlready = await paymentInstance.isSetupAlready(); | |
| return isPaymentSetupAlready; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 18-18: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🤖 Prompt for AI Agents
In packages/lib/setupPaymentService.ts around lines 18–25, avoid using optional
chaining with the `in` operator and add stronger runtime guards: explicitly
check that paymentApp is an object, that it has a `lib` property which is an
object, that `PaymentService` exists on paymentApp.lib and is a constructor
(typeof === "function"); instantiate with `new PaymentService(credential)` only
after those checks, verify that the resulting instance has an `isSetupAlready`
method (typeof === "function"), `await` the call to `isSetupAlready()` to
normalize async behavior and avoid unhandled rejections, coerce the result to a
boolean before returning, and return a safe default (e.g., false) if any guard
fails.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/lib/setupPaymentService.ts (1)
14-24: Fix unsafe optional chaining within, removets-expect-error, add runtime guards, and await the resultBiome is correct: using optional chaining on the RHS of
inis unsafe and can throw. Also, returning the raw promise fromisSetupAlready()defers errors and complicates consumers. Guard shapes, await, and normalize to boolean.- // @ts-expect-error FIXME - const paymentApp = (await paymentLoaders[app.dirName as PaymentLoaderKey]?.()) as PaymentApp | null; - if (paymentApp && "lib" in paymentApp && paymentApp?.lib && "PaymentService" in paymentApp?.lib) { - const PaymentService = paymentApp.lib.PaymentService; - const paymentInstance = new PaymentService(credential); - - const isPaymentSetupAlready = paymentInstance.isSetupAlready(); - - return isPaymentSetupAlready; - } + const loadPaymentApp = paymentLoaders[app.dirName as PaymentLoaderKey]; + if (typeof loadPaymentApp !== "function") return undefined; + + let paymentApp: unknown; + try { + paymentApp = await loadPaymentApp(); + } catch { + // Treat loader failures as "unknown" + return undefined; + } + + if (!paymentApp || typeof paymentApp !== "object") return undefined; + const lib = (paymentApp as any).lib; + if (!lib || typeof lib !== "object") return undefined; + + const PaymentService = (lib as any).PaymentService as + | (new (credential: CredentialDataWithTeamName) => { isSetupAlready(): boolean | Promise<boolean> }) + | undefined; + if (typeof PaymentService !== "function") return undefined; + + const paymentInstance = new PaymentService(credential); + if (typeof (paymentInstance as any)?.isSetupAlready !== "function") return undefined; + + const isPaymentSetupAlready = await paymentInstance.isSetupAlready(); + return Boolean(isPaymentSetupAlready);packages/app-store/_utils/prepareAppsWithCredentials.ts (2)
75-86: Redact secret credential.key before returning to clientsBoth
credentialandcredentialsare returned with rawkeyvalues. This violates the guideline to never expose credential.key via tRPC/API and is a high‑risk secret leak for the /apps/installed/* pages. Sanitize before pushing toreducedArray.Apply this diff within the function to sanitize the outgoing payload:
- const credential: (typeof appCredentials)[number] | null = appCredentials[0] || null; - - reducedArray.push({ + // Sanitize credentials before returning them across server boundaries + const safeCredentials = appCredentials.map((c) => getPiiFreeCredential(c)); + const safeCredential = safeCredentials[0] ?? null; + + reducedArray.push({ ...appMeta, /** * @deprecated use `credentials` */ - credential, - credentials: appCredentials, + credential: safeCredential, + credentials: safeCredentials, /** Option to display in `location` field while editing event types */ locationOption, });Optionally, if you prefer to drop the
keyfield entirely rather than return a boolean presence flag, replace the map with:const safeCredentials = appCredentials.map(({ key: _omit, ...rest }) => rest);Verification script to ensure no raw
credential.keycrosses API boundaries:#!/bin/bash # 1) Find all places that return app-store data via TRPC/HTTP rg -nP -C3 --type=ts '(createTRPCRouter|publicProcedure|privateProcedure|router\(|NextResponse\.json|res\.json)\b' # 2) Ensure none of these responses include credential.key rg -nP -C2 --type=ts '\bcredential\.key\b|credentials?:\s*\[?[^{\]]*key' # 3) Confirm sanitize happens at all app helpers rg -nP -C3 --type=ts 'prepareAppsWithCredentials\(|get(Calendar|Video)Apps\('
22-26: Fix type soundness: credential can be null and (optionally) use a sanitized credential typeRuntime can push
credential = null, butPreparedApprequires non-null. This masking viaas PreparedApp[]can cause downstream NPEs. Also, once you sanitize, thekeytype no longer matches the source type.Apply this diff to make types accurate and future‑proof:
-export type PreparedApp = App & { - credential: CredentialDataWithTeamName; - credentials: CredentialDataWithTeamName[]; - locationOption: LocationOption | null; -}; +// Public shape returned to clients; key is sanitized by getPiiFreeCredential (boolean) or omitted. +type PublicCredential = ReturnType<typeof getPiiFreeCredential> & { team?: { name: string } | null }; + +export type PreparedApp = App & { + credential: PublicCredential | null; + credentials: PublicCredential[]; + locationOption: LocationOption | null; +};Additionally, make the function’s return type explicit (see next comment).
🧹 Nitpick comments (6)
packages/lib/setupPaymentService.ts (3)
1-2: Prefer named export over default import for better tree-shaking and refactorsPer repo guidelines, avoid default exports in libraries. If feasible, switch
paymentLoadersto a named export in its module and update this import.-import paymentLoaders from "@calcom/app-store/_utils/payments/paymentLoaders"; +import { paymentLoaders } from "@calcom/app-store/_utils/payments/paymentLoaders";Note: This requires changing the exporter in
paymentLoaders.ts.
7-13: Annotate an explicit return type to document contractCallers likely rely on “boolean when available, otherwise undefined.” Make that explicit.
export const setupPaymentService = async ({ app, credential, }: { app: Omit<PreparedApp, "credentials" | "key" | "credential">; credential: CredentialDataWithTeamName; -}) => { +}): Promise<boolean | undefined> => {Please confirm all callers handle
undefineddistinctly fromfalse.
7-24: Add targeted tests for loader and service pathsCover both sync and async
isSetupAlready()implementations, missing loader, missing/invalidPaymentService, and loader throwing.Recommended cases:
- returns true (boolean)
- returns Promise
- loader missing for
app.dirName-> undefined- loader throws -> undefined
libpresent butPaymentServicemissing -> undefinedI can draft Jest tests using the existing paymentLoaders mock. Want me to push a test file?
packages/app-store/_utils/prepareAppsWithCredentials.ts (3)
28-33: Add explicit return type and pre-index credentials for better scalabilitySpecify the return type to catch regressions at compile time. Also, filtering
credentialsfor each app is O(Napps×Ncreds). Pre-index byappIdto reduce to O(Napps + Ncreds). This helps the page-load objective when apps/credentials grow.Apply:
-export const prepareAppsWithCredentials = ( +export const prepareAppsWithCredentials = ( apps: App[], credentials: CredentialDataWithTeamName[], filterOnCredentials?: boolean -) => { - return apps.reduce((reducedArray, appMeta) => { - const appCredentials = credentials.filter((credential) => credential.appId === appMeta.slug); +) : PreparedApp[] => { + // Group credentials by appId for O(1) lookup per app + const credsByAppId = credentials.reduce<Map<string, CredentialDataWithTeamName[]>>((map, c) => { + const list = map.get(c.appId); + if (list) list.push(c); + else map.set(c.appId, [c]); + return map; + }, new Map()); + + return apps.reduce<PreparedApp[]>((reducedArray, appMeta) => { + const appCredentials = [...(credsByAppId.get(appMeta.slug) ?? [])];
40-64: Guard missing global key and avoid duplicate injection; grammar nit
- If
appMeta.keyis absent, current!non-null assertion is unsafe.- Prevent pushing multiple synthetic credentials when this function is called repeatedly.
- Minor: “it’s key” → “its key”.
Apply:
- /** If the app is a globally installed one, let's inject it's key */ - if (appMeta.isGlobal) { + /** If the app is a globally installed one, inject its key once */ + if ( + appMeta.isGlobal && + !appCredentials.some((c) => c.userId === 0 && c.appId === appMeta.slug) + ) { + if (!appMeta.key) { + logger.warn( + `${appMeta.type} marked as global but has no key; skipping credential injection for ${appMeta.slug}` + ); + } else { const credential = { id: 0, type: appMeta.type, - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - key: appMeta.key!, + key: appMeta.key, userId: 0, user: { email: "" }, teamId: null, appId: appMeta.slug, invalid: false, delegatedTo: null, delegatedToId: null, delegationCredentialId: null, team: { name: "Global", }, }; logger.debug( `${appMeta.type} is a global app, injecting credential`, safeStringify(getPiiFreeCredential(credential)) ); appCredentials.push(credential); - } + } + }
88-90: Remove unsafe cast and rely on accurate typesAfter adding an explicit return type and updating
PreparedApp, thisas PreparedApp[]cast becomes unnecessary. Remove it to let the compiler enforce correctness.Apply:
- }, [] as PreparedApp[]); + }, []);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/app-store/_utils/prepareAppsWithCredentials.ts(1 hunks)packages/lib/setupPaymentService.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/app-store/_utils/prepareAppsWithCredentials.tspackages/lib/setupPaymentService.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/_utils/prepareAppsWithCredentials.tspackages/lib/setupPaymentService.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/_utils/prepareAppsWithCredentials.tspackages/lib/setupPaymentService.ts
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/setupPaymentService.ts
🧠 Learnings (2)
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : Ensure the `credential.key` field is never returned from tRPC endpoints or APIs
Applied to files:
packages/app-store/_utils/prepareAppsWithCredentials.ts
📚 Learning: 2025-08-08T09:12:08.280Z
Learnt from: hariombalhara
PR: calcom/cal.com#22968
File: packages/features/auth/lib/next-auth-options.ts:327-327
Timestamp: 2025-08-08T09:12:08.280Z
Learning: In packages/features/auth/lib/next-auth-options.ts, do not log credentials in authorize() handlers (e.g., the "saml-idp" CredentialsProvider). Remove accidental console.log statements and avoid including credential contents in logs; prefer either no logging or structured logs without sensitive data.
Applied to files:
packages/app-store/_utils/prepareAppsWithCredentials.ts
🧬 Code graph analysis (2)
packages/app-store/_utils/prepareAppsWithCredentials.ts (2)
packages/app-store/locations.ts (1)
EventLocationType(63-63)packages/lib/piiFreeData.ts (1)
getPiiFreeCredential(61-72)
packages/lib/setupPaymentService.ts (2)
packages/app-store/_utils/prepareAppsWithCredentials.ts (2)
PreparedApp(22-26)CredentialDataWithTeamName(16-20)packages/app-store/_utils/payments/paymentLoaders.ts (1)
PaymentLoaderKey(16-16)
🪛 Biome (2.1.2)
packages/lib/setupPaymentService.ts
[error] 16-16: 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 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
packages/app-store/_utils/prepareAppsWithCredentials.ts (4)
66-73: Location option derivation LGTMDerives
locationOptiononly when the app has credentials (including globals). This keeps UI options relevant. No change requested.
1-3: Import usage LGTM
getPiiFreeCredentialandsafeStringifyare used appropriately for logging; once you apply the sanitization patch,getPiiFreeCredentialwill also serve the payload.
9-14: Type definition for LocationOption LGTMConcise and matches usage; no unnecessary fields.
16-20: CredentialDataWithTeamName shape LGTM as an internal/source typeThis remains useful as the input to the function. After switching the public return type to a sanitized form, keep this type for internal filtering and injection logic.
|
Thanks for reviewing this! Although with the current release I was able to get the compile time down to 7.3s on my machine, I think splitting the heavy service is a better and clearer direction. If we can continue generating the video and payment service maps, the results should be even better. Closing this PR in favor of that approach. |
2 participants
What does this PR do?
Before
○ Compiling /apps/installed/[category] ... ✓ Compiled /apps/installed/[category] in 13.6sAfter
○ Compiling /apps/installed/[category] ... ✓ Compiled /apps/installed/[category] in 5.4sRelated to #23104
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist