refactor: circular deps between app store and lib [5]#23936
Conversation
WalkthroughRepoints numerous imports across the monorepo from legacy package paths (mostly @calcom/lib) to new package locations (notably @calcom/app-store and @calcom/features/*). Adds a new exported locationsResolver(t) in packages/app-store/locations.ts and removes the legacy locationsResolver file. Moves formatPrice into packages/lib/currencyConversions and removes it from packages/lib/price. Updates videoClient imports/exports and adjusts tests/mocks to the new module paths. No exported function signatures or runtime control flow were changed beyond import/source relocations. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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
|
@CodeRabbit Go through the file changes and give me a list of the paths of the files that were migrated |
| /* eslint-disable @typescript-eslint/no-empty-function */ | ||
| import type { TFunction } from "i18next"; | ||
| import { isValidPhoneNumber } from "libphonenumber-js"; | ||
| // eslint-disable-next-line @calcom/eslint/deprecated-imports-next-router | ||
| import { z } from "zod"; | ||
|
|
||
| import { getEventLocationType } from "@calcom/app-store/locations"; | ||
|
|
||
| export const locationsResolver = (t: TFunction) => { | ||
| return z | ||
| .array( | ||
| z | ||
| .object({ | ||
| type: z.string(), | ||
| address: z.string().optional(), | ||
| link: z.string().url().optional(), | ||
| phone: z | ||
| .string() | ||
| .refine((val) => isValidPhoneNumber(val)) | ||
| .optional(), | ||
| hostPhoneNumber: z | ||
| .string() | ||
| .refine((val) => isValidPhoneNumber(val)) | ||
| .optional(), | ||
| displayLocationPublicly: z.boolean().optional(), | ||
| credentialId: z.number().optional(), | ||
| teamName: z.string().optional(), | ||
| }) | ||
| .passthrough() | ||
| .superRefine((val, ctx) => { | ||
| if (val?.link) { | ||
| const link = val.link; | ||
| const eventLocationType = getEventLocationType(val.type); | ||
| if ( | ||
| eventLocationType && | ||
| !eventLocationType.default && | ||
| eventLocationType.linkType === "static" && | ||
| eventLocationType.urlRegExp | ||
| ) { | ||
| const valid = z.string().regex(new RegExp(eventLocationType.urlRegExp)).safeParse(link).success; |
There was a problem hiding this comment.
Moved to @calcom/app-store/locations.ts
| import { convertFromSmallestToPresentableCurrencyUnit } from "@calcom/app-store/_utils/payments/currencyConversions"; | ||
|
|
||
| export const formatPrice = (price: number, currency: string | undefined, locale = "en") => { | ||
| switch (currency) { | ||
| case "BTC": | ||
| return `${price} sats`; | ||
| default: | ||
| currency = currency?.toUpperCase() || "USD"; | ||
| return `${Intl.NumberFormat(locale, { | ||
| style: "currency", | ||
| currency: currency, | ||
| }).format(convertFromSmallestToPresentableCurrencyUnit(price, currency))}`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Removed this file and created a @calcom/lib/currencyConversion.ts file that contains formatPrice as well as all the util functions that were in @calcom/app-store/_utils/payments/currencyConversions.ts
| } | ||
| }; | ||
|
|
||
| export const formatPrice = (price: number, currency: string | undefined, locale = "en") => { |
There was a problem hiding this comment.
Copied from @calcom/lib/price.ts that is now removed
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@hbjORbj Based on my analysis of the PR changes, here are the paths of the files that were migrated during this circular dependency refactor: Files Migrated/Moved:From app store to lib:
Modified files with import changes:
The migration primarily involved moving app-related utility functions from the app store package to the lib package to break circular dependencies, while updating all the import statements throughout the codebase to reference the new locations. 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 7862 🏁 Script executed: Length of output: 358 @hbjORbj Let me analyze the PR diff more thoroughly to identify the migrated files: Based on my analysis of the PR changes, here are the paths of the files that were migrated during this circular dependency refactor: Files Migrated (Based on Analysis):The GitHub CLI output shows this PR modified 69+ files, but most appear to be import updates rather than file migrations. However, looking at the pattern of changes and the PR objective to reduce circular dependencies between app store and lib, the key migrations likely include: Files likely migrated from app-store to lib:
Files with significant import changes (indicating dependency restructuring):
Note: The exact list of migrated files would require examining the specific diff content to see which functions/utilities were moved from the app-store package to the lib package. The majority of the 69+ changed files appear to be import statement updates throughout the codebase to accommodate the new locations of the migrated utilities. Would you like me to examine specific files or sections of the diff to provide more detailed information about what was actually moved? 🏁 Script executed: Length of output: 4212 🏁 Script executed: Length of output: 11838 🏁 Script executed: Length of output: 1216 🏁 Script executed: Length of output: 1161 🏁 Script executed: Length of output: 8204 🏁 Script executed: Length of output: 765 @hbjORbj Based on my analysis of the PR diff, here are the specific file paths that were migrated during this circular dependency refactor: Files Migrated (Moved from one location to another):1. From
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
packages/features/tasker/tasks/analytics/sendAnalyticsEvent.ts (1)
29-33: Avoid logging raw payload; risk of PII leakage and noisy logsOn failures we log the entire
payload. Depending on schema, this may contain PII. Log minimal context (size, a short sample, or a hash) instead.Apply this diff to reduce exposure:
- log.error( - `[Will retry] Error creating analytics event: error: ${safeStringify(err)} payload: ${safeStringify({ - payload, - })}` - ); + const payloadSize = typeof payload === "string" ? payload.length : 0; + const payloadPreview = typeof payload === "string" ? payload.slice(0, 256) : ""; + log.error( + `[Will retry] Error creating analytics event: error: ${safeStringify(err)} ` + + `payloadSize:${payloadSize} payloadPreview:${safeStringify(payloadPreview)}` + );If you have known safe fields (e.g., eventName), prefer an allowlist preview over a substring.
packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (4)
61-71: Prisma: avoid include; select only needed fieldsReplace nested include with select to comply with our Prisma guidelines and cut payload.
- eventType: { - include: { - team: { - select: { - id: true, - name: true, - parentId: true, - }, - }, - }, - }, + eventType: { + select: { + teamId: true, + parentId: true, + hideOrganizerEmail: true, + customReplyToEmail: true, + bookingFields: true, + team: { + select: { id: true, name: true, parentId: true }, + }, + }, + },
73-75: Prisma: narrow attendees and references selectionSelect only fields used by this handler.
- attendees: true, - references: true, + attendees: { + select: { + email: true, + name: true, + username: true, + locale: true, + timeZone: true, + phoneNumber: true, + }, + }, + references: { + select: { + uid: true, + type: true, + credentialId: true, + delegationCredentialId: true, + externalCalendarId: true, + }, + },
108-110: Prisma: select only teamIdAvoid returning full membership objects.
- select: - teams: true, - , + select: { + teams: { select: { teamId: true } }, + },
134-139: Missing slug in eventType selectevent.slug is later read for payload; currently always undefined.
const bookingBelongsToTeam = !!bookingToReschedule.eventType?.teamId; … if (bookingToReschedule.eventTypeId) { event = await prisma.eventType.findUniqueOrThrow({ select: { + slug: true, title: true, schedulingType: true, recurringEvent: true, },apps/web/app/api/recorded-daily-video/route.ts (1)
2-2: Webhook signature: use constant‑time comparisonAvoid string equality for HMAC verification; use timingSafeEqual and compare raw digests.
-import { createHmac } from "crypto"; +import { createHmac, timingSafeEqual } from "crypto";-const computeSignature = (hmacSecret: string, reqBody: any, webhookTimestampHeader: string | null) => { +const computeSignature = (hmacSecret: string, reqBody: unknown, webhookTimestampHeader: string | null) => { const signature = `${webhookTimestampHeader}.${JSON.stringify(reqBody)}`; const base64DecodedSecret = Buffer.from(hmacSecret, "base64"); const hmac = createHmac("sha256", base64DecodedSecret); - const computed_signature = hmac.update(signature).digest("base64"); - return computed_signature; + return hmac.update(signature).digest(); };- const webhookTimestamp = headersList.get("x-webhook-timestamp"); - const computed_signature = computeSignature(hmacSecret, body, webhookTimestamp); - - if (headersList.get("x-webhook-signature") !== computed_signature) { + const webhookTimestamp = headersList.get("x-webhook-timestamp"); + const providedSigB64 = headersList.get("x-webhook-signature"); + if (!providedSigB64) { + return NextResponse.json({ message: "Missing signature" }, { status: 403 }); + } + const providedSig = Buffer.from(providedSigB64, "base64"); + const computedSig = computeSignature(hmacSecret, body, webhookTimestamp); + if (providedSig.length !== computedSig.length || !timingSafeEqual(providedSig, computedSig)) { return NextResponse.json({ message: "Signature does not match" }, { status: 403 }); }packages/app-store/vital/lib/reschedule.ts (1)
35-56: Replace Prisma include with select; drop extra query.Repo guideline: for Prisma, only select needed fields; never use include. You can also eliminate the second eventType query by selecting title here.
- eventType: { - include: { - team: { - select: { - id: true, - name: true, - }, - }, - }, - }, + eventType: { + select: { + title: true, + hideOrganizerEmail: true, + team: { select: { id: true, name: true } }, + }, + }, @@ - const userOwner = bookingToReschedule.user; - const event = await prisma.eventType.findUniqueOrThrow({ - select: { - title: true, - }, - where: { - id: bookingToReschedule.eventTypeId, - }, - }); + const userOwner = bookingToReschedule.user; @@ - type: event.title, + type: bookingToReschedule.eventType.title, @@ - hideOrganizerEmail: bookingToReschedule.eventType?.hideOrganizerEmail, + hideOrganizerEmail: bookingToReschedule.eventType?.hideOrganizerEmail, @@ - team: !!bookingToReschedule.eventType?.team + team: !!bookingToReschedule.eventType?.team ? { - name: bookingToReschedule.eventType.team.name, - id: bookingToReschedule.eventType.team.id, + name: bookingToReschedule.eventType.team.name, + id: bookingToReschedule.eventType.team.id, members: [], } : undefined,Also applies to: 70-77, 111-129
packages/features/form-builder/FormBuilder.tsx (3)
221-226: Potential runtime crash when options source is missingDestructuring with a fallback of [] will throw if the record key is absent.
Apply this guard:
- const { - source: { label: sourceLabel }, - value, - } = dataStore.options[field.getOptionsAt as keyof typeof dataStore] ?? []; - options = value; - options.forEach((option) => { - sources.push({ - id: option.value, - label: sourceLabel, - type: "system", - }); - }); + const entry = dataStore.options[field.getOptionsAt as keyof typeof dataStore]; + if (entry) { + const { source: { label: sourceLabel }, value } = entry; + options = value; + options.forEach((option) => { + sources.push({ id: option.value, label: sourceLabel, type: "system" }); + }); + } else { + options = []; + }
441-452: Avoid side-effects in render (calling onChange during render)Calling onChange while rendering can double-fire under React 18 Strict Mode and cause jitter. Initialize via effect instead.
- if (!value) { - onChange([ - { label: "Option 1", value: "Option 1" }, - { label: "Option 2", value: "Option 2" }, - ]); - } + useEffect(() => { + if (!value || value.length === 0) { + onChange?.([ + { label: t("option_1"), value: t("option_1") }, + { label: t("option_2"), value: t("option_2") }, + ]); + } + }, [value, onChange, t]);
756-767: Avoid side-effects in render for “required” defaultingSet a defaultValue on the Controller instead of mutating during render.
- <Controller - name="required" - control={fieldForm.control} + <Controller + name="required" + control={fieldForm.control} + defaultValue={true} render={({ field: { value, onChange } }) => { - let isRequired = shouldConsiderRequired + let isRequired = shouldConsiderRequired ? shouldConsiderRequired(fieldForm.getValues()) : value; - //by default question should be required - if (isRequired === undefined) { - isRequired = true; - onChange(isRequired); - }packages/app-store/locations.ts (1)
460-466: Bug: assignment typo uses equality (==) instead of =This prevents the intended message from being set for cancelled/rejected bookings.
- locationToDisplay == t("web_conference"); + locationToDisplay = t("web_conference");apps/api/v1/pages/api/bookings/[id]/transcripts/[recordingId]/_get.ts (1)
61-67: Prisma: replace include with select (repo guideline).Avoid
include; select only needed fields fromreferences.- const booking = await prisma.booking.findUnique({ - where: { id: bookingId }, - // eslint-disable-next-line @calcom/eslint/no-prisma-include-true - include: { references: true }, - }); + const booking = await prisma.booking.findUnique({ + where: { id: bookingId }, + select: { + references: { + select: { type: true, meetingId: true }, + }, + }, + });packages/features/bookings/lib/handleNewBooking.ts (1)
1184-1189: Prisma: use select for profile lookup.Select only
organizationIdper guideline; no need for full row.- const organizerOrganizationProfile = await prisma.profile.findFirst({ - where: { - userId: organizerUser.id, - username: dynamicUserList[0], - }, - }); + const organizerOrganizationProfile = await prisma.profile.findFirst({ + where: { + userId: organizerUser.id, + username: dynamicUserList[0], + }, + select: { organizationId: true }, + });
🧹 Nitpick comments (47)
packages/features/tasker/tasks/analytics/sendAnalyticsEvent.ts (2)
5-5: Prefer named exports over default exports for better tree‑shaking and refactorsIf
./analyticsManagersupports it, switch to a named export.Apply this diff if feasible:
-import AnalyticsManager from "./analyticsManager"; +import { AnalyticsManager } from "./analyticsManager";If it doesn’t yet export a named symbol, consider re-exporting it as named in that module to align with repo guidelines.
25-26: Remove unreachable null-check afternew
new AnalyticsManager(credential)won’t return a falsy value; the check is dead code.Apply this diff:
- const manager = new AnalyticsManager(credential); - - if (!manager) return; + const manager = new AnalyticsManager(credential);packages/features/eventtypes/lib/checkForEmptyAssignment.ts (2)
1-1: Type-only import LGTM; consider extracting shared types to avoid lib → components dependencyImporting as
import typeis correct. To improve layering, moveEventTypeAssignedUsers/EventTypeHoststo a shared types module solib/doesn’t depend oncomponents/.Apply within-file change:
-import type { EventTypeAssignedUsers, EventTypeHosts } from "../components/EventType"; +import type { EventTypeAssignedUsers, EventTypeHosts } from "../types";New shared types barrel (create file packages/features/eventtypes/types.ts):
export type { EventTypeAssignedUsers, EventTypeHosts } from "./components/EventType";
17-29: Simplify to a single returnSame behavior, fewer branches.
- // If Team-events have assignAllTeamMembers checked, return false as assignment is complete. - if (assignAllTeamMembers) { - return false; - } - - // For managed eventtype check if assigned users are empty. - // For non-managed eventtype check if hosts are empty. - if (isManagedEventType ? assignedUsers.length === 0 : hosts.length === 0) { - return true; - } - - return false; + // Empty only if not assigning all members and the relevant list is empty. + return ( + !assignAllTeamMembers && + (isManagedEventType ? assignedUsers.length === 0 : hosts.length === 0) + );packages/app-store/wipemycalother/lib/reschedule.ts (4)
22-22: Prefer named exports over default exports.This is a library utility, not a page/layout; follow our guideline to aid tree‑shaking and refactors.
-const Reschedule = async (bookingUid: string, cancellationReason: string) => { +export const Reschedule = async (bookingUid: string, cancellationReason: string) => { … -}; - -export default Reschedule; +};Also applies to: 184-184
89-90: Use Date object for Prisma DateTime.Avoid passing ISO strings; Prisma expects
Dateand will serialize appropriately.- updatedAt: dayjs().toISOString(), + updatedAt: dayjs().toDate(),
92-95: Potential crash when attendees array is empty.
[mainAttendee] = bookingToReschedule.attendees;followed bymainAttendee.localewill throw if there are no attendees. Add a guard/fallback.- const [mainAttendee] = bookingToReschedule.attendees; - const tAttendees = await getTranslation(mainAttendee.locale ?? "en", "common"); + const [mainAttendee] = bookingToReschedule.attendees; + const tAttendees = await getTranslation((mainAttendee?.locale ?? "en"), "common"); + // Optionally, early-return or log if attendees are unexpectedly empty.
95-109: Ensure usersToPeopleType always returns Person[].It may return
undefinedwhenusersis falsy; tighten to always return an array.- const usersToPeopleType = ( + const usersToPeopleType = ( users: PersonAttendeeCommonFields[], selectedLanguage: TFunction ): Person[] => { - return users?.map((user) => { + if (!users || users.length === 0) return []; + return users.map((user) => { return { email: user.email || "", name: user.name || "", username: user?.username || "", language: { translate: selectedLanguage, locale: user.locale || "en" }, timeZone: user?.timeZone, phoneNumber: user?.phoneNumber, }; }); };packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (4)
81-81: Prisma: avoid returning full workflow remindersWe only need ids to delete.
- workflowReminders: true, + workflowReminders: { select: { id: true } },Ensure WorkflowRepository.deleteAllWorkflowReminders accepts an array with only ids.
129-129: Unreachable guardfindUniqueOrThrow throws on missing record; this check never runs.
- if (!bookingToReschedule) return;
152-154: Use Date instead of ISO string for DateTime fieldPrisma DateTime accepts Date; prefer Date to avoid implicit parsing.
- updatedAt: dayjs().toISOString(), + updatedAt: new Date(),
162-164: Consistent safe loggingUse safeStringify instead of JSON.stringify.
- await Promise.all(webhookPromises).catch((error) => { - log.error("Error while deleting scheduled webhook triggers", JSON.stringify({ error })); - }); + await Promise.all(webhookPromises).catch((error) => { + log.error("Error while deleting scheduled webhook triggers", safeStringify({ error })); + });packages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts (1)
59-70: Consider transactional deletesDelete attendee and seat in a single transaction to avoid partial state on failure.
- await Promise.all([ - prisma.bookingSeat.delete({ - where: { referenceUid: seatReferenceUid }, - }), - prisma.attendee.delete({ - where: { id: seatReference.attendeeId }, - }), - ]); + await prisma.$transaction([ + prisma.bookingSeat.delete({ where: { referenceUid: seatReferenceUid } }), + prisma.attendee.delete({ where: { id: seatReference.attendeeId } }), + ]);packages/platform/atoms/event-types/wrappers/EventPaymentsTabPlatformWrapper.tsx (1)
20-40: Localize user-facing strings and guard window usageUse t() for text and avoid direct window access during SSR.
+import { useLocale } from "@calcom/lib/hooks/useLocale"; … const EventPaymentsTabPlatformWrapper = ({ eventType }: { eventType: EventTypeSetupProps["eventType"] }) => { + const { t } = useLocale(); const { allowConnect, checked } = useCheck({ teamId: eventType.teamId }); … - if (!checked) return <div>Checking...</div>; + if (!checked) return <div>{t("checking")}</div>; … <EmptyScreen Icon="grid-3x3" - headline="Stripe not connected" - description="You need to connect Stripe to use this feature. Please click on the button below to connect." + headline={t("stripe_not_connected")} + description={t("stripe_connect_required_desc")} buttonRaw={ <StripeConnect teamId={eventType.teamId} - label="Connect to Stripe" - loadingLabel="Connect to Stripe" - alreadyConnectedLabel="Connect to Stripe" + label={t("connect_to_stripe")} + loadingLabel={t("connect_to_stripe")} + alreadyConnectedLabel={t("connect_to_stripe")} isClickable={true} color="secondary" - redir={window.location.href} + redir={typeof window !== "undefined" ? window.location.href : ""} /> } />Please confirm translation keys exist; if not, I can add them.
packages/features/instant-meeting/handleInstantMeeting.ts (2)
260-264: Prisma: drop unnecessary include on createWe don’t use attendees from the returned object; avoid extra read.
- const createBookingObj = { - include: { - attendees: true, - }, - data: newBookingData, - }; + const createBookingObj = { + data: newBookingData, + };
347-350: Prefer named export over default (keep default temporarily if needed)Aligns with repo guidance on named exports.
-export default async function handler(req: NextApiRequest) { - return _handler(req.body); -} +export async function handler(req: NextApiRequest) { + return _handler(req.body); +} +// Back-compat; remove in follow-up PR +export default handler;packages/features/eventtypes/components/tabs/apps/EventAppsTab.tsx (1)
179-181: Localize “App Store” labelAvoid hardcoded text in TSX; use t().
- <Link key="available_apps_desc" className="cursor-pointer underline" href="/apps"> - App Store - </Link>, + <Link key="available_apps_desc" className="cursor-pointer underline" href="/apps"> + {t("app_store")} + </Link>,packages/features/bookings/lib/handleSeats/lib/lastAttendeeDeleteBooking.ts (2)
57-66: Harden external deletions: use allSettled and always cancel booking (with logging).Today, any single rejection prevents the booking status update and swallows errors. Prefer allSettled, log failures, and still mark the booking cancelled.
- await Promise.all(integrationsToDelete).then(async () => { - await prisma.booking.update({ - where: { - id: originalRescheduledBooking.id, - }, - data: { - status: BookingStatus.CANCELLED, - }, - }); - }); + const results = await Promise.allSettled(integrationsToDelete); + const failures = results.filter((r) => r.status === "rejected"); + if (failures.length) { + // TODO: replace console with structured logger + console.error("Failed to delete some integrations", failures); + } + await prisma.booking.update({ + where: { id: originalRescheduledBooking.id }, + data: { status: BookingStatus.CANCELLED }, + });
15-19: Prefer named export over default for libraries.Improves tree‑shaking and refactors.
-const lastAttendeeDeleteBooking = async ( +export const lastAttendeeDeleteBooking = async ( originalRescheduledBooking: OriginalRescheduledBooking, filteredAttendees: Partial<Attendee>[] | undefined, originalBookingEvt?: CalendarEvent ) => { ... -}; - -export default lastAttendeeDeleteBooking; +};Also applies to: 72-72
packages/app-store/vital/lib/reschedule.ts (1)
182-182: Prefer named export over default.-export default Reschedule; +export { Reschedule };apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/RouteBuilder.tsx (1)
530-531: Localize hard‑coded UI strings.Per guidelines, wrap visible text in t(). Examples below.
- <span className="text-emphasis ml-2 text-sm font-medium">Conditions</span> + <span className="text-emphasis ml-2 text-sm font-medium">{t("conditions")}</span> @@ - <span className="text-emphasis ml-2 text-sm font-medium"> - And connect with specific team members - </span> + <span className="text-emphasis ml-2 text-sm font-medium"> + {t("connect_with_specific_team_members")} + </span> @@ - <span className="text-emphasis ml-2 text-sm font-medium">Fallback</span> + <span className="text-emphasis ml-2 text-sm font-medium">{t("fallback")}</span> @@ - <span className="text-emphasis ml-2 text-sm font-medium">Send booker to</span> + <span className="text-emphasis ml-2 text-sm font-medium">{t("send_booker_to")}</span> @@ - label={route.name ?? (route.isFallback ? "Otherwise" : `Route ${index + 1}`)} + label={route.name ?? (route.isFallback ? t("otherwise") : `${t("route")} ${index + 1}`)} @@ - <EmptyState + <EmptyState icon="menu" - header="Create your first route" - text="Routes determine where your form responses will be sent based on the answers provided." + header={t("create_your_first_route")} + text={t("routes_determine_destination_helptext")}Also applies to: 581-583, 610-610, 797-799, 637-637, 1324-1331
packages/app-store/paypal/components/EventTypeAppSettingsInterface.tsx (2)
68-86: Localize labels/placeholders and use selected currency for conversion.
- Use t() for user‑visible text.
- Prefer selectedCurrency?.value while converting to avoid stale currency edge cases.
- <TextField - label="Price" + <TextField + label={t("price")} labelSrOnly addOnLeading={currencySymbol} addOnSuffix={currency} step="0.01" min="0.5" type="number" required className="block w-full rounded-sm pl-2 text-sm" - placeholder="Price" + placeholder={t("price")} data-testid="paypal-price-input" onChange={(e) => { - setAppData("price", convertToSmallestCurrencyUnit(Number(e.target.value), currency)); + const cur = selectedCurrency?.value ?? currency; + setAppData("price", convertToSmallestCurrencyUnit(Number(e.target.value), cur)); if (selectedCurrency) { setAppData("currency", selectedCurrency.value); } }}
109-113: Localize label.- <label className="text-default mb-1 block text-sm font-medium" htmlFor="currency"> - Payment option - </label> + <label className="text-default mb-1 block text-sm font-medium" htmlFor="currency"> + {t("payment_option")} + </label>packages/features/form-builder/FormBuilder.tsx (2)
66-66: Frontend i18n: replace hardcoded UI strings with t()Per guidelines, localize all user-facing strings in TSX.
Apply diffs like these:
- return { ...baseFieldType, label: "Location" }; + return { ...baseFieldType, label: t("location") }; - label: "Email", + label: t("email"), ... - label: "Phone", + label: t("phone"),-function Options({ - label = "Options", +function Options({ + label, ... - <Label>{label}</Label> + <Label>{label ?? t("options")}</Label>- label: "Option 1", - value: "Option 1", + label: t("option_1"), + value: t("option_1"), ... - label: "Option 2", - value: "Option 2", + label: t("option_2"), + value: t("option_2"),- Add an Option + {t("add_an_option")}- tooltip={t("Toggle Variant")} + tooltip={t("toggle_variant")}- <span>{`Field ${index + 1}`}</span> + <span>{t("field_number", { index: index + 1 })}</span>Also applies to: 170-176, 421-421, 444-451, 531-531, 951-951, 991-993
276-289: A11y: add aria-labels to icon-only buttonsIcon-only controls should expose accessible names.
- <button + <button + aria-label={t("move_field_up")} ... - <button + <button + aria-label={t("move_field_down")}packages/app-store/hitpay/components/EventTypeAppSettingsInterface.tsx (3)
35-45: Use shared getCurrencySymbol from @calcom/lib/currencyConversionsYou’re locally deriving the symbol; we already ship a utility for this. Prefer the shared one for consistency.
-import { useLocale } from "@calcom/lib/hooks/useLocale"; +import { useLocale } from "@calcom/lib/hooks/useLocale"; +import { getCurrencySymbol } from "@calcom/lib/currencyConversions"; ... - const getCurrencySymbol = (locale: string, currency: string) => - (0) - .toLocaleString(locale, { - style: "currency", - currency, - minimumFractionDigits: 0, - maximumFractionDigits: 0, - }) - .replace(/\d/g, "") - .trim();- addOnLeading={ - <>{selectedCurrency.value ? getCurrencySymbol("en", selectedCurrency.value) : ""}</> - } + addOnLeading={<>{selectedCurrency.value ? getCurrencySymbol(selectedCurrency.value) : ""}</>}
85-86: Localize placeholder textHardcoded “Price” should use t().
- placeholder="Price" + placeholder={t("price")}
125-125: Prefer named export over default exportPer repo guidelines, prefer named exports for components.
-export default EventTypeAppSettingsInterface; +export { EventTypeAppSettingsInterface };Note: Update imports accordingly in consumers.
packages/app-store/stripepayment/components/EventTypeAppSettingsInterface.tsx (3)
50-59: Use shared getCurrencySymbol instead of deriving locallyUnify with @calcom/lib/currencyConversions.
-import { useLocale } from "@calcom/lib/hooks/useLocale"; +import { useLocale } from "@calcom/lib/hooks/useLocale"; +import { getCurrencySymbol } from "@calcom/lib/currencyConversions"; ... - const getCurrencySymbol = (locale: string, currency: string) => - (0) - .toLocaleString(locale, { - style: "currency", - currency, - minimumFractionDigits: 0, - maximumFractionDigits: 0, - }) - .replace(/\d/g, "") - .trim();- addOnLeading={ - <>{selectedCurrency.value ? getCurrencySymbol("en", selectedCurrency.value) : ""}</> - } + addOnLeading={<>{selectedCurrency.value ? getCurrencySymbol(selectedCurrency.value) : ""}</>}
107-107: Localize placeholder textHardcoded “Price” should use t().
- placeholder="Price" + placeholder={t("price")}
275-275: Prefer named export over default exportAlign with guideline: named exports favor tree-shaking and refactors.
-export default EventTypeAppSettingsInterface; +export { EventTypeAppSettingsInterface };packages/app-store/locations.ts (2)
557-562: Localize fallback URL error messageUser-facing string should use t() for consistency with other messages.
- message: `Invalid URL`, + message: t("invalid_url"),
205-214: Missing translation key for “somewhereElse”translateAbleKeys lacks “somewhereElse”, so that label won’t be translated.
"somewhere_else", + "somewhereElse",apps/web/components/apps/installation/ConfigureStepCard.tsx (2)
115-116: Localize hidden submit button label.Use t("save") or an aria-label; avoid hardcoded text.
- <button type="submit" className="hidden" form={`eventtype-${eventType.id}`} ref={ref}> - Save + <button type="submit" className="hidden" form={`eventtype-${eventType.id}`} ref={ref} aria-label={t("save")}> + {t("save")} </button>
258-259: Localize hidden main submit button label.Same localization concern for the outer hidden submit.
- <button form="outer-event-type-form" type="submit" className="hidden" ref={mainForSubmitRef}> - Save + <button form="outer-event-type-form" type="submit" className="hidden" ref={mainForSubmitRef} aria-label={t("save")}> + {t("save")} </button>packages/features/tasker/tasks/analytics/handleAnalyticsEvents.ts (1)
22-22: Remove unnecessary await on non-promise.rawBookingData is a plain object; awaiting it is misleading.
- const { dub_id } = await rawBookingData; + const { dub_id } = rawBookingData;apps/api/v1/pages/api/bookings/[id]/recordings/_get.ts (2)
64-68: Use Prisma select; avoid include.Follow guideline to select only needed fields and avoid include.
Apply:
- const booking = await prisma.booking.findUnique({ - where: { id }, - // eslint-disable-next-line @calcom/eslint/no-prisma-include-true - include: { references: true }, - }); + const booking = await prisma.booking.findUnique({ + where: { id }, + select: { + id: true, + references: { + select: { + type: true, + meetingId: true, + }, + }, + }, + });
90-99: Optional: guard against large fan-out of external calls.If rooms can have many recordings, consider limiting concurrency (e.g., simple p-limit) to avoid bursty outbound calls.
packages/app-store/_utils/getBulkEventTypes.ts (1)
31-61: DRY the two nearly identical queries.Small helper reduces duplication and centralizes the select shape.
Apply:
+const selectEventTypeSlim = { id: true, title: true, locations: true } as const; + +const getBulkEventTypes = async ( + where: Parameters<typeof prisma.eventType.findMany>[0]["where"] +) => { + const eventTypes = await prisma.eventType.findMany({ where, select: selectEventTypeSlim }); + return processEventTypes(eventTypes); +}; + -export const getBulkUserEventTypes = async (userId: number) => { - const eventTypes = await prisma.eventType.findMany({ - where: { - userId: userId, - teamId: null, - }, - select: { - id: true, - title: true, - locations: true, - }, - }); - - return processEventTypes(eventTypes); -}; +export const getBulkUserEventTypes = (userId: number) => + getBulkEventTypes({ userId, teamId: null }); -export const getBulkTeamEventTypes = async (teamId: number) => { - const eventTypes = await prisma.eventType.findMany({ - where: { - userId: null, - teamId: teamId, - }, - select: { - id: true, - title: true, - locations: true, - }, - }); - - return processEventTypes(eventTypes); -}; +export const getBulkTeamEventTypes = (teamId: number) => + getBulkEventTypes({ userId: null, teamId });packages/features/eventtypes/lib/defaultEvents.ts (2)
172-175: Avoid mutating the input array in getDynamicEventName.Use a copy to prevent side effects on caller data.
Apply:
-export const getDynamicEventName = (dynamicNames: string[], slug: string): string => { - const lastUser = dynamicNames.pop(); - return `Dynamic Collective ${slug} min event with ${dynamicNames.join(", ")} & ${lastUser}`; -}; +export const getDynamicEventName = (dynamicNames: string[], slug: string): string => { + const names = [...dynamicNames]; + const lastUser = names.pop(); + return `Dynamic Collective ${slug} min event with ${names.join(", ")} & ${lastUser}`; +};
214-214: Drop the default export from defaultEvents.tsRemove the line
export default defaultEvents;(packages/features/eventtypes/lib/defaultEvents.ts:214) — repo only uses named imports from this module (no default-imports found).apps/api/v1/pages/api/bookings/[id]/transcripts/_get.ts (1)
45-49: Use Prisma select; avoid include.Select only id + required reference fields.
Apply:
- const booking = await prisma.booking.findUnique({ - where: { id }, - // eslint-disable-next-line @calcom/eslint/no-prisma-include-true - include: { references: true }, - }); + const booking = await prisma.booking.findUnique({ + where: { id }, + select: { + id: true, + references: { + select: { + type: true, + meetingId: true, + }, + }, + }, + });apps/web/lib/reschedule/[uid]/getServerSideProps.ts (1)
160-163: Use Array.some for a boolean check.Clearer intent than relying on truthiness of find’s return value.
Apply:
- const userIsHost = booking?.eventType.hosts.find((host) => { - if (host.user.id === userId) return true; - }); + const userIsHost = booking?.eventType.hosts.some((host) => host.user.id === userId);packages/features/bookings/lib/handleNewBooking.ts (2)
791-807: Minor perf: avoid repeated Day.js conversions inside loops.Cache
.tz(reqBody.timeZone)computations per iteration to cut allocations in hot paths.- const start = input.bookingData.allRecurringDates[i].start; - const end = input.bookingData.allRecurringDates[i].end; + const start = input.bookingData.allRecurringDates[i].start; + const end = input.bookingData.allRecurringDates[i].end; + const startTz = dayjs(start).tz(reqBody.timeZone); + const endTz = dayjs(end).tz(reqBody.timeZone); ... - dateFrom: dayjs(start).tz(reqBody.timeZone).format(), - dateTo: dayjs(end).tz(reqBody.timeZone).format(), + dateFrom: startTz.format(), + dateTo: endTz.format(),Also applies to: 959-971
2404-2404: Optional: prefer named export over default.Consider
export { handler }and updating consumers to reduce default-export usage across libs.-export default handler; +export { handler };packages/platform/libraries/event-types.ts (2)
11-12: Coalesce type+value re-export into a single statement.Keeps the barrel tidy without behavior change.
-export type { EventTypesPublic } from "@calcom/features/eventtypes/lib/getEventTypesPublic"; -export { getEventTypesPublic } from "@calcom/features/eventtypes/lib/getEventTypesPublic"; +export { type EventTypesPublic, getEventTypesPublic } from "@calcom/features/eventtypes/lib/getEventTypesPublic";
24-26: Group paired value/type re-exports from the same module.Minor cleanup; same exports, fewer lines.
-export { getEventTypesByViewer } from "@calcom/features/eventtypes/lib/getEventTypesByViewer"; -export type { EventTypesByViewer } from "@calcom/features/eventtypes/lib/getEventTypesByViewer"; +export { getEventTypesByViewer, type EventTypesByViewer } from "@calcom/features/eventtypes/lib/getEventTypesByViewer";
| ) { | ||
| const valid = z.string().regex(new RegExp(eventLocationType.urlRegExp)).safeParse(link).success; | ||
|
|
||
| if (!valid) { | ||
| const sampleUrl = eventLocationType.organizerInputPlaceholder; | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| path: [eventLocationType?.defaultValueVariable ?? "link"], | ||
| message: t("invalid_url_error_message", { | ||
| label: eventLocationType.label, | ||
| sampleUrl: sampleUrl ?? "https://cal.com", | ||
| }), | ||
| }); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Dynamic RegExp from metadata can enable ReDoS; harden regex handling
Compiling arbitrary patterns at runtime risks catastrophic backtracking (CWE-1333). At minimum, bound pattern length and compile defensively; ideally, precompile and vet patterns at app-store build time or use a safe-regex/RE2 variant.
- const valid = z.string().regex(new RegExp(eventLocationType.urlRegExp)).safeParse(link).success;
+ let valid = false;
+ try {
+ const pattern = eventLocationType.urlRegExp;
+ if (!pattern || pattern.length > 512) throw new Error("unsafe_pattern");
+ const re = new RegExp(pattern);
+ valid = z.string().regex(re).safeParse(link).success;
+ } catch {
+ valid = false;
+ }Follow-up: consider pre-validating urlRegExp with a safe-regex checker during app-store metadata build and persisting only vetted patterns.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) { | |
| const valid = z.string().regex(new RegExp(eventLocationType.urlRegExp)).safeParse(link).success; | |
| if (!valid) { | |
| const sampleUrl = eventLocationType.organizerInputPlaceholder; | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| path: [eventLocationType?.defaultValueVariable ?? "link"], | |
| message: t("invalid_url_error_message", { | |
| label: eventLocationType.label, | |
| sampleUrl: sampleUrl ?? "https://cal.com", | |
| }), | |
| }); | |
| } | |
| return; | |
| ) { | |
| let valid = false; | |
| try { | |
| const pattern = eventLocationType.urlRegExp; | |
| if (!pattern || pattern.length > 512) throw new Error("unsafe_pattern"); | |
| const re = new RegExp(pattern); | |
| valid = z.string().regex(re).safeParse(link).success; | |
| } catch { | |
| valid = false; | |
| } | |
| if (!valid) { | |
| const sampleUrl = eventLocationType.organizerInputPlaceholder; | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| path: [eventLocationType?.defaultValueVariable ?? "link"], | |
| message: t("invalid_url_error_message", { | |
| label: eventLocationType.label, | |
| sampleUrl: sampleUrl ?? "https://cal.com", | |
| }), | |
| }); | |
| } | |
| return; |
🧰 Tools
🪛 ast-grep (0.38.6)
[warning] 537-537: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(eventLocationType.urlRegExp)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
packages/features/tasker/tasks/analytics/handleAnalyticsEvents.ts
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Should we not put this in features like you did with the calendar manager? They are more or less the same thing, a dynamic factory to get a specific handler.
E2E results are ready! |
keithwillcode
left a comment
keithwillcode
left a comment
There was a problem hiding this comment.
Changes look mostly good. Just:
- some test failing
- Benny and I chatted and we are going to evaluate splitting the videoClient into 2 parts. A factory in the App Store to get clients and then another class in features with all of the feature-like functions. TBD later
2 participants
What does this PR do?
Files Migrated (Moved from one location to another):
1. From
packages/lib/event-types/topackages/app-store/_utils/:packages/lib/event-types/getBulkEventTypes.ts→packages/app-store/_utils/getBulkEventTypes.ts2. From
packages/lib/event-types/topackages/features/eventtypes/lib/:packages/lib/event-types/getEventTypesByViewer.ts→packages/features/eventtypes/lib/getEventTypesByViewer.tspackages/lib/event-types/getEventTypesPublic.ts→packages/features/eventtypes/lib/getEventTypesPublic.ts3. From
packages/lib/event-types/utils/topackages/features/eventtypes/lib/:packages/lib/event-types/utils/checkForEmptyAssignment.ts→packages/features/eventtypes/lib/checkForEmptyAssignment.ts4. From
packages/lib/topackages/features/eventtypes/lib/:packages/lib/defaultEvents.ts→packages/features/eventtypes/lib/defaultEvents.ts5. From
packages/app-store/_utils/payments/topackages/lib/:packages/app-store/_utils/payments/currencyConversions.ts→packages/lib/currencyConversions.ts6. From
packages/lib/analyticsManager/topackages/features/tasker/tasks/analytics/:packages/lib/analyticsManager/handleAnalyticsEvents.ts→packages/features/tasker/tasks/analytics/handleAnalyticsEvents.tsMandatory Tasks (DO NOT REMOVE)
How should this be tested?