refactor: Migrate trpc routers in App store package to Trpc package #23536
Conversation
|
Warning Rate limit exceeded@hbjORbj has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR removes default export re-exports from app-store Basecamp3 and Routing Forms trpc-router files and updates viewer router imports to local app routers. It refactors numerous server handlers to use @calcom/app-store absolute import aliases instead of relative paths. It introduces packages/app-store/routing-forms/lib/formSubmissionUtils.ts, adding types (FORM_SUBMITTED_WEBHOOK_RESPONSES) and functions (getFieldResponse, sendResponseEmail, _onFormSubmission, onFormSubmission). Related test and type-only imports are updated accordingly. Several routing-forms handlers switch imports for zod and various libs to package-scoped paths. No exported TRPC router signatures are changed. Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
hbjORbj
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (58)
packages/app-store/routing-forms/trpc/getIncompleteBookingSettings.handler.ts (1)
91-103: Credential leakage risk: addselectto avoid returningcredential.key.
This tRPC handler returns acredentialfetched without aselect, which likely includeskey. Our policy forbids exposingcredential.keyfrom APIs; only internal token-refresh utilities may access it.Apply:
if (userId) { // Assume that a user will have one credential per app - const credential = await prisma.credential.findFirst({ + const credential = await prisma.credential.findFirst({ where: { appId: { in: enabledIncompleteBookingApps, }, userId, }, + select: { + ...safeCredentialSelectWithoutUser, + }, }); return { incompleteBookingActions, credentials: credential ? [{ ...credential, team: null }] : [] }; }packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (1)
79-104: Replaceincludewith an explicitselectin the Prisma read query
ThefindFirstcall atpackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts:79still usesinclude. Per Cal.com Prisma guidelines for read operations, switch to aselectclause listing only the scalar fields and relations consumed bygetSerializableFormto prevent overfetch.packages/app-store/basecamp3/trpc/projects.handler.ts (1)
41-46: Handle non-2xx responses and JSON parse errors.Avoid returning opaque fetch failures; surface proper TRPC errors.
- const resp = await fetch(url, { - headers: { "User-Agent": user_agent as string, Authorization: `Bearer ${credentialKey.access_token}` }, - }); - const projects = await resp.json(); + const resp = await fetch(url, { + headers: { "User-Agent": user_agent as string, Authorization: `Bearer ${credentialKey.access_token}` }, + }); + if (!resp.ok) { + throw new TRPCError({ code: "BAD_GATEWAY", message: `Basecamp error ${resp.status}` }); + } + let projects; + try { + projects = await resp.json(); + } catch { + throw new TRPCError({ code: "BAD_GATEWAY", message: "Invalid JSON from Basecamp" }); + } return { currentProject: credentialKey.projectId, projects };packages/trpc/server/routers/viewer/teams/acceptOrLeave.handler.ts (2)
24-27: Prisma: replace include with minimal select.
Limit payload and follow project guideline “never use include; always use select.”- include: { - team: true, - }, + select: { + team: { + select: { id: true, isOrganization: true, parentId: true }, + }, + },
65-68: Prisma: replace include with minimal select on delete.
Only parentId is used later.- include: { - team: true, - }, + select: { + team: { select: { parentId: true } }, + },packages/trpc/server/routers/viewer/organizations/verifyCode.handler.ts (1)
54-57: Guard missing CALENDSO_ENCRYPTION_KEY in production.
Avoid generating unstable secrets if the key is unset.- const secret = createHash("md5") - .update(email + process.env.CALENDSO_ENCRYPTION_KEY) - .digest("hex"); + const encryptionKey = process.env.CALENDSO_ENCRYPTION_KEY; + if (IS_PRODUCTION && !encryptionKey) { + throw new TRPCError({ code: "INTERNAL_SERVER_ERROR" }); + } + const secret = createHash("md5").update(email + encryptionKey).digest("hex");packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts (1)
56-61: Drop unused selection to minimize query cost.
members.userId isn’t referenced; remove it.- members: { - select: { - userId: true, - }, - },packages/trpc/server/routers/viewer/teams/roundRobin/getRoundRobinHostsToReasign.handler.ts (1)
70-73: Cursor/orderBy mismatch can break pagination; add deterministic tie-breakerUsing a cursor on (userId,eventTypeId) while ordering by user.name, priority risks duplicates/skips. Add a unique tie-breaker (userId asc) to align with the cursor.
- orderBy: [{ user: { name: "asc" } }, { priority: "desc" }], + // Ensure deterministic pagination aligned with cursor (userId,eventTypeId) + orderBy: [{ user: { name: "asc" } }, { priority: "desc" }, { userId: "asc" }],Also applies to: 96-97
packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts (1)
61-95: Replace Prisma include with select to reduce overfetching and follow guidelinesAvoid include; select only required fields (you only read children.members[userId, teamId] and user.profiles.username).
- const requestedMember = await prisma.membership.findFirst({ + const requestedMember = await prisma.membership.findFirst({ where: { userId: input.userId, teamId: organizationId, accepted: true, }, - include: { - team: { - include: { - children: { - where: { - members: { - some: { - userId: input.userId, - }, - }, - }, - include: { - members: true, - }, - }, - }, - }, - user: { - select: { - username: true, - profiles: { - select: { - username: true, - }, - }, - }, - }, - }, + select: { + team: { + select: { + children: { + where: { + members: { some: { userId: input.userId } }, + }, + select: { + members: { + select: { teamId: true, userId: true }, + }, + }, + }, + }, + }, + user: { + select: { + username: true, + profiles: { select: { username: true } }, + }, + }, + }, });packages/trpc/server/routers/loggedInViewer/routingFormOrder.handler.ts (1)
18-51: Avoid Prisma include; select only required fieldsGuideline: “only select data you need; never use include.” The result is only used to build a Set of IDs, so select id only.
- const forms = await prisma.app_RoutingForms_Form.findMany({ - where: { + const forms = await prisma.app_RoutingForms_Form.findMany({ + where: { OR: [ { userId: user.id }, { team: { members: { some: { userId: user.id, accepted: true } }, }, }, ], - }, - orderBy: { - createdAt: "desc", - }, - include: { - team: { - include: { - members: true, - }, - }, - _count: { - select: { responses: true }, - }, - }, + }, + select: { id: true }, });packages/trpc/server/routers/loggedInViewer/unlinkConnectedAccount.handler.ts (2)
21-24: Avoid locale-sensitive lowercasingUse toLowerCase() to prevent Turkish-I issues when deriving provider id.
- provider: user.identityProvider.toLocaleLowerCase(), + provider: user.identityProvider.toLowerCase(),
29-39: Overly restrictive update filter (GOOGLE-only) breaks unlink for other providersThe update targets only GOOGLE. Users linked with Apple/Microsoft/etc. won’t be switched back to CAL. Update by id (or use updateMany if you want conditional guards).
- const _user = await prisma.user.update({ - where: { - id: user.id, - identityProvider: IdentityProvider.GOOGLE, - identityProviderId: { not: null }, - }, - data: { - identityProvider: IdentityProvider.CAL, - identityProviderId: null, - }, - }); + const _user = await prisma.user.update({ + where: { id: user.id }, + data: { + identityProvider: IdentityProvider.CAL, + identityProviderId: null, + }, + });Alternative (keeps guards, returns count):
- const _user = await prisma.user.update({ … }); + const { count } = await prisma.user.updateMany({ + where: { + id: user.id, + identityProviderId: { not: null }, + identityProvider: { not: IdentityProvider.CAL }, + }, + data: { identityProvider: IdentityProvider.CAL, identityProviderId: null }, + }); + if (count === 0) return { message: "account_unlinked_error" };packages/app-store/routing-forms/trpc/saveIncompleteBookingSettings.handler.ts (1)
47-76: Add composite unique on (formId, actionType) and switch to upsert
Add@@unique([formId, actionType])to theApp_RoutingForms_IncompleteBookingActionsmodel inpackages/prisma/schema.prisma, run the migration to enforce it, then refactorpackages/app-store/routing-forms/trpc/saveIncompleteBookingSettings.handler.tsto useawait prisma.app_RoutingForms_IncompleteBookingActions.upsert({ … })in place of the current
findFirst+create/updatelogic.packages/trpc/server/routers/viewer/apps/locationOptions.handler.ts (1)
21-27: Add a stable category key to grouped options and use it for reordering instead of comparing labels
getLocationGroupedOptions currently returns only{ label, options }, so comparinggroup.label === "Conferencing"breaks in localized locales. Extend its return shape to include the raw category (e.g.key: category) and update locationOptionsHandler to usegroup.key === "conferencing"when moving the conferencing group.packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts (1)
17-23: Select only what you need from Prisma.Limit the credential query to id since you only check existence.
const credential = await prisma.credential.findFirst({ where: { id: credentialId, userId: user.id, }, + select: { id: true }, });packages/lib/server/getUsersCredentials.ts (1)
35-38: Strip serviceAccountKey from non-privileged helper to prevent accidental leakage.
getUsersCredentials()should never surface any secret material. It currently only removesdelegatedTo. DropserviceAccountKeyas well.export async function getUsersCredentials(user: User) { const credentials = await getUsersCredentialsIncludeServiceAccountKey(user); - return credentials.map(({ delegatedTo: _1, ...rest }) => rest); + return credentials.map(({ delegatedTo: _1, serviceAccountKey: _2, ...rest }) => rest); }packages/trpc/server/routers/loggedInViewer/addSecondaryEmail.handler.ts (4)
28-33: Useselectfor existence check to avoid fetching full user rows.Only the presence matters here; select the minimal field.
- const existingPrimaryEmail = await prisma.user.findUnique({ - where: { - email: input.email, - }, - }); + const existingPrimaryEmail = await prisma.user.findUnique({ + where: { email: input.email }, + select: { id: true }, + });
38-43: Same here: minimize thesecondaryEmaillookup payload.- const existingSecondaryEmail = await prisma.secondaryEmail.findUnique({ - where: { - email: input.email, - }, - }); + const existingSecondaryEmail = await prisma.secondaryEmail.findUnique({ + where: { email: input.email }, + select: { id: true }, + });
48-57: Handle uniqueness race and return only whitelisted fields.Between the checks and the create, a concurrent request can insert the same email and trigger a unique-constraint error. Catch it and map to BAD_REQUEST. Also, select only the fields you need to use/return.
- const updatedData = await prisma.secondaryEmail.create({ - data: { ...input, userId: user.id }, - }); - - await sendEmailVerification({ - email: updatedData.email, - username: user?.username ?? undefined, - language: user.locale, - secondaryEmailId: updatedData.id, - }); - - return { - data: updatedData, - message: "Secondary email added successfully", - }; + let created: { id: string; email: string }; + try { + created = await prisma.secondaryEmail.create({ + data: { ...input, userId: user.id }, + select: { id: true, email: true }, + }); + } catch (e: any) { + // Unique constraint violation (e.g., email already exists) + if (e?.code === "P2002") { + throw new TRPCError({ code: "BAD_REQUEST", message: "Email already taken" }); + } + throw e; + } + + await sendEmailVerification({ + email: created.email, + username: user?.username ?? undefined, + language: user.locale, + secondaryEmailId: created.id, + }); + + return { + data: created, + message: "Secondary email added successfully", + };Add this import near the top (outside the selected range):
import { Prisma } from "@prisma/client";Also applies to: 59-63
29-31: Lowercase and validateinput.emailfor case-insensitive uniqueness UpdateZAddSecondaryEmailInputSchematoz.string().email().transform(e => e.toLowerCase())(or lowercaseinput.emailin the handler) before using it in thewhereclause, and add a case-insensitive unique index on thepackages/trpc/server/routers/viewer/me/getUserTopBanners.handler.ts (1)
19-26: Removekeyfrom credentialForCalendarServiceSelect
The select inpackages/prisma/selects/credential.tscurrently includeskey: true, exposing secret credentials. Remove this line to prevent leakingcredential.key.packages/trpc/server/routers/loggedInViewer/addNotificationsSubscription.handler.ts (2)
24-32: JSON.parse can throw; also avoid logging secrets.Invalid JSON throws before Zod runs, causing a 500. Moreover, logging the raw subscription leaks sensitive keys (auth, p256dh).
- const parsedSubscription = subscriptionSchema.safeParse(JSON.parse(subscription)); + let parsed: unknown; + try { + parsed = JSON.parse(subscription); + } catch { + log.warn("Invalid subscription JSON", { userId: user.id }); + throw new TRPCError({ code: "BAD_REQUEST", message: "Invalid subscription" }); + } + const parsedSubscription = subscriptionSchema.safeParse(parsed); @@ - log.error("Invalid subscription", parsedSubscription.error, JSON.stringify(subscription)); + log.error("Invalid subscription", { + userId: user.id, + issues: parsedSubscription.error.issues?.map((i) => ({ path: i.path, code: i.code })), + });
34-47: Add unique constraint or retain explicit flow
The Prisma schema’s NotificationsSubscriptions model (in packages/prisma/schema.prisma) doesn’t declare userId as unique, so callingprisma.notificationsSubscriptions.upsert({ where: { userId: user.id }, … })will fail at runtime. Either:
- Add a unique constraint—e.g.
then runmodel NotificationsSubscriptions { id Int @id @default(autoincrement()) userId Int @unique … }prisma migrate, or- Keep the current find-then-create/update logic and minimize payload by selecting only id:
const existingSubscription = await prisma.notificationsSubscriptions.findFirst({ where: { userId: user.id }, select: { id: true }, });packages/app-store/routing-forms/trpc/formMutation.handler.ts (4)
150-171: Settings are not persisted on create.Upsert’s create branch omits settings; user-provided settings are dropped on new form creation.
Apply:
create: { user: { connect: { id: user.id } }, fields, name: name, description, - // Prisma doesn't allow setting null value directly for JSON. It recommends using JsonNull for that case. + // Prisma doesn't allow setting null value directly for JSON. It recommends using JsonNull for that case. routes: routes === null ? Prisma.JsonNull : routes, + settings: settings === null ? Prisma.JsonNull : settings, id: id, ...(teamId
202-211: Restrict Prisma selection to only needed fields.Fetch only routes and fields; avoid loading entire form.
Apply:
- const router = await prisma.app_RoutingForms_Form.findFirst({ - where: { id: field.routerId, userId: user.id }, - }); + const router = await prisma.app_RoutingForms_Form.findFirst({ + where: { id: field.routerId, userId: user.id }, + select: { routes: true, fields: true }, + });
229-238: Same: minimize payload on router lookup.Apply:
- const router = await prisma.app_RoutingForms_Form.findFirst({ - where: { id: route.id, userId: user.id }, - }); + const router = await prisma.app_RoutingForms_Form.findFirst({ + where: { id: route.id, userId: user.id }, + select: { routes: true, fields: true }, + });
267-276: Same: minimize payload for connected form.Only fields are used.
Apply:
- const connectedFormDb = await prisma.app_RoutingForms_Form.findUnique({ - where: { id: connectedForm.id }, - }); + const connectedFormDb = await prisma.app_RoutingForms_Form.findUnique({ + where: { id: connectedForm.id }, + select: { fields: true }, + });packages/trpc/server/routers/viewer/teams/createInvite.handler.ts (1)
42-44: Bug: token expiry computed from midnight, not “now + 7d”.new Date().setHours(168) sets hours since local midnight; tokens can expire up to ~24h early/late.
Apply:
- expires: new Date(new Date().setHours(168)), // +1 week, + // +1 week from now (UTC-safe arithmetic) + expires: new Date(Date.now() + 7 * 24 * 60 * 60 * 1000),(Optional) Consider using a helper like addDays for clarity.
packages/trpc/server/routers/viewer/teams/legacyListMembers.handler.ts (3)
49-58: Prisma: add select to minimize fetched data.
This query only needs teamId but currently returns full Membership rows.Apply:
const memberships = await prisma.membership.findMany({ where: { teamId: { in: input.teamIds }, userId: ctx.user.id, accepted: true, ...(input.adminOrOwnedTeamsOnly ? { role: { in: [MembershipRole.ADMIN, MembershipRole.OWNER] } } : {}), }, + select: { teamId: true }, });
20-22: AddisPrivatetoSessionUserOrganization.
legacyListMembers.handler.tsusesctx.user.organization.isPrivate, but theSessionUserOrganizationinterface inpackages/lib/sessionUser.tsdoesn’t include it. Add a lineisPrivate: boolean;to that interface.
75-105: Align cursor with orderBy fields for stable pagination
- In the prisma.membership.findMany call (legacyListMembers.handler.ts lines 98–104), you’re using distinct: ["userId"] and orderBy: [{ userId: "asc" }, { id: "asc" }] but the cursor is only on id. This mismatch can skip or duplicate records. Replace the scalar cursor with a compound cursor matching the ordering—e.g.:
to ensure consistent, stable paging.cursor: { userId_id: { userId: lastUserId, id: lastMembershipId } }packages/trpc/server/routers/viewer/teams/hasActiveTeamPlan.handler.ts (1)
35-37: Prisma: select only needed fields from platformBilling.
Avoid fetching entire row when only plan is used.- const platformBilling = await prisma.platformBilling.findUnique({ where: { id: team.id } }); + const platformBilling = await prisma.platformBilling.findUnique({ + where: { id: team.id }, + select: { plan: true }, + });packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (1)
111-114: Avoid Prismainclude; explicitlyselectonly needed fields.Returning
team: trueanduser: truerisks leaking sensitive fields to the client. Useselectwith a minimal shape.Apply this diff (adjust selected fields to your API contract):
const updatedMembership = await prisma.membership.findUnique({ where: { userId_teamId: { userId: input.memberId, teamId: input.teamId }, }, - include: { - team: true, - user: true, - }, + select: { + id: true, + role: true, + userId: true, + teamId: true, + team: { + select: { + id: true, + name: true, + slug: true, + }, + }, + user: { + select: { + id: true, + username: true, + name: true, + avatarUrl: true, + }, + }, + }, });packages/features/ee/organizations/lib/OrganizationPermissionService.ts (2)
43-47: Select only needed columns in Prisma query.You only use
isComplete; avoid fetching the whole row.Apply:
const orgOnboarding = await prisma.organizationOnboarding.findUnique({ where: { orgOwnerEmail: email, }, + select: { isComplete: true }, });
75-90: Usecountinstead offindManywhen only comparing lengths.Reduces data transfer and memory.
Apply:
- const teamMemberships = await prisma.membership.findMany({ + const memberCount = await prisma.membership.count({ where: { userId: this.user.id, teamId: { in: teamIds, }, role: { in: ["OWNER", "ADMIN"], }, }, }); - - return teamMemberships.length === teamIds.length; + return memberCount === teamIds.length;packages/app-store/routing-forms/trpc/getResponseWithFormFields.handler.ts (1)
27-65: Prisma: replace include with select and return only required fieldsGuideline: never use
include; select only what you need. Also explicitly selectresponse(used later).- const formResponse = await prisma.app_RoutingForms_FormResponse.findUnique({ - where: { - id: formResponseId, - }, - include: { - form: { - include: { - user: { - select: { - id: true, - movedToProfileId: true, - organization: { - select: { - slug: true, - }, - }, - username: true, - theme: true, - brandColor: true, - darkBrandColor: true, - metadata: true, - }, - }, - team: { - select: { - id: true, - members: true, - slug: true, - parent: { - select: { slug: true }, - }, - parentId: true, - metadata: true, - }, - }, - }, - }, - }, - }); + const formResponse = await prisma.app_RoutingForms_FormResponse.findUnique({ + where: { id: formResponseId }, + select: { + response: true, + form: { + select: { + user: { + select: { + id: true, + movedToProfileId: true, + organization: { select: { slug: true } }, + username: true, + theme: true, + brandColor: true, + darkBrandColor: true, + metadata: true, + }, + }, + team: { + select: { + id: true, + // Narrow further if possible based on getSerializableForm usage + members: { select: { id: true } }, + slug: true, + parent: { select: { slug: true } }, + parentId: true, + metadata: true, + }, + }, + }, + }, + }, + });If
getSerializableFormneeds more fromteam.members, expand the member selection minimally rather than returning entire rows.packages/trpc/server/routers/viewer/me/myStats.handler.ts (1)
19-22: Potential runtime TypeError: optional chaining stops before reduce().
If additionalUserInfo is undefined, additionalUserInfo?.teams yields undefined and .reduce will throw. Guard teams as well and default to 0.Apply this diff:
- const sumOfTeamEventTypes = additionalUserInfo?.teams.reduce( - (sum, team) => sum + team.team.eventTypes.length, - 0 - ); + const sumOfTeamEventTypes = + additionalUserInfo?.teams?.reduce( + (sum, team) => sum + team.team.eventTypes.length, + 0 + ) ?? 0;packages/trpc/server/routers/viewer/calVideo/getMeetingInformation.handler.ts (1)
39-42: Don’t mask TRPCError details in catch; preserve known errors.
Current catch converts all errors to a generic BAD_REQUEST, hiding specific messages thrown above.Apply this diff:
- } catch (err) { - throw new TRPCError({ - code: "BAD_REQUEST", - }); - } + } catch (err) { + if (err instanceof TRPCError) throw err; + throw new TRPCError({ code: "BAD_REQUEST" }); + }packages/trpc/server/routers/loggedInViewer/removeNotificationsSubscription.handler.ts (1)
16-29: Make deletion idempotent and avoid fetching unnecessary columns.
Replace read-then-delete with a single deleteMany by userId. Fewer queries, no race on P2025, and adheres to “select only what you need.”Apply this diff:
- // We just use findFirst because there will only be single unique subscription for a user - const subscriptionToDelete = await prisma.notificationsSubscriptions.findFirst({ - where: { - userId: user.id, - }, - }); - - if (subscriptionToDelete) { - await prisma.notificationsSubscriptions.delete({ - where: { - id: subscriptionToDelete.id, - }, - }); - } + // Delete any existing subscriptions for this user (should be at most one) + await prisma.notificationsSubscriptions.deleteMany({ + where: { userId: user.id }, + });packages/trpc/server/routers/viewer/oAuth/generateAuthCode.handler.ts (2)
38-51: Prisma: select only needed fields for team lookup.Return only team.id to align with our Prisma guideline and avoid overfetching.
const team = teamSlug ? await prisma.team.findFirst({ where: { slug: teamSlug, members: { some: { userId: ctx.user.id, role: { - in: ["OWNER", "ADMIN"], + in: ["OWNER", "ADMIN"], }, }, }, }, + select: { id: true }, }) : undefined;
64-65: Fix scopes type: tuple cast is incorrect.Use AccessScope[]; casting to [AccessScope] is a single-item tuple and weakens typing.
- scopes: scopes as [AccessScope], + scopes: scopes as AccessScope[],packages/lib/server/repository/__tests__/filterSegments/create.test.ts (1)
96-113: Test isolation issue: prior ADMIN membership contaminates this test.Earlier you create ADMIN membership (teamId: 1). This test then creates MEMBER for the same (userId, teamId). Depending on constraints/order, the user may still be considered admin, making this test flaky. Use a different teamId (e.g., 2) here.
- teamId: 1, + teamId: 2, @@ - teamId: 1, + teamId: 2,Alternatively, clear memberships between tests (e.g., deleteMany for (userId, teamId:1)) in a beforeEach.
packages/lib/hasEditPermissionForUser.ts (2)
14-39: Reduce to a single DB query and avoid fetching unused columns.Current approach does 2 queries and pulls whole membership rows. Use a team intersection count instead.
export async function hasEditPermissionForUserID({ ctx, input }: InputOptions) { - const { user } = ctx; - - const authedUsersTeams = await prisma.membership.findMany({ - where: { - userId: user.id, - accepted: true, - role: { - in: [MembershipRole.ADMIN, MembershipRole.OWNER], - }, - }, - }); - - const targetUsersTeams = await prisma.membership.findMany({ - where: { - userId: input.memberId, - accepted: true, - }, - }); - - const teamIdOverlaps = authedUsersTeams.some((authedTeam) => { - return targetUsersTeams.some((targetTeam) => targetTeam.teamId === authedTeam.teamId); - }); - - return teamIdOverlaps; + const { user } = ctx; + const overlapCount = await prisma.team.count({ + where: { + AND: [ + { + members: { + some: { + userId: user.id, + accepted: true, + role: { in: [MembershipRole.ADMIN, MembershipRole.OWNER] }, + }, + }, + }, + { + members: { + some: { + userId: input.memberId, + accepted: true, + }, + }, + }, + ], + }, + }); + return overlapCount > 0; }
41-64: Same optimization for read-permissions.export async function hasReadPermissionsForUserId({ userId, memberId, }: InputOptions["input"] & { userId: number }) { - const authedUsersTeams = await prisma.membership.findMany({ - where: { - userId, - accepted: true, - }, - }); - - const targetUsersTeams = await prisma.membership.findMany({ - where: { - userId: memberId, - accepted: true, - }, - }); - - const teamIdOverlaps = authedUsersTeams.some((authedTeam) => { - return targetUsersTeams.some((targetTeam) => targetTeam.teamId === authedTeam.teamId); - }); - - return teamIdOverlaps; + const overlapCount = await prisma.team.count({ + where: { + AND: [ + { members: { some: { userId, accepted: true } } }, + { members: { some: { userId: memberId, accepted: true } } }, + ], + }, + }); + return overlapCount > 0; }packages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.ts (2)
75-97: findUnique cannot use relational filters — switch to findFirst.Prisma’s findUnique only accepts unique fields; relational filters under where are ignored/invalid. Use findFirst to enforce both constraints.
- const user = await prisma.user.findUnique({ + const user = await prisma.user.findFirst({ where: { id: input.toTeamUserId, /** You can only redirect OOO for members of teams you belong to */ teams: { some: { team: { members: { some: { userId: oooUserId, accepted: true, }, }, }, }, }, }, select: { id: true, }, });
172-196: Upsert where must use a unique selector only.Unless there’s a composite unique on (uuid, userId), include only uuid in where to avoid type/runtime errors.
const createdOrUpdatedOutOfOffice = await prisma.outOfOfficeEntry.upsert({ - where: { - uuid: input.uuid ?? "", - userId: oooUserId, - }, + where: { uuid: input.uuid ?? "" }, create: { uuid: uuidv4(), start: startTimeUtc.toISOString(), end: endTimeUtc.toISOString(), notes: input.notes, userId: oooUserId, reasonId: input.reasonId, toUserId: toUserId, createdAt: new Date(), updatedAt: new Date(), }, update: { start: startTimeUtc.toISOString(), end: endTimeUtc.toISOString(), notes: input.notes, - userId: oooUserId, reasonId: input.reasonId, toUserId: toUserId ? toUserId : null, }, });packages/trpc/server/routers/viewer/apps/appCredentialsByType.handler.ts (1)
23-48: Critical: sanitize credential.key leaks in handler
safeCredentialSelectWithoutUserisn’t defined/imported—usesafeCredentialSelect(omittingkey) or define a variant withoutuserand explicitkey: false.delegationCredentialsfromgetAllDelegationCredentialsForUserByAppTypeare returned unsanitized (includingkey)—map or select to excludekeybefore returning.packages/trpc/server/routers/viewer/attributes/findTeamMembersMatchingAttributeLogic.handler.ts (1)
57-67: Restrict fetched fields in findByIds to only id, name, and email
TheUserRepository.findByIdscall in your handler (packages/trpc/server/routers/viewer/attributes/findTeamMembersMatchingAttributeLogic.handler.ts:57) currently uses the globaluserSelect, which includes sensitive fields liketwoFactorSecret,backupCodes,metadata, etc. For this endpoint you only returnid,name, andpackages/app-store/basecamp3/trpc/projectMutation.handler.ts (2)
29-34: Filter credential by app type to avoid picking a wrong provider.
Current query fetches first credential by user; may not be Basecamp. Restrict by type.- const credential = await prisma.credential.findFirst({ - where: { - userId: user?.id, - }, + const credential = await prisma.credential.findFirst({ + where: { + userId: user.id, + type: "basecamp3", + },
29-35: Trim credentialForCalendarServiceSelect to only required fields: in packages/prisma/selects/credential.ts, remove appId, userId, user.email, teamId, invalid, and delegationCredentialId—keep only id, type, key (and any refresh-specific fields).packages/trpc/server/routers/viewer/me/updateProfile.handler.ts (2)
265-289: Guard timezone updates to avoid writing undefined.If data.timeZone is undefined but differs from user.timeZone, this block runs and may write an undefined timeZone to schedules. Guard on presence.
- if (user.timeZone !== data.timeZone && updatedUser.schedules.length > 0) { + if (data.timeZone && user.timeZone !== data.timeZone && updatedUser.schedules.length > 0) {
348-371: Defensive check when updating secondary emails.If an input ID isn’t found in DB (edge case), keyedSecondaryEmailsFromDB[updated.id] is undefined, causing a crash. Filter to existing IDs.
- const keyedSecondaryEmailsFromDB = keyBy(secondaryEmailsFromDB, "id"); - - const recordsToModifyQueue = modifiedRecords.map((updated) => { + const keyedSecondaryEmailsFromDB = keyBy(secondaryEmailsFromDB, "id"); + const existingIds = new Set(secondaryEmailsFromDB.map((s) => s.id)); + const recordsToModifyQueue = modifiedRecords + .filter((u) => existingIds.has(u.id)) + .map((updated) => { let emailVerified = keyedSecondaryEmailsFromDB[updated.id].emailVerified; if (secondaryEmail?.id === updated.id) { emailVerified = primaryEmailVerified; } else if (updated.email !== keyedSecondaryEmailsFromDB[updated.id].email) { emailVerified = null; }packages/trpc/server/middlewares/sessionMiddleware.ts (3)
74-85: Broken avatar URL construction (always returns "orgId=...").Using && between strings returns the rhs; the URL is lost. Build the URL explicitly.
Apply:
return { ...user, - avatar: `${WEBAPP_URL}/${user.username}/avatar.png?${organization.id}` && `orgId=${organization.id}`, + avatar: + `${WEBAPP_URL}/${user.username}/avatar.png` + + (organization.id ? `?orgId=${organization.id}` : ""), // TODO: OrgNewSchema - later - We could consolidate the props in user.profile?.organization as organization is a profile thing now. organization,
120-134: Guard against producing upId = "usr-undefined".If session exists but user is null, current logic can assign a truthy but invalid upId. Fail fast unless a valid userId is present.
Apply:
let sessionWithUpId = null; if (session) { - let upId = session.upId; - if (!upId) { - upId = foundProfile?.upId ?? `usr-${user?.id}`; - } - - if (!upId) { - throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "No upId found for session" }); - } + let upId = session.upId ?? foundProfile?.upId; + if (!upId) { + if (user?.id) { + upId = `usr-${user.id}`; + } else { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "No upId and no authenticated user in session", + }); + } + } sessionWithUpId = { ...session, upId, }; }
1-185: Consolidate duplicate type and enforce named exports & Prisma selects
- Remove the redundant
export type TrpcSessionUserinpackages/trpc/server/middlewares/sessionMiddleware.ts(line 88) and import it from its single source.- Replace all
export defaultinpackages/trpc/server/routers/**andpackages/app-store/**with named exports.- Refactor Prisma calls in
packages/trpc/server/routers/**to useselectinstead ofinclude.No remaining imports from
@calcom/trpc/server/typeswere found.packages/app-store/routing-forms/trpc/deleteForm.handler.ts (1)
37-41: Fix grammar in user-facing error message."It's" → "its".
Apply:
- message: "This form is being used by other forms. Please remove it's usage from there first.", + message: "This form is being used by other forms. Please remove its usage there first.",packages/trpc/server/routers/viewer/teams/listInvites.handler.ts (1)
12-19: Use Prisma select to minimize returned columns and prefer direct FK filter.Per repo guideline, select only needed fields and use userId to avoid an extra join.
Apply:
- return await prisma.membership.findMany({ - where: { - user: { - id: userId, - }, - accepted: false, - }, - }); + return await prisma.membership.findMany({ + where: { + userId, + accepted: false, + }, + select: { + id: true, + teamId: true, + role: true, + accepted: true, + createdAt: true, + // If team metadata is required, expand via select (not include) + // team: { select: { id: true, name: true, slug: true } }, + }, + });packages/app-store/routing-forms/trpc/forms.handler.ts (1)
31-54: Replace include with select and enumerate required fields
• Switch to Prisma select in findMany and explicitly list all form properties used downstream: id, userId, teamId, name, description, position, fields, routes, settings, createdAt, updatedAt
• Keep nested selects for team (id,name) and_count.responses
• Confirm this covers all accesses in getSerializableForm and canEditEntity
packages/trpc/server/routers/viewer/eventTypes/getHashedLinks.handler.ts
Outdated
Show resolved
Hide resolved
hbjORbj
changed the title
@calcom/types from @calcom/trpc
E2E results are ready! |
hbjORbj
force-pushed
the
refactor/circular-deps
branch
from
1178974 to
9eac700
Compare
hbjORbj
changed the title
@calcom/types from @calcom/trpc@calcom/lib from @calcom/trpc
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 (12)
packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts (1)
22-51: Replace heavy include with minimal select (and follow Prisma guideline).You only use workflow ids for authorization; fetching relations is wasteful and violates the “no include; use select” guideline. Simplify to select just id and drop the intermediate includedFields.
Apply:
- const { include: includedFields } = { - include: { - activeOn: { - select: { - eventType: { - select: { - id: true, - title: true, - parentId: true, - _count: { - select: { - children: true, - }, - }, - }, - }, - }, - }, - steps: true, - team: { - select: { - id: true, - slug: true, - name: true, - members: true, - logoUrl: true, - }, - }, - }, - } satisfies Prisma.WorkflowDefaultArgs; + // No relations needed for auth check; select only id. const allWorkflows = await prisma.workflow.findMany({ where: { OR: [ { userId: user.id }, { team: { members: { some: { userId: user.id, accepted: true } }, }, }, ], }, - include: includedFields, - orderBy: [ - { position: "desc" }, - { id: "asc" }, - ], + select: { id: true }, });Also applies to: 53-81
packages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.ts (1)
91-103: Exclude sensitive credential fields in Prisma queries
- At packages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.ts:63, change the
findManycall to use aselectthat omitscredential.key:- const credentials = await prisma.credential.findMany({ - where: { appId: { in: enabledIncompleteBookingApps }, userId: null }, - }); + const credentials = await prisma.credential.findMany({ + where: { appId: { in: enabledIncompleteBookingApps }, userId: null }, + select: { + ...safeCredentialSelectWithoutUser, // excludes `key` + user: { select: { email: true, name: true } }, + }, +});
- At lines 91–103 (the
findFirst), apply the sameselectpattern to prevent leakingcredential.key(as shown in the original diff).packages/trpc/server/routers/viewer/_router.tsx (1)
75-76: Remove deprecated trpc-router imports
- apps/web/pages/api/trpc/appBasecamp3/[trpc].ts: remove the
@calcom/app-store/basecamp3/trpc-routerimport- apps/web/pages/api/trpc/appRoutingForms/[trpc].ts: remove the
@calcom/app-store/routing-forms/trpc-routerimportpackages/trpc/server/routers/apps/routing-forms/forms.handler.ts (1)
31-54: Use Prisma select (never include) and parallelize queriesFollow repo guideline: “never use include; always select.” Also run count in parallel.
- const forms = await prisma.app_RoutingForms_Form.findMany({ - where, - orderBy: [ - { - position: "desc", - }, - { - createdAt: "desc", - }, - ], - include: { - team: { - select: { - id: true, - name: true, - }, - }, - _count: { - select: { - responses: true, - }, - }, - }, - }); - - const totalForms = await prisma.app_RoutingForms_Form.count({ - where: entityPrismaWhereClause({ - userId: user.id, - }), - }); + const [forms, totalForms] = await Promise.all([ + prisma.app_RoutingForms_Form.findMany({ + where, + orderBy: [{ position: "desc" }, { createdAt: "desc" }], + select: { + id: true, + name: true, + description: true, + userId: true, + teamId: true, + position: true, + createdAt: true, + updatedAt: true, + disabled: true, + updatedById: true, + settings: true, + fields: true, + routes: true, + team: { select: { id: true, name: true } }, + _count: { select: { responses: true } }, + }, + }), + prisma.app_RoutingForms_Form.count({ + where: entityPrismaWhereClause({ userId: user.id }), + }), + ]);Also applies to: 56-60
packages/trpc/server/routers/apps/routing-forms/formMutation.handler.ts (4)
149-170: Bug: settings not persisted on createCreate branch drops input.settings; users lose provided settings on new forms.
create: { user: { connect: { id: user.id, }, }, fields, name: name, description, + settings: settings === null ? Prisma.JsonNull : settings, // Prisma doesn't allow setting null value directly for JSON. It recommends using JsonNull for that case. routes: routes === null ? Prisma.JsonNull : routes, id: id, ...(teamId
201-207: Only select needed fields from PrismaAvoid fetching full records; you only use routes/fields here.
- const router = await prisma.app_RoutingForms_Form.findFirst({ - where: { - id: field.routerId, - userId: user.id, - }, - }); + const router = await prisma.app_RoutingForms_Form.findFirst({ + where: { id: field.routerId, userId: user.id }, + select: { id: true, fields: true, routes: true }, + });
228-234: Only select needed fields from PrismaSame optimization for route router fetch.
- const router = await prisma.app_RoutingForms_Form.findFirst({ - where: { - id: route.id, - userId: user.id, - }, - }); + const router = await prisma.app_RoutingForms_Form.findFirst({ + where: { id: route.id, userId: user.id }, + select: { id: true, fields: true, routes: true }, + });
266-270: Only select needed fields from PrismaYou only read fields for parsing and the id for update.
- const connectedFormDb = await prisma.app_RoutingForms_Form.findUnique({ - where: { - id: connectedForm.id, - }, - }); + const connectedFormDb = await prisma.app_RoutingForms_Form.findUnique({ + where: { id: connectedForm.id }, + select: { id: true, fields: true }, + });packages/trpc/server/routers/apps/routing-forms/formQuery.handler.ts (1)
20-37: Use Prisma select (never include)Replace include with explicit select; keep only required fields.
- const form = await prisma.app_RoutingForms_Form.findFirst({ + const form = await prisma.app_RoutingForms_Form.findFirst({ where: { AND: [ entityPrismaWhereClause({ userId: user.id }), { id: input.id, }, ], }, - include: { - team: { select: { slug: true, name: true } }, - _count: { - select: { - responses: true, - }, - }, - }, + select: { + id: true, + name: true, + description: true, + userId: true, + teamId: true, + createdAt: true, + updatedAt: true, + disabled: true, + updatedById: true, + settings: true, + routes: true, + fields: true, + team: { select: { slug: true, name: true } }, + _count: { select: { responses: true } }, + }, });packages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.ts (2)
27-65: Use Prisma select (never include)Select only needed fields on response, form, user, and team.
- const formResponse = await prisma.app_RoutingForms_FormResponse.findUnique({ - where: { - id: formResponseId, - }, - include: { - form: { - include: { - user: { - select: { - id: true, - movedToProfileId: true, - organization: { - select: { - slug: true, - }, - }, - username: true, - theme: true, - brandColor: true, - darkBrandColor: true, - metadata: true, - }, - }, - team: { - select: { - id: true, - members: true, - slug: true, - parent: { - select: { slug: true }, - }, - parentId: true, - metadata: true, - }, - }, - }, - }, - }, - }); + const formResponse = await prisma.app_RoutingForms_FormResponse.findUnique({ + where: { id: formResponseId }, + select: { + id: true, + response: true, + form: { + select: { + id: true, + name: true, + description: true, + userId: true, + teamId: true, + settings: true, + routes: true, + fields: true, + user: { + select: { + id: true, + movedToProfileId: true, + organization: { select: { slug: true } }, + username: true, + theme: true, + brandColor: true, + darkBrandColor: true, + metadata: true, + }, + }, + team: { + select: { + id: true, + members: true, + slug: true, + parent: { select: { slug: true } }, + parentId: true, + metadata: true, + }, + }, + }, + }, + }, + });
1-99: Remove Prisma include usage from routing-forms routers
- packages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.ts (lines 31–34): nested
includeon form.user and form.team- packages/trpc/server/routers/apps/routing-forms/forms.handler.ts (lines 41–42):
includeon team- packages/trpc/server/routers/apps/routing-forms/formQuery.handler.ts (lines 29–30):
includeon team selectpackages/trpc/server/routers/apps/routing-forms/utils.ts (1)
142-165: Bug: promises from sendGenericWebhookPayload are not returned, so they’re never awaitedThe map callback doesn’t return the promise.
promisesFormSubmittedbecomes an array ofundefined, andPromise.allwon’t await webhook sends.Apply:
- const promisesFormSubmitted = webhooksFormSubmitted.map((webhook) => { - sendGenericWebhookPayload({ + const promisesFormSubmitted = webhooksFormSubmitted.map((webhook) => { + return sendGenericWebhookPayload({ secretKey: webhook.secret, triggerEvent: "FORM_SUBMITTED", createdAt: new Date().toISOString(), webhook, data: { formId: form.id, formName: form.name, teamId: form.teamId, responses: fieldResponsesByIdentifier, }, rootData: { // Send responses unwrapped at root level for backwards compatibility ...Object.entries(fieldResponsesByIdentifier).reduce((acc, [key, value]) => { acc[key] = value.value; return acc; }, {} as Record<string, FormResponse[keyof FormResponse]["value"]>), }, - }).catch((e) => { + }).catch((e) => { console.error(`Error executing routing form webhook`, webhook, e); - }); - }); + }); + });
🧹 Nitpick comments (17)
packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts (1)
89-101: Make updates atomic and avoid mutating input.Wrap updates in a transaction and don’t mutate input.ids with reverse().
- await Promise.all( - input.ids.reverse().map((id, position) => { - return prisma.workflow.update({ - where: { id: id }, - data: { position }, - }); - }) - ); + await prisma.$transaction( + [...input.ids].reverse().map((id, position) => + prisma.workflow.update({ + where: { id }, + data: { position }, + }) + ) + );packages/trpc/server/routers/apps/basecamp3/trpc-router.ts (1)
1-1: Prefer named export over default re-export.Default exports are discouraged; consider re-exporting a named router.
-export { default } from "./_router"; +export { default as basecamp3Router } from "./_router"; +// Or (preferred if _router can expose a named export): +// export { router as basecamp3Router } from "./_router";packages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.ts (3)
25-31: Select only needed fields from incompleteBookingActions.Return a minimal shape to reduce payload.
- const [incompleteBookingActions, form] = await Promise.all([ - prisma.app_RoutingForms_IncompleteBookingActions.findMany({ - where: { formId: input.formId }, - }), + const [incompleteBookingActions, form] = await Promise.all([ + prisma.app_RoutingForms_IncompleteBookingActions.findMany({ + where: { formId: input.formId }, + select: { id: true, formId: true, actionType: true, data: true, enabled: true, credentialId: true }, + }),
18-18: Naming nit: unify casing.Consider renaming getInCompleteBookingSettingsHandler → getIncompleteBookingSettingsHandler.
106-106: Avoid default export.Prefer named export for handlers per repo guidelines.
-export default getInCompleteBookingSettingsHandler; +export { getInCompleteBookingSettingsHandler };packages/trpc/server/routers/apps/routing-forms/saveIncompleteBookingSettings.handler.ts (2)
47-53: Minimize fetch during existence check; consider upsert with uniqueness.You only need id; fetch with select to reduce I/O. If (formId, actionType) is unique, prefer upsert.
- const existingAction = await prisma.app_RoutingForms_IncompleteBookingActions.findFirst({ - where: { - formId: formId, - actionType: actionType, - }, - }); + const existingAction = await prisma.app_RoutingForms_IncompleteBookingActions.findFirst({ + where: { formId, actionType }, + select: { id: true }, + });If unique key exists:
await prisma.app_RoutingForms_IncompleteBookingActions.upsert({ where: { formId_actionType: { formId, actionType } }, update: { data: parsedData.data, enabled: input.enabled, credentialId: input?.credentialId }, create: { formId, actionType, data: parsedData.data, enabled: input.enabled, credentialId: input?.credentialId }, });
79-79: Avoid default export.Use named exports for handlers.
-export default saveIncompleteBookingSettingsHandler; +export { saveIncompleteBookingSettingsHandler };packages/trpc/server/routers/apps/routing-forms/deleteForm.handler.ts (2)
39-40: Fix grammar in user-facing error.Use “its” (possessive), not “it’s” (contraction).
- message: "This form is being used by other forms. Please remove it's usage from there first.", + message: "This form is being used by other forms. Please remove its usage from there first.",
59-59: Avoid default export.Prefer a named export for consistency.
-export default deleteFormHandler; +export { deleteFormHandler };packages/trpc/server/routers/viewer/_router.tsx (1)
45-89: Prefer named exports for app routersThese imports depend on default exports in ../apps/.../_router. Consider switching those modules to named exports for consistency and better tree-shaking.
packages/trpc/server/routers/apps/routing-forms/forms.handler.ts (2)
29-30: Use safeStringify for consistencyAvoid JSON.stringify on complex objects.
- log.debug("Getting forms where", JSON.stringify(where)); + log.debug("Getting forms where", safeStringify(where));
112-112: Prefer named export over defaultExport the handler as a named symbol to match codebase conventions.
packages/trpc/server/routers/apps/routing-forms/formMutation.handler.ts (2)
112-180: Consider wrapping connected-form updates and upsert in a transactionPre-updating connected forms can leave data inconsistent if the main upsert fails. Prefer a single $transaction.
424-424: Prefer named export over defaultAlign with repo convention discouraging default exports.
packages/trpc/server/routers/apps/routing-forms/formQuery.handler.ts (1)
53-53: Prefer named export over defaultDiscourage default exports in ts modules.
packages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.ts (1)
98-98: Prefer named export over defaultMatches repo guideline against default exports.
packages/trpc/server/routers/apps/routing-forms/utils.ts (1)
237-242: Nit: boolean coercion for clarityMinor readability: coerce to boolean to avoid relying on numeric truthiness.
- const isTeamForm = form.teamId; + const isTeamForm = !!form.teamId;
📜 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 (14)
packages/app-store/basecamp3/trpc-router.ts(0 hunks)packages/app-store/routing-forms/trpc-router.ts(0 hunks)packages/trpc/server/routers/apps/basecamp3/trpc-router.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/deleteForm.handler.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/formMutation.handler.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/formQuery.handler.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/forms.handler.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/onFormSubmission.test.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/saveIncompleteBookingSettings.handler.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/utils.ts(2 hunks)packages/trpc/server/routers/viewer/_router.tsx(1 hunks)packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/app-store/basecamp3/trpc-router.ts
- packages/app-store/routing-forms/trpc-router.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/trpc/server/routers/apps/routing-forms/onFormSubmission.test.tspackages/trpc/server/routers/apps/routing-forms/saveIncompleteBookingSettings.handler.tspackages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.tspackages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.tspackages/trpc/server/routers/apps/basecamp3/trpc-router.tspackages/trpc/server/routers/apps/routing-forms/deleteForm.handler.tspackages/trpc/server/routers/viewer/workflows/workflowOrder.handler.tspackages/trpc/server/routers/apps/routing-forms/formMutation.handler.tspackages/trpc/server/routers/apps/routing-forms/forms.handler.tspackages/trpc/server/routers/apps/routing-forms/utils.tspackages/trpc/server/routers/apps/routing-forms/formQuery.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/apps/routing-forms/onFormSubmission.test.tspackages/trpc/server/routers/apps/routing-forms/saveIncompleteBookingSettings.handler.tspackages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.tspackages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.tspackages/trpc/server/routers/apps/basecamp3/trpc-router.tspackages/trpc/server/routers/apps/routing-forms/deleteForm.handler.tspackages/trpc/server/routers/viewer/workflows/workflowOrder.handler.tspackages/trpc/server/routers/apps/routing-forms/formMutation.handler.tspackages/trpc/server/routers/viewer/_router.tsxpackages/trpc/server/routers/apps/routing-forms/forms.handler.tspackages/trpc/server/routers/apps/routing-forms/utils.tspackages/trpc/server/routers/apps/routing-forms/formQuery.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/apps/routing-forms/onFormSubmission.test.tspackages/trpc/server/routers/apps/routing-forms/saveIncompleteBookingSettings.handler.tspackages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.tspackages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.tspackages/trpc/server/routers/apps/basecamp3/trpc-router.tspackages/trpc/server/routers/apps/routing-forms/deleteForm.handler.tspackages/trpc/server/routers/viewer/workflows/workflowOrder.handler.tspackages/trpc/server/routers/apps/routing-forms/formMutation.handler.tspackages/trpc/server/routers/viewer/_router.tsxpackages/trpc/server/routers/apps/routing-forms/forms.handler.tspackages/trpc/server/routers/apps/routing-forms/utils.tspackages/trpc/server/routers/apps/routing-forms/formQuery.handler.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/_router.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-17T22:00:16.329Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.329Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.
Applied to files:
packages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.ts
🔇 Additional comments (12)
packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts (1)
1-1: Import-path change looks good.Localizing the TFormSchema import is fine and non-functional.
packages/trpc/server/routers/apps/routing-forms/getIncompleteBookingSettings.handler.ts (1)
1-1: Import alias switch is fine.Path consolidation to @calcom/app-store is OK.
packages/trpc/server/routers/apps/routing-forms/saveIncompleteBookingSettings.handler.ts (1)
1-1: Import alias switch is fine.packages/trpc/server/routers/apps/routing-forms/deleteForm.handler.ts (1)
1-1: Import alias switch is fine.packages/trpc/server/routers/viewer/_router.tsx (1)
6-7: Import path migration to local app routers — LGTMRemoves dependency on app-store routers in viewer layer; aligns with PR goal.
packages/trpc/server/routers/apps/routing-forms/forms.handler.ts (1)
3-4: Alias imports — LGTMConsolidates to package aliases; no runtime impact.
packages/trpc/server/routers/apps/routing-forms/formMutation.handler.ts (1)
4-11: Alias import migration — LGTMPath refactor only; no behavioral change.
packages/trpc/server/routers/apps/routing-forms/formQuery.handler.ts (1)
1-1: Alias import — LGTMPath-only change.
packages/trpc/server/routers/apps/routing-forms/getResponseWithFormFields.handler.ts (1)
3-5: Alias imports — LGTMKeeps all routing-forms imports consistent.
packages/trpc/server/routers/apps/routing-forms/onFormSubmission.test.ts (1)
30-37: All mocks updated to use alias pathNo remaining tests reference the old relative import for response-email.
packages/trpc/server/routers/apps/routing-forms/utils.ts (2)
3-8: Good: type-only imports via package aliasType-only import reduces runtime coupling and aligns with the migration to aliases.
226-228: Verify that dynamic import alias resolves in build/runtime
Nopathsentry for@calcom/app-store/*in tsconfig.json norexportsin package.json—tests mock this alias, but the runtime build may fail. Ensure your bundler or a tsconfig‐paths plugin handles@calcom/app-store/..., or switch to a relative import.
Optional: convert to a named export (e.g.export class ResponseEmail) and import via destructuring to align with the “no default exports” guideline.
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 (12)
packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts (2)
22-51: Prisma: avoid include; select only what you need and scope by input IDs.Per repo guidelines, don’t use include. This handler only uses workflow IDs to validate ownership, so fetch just those IDs and avoid loading relations. Also constrain the query with id in input.ids to reduce I/O.
Apply this diff:
- const { include: includedFields } = { - include: { - activeOn: { - select: { - eventType: { - select: { - id: true, - title: true, - parentId: true, - _count: { - select: { - children: true, - }, - }, - }, - }, - }, - }, - steps: true, - team: { - select: { - id: true, - slug: true, - name: true, - members: true, - logoUrl: true, - }, - }, - }, - } satisfies Prisma.WorkflowDefaultArgs; + // Only IDs are needed to validate ownership. const allWorkflows = await prisma.workflow.findMany({ - where: { + where: { + id: { in: input.ids }, OR: [ { userId: user.id, }, { team: { members: { some: { userId: user.id, accepted: true, }, }, }, }, ], }, - include: includedFields, + select: { id: true }, orderBy: [ { position: "desc", }, { id: "asc", }, ], });Also applies to: 53-80
89-100: Make reordering atomic and avoid mutating input.
- reverse() mutates input.ids; avoid side effects.
- Use a transaction so positions aren’t left half-updated on failure.
- await Promise.all( - input.ids.reverse().map((id, position) => { - return prisma.workflow.update({ - where: { - id: id, - }, - data: { - position, - }, - }); - }) - ); + const ids = [...input.ids].reverse(); + await prisma.$transaction( + ids.map((id, position) => + prisma.workflow.update({ + where: { id }, + data: { position }, + }) + ) + );packages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.ts (4)
110-112: Do not log secrets; sanitize webhook object in error logs.
webhookcontainssecret; logging it risks leakage.Apply this diff:
- }).catch((e) => { - console.error(`Error executing FORM_SUBMITTED_NO_EVENT webhook`, webhook, e); - }); + }).catch((e) => { + const { subscriberUrl, appId } = webhook ?? {}; + console.error( + `Error executing FORM_SUBMITTED_NO_EVENT webhook`, + { subscriberUrl, appId }, + e + ); + });
83-91: Fix potential crash: guardresponse.responsebeforeObject.values(...).
Object.values(undefined|null)throws. The null-check happens too late (inside the inner callback).Apply this diff:
- const hasDuplicate = - emailValue && - recentResponses.some((response) => { - return Object.values(response.response as Record<string, { value: string; label: string }>).some( - (field) => { - if (!response.response || typeof response.response !== "object") return false; - - return typeof field.value === "string" && field.value.toLowerCase() === emailValue.toLowerCase(); - } - ); - }); + const hasDuplicate = + !!emailValue && + recentResponses.some((response) => { + const r = response.response; + if (!r || typeof r !== "object") return false; + return Object.values(r as Record<string, { value: string; label: string }>).some((field) => { + return typeof field.value === "string" && field.value.toLowerCase() === emailValue.toLowerCase(); + }); + });
43-49: Prisma: select only what you need.Comply with our guideline to always use
selectand minimize fetched data.Apply this diff:
const bookingFromResponse = await prisma.booking.findFirst({ where: { routedFromRoutingFormReponse: { id: responseId, }, }, + select: { id: true }, });
57-71: Prisma: restrict fields for recent responses.Only
responseis used below; limit selection to reduce memory/IO.Apply this diff:
- const recentResponses = - (await prisma.app_RoutingForms_FormResponse.findMany({ + const recentResponses = + (await prisma.app_RoutingForms_FormResponse.findMany({ where: { formId: form.id, createdAt: { gte: sixtyMinutesAgo, lt: new Date(), }, routedToBookingUid: { not: null, }, NOT: { id: responseId, }, }, - })) ?? []; + select: { response: true }, + })) ?? [];packages/trpc/server/routers/apps/basecamp3/projects.handler.ts (3)
38-38: Fix SSRF/token exfil risk by not trusting account.hrefBuild the URL from the known Basecamp host and account.id instead of credential-controlled href.
- const url = `${credentialKey.account.href}/projects.json`; + const url = `https://3.basecampapi.com/${credentialKey.account.id}/projects.json`;
40-44: Handle non-2xx responses (and add a timeout) from BasecampAvoid silent failures and hanging requests.
- const resp = await fetch(url, { - headers: { "User-Agent": user_agent as string, Authorization: `Bearer ${credentialKey.access_token}` }, - }); - const projects = await resp.json(); + const ac = new AbortController(); + const to = setTimeout(() => ac.abort(), 10_000); + const resp = await fetch(url, { + headers: { "User-Agent": user_agent as string, Authorization: `Bearer ${credentialKey.access_token}` }, + signal: ac.signal, + }); + clearTimeout(to); + if (!resp.ok) { + throw new TRPCError({ code: "BAD_GATEWAY", message: `Basecamp projects fetch failed: ${resp.status}` }); + } + const projects = await resp.json();
30-32: Don’t return undefined when account is missing; throw a typed errortRPC consumers expect a defined shape or an error.
- if (!credentialKey.account) { - return; - } + if (!credentialKey.account?.id) { + throw new TRPCError({ code: "PRECONDITION_FAILED", message: "Basecamp account not linked." }); + }packages/trpc/server/routers/apps/basecamp3/projectMutation.handler.ts (3)
55-56: Avoid crash when schedule dock is missing and handle non-2xx
.find(...).idwill throw when the “schedule” dock isn’t present; also check response.ok.- const scheduleJson = await scheduleResponse.json(); - const scheduleId = scheduleJson.dock.find((dock: IDock) => dock.name === "schedule").id; + if (!scheduleResponse.ok) { + throw new TRPCError({ code: "BAD_GATEWAY", message: `Basecamp project fetch failed: ${scheduleResponse.status}` }); + } + const scheduleJson = await scheduleResponse.json(); + const scheduleDock = Array.isArray(scheduleJson?.dock) + ? scheduleJson.dock.find((dock: IDock) => dock.name === "schedule") + : undefined; + if (!scheduleDock?.id) { + throw new TRPCError({ code: "FAILED_PRECONDITION", message: "Basecamp project has no schedule dock." }); + } + const scheduleId = scheduleDock.id;
41-43: Normalize expires_at units before comparisonPrevents premature/late refresh if stored as seconds.
- if (credentialKey.expires_at < Date.now()) { + const expMs = + typeof credentialKey.expires_at === "number" && credentialKey.expires_at < 1e12 + ? credentialKey.expires_at * 1000 + : (credentialKey.expires_at as number); + if (expMs <= Date.now()) { credentialKey = (await refreshAccessToken(credential)) as BasecampToken; }
29-35: Addkeyto the selected fields
refreshAccessTokenreadscredential.key, butcredentialForCalendarServiceSelectcurrently only includesid,appId, andtype. Addkey: true(and any other fields the refresh needs) to avoid missing-data errors. Applies equally to the select at lines 57–60.
🧹 Nitpick comments (12)
packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts (2)
103-104: Simplify SupportedFilters type.Reduces verbosity without changing meaning.
-type SupportedFilters = Omit<NonNullable<NonNullable<TFormSchema>["filters"]>, "upIds"> | undefined; +type SupportedFilters = Omit<NonNullable<TFormSchema["filters"]>, "upIds"> | undefined;
162-168: Drop redundant null-check on entry.entries(filters) won’t yield falsy entries; the filter check below already guards values.
- for (const entry of entries(filters)) { - if (!entry) { - continue; - } + for (const entry of entries(filters)) { const [filterName, filter] = entry;packages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.ts (5)
56-72: Nit:findManynever returnsnull;?? []is unnecessary.Minor cleanup.
Apply this diff:
- const recentResponses = - (await prisma.app_RoutingForms_FormResponse.findMany({ + const recentResponses = await prisma.app_RoutingForms_FormResponse.findMany({ ... - })) ?? []; + });
55-63: Stabilize time window using a singlenow.Avoid slight drift by capturing
nowonce.Apply this diff:
- const sixtyMinutesAgo = new Date(Date.now() - 60 * 60 * 1000); + const now = new Date(); + const sixtyMinutesAgo = new Date(now.getTime() - 60 * 60 * 1000); const recentResponses = (await prisma.app_RoutingForms_FormResponse.findMany({ where: { formId: form.id, createdAt: { gte: sixtyMinutesAgo, - lt: new Date(), + lt: now, },
26-26: Strengthen validation forresponses.
z.any()defeats schema guarantees; prefer a shaped schema (e.g.,z.record(z.object({ value: z.string(), label: z.string() }))) or reuse the TRPC zod schema forFORM_SUBMITTED_WEBHOOK_RESPONSES.Would you like me to draft a zod schema aligned with
FORM_SUBMITTED_WEBHOOK_RESPONSES?
126-131: Guard against missing handler entry.If
actionTypeis unmapped, this would throw.Apply this diff:
- if (emailValue) { - await bookingActionFunction(incompleteBookingAction, emailValue); - } + if (emailValue && typeof bookingActionFunction === "function") { + await bookingActionFunction(incompleteBookingAction, emailValue); + }
115-120: Use a Prismaselectto fetch only needed fields
Handlers/readers use onlyactionType(to pick the function), plusdataandcredentialIdinside the action. Update the query on lines 115–120 to:const incompleteBookingActions = await prisma.app_RoutingForms_IncompleteBookingActions.findMany({ where: { formId: form.id }, select: { actionType: true, data: true, credentialId: true, }, });This limits the payload to only what’s used by the webhook and action handlers.
packages/trpc/server/routers/apps/basecamp3/projects.handler.ts (3)
34-36: Normalize expires_at units before comparing to Date.now()Avoid mismatches if expires_at is seconds.
- if (credentialKey.expires_at < Date.now()) { + const expMs = + typeof credentialKey.expires_at === "number" && credentialKey.expires_at < 1e12 + ? credentialKey.expires_at * 1000 + : (credentialKey.expires_at as number); + if (expMs <= Date.now()) { credentialKey = (await refreshAccessToken(credential)) as BasecampToken; }
1-1: Prefer named exports over default exportsIf @calcom/app-store/_utils/getAppKeysFromSlug supports it, switch to named export for clearer refactors and tree-shaking.
20-25: Filter by Basecamp appId to avoid ambiguous credentials
findFirst by only userId may return any of a user’s credentials. Add an appId (and/or type) filter since credentialForCalendarServiceSelect already selects those fields. E.g.:const credential = await prisma.credential.findFirst({ where: { - userId: user?.id, + userId: user?.id, + appId: "basecamp3", }, select: credentialForCalendarServiceSelect, });Ensure
"basecamp3"matches the slug in your App table (and add a type: "basecamp3" clause if your logic uses the type field).packages/trpc/server/routers/apps/basecamp3/projectMutation.handler.ts (2)
45-54: Add a request timeout to Basecamp fetchImprove resilience under network stalls.
- const scheduleResponse = await fetch( + const ac = new AbortController(); + const to = setTimeout(() => ac.abort(), 10_000); + const scheduleResponse = await fetch( `https://3.basecampapi.com/${basecampUserId}/projects/${projectId}.json`, { headers: { "User-Agent": user_agent as string, Authorization: `Bearer ${credentialKey.access_token}`, }, + signal: ac.signal, } ); + clearTimeout(to);
1-1: Prefer named exports over default exportsIf possible, refactor getAppKeysFromSlug to a named export.
📜 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 (4)
packages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.ts(1 hunks)packages/trpc/server/routers/apps/basecamp3/projectMutation.handler.ts(1 hunks)packages/trpc/server/routers/apps/basecamp3/projects.handler.ts(1 hunks)packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.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/trpc/server/routers/apps/basecamp3/projects.handler.tspackages/trpc/server/routers/apps/basecamp3/projectMutation.handler.tspackages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.tspackages/trpc/server/routers/viewer/workflows/workflowOrder.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/apps/basecamp3/projects.handler.tspackages/trpc/server/routers/apps/basecamp3/projectMutation.handler.tspackages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.tspackages/trpc/server/routers/viewer/workflows/workflowOrder.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/apps/basecamp3/projects.handler.tspackages/trpc/server/routers/apps/basecamp3/projectMutation.handler.tspackages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.tspackages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts
🧠 Learnings (1)
📚 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/trpc/server/routers/apps/basecamp3/projects.handler.tspackages/trpc/server/routers/apps/basecamp3/projectMutation.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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (6)
packages/trpc/server/routers/viewer/workflows/workflowOrder.handler.ts (2)
9-9: Import path update looks correct.Relative path resolves from viewer/workflows → ../.. → apps/routing-forms/forms.schema. No runtime impact.
89-100: No DB-level unique constraint onWorkflow.position; two-phase offset is unnecessary
The Prisma schema showsposition Int @default(0)with no@uniqueor@@uniqueonposition, so transient unique-key conflicts won’t occur. If you do need to enforce per-user or per-team ordering at the database level, add a composite unique (e.g.@@unique([userId, position])); otherwise, no change required.packages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.ts (1)
6-6: Type-only import path update looks good.Type-only keeps it compile-time only; no runtime coupling introduced.
packages/trpc/server/routers/apps/basecamp3/projects.handler.ts (1)
43-45: Good: No leakage of credential.key in API responseOnly derived fields are returned; aligns with data-exposure guidelines.
packages/trpc/server/routers/apps/basecamp3/projectMutation.handler.ts (2)
62-63: Good: Response does not expose credential.keyComplies with API exposure rules.
33-34: Good: Using Prisma select (not include)Aligns with query minimization guidance.
| @@ -1,13 +1,244 @@ | |||
| import type { Prisma } from "@prisma/client"; | |||
| import type { App_RoutingForms_Form, User } from "@prisma/client"; | |||
There was a problem hiding this comment.
All the new code added to this file are copied from packages/app-store/routing-forms/trpc/utils.ts
| import { createFallbackRoute } from "../lib/createFallbackRoute"; | ||
| import { getSerializableForm } from "../lib/getSerializableForm"; | ||
| import { isFallbackRoute } from "../lib/isFallbackRoute"; | ||
| import { isFormCreateEditAllowed } from "../lib/isFormCreateEditAllowed"; |
There was a problem hiding this comment.
isFormCreateEditAllowed wasn't being used
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
packages/trpc/server/routers/apps/routing-forms/formMutation.schema.ts (1)
6-18: Optional: harden schema (.strict) and align on nullishConsider rejecting unknown keys and using
.nullish()consistently.export const ZFormMutationInputSchema = z.object({ id: z.string(), name: z.string(), - description: z.string().nullable().optional(), + description: z.string().nullish(), disabled: z.boolean().optional(), fields: zodFields, routes: zodRoutes, addFallback: z.boolean().optional(), duplicateFrom: z.string().nullable().optional(), teamId: z.number().nullish().default(null), shouldConnect: z.boolean().optional(), settings: RoutingFormSettings.optional(), -}); +}).strict();packages/app-store/routing-forms/lib/formSubmissionUtils.ts (9)
22-35: Tighten webhook response types to avoid nested arrays and redundancy
SelectFieldWebhookResponsecurrently includesstring[], andresponseunions it again (and also allowsSelectFieldWebhookResponse[]), which permits shapes likestring[][]. Narrow the types to a single-item type and its array form.-type SelectFieldWebhookResponse = string | number | string[] | { label: string; id: string | null }; -export type FORM_SUBMITTED_WEBHOOK_RESPONSES = Record< +type SelectFieldItem = string | number | { label: string; id: string | null }; +export type FORM_SUBMITTED_WEBHOOK_RESPONSES = Record< string, { /** * Deprecates `value` prop as it now has both the id(that doesn't change) and the label(that can change but is human friendly) */ - response: number | string | string[] | SelectFieldWebhookResponse | SelectFieldWebhookResponse[]; + response: number | string | string[] | SelectFieldItem | SelectFieldItem[]; /** * @deprecated Use `response` instead */ value: FormResponse[keyof FormResponse]["value"]; } >;
37-60: Make predicate a boolean type guard; remove redundant options null-check
isOptionsFieldreturnsfield.options(an array) instead of a boolean, and we re-checkfield.optionslater. Make it a proper type guard and delete the dead branch.-function isOptionsField(field: Pick<SerializableField, "type" | "options">) { - return (field.type === "select" || field.type === "multiselect") && field.options; -} +function isOptionsField( + field: Pick<SerializableField, "type" | "options"> +): field is Pick<SerializableField, "type"> & { options: NonNullable<SerializableField["options"]> } { + return (field.type === "select" || field.type === "multiselect") && Array.isArray(field.options) && field.options.length > 0; +} @@ - if (!field.options) { - return { - value: fieldResponseValue, - response: fieldResponseValue, - }; - }
88-102: Skip email when no recipients; de-duplicate addressesPrevent no-op work and duplicate sends.
export const sendResponseEmail = async ( form: Pick<App_RoutingForms_Form, "id" | "name" | "fields">, orderedResponses: OrderedResponses, toAddresses: string[] ) => { - try { + try { + const uniqTo = [...new Set(toAddresses)].filter(Boolean); + if (uniqTo.length === 0) return; if (typeof window === "undefined") { const { default: ResponseEmail } = await import("../emails/templates/response-email"); - const email = new ResponseEmail({ form: form, toAddresses, orderedResponses }); + const email = new ResponseEmail({ form: form, toAddresses: uniqTo, orderedResponses }); await email.sendEmail(); } } catch (e) { moduleLogger.error("Error sending response email", e); } };
104-109: Avoid truthy check for teamIdIf
teamIdcould ever be0, the current truthy check misclassifies the form. Use an explicit nullish check.- const isTeamForm = form.teamId; + const isTeamForm = form.teamId != null;
111-127: Enforce server-only execution for submission side-effectsThis function sends webhooks and emails. Add an explicit guard (or
import "server-only"in Next.js) to prevent accidental browser execution.export async function _onFormSubmission( @@ ) { + if (typeof window !== "undefined") { + throw new TRPCError({ code: "FORBIDDEN", message: "onFormSubmission must run on the server" }); + }
129-143: Stop trusting client-provided labels for webhook keys; reuse the found field + stable keyUse the already-found
fieldand preferidentifier ?? field.label(oridentifier ?? field.id) to avoid relying on the submission’s label. This reduces collision/spoofing risk. Verify BC with existing consumers.- for (const [fieldId, fieldResponse] of Object.entries(response)) { - const field = form.fields.find((f) => f.id === fieldId); + for (const [fieldId, fieldResponse] of Object.entries(response)) { + const field = form.fields.find((f) => f.id === fieldId); if (!field) { throw new Error(`Field with id ${fieldId} not found`); } - // Use the label lowercased as the key to identify a field. - // TODO: We seem to be using label from the response, Can we not use the field.label - const key = - form.fields.find((f) => f.id === fieldId)?.identifier || - (fieldResponse.label as keyof typeof fieldResponsesByIdentifier); + // Prefer identifier; fallback to field.label to keep key stable and server-sourced + const key = (field.identifier || field.label) as keyof typeof fieldResponsesByIdentifier; fieldResponsesByIdentifier[key] = getFieldResponse({ fieldResponseValue: fieldResponse.value, field, }); }
192-193: Remove double await in dynamic importNo need to await the default export.
- const tasker: Tasker = await (await import("@calcom/features/tasker")).default; + const tasker: Tasker = (await import("@calcom/features/tasker")).default;
194-210: Schedule in UTC for consistency across hosts (optional)If
@calcom/dayjshas.utc(), prefer UTC for deterministic scheduling; otherwise ignore.- const scheduledAt = dayjs().add(15, "minute").toDate(); + const scheduledAt = dayjs.utc().add(15, "minute").toDate();
215-220: Guard against missing field responses when building orderedResponsesAvoid pushing
undefinedif a field has no submitted value; log or skip.- const orderedResponses = form.fields.reduce((acc, field) => { - acc.push(response[field.id]); - return acc; - }, [] as OrderedResponses); + const orderedResponses = form.fields.reduce<OrderedResponses>((acc, field) => { + const r = (response as Record<string, OrderedResponses[number]>)[field.id]; + if (r) acc.push(r); + else moduleLogger.warn(`Missing response for field ${field.id}`); + return acc; + }, []);
📜 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 (4)
packages/app-store/routing-forms/lib/formSubmissionUtils.test.ts(1 hunks)packages/app-store/routing-forms/lib/formSubmissionUtils.ts(1 hunks)packages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.ts(1 hunks)packages/trpc/server/routers/apps/routing-forms/formMutation.schema.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/app-store/routing-forms/lib/formSubmissionUtils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/tasker/tasks/triggerFormSubmittedNoEvent/triggerFormSubmittedNoEventWebhook.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/apps/routing-forms/formMutation.schema.tspackages/app-store/routing-forms/lib/formSubmissionUtils.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/apps/routing-forms/formMutation.schema.tspackages/app-store/routing-forms/lib/formSubmissionUtils.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/apps/routing-forms/formMutation.schema.tspackages/app-store/routing-forms/lib/formSubmissionUtils.ts
🧬 Code graph analysis (1)
packages/app-store/routing-forms/lib/formSubmissionUtils.ts (5)
packages/app-store/routing-forms/types/types.d.ts (4)
FormResponse(19-27)SerializableField(83-83)OrderedResponses(66-66)SerializableForm(42-54)packages/lib/getOrgIdFromMemberOrTeamId.ts (1)
getOrgIdFromMemberOrTeamId(47-58)packages/features/webhooks/lib/sendPayload.ts (1)
sendGenericWebhookPayload(233-257)packages/features/tasker/tasker.ts (1)
Tasker(42-48)packages/lib/sentryWrapper.ts (1)
withReporting(107-139)
⏰ 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 (4)
packages/trpc/server/routers/apps/routing-forms/formMutation.schema.ts (2)
3-3: Centralizing zod import—LGTMSwitching to the package alias removes brittle relative paths and aligns with the refactor goal.
3-3: Fix missing path alias, exports, and circular dependency
- Add a
@calcom/app-store/*entry in your TSConfigpathsso imports resolve inpackages/trpc- Ensure
packages/app-store/routing-forms/zodexists and exports bothzodFieldsandzodRoutes- Remove all
@calcom/trpcimports from thepackages/app-storepackage to break the circular dependency⛔ Skipped due to learnings
Learnt from: nangelina PR: calcom/cal.com#23486 File: packages/app-store/kyzonspacevideo/package.json:7-12 Timestamp: 2025-09-01T10:25:51.923Z Learning: In Cal.com's monorepo, app-store packages don't need to declare `zod` as a direct dependency in their package.json files. The monorepo uses yarn workspaces with dependency hoisting, where `zod` is available through workspace-level dependency management. Most app-store packages successfully import zod without declaring it as a dependency, following the established monorepo pattern.packages/app-store/routing-forms/lib/formSubmissionUtils.ts (2)
80-86: Confirm single-select webhook shape (array vs single object)
responsefor select fields is always an array of{label,id}even for single-select. Verify consumers expect an array for single-select; otherwise return a single object forselectand an array formultiselect.
279-295: Prisma query follows select-only guideline — LGTMUses
findManywith a minimalselectand noinclude. This aligns with our Prisma data-shaping rules.
What does this PR do?
packages/app-store/basecamp3/trpc/->packages/trpc/server/routers/apps/basecamp3/packages/app-store/routing-forms/trpc/->packages/trpc/server/routers/apps/routing-forms/Mandatory Tasks (DO NOT REMOVE)
How should this be tested?