refactor: circular deps between app store and lib [2]#23734
Conversation
WalkthroughThis change redirects many imports from package aliases to local/app-store paths, moves several utilities from @calcom/lib into packages/app-store/_utils, replaces TRPC-derived user types with explicit internal User types (including Prisma.JsonValue for metadata), switches some error handling to throw Error (with TRPC handlers mapping them back to TRPCError), updates tests for a required email field, re-exports platform library symbols from new locations, and bumps @calcom/platform-libraries to 0.0.351. Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ 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. Comment |
hbjORbj
changed the title
dosubot
bot
added
app-store
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/server/getUsersCredentials.ts (1)
33-36: Strip secret fields in safe helper and use it in API handlers
- Update packages/lib/server/getUsersCredentials.ts to drop both
keyandserviceAccountKey:- return credentials.map(({ delegatedTo: _1, ...rest }) => rest); + return credentials.map(({ delegatedTo: _1, key: _2, serviceAccountKey: _3, ...rest }) => rest);- Replace calls to
getUsersCredentialsIncludeServiceAccountKeyin any tRPC/public endpoint that returns credentials with the safegetUsersCredentials, or explicitly omit.key/.serviceAccountKeybefore sending responses.
🧹 Nitpick comments (4)
packages/app-store/_utils/getDefaultLocations.test.ts (1)
7-7: Unify import source for location constants
Implementation files importDailyLocationType/MeetLocationTypefrom../constantswhile tests pull them from../locations. Consolidate all imports to a single module (eitherconstantsorlocations) to prevent split definitions.packages/app-store/_utils/getDefaultLocations.ts (3)
2-9: Localizing imports is good; unify location constants source.Consider standardizing on a single module (constants or locations) to avoid divergence between app and tests.
11-14: Type of metadata can be relaxed to unknown (parsed by Zod).Using unknown better reflects that raw metadata is untrusted until parsed.
type User = { id: number; email: string; - metadata: Prisma.JsonValue; + metadata: unknown; };
23-27: Use .find instead of .filter()[0] for readability and perf.Equivalent result without intermediate array.
- const foundApp = getApps(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 = getApps(credentials, true).find( + (app) => app.slug === defaultConferencingData.appSlug + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/app-store/_utils/getDefaultLocations.test.ts(3 hunks)packages/app-store/_utils/getDefaultLocations.ts(1 hunks)packages/lib/server/getUsersCredentials.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/app-store/_utils/getDefaultLocations.test.tspackages/lib/server/getUsersCredentials.tspackages/app-store/_utils/getDefaultLocations.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/getDefaultLocations.test.tspackages/lib/server/getUsersCredentials.tspackages/app-store/_utils/getDefaultLocations.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/getDefaultLocations.test.tspackages/lib/server/getUsersCredentials.tspackages/app-store/_utils/getDefaultLocations.ts
🧠 Learnings (2)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/app-store/_utils/getDefaultLocations.test.ts
📚 Learning: 2025-09-01T08:56:14.071Z
Learnt from: nangelina
PR: calcom/cal.com#23486
File: packages/app-store/kyzon-space/lib/tokenManager.ts:25-31
Timestamp: 2025-09-01T08:56:14.071Z
Learning: In token refresh utilities like tokenManager.ts, accessing credential.key from Prisma is legitimate and necessary for OAuth token refresh flows. These internal utilities need stored credentials to refresh tokens and don't expose them in API responses.
Applied to files:
packages/lib/server/getUsersCredentials.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (5)
packages/lib/server/getUsersCredentials.ts (2)
6-6: Good decoupling from TRPC types.Explicit User keeps this module independent of TRPC session typings.
16-20: credentialForCalendarServiceSelect excludes thekeyfield
The select only includes safe fields (id,appId,type, etc.) and does not includekey.packages/app-store/_utils/getDefaultLocations.test.ts (3)
12-12: Making email required in test User is aligned with runtime needs.Good update; avoids undefined email paths.
36-36: LGTM: test user now includes email.
56-56: LGTM: test user now includes email (default app unset case).
| @@ -21,19 +20,13 @@ export const bulkUpdateTeamEventsToDefaultLocation = async ({ | |||
| const defaultApp = teamMetadataSchema.parse(team?.metadata)?.defaultConferencingApp; | |||
|
|
|||
| if (!defaultApp) { | |||
| throw new TRPCError({ | |||
There was a problem hiding this comment.
An app store utility function shouldn't be throwing a TRPC error
| @@ -18,19 +17,13 @@ export const bulkUpdateEventsToDefaultLocation = async ({ | |||
| const defaultApp = userMetadataSchema.parse(user.metadata)?.defaultConferencingApp; | |||
|
|
|||
| if (!defaultApp) { | |||
| throw new TRPCError({ | |||
There was a problem hiding this comment.
An app store utility function shouldn't be throwing a TRPC error
| import { getDefaultLocations } from "./getDefaultLocations"; | ||
|
|
||
| type User = { | ||
| id: number; | ||
| email?: string; |
There was a problem hiding this comment.
Shouldn't be optional since SessionUser["email"] was a string (not string | undefined)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app-store/_utils/getConnectedApps.ts (1)
138-138: UnifybuildNonDelegationCredentialsimports
Replace every import ofbuildNonDelegationCredentialsfrom…/delegationCredential/clientAndServerwith the new path@calcom/lib/delegationCredential/serveracross the codebase.
♻️ Duplicate comments (2)
packages/app-store/_utils/bulkUpdateEventsToDefaultLocation.ts (1)
20-20: Good: use plain Error in util (no TRPCError).
This aligns with prior feedback that utils shouldn’t depend on tRPC.packages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.ts (1)
23-23: Good: switched to plain Error in util.
Removes tRPC coupling.
🧹 Nitpick comments (2)
packages/app-store/_utils/bulkUpdateEventsToDefaultLocation.ts (1)
26-26: Nit: fix typo in error message.- throw new Error(`Default conferencing app '${defaultApp.appSlug}' doesnt exist.`); + throw new Error(`Default conferencing app '${defaultApp.appSlug}' doesn't exist.`);packages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.ts (1)
29-29: Nit: fix apostrophe in error message.- throw new Error(`Default conferencing app '${defaultApp.appSlug}' doesnt exist.`); + throw new Error(`Default conferencing app '${defaultApp.appSlug}' doesn't exist.`);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
packages/app-store/_utils/bulkUpdateEventsToDefaultLocation.ts(2 hunks)packages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.ts(2 hunks)packages/app-store/_utils/getConnectedApps.ts(1 hunks)packages/app-store/types.d.ts(1 hunks)packages/platform/atoms/connect/conferencing-apps/hooks/useAtomsGetInstalledConferencingApps.ts(1 hunks)packages/platform/libraries/app-store.ts(1 hunks)packages/platform/libraries/event-types.ts(1 hunks)packages/trpc/server/routers/viewer/apps/integrations.handler.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.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/trpc/server/routers/viewer/apps/integrations.handler.tspackages/platform/libraries/event-types.tspackages/platform/atoms/connect/conferencing-apps/hooks/useAtomsGetInstalledConferencingApps.tspackages/platform/libraries/app-store.tspackages/app-store/_utils/bulkUpdateEventsToDefaultLocation.tspackages/app-store/types.d.tspackages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.tspackages/app-store/_utils/getConnectedApps.tspackages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.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/trpc/server/routers/viewer/apps/integrations.handler.tspackages/platform/libraries/event-types.tspackages/platform/atoms/connect/conferencing-apps/hooks/useAtomsGetInstalledConferencingApps.tspackages/platform/libraries/app-store.tspackages/app-store/_utils/bulkUpdateEventsToDefaultLocation.tspackages/app-store/types.d.tspackages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.tspackages/app-store/_utils/getConnectedApps.tspackages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/trpc/server/routers/viewer/apps/integrations.handler.tspackages/platform/libraries/event-types.tspackages/platform/atoms/connect/conferencing-apps/hooks/useAtomsGetInstalledConferencingApps.tspackages/platform/libraries/app-store.tspackages/app-store/_utils/bulkUpdateEventsToDefaultLocation.tspackages/app-store/types.d.tspackages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.tspackages/app-store/_utils/getConnectedApps.tspackages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.ts
🧠 Learnings (1)
📚 Learning: 2025-09-01T07:31:00.963Z
Learnt from: nangelina
PR: calcom/cal.com#23486
File: packages/app-store/kyzon-space/config.json:5-5
Timestamp: 2025-09-01T07:31:00.963Z
Learning: In Cal.com's video integration system, credential types (e.g., "kyzon-space_video") are transformed to app names by removing underscores using `cred.type.split("_").join("")` in videoClient.ts line 28. This means the key in packages/app-store/index.ts should match the underscore-removed version (e.g., "kyzon-spacevideo") rather than the original type name. This same pattern is used in other parts of the system like getCalendar.ts.
Applied to files:
packages/platform/libraries/app-store.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.ts (1)
packages/app-store/_utils/bulkUpdateEventsToDefaultLocation.ts (1)
bulkUpdateEventsToDefaultLocation(8-52)
⏰ 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 (11)
packages/app-store/_utils/bulkUpdateEventsToDefaultLocation.ts (1)
5-6: Imports moved to local utils resolve circular deps — LGTM.packages/platform/atoms/connect/conferencing-apps/hooks/useAtomsGetInstalledConferencingApps.ts (1)
4-4: Type-only import path switch looks correct.
No runtime impact; keeps hook lean.packages/trpc/server/routers/viewer/apps/integrations.handler.ts (1)
1-1: Import path updated to new app-store utils — looks good.packages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.ts (2)
1-1: Import path updated to new util — OK.
4-4: tRPCError import addition — OK.packages/platform/libraries/app-store.ts (2)
23-23: Re-export getConnectedApps from new source — LGTM.
27-27: Re-export ConnectedApps type from new source — LGTM.packages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.ts (1)
4-5: Localizing imports to resolve circular deps — LGTM.packages/platform/libraries/event-types.ts (1)
30-31: Re-exports updated to app-store utils — looks correct and consistent.packages/app-store/_utils/getConnectedApps.ts (2)
6-6: Import moved correctly from local to library path.The import has been properly moved from a local relative path to the library path to avoid circular dependencies.
13-16: Import paths correctly converted to relative paths.The imports have been properly updated from app-store scoped modules to relative paths, which helps eliminate circular dependencies between app store and lib packages.
| @@ -1,7 +1,7 @@ | |||
| import type { ConnectedApps } from "_utils/getConnectedApps"; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential module resolution issue: use a relative path for intra-package type import.
Bare "_utils/..." may fail without a matching TS path alias at publish/consume time. Prefer a relative import.
-import type { ConnectedApps } from "_utils/getConnectedApps";
+import type { ConnectedApps } from "./_utils/getConnectedApps";To verify, check whether packages/app-store/tsconfig.json sets baseUrl/paths for "_utils/*"; if not, switch to relative.
🏁 Script executed:
#!/bin/bash
# Inspect TS config for app-store package
fd tsconfig.json packages/app-store | xargs -I{} sh -c 'echo "== {} =="; cat "{}" | jq "{baseUrl: .compilerOptions.baseUrl, paths: .compilerOptions.paths}"'Length of output: 95
🏁 Script executed:
#!/bin/bash
file="packages/app-store/tsconfig.json"
echo "== Inspecting $file =="
# Check for baseUrl
grep -n '"baseUrl"' "$file" || echo "baseUrl not found"
# Check for paths block
grep -nC3 '"paths"' "$file" || echo "paths not found"
# Check for _utils alias specifically
grep -nC2 '"_utils/*"' "$file" || echo "no '_utils/*' alias defined"Length of output: 397
Use a relative import for ConnectedApps in types.d.ts
packages/app-store/tsconfig.json defines no _utils/* path alias, so
import type { ConnectedApps } from "_utils/getConnectedApps";will fail resolution. Change to:
-import type { ConnectedApps } from "_utils/getConnectedApps";
+import type { ConnectedApps } from "./_utils/getConnectedApps";🤖 Prompt for AI Agents
packages/app-store/types.d.ts lines 1-1: the import uses the undefined path
alias "_utils" so TypeScript can't resolve it; update the import to a proper
relative path to the getConnectedApps module (e.g., adjust to the correct
relative path from this file such as ./utils/getConnectedApps or
../utils/getConnectedApps depending on the actual file location) so the import
resolves without relying on a non-existent path alias.
| try { | ||
| return bulkUpdateEventsToDefaultLocation({ | ||
| eventTypeIds, | ||
| user: ctx.user, | ||
| prisma, | ||
| }); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: error.message, | ||
| }); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Bug: try/catch won’t catch async rejection (missing await).
The handler isn’t async; returning the Promise bypasses the catch. Make the handler async and await the call.
-export const bulkUpdateToDefaultLocationHandler = ({ ctx, input }: BulkUpdateToDefaultLocationOptions) => {
+export const bulkUpdateToDefaultLocationHandler = async ({ ctx, input }: BulkUpdateToDefaultLocationOptions) => {
const { eventTypeIds } = input;
try {
- return bulkUpdateEventsToDefaultLocation({
+ return await bulkUpdateEventsToDefaultLocation({
eventTypeIds,
user: ctx.user,
prisma,
});
} catch (error) {
if (error instanceof Error) {
throw new TRPCError({
code: "BAD_REQUEST",
message: error.message,
});
}
throw error;
}
}📝 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.
| try { | |
| return bulkUpdateEventsToDefaultLocation({ | |
| eventTypeIds, | |
| user: ctx.user, | |
| prisma, | |
| }); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: error.message, | |
| }); | |
| } | |
| throw error; | |
| } | |
| export const bulkUpdateToDefaultLocationHandler = async ({ ctx, input }: BulkUpdateToDefaultLocationOptions) => { | |
| const { eventTypeIds } = input; | |
| try { | |
| return await bulkUpdateEventsToDefaultLocation({ | |
| eventTypeIds, | |
| user: ctx.user, | |
| prisma, | |
| }); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: error.message, | |
| }); | |
| } | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
packages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.ts
around lines 18 to 32, the try/catch won't catch Promise rejections because the
handler is not async and the call is returned without awaiting; make the handler
function async and await the call to bulkUpdateEventsToDefaultLocation inside
the try block so thrown/rejected errors are caught, then rethrow as the
TRPCError as currently implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (1)
61-64: Bug: user-connection condition uses schedulingType instead of teamIdThis makes the expression almost always truthy, so the creator won’t be connected to non-team, non-managed event types.
Apply this diff:
- // Only connecting the current user for non-managed event types and non team event types - users: isManagedEventType || schedulingType ? undefined : { connect: { id: userId } }, + // Only connect the current user for non-managed, non-team event types + users: isManagedEventType || teamId ? undefined : { connect: { id: userId } },packages/app-store/paypal/api/webhook.ts (2)
47-51: Ineffective credentials null-check; can lead to using empty creds.findPaymentCredentials always returns an object (even on error), so this check never trips; you may proceed with empty clientId/secretKey/webhookId and get opaque failures. Validate fields explicitly.
Apply this diff:
- const foundCredentials = await findPaymentCredentials(booking.id); - if (!foundCredentials) throw new HttpCode({ statusCode: 204, message: "No credentials found" }); - const { webhookId, ...credentials } = foundCredentials; - - const paypalClient = new Paypal(credentials); + const creds = await findPaymentCredentials(booking.id); + if (!creds?.clientId || !creds?.secretKey || !creds?.webhookId) { + throw new HttpCode({ statusCode: 204, message: "No credentials found" }); + } + const { webhookId, clientId, secretKey } = creds; + const paypalClient = new Paypal({ clientId, secretKey });
153-200: Returnnullfrom findPaymentCredentials on error instead of empty strings.Empty strings mask the “no creds” condition and cause downstream failures.
Apply this diff:
-export const findPaymentCredentials = async ( - bookingId: number -): Promise<{ clientId: string; secretKey: string; webhookId: string }> => { +export const findPaymentCredentials = async ( + bookingId: number +): Promise<{ clientId: string; secretKey: string; webhookId: string } | null> => { @@ - } catch (err) { - console.error(err); - return { - clientId: "", - secretKey: "", - webhookId: "", - }; - } + } catch (err) { + console.error(err); + return null; + }packages/app-store/alby/api/webhook.ts (1)
49-71: Prisma: select only needed fields and limit relation.Load just what’s used and avoid fetching full Credential rows. This also aligns with our “select-only” guideline.
Apply this diff:
const payment = await prisma.payment.findFirst({ where: { uid: parsedPayload.metadata.payer_data.referenceId, }, select: { id: true, amount: true, bookingId: true, booking: { select: { user: { select: { - credentials: { - where: { - type: "alby_payment", - }, - }, + credentials: { + where: { type: "alby_payment" }, + select: { key: true }, + take: 1, + }, }, }, }, }, }, });packages/features/ee/payments/api/webhook.ts (1)
56-60: Prisma: restrict fields to only what’s used.Select id, bookingId, and data instead of fetching the full row.
Apply this diff:
const payment = await prisma.payment.findFirst({ where: { externalId: setupIntent.id, }, - }); + select: { + id: true, + bookingId: true, + data: true, + }, + });packages/app-store/btcpayserver/api/webhook.ts (1)
90-92: Unreachable success response after handlePaymentSuccess.handlePaymentSuccess throws HttpCode(200); the
res.status(200).json({ success: true })won’t execute.Apply this diff:
- await handlePaymentSuccess(payment.id, payment.bookingId); - return res.status(200).json({ success: true }); + await handlePaymentSuccess(payment.id, payment.bookingId); + return;
🧹 Nitpick comments (5)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (1)
83-90: Prisma: select only needed fields in membership lookupAvoid fetching full rows; only role is used.
Apply this diff:
const hasMembership = await ctx.prisma.membership.findFirst({ where: { - userId, - teamId: teamId, - accepted: true, + userId, + teamId: teamId, + accepted: true, }, + select: { role: true }, });packages/app-store/paypal/api/webhook.ts (1)
157-157: Open TODO: team bookings with PayPal.If team bookings can be paid via PayPal, user-scoped credential lookup will fail. Consider team/org scoping with fallback.
I can draft a team-aware lookup (owner → team → user fallback) if helpful.
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
103-107: Consider returning a result instead of throwing HttpCode(200).Throwing for the success path complicates control flow and produces unreachable code in some handlers. A return value lets API layers decide the response.
Example:
- throw new HttpCode({ - statusCode: 200, - message: `Booking with id '${booking.id}' was paid and confirmed.`, - }); + return { bookingId: booking.id, confirmed: isConfirmed };Follow-up would require updating callers to send the response explicitly.
packages/features/ee/payments/api/webhook.ts (1)
45-50: Duplicate “payment not found” check.Second guard is redundant.
Apply this diff:
if (!payment?.bookingId) { log.error("Stripe: Payment Not Found", safeStringify(paymentIntent), safeStringify(payment)); throw new HttpCode({ statusCode: 204, message: "Payment not found" }); } - if (!payment?.bookingId) throw new HttpCode({ statusCode: 204, message: "Payment not found" });packages/app-store/btcpayserver/api/webhook.ts (1)
49-51: Header casing nit.Node lowercases header names; checking "BTCPay-Sig" is unnecessary (harmless).
You can simplify to
const signature = req.headers["btcpay-sig"];.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/app-store/_utils/payments/handlePaymentSuccess.ts(1 hunks)packages/app-store/alby/api/webhook.ts(1 hunks)packages/app-store/btcpayserver/api/webhook.ts(1 hunks)packages/app-store/hitpay/api/webhook.ts(1 hunks)packages/app-store/paypal/api/webhook.ts(1 hunks)packages/features/ee/payments/api/webhook.ts(1 hunks)packages/platform/libraries/package.json(1 hunks)packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/platform/libraries/package.json
- packages/app-store/hitpay/api/webhook.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:
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.tspackages/app-store/paypal/api/webhook.tspackages/app-store/_utils/payments/handlePaymentSuccess.tspackages/app-store/alby/api/webhook.tspackages/app-store/btcpayserver/api/webhook.tspackages/features/ee/payments/api/webhook.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/trpc/server/routers/viewer/eventTypes/heavy/create.handler.tspackages/app-store/paypal/api/webhook.tspackages/app-store/_utils/payments/handlePaymentSuccess.tspackages/app-store/alby/api/webhook.tspackages/app-store/btcpayserver/api/webhook.tspackages/features/ee/payments/api/webhook.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.tspackages/app-store/paypal/api/webhook.tspackages/app-store/_utils/payments/handlePaymentSuccess.tspackages/app-store/alby/api/webhook.tspackages/app-store/btcpayserver/api/webhook.tspackages/features/ee/payments/api/webhook.ts
🧠 Learnings (1)
📚 Learning: 2025-09-09T03:29:42.984Z
Learnt from: emrysal
PR: calcom/cal.com#23692
File: packages/lib/server/service/InsightsBookingBaseService.ts:16-16
Timestamp: 2025-09-09T03:29:42.984Z
Learning: In the Cal.com codebase, readonlyPrisma is still an instance of PrismaClient, making type changes from `typeof readonlyPrisma` to `PrismaClient` less critical since they are fundamentally compatible types.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Docs
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build API v1
- GitHub Check: Production builds / Build API v2
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (8)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (2)
3-3: Import path change to app-store utils looks correctThis aligns with the refactor goal and preserves the call site contract.
131-137: Confirm nullability of profileIdprofile.id can be null by type; ensure EventTypeRepository.create and the underlying schema accept null (or switch to undefined when null).
packages/app-store/paypal/api/webhook.ts (1)
5-5: Import path change looks good.This helps break the circular dependency chain as intended.
packages/app-store/alby/api/webhook.ts (1)
5-5: Import path change looks good.Aligned with the refactor to central utils.
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
12-12: Logger import change LGTM.Using the centralized logger is consistent with other modules.
packages/features/ee/payments/api/webhook.ts (1)
6-6: Import path change looks good.Consistent with app-store utils centralization.
packages/app-store/btcpayserver/api/webhook.ts (2)
6-6: Import path change looks good.Matches the refactor direction.
60-65: Query already compliant: uses select-only with credential.key
findByExternalIdIncludeBookingUserCredentialsemploysselectexclusively and restricts credentials to{ key: true }, so no changes required.
E2E results are ready! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/v2/package.json (2)
41-41: Optional: allow patch updates automatically.If you want Renovate/Dependabot to float patch releases for 0.0.x while staying on the npm alias, use a caret range.
Apply:
- "@calcom/platform-libraries": "npm:@calcom/platform-libraries@0.0.351", + "@calcom/platform-libraries": "npm:@calcom/platform-libraries@^0.0.351",
41-41: Optional: consider switching back to workspace once circulars are resolved.If the npm alias is no longer required to break cycles, aligning with other internal deps via workspace "*" can simplify local dev and prevent drift. If it’s still needed, ignore this.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
apps/api/v2/package.json(1 hunks)
🔇 Additional comments (1)
apps/api/v2/package.json (1)
41-41: Dependency bump to @calcom/platform-libraries@0.0.351 looks aligned with the refactor.This version likely contains the re-exports your PR depends on; change looks appropriate.
| @@ -21,19 +20,13 @@ export const bulkUpdateTeamEventsToDefaultLocation = async ({ | |||
| const defaultApp = teamMetadataSchema.parse(team?.metadata)?.defaultConferencingApp; | |||
There was a problem hiding this comment.
This should actually be a safeParse, we never want to hard crash on read.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.ts (2)
30-30: Nit: fix apostrophe in error messageMinor grammar tweak.
- throw new Error(`Default conferencing app '${defaultApp.appSlug}' doesnt exist.`); + throw new Error(`Default conferencing app '${defaultApp.appSlug}' doesn't exist.`);
51-53: Avoid misleading type assertion; cast to Prisma JSON instead of LocationObject[]
locationsis persisted as JSON. Assertingas LocationObject[]hides mismatches and weakens type-safety. Prefer casting to Prisma’s JSON types (or construct a typed variable and let TS validate).-import type { PrismaClient } from "@calcom/prisma"; +import type { PrismaClient, Prisma } from "@calcom/prisma"; @@ - locations: [ - { type: appType, link: defaultApp.appLink, credentialId: credential?.id }, - ] as LocationObject[], + locations: [ + { type: appType, link: defaultApp.appLink, credentialId: credential?.id }, + ] as Prisma.JsonArray,If you want stricter checks, build the value first and let TS validate the shape:
const locations: LocationObject[] = [ { type: appType, link: defaultApp.appLink, credentialId: credential?.id }, ]; // then: data: { locations: locations as unknown as Prisma.JsonArray }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.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/bulkUpdateTeamEventsToDefaultLocation.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/bulkUpdateTeamEventsToDefaultLocation.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/bulkUpdateTeamEventsToDefaultLocation.ts
🔇 Additional comments (4)
packages/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation.ts (4)
4-5: Good: moved to local utils without default exportsSwitching to relative imports removes the circular dep risk and keeps named exports. LGTM.
20-21: Correct: use safeParse to avoid hard crashes on bad metadataThis aligns with prior feedback and gracefully handles malformed JSON. 👍
33-41: Prisma selection is minimal and safeSelecting only
idavoids over-fetch and ensures we never risk leaking credential secrets. Matches the repo’s Prisma guideline. 👍
24-24: Plain Error in util is OK — tRPC handler converts it to TRPCErrorapps/api/v2/src/modules/atoms/services/event-types-atom.service.ts calls bulkUpdateTeamEventsToDefaultLocation; the API boundary at packages/trpc/server/routers/viewer/eventTypes/bulkUpdateToDefaultLocation.handler.ts catches generic Error and rethrows new TRPCError({ code: "BAD_REQUEST", message: error.message }). No changes required.
| @@ -1,9 +1,8 @@ | |||
| import type { LocationObject } from "@calcom/app-store/locations"; | |||
There was a problem hiding this comment.
What a nasty import. Love this change
3 participants
What does this PR do?
Migration:
@calcom/lib/server/getDefaultLocations -> @calcom/app-store/_utils/getDefaultLocations@calcom/lib/bulkUpdateTeamEventsToDefaultLocation -> @calcom/app-store/_utils/bulkUpdateTeamEventsToDefaultLocation@calcom/lib/bulkUpdateEventsToDefaultLocation -> @calcom/app-store/_utils/bulkUpdateEventsToDefaultLocation@calcom/lib/getConnectedApps-> @calcom/app-store/_utils/getConnectedApps@calcom/lib/payment/handlePaymentSuccess-> @calcom/app-store/_utils/payments/handlePaymentSuccessMandatory Tasks (DO NOT REMOVE)
How should this be tested?