refactor: move a constant to a dedicated file in App store package#23671
refactor: move a constant to a dedicated file in App store package#23671
Conversation
WalkthroughThe change centralizes the DailyLocationType constant by adding it to packages/app-store/constants.ts and updating packages/app-store/locations.ts to re-export it from this source. All references across the codebase switch imports from @calcom/app-store/locations to @calcom/app-store/constants, including in bookings handlers, scheduleNoShowTriggers (and its integration test), defaultEvents, getDefaultLocations, videoClient, and TRPC viewer schemas/handlers. No logic, control flow, or API signatures are modified; only import paths and the constant’s source are updated. 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. Comment |
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app-store/locations.ts (2)
459-467: Bug: assignment typo prevents message update for cancelled/rejected bookings
locationToDisplay == t("web_conference")compares instead of assigns, so the string never updates.Apply:
- if (bookingStatus === BookingStatus.CANCELLED || bookingStatus === BookingStatus.REJECTED) { - locationToDisplay == t("web_conference"); + if (bookingStatus === BookingStatus.CANCELLED || bookingStatus === BookingStatus.REJECTED) { + locationToDisplay = t("web_conference");
488-495: Fix return/variable type to always be an arrayThe current type
DefaultEventLocationType["type"] | EventLocationTypeFromApp["type"][]reads as “string OR array of strings,” but this function always returns an array.Apply:
-export const getOrganizerInputLocationTypes = () => { - const result: DefaultEventLocationType["type"] | EventLocationTypeFromApp["type"][] = []; +export const getOrganizerInputLocationTypes = (): ( + DefaultEventLocationType["type"] | EventLocationTypeFromApp["type"] +)[] => { + const result: ( + DefaultEventLocationType["type"] | EventLocationTypeFromApp["type"] + )[] = [];
🧹 Nitpick comments (5)
packages/app-store/constants.ts (1)
1-5: Consider a typed LocationTypes map for stronger typingDefining a single const map and a union type can reduce stringly-typed usage across the codebase.
Non-breaking addition:
+export const LocationTypes = { + Meet: "integrations:google:meet", + MSTeams: "integrations:office365_video", + Daily: "integrations:daily", +} as const; +export type LocationType = typeof LocationTypes[keyof typeof LocationTypes];packages/lib/defaultEvents.ts (1)
214-214: Prefer named export over default exportHelps tree-shaking and consistency across the monorepo. Can be a follow-up PR.
-export default defaultEvents; +export { defaultEvents };packages/app-store/locations.ts (2)
429-433: Nit: clearer error message when defaultValueVariable is missingInterpolating an undefined variable produces a confusing log.
Apply:
- if (!defaultValueVariable) { - console.error(`${defaultValueVariable} not set for ${bookingLocation.type}`); + if (!defaultValueVariable) { + console.error(`defaultValueVariable not set for ${bookingLocation.type}`); return ""; }
497-504: Return boolean for isAttendeeInputRequiredThe name implies boolean but it returns a possibly string value. Returning a boolean avoids truthiness pitfalls.
Apply:
-export const isAttendeeInputRequired = (locationType: string) => { +export const isAttendeeInputRequired = (locationType: string): boolean => { const location = locations.find((l) => l.type === locationType); if (!location) { // Consider throwing an error here. This shouldn't happen normally. - return false; + return false; } - return location.attendeeInputType; + return Boolean(location.attendeeInputType); };packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.ts (1)
36-39: Trim once and compare against normalized valueSlight readability/robustness improvement: normalize once to handle incidental whitespace.
Apply:
- const isCalVideoLocation = booking.location === DailyLocationType || booking.location?.trim() === ""; + const locationTrimmed = booking.location?.trim() ?? null; + const isCalVideoLocation = locationTrimmed === DailyLocationType || locationTrimmed === "";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/app-store/constants.ts(1 hunks)packages/app-store/locations.ts(1 hunks)packages/features/bookings/lib/handleCancelBooking.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.ts(1 hunks)packages/lib/defaultEvents.ts(1 hunks)packages/lib/server/getDefaultLocations.ts(1 hunks)packages/lib/videoClient.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/editLocation.schema.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/create.handler.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/update.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/app-store/constants.tspackages/trpc/server/routers/viewer/eventTypes/create.handler.tspackages/lib/defaultEvents.tspackages/trpc/server/routers/viewer/bookings/editLocation.schema.tspackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.tspackages/lib/server/getDefaultLocations.tspackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.tspackages/lib/videoClient.tspackages/app-store/locations.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.tspackages/features/bookings/lib/handleCancelBooking.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/app-store/constants.tspackages/trpc/server/routers/viewer/eventTypes/create.handler.tspackages/lib/defaultEvents.tspackages/trpc/server/routers/viewer/bookings/editLocation.schema.tspackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.tspackages/lib/server/getDefaultLocations.tspackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.tspackages/lib/videoClient.tspackages/app-store/locations.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.tspackages/features/bookings/lib/handleCancelBooking.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/app-store/constants.tspackages/trpc/server/routers/viewer/eventTypes/create.handler.tspackages/lib/defaultEvents.tspackages/trpc/server/routers/viewer/bookings/editLocation.schema.tspackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.tspackages/lib/server/getDefaultLocations.tspackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.tspackages/lib/videoClient.tspackages/app-store/locations.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.tspackages/features/bookings/lib/handleCancelBooking.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
📚 Learning: 2025-09-01T10:25:51.923Z
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.
Applied to files:
packages/trpc/server/routers/viewer/bookings/editLocation.schema.ts
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.tspackages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.tspackages/features/bookings/lib/handleCancelBooking.ts
📚 Learning: 2025-09-01T07:31:00.963Z
Learnt from: nangelina
PR: calcom/cal.com#23486
File: packages/app-store/kyzon-space/config.json:5-5
Timestamp: 2025-09-01T07:31:00.963Z
Learning: In Cal.com's video integration system, credential types (e.g., "kyzon-space_video") are transformed to app names by removing underscores using `cred.type.split("_").join("")` in videoClient.ts line 28. This means the key in packages/app-store/index.ts should match the underscore-removed version (e.g., "kyzon-spacevideo") rather than the original type name. This same pattern is used in other parts of the system like getCalendar.ts.
Applied to files:
packages/lib/videoClient.tspackages/app-store/locations.ts
🧬 Code graph analysis (2)
packages/app-store/constants.ts (1)
packages/app-store/locations.ts (1)
DailyLocationType(21-21)
packages/app-store/locations.ts (1)
packages/app-store/constants.ts (3)
MeetLocationType(1-1)MSTeamsLocationType(3-3)DailyLocationType(5-5)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
packages/app-store/constants.ts (1)
5-5: Centralize DailyLocationType — LGTMImport source consolidation looks good and keeps a single source of truth.
packages/trpc/server/routers/viewer/eventTypes/create.handler.ts (1)
3-3: Import path switched to constants — LGTMNo behavioral change; aligns with the new single source.
packages/lib/videoClient.ts (1)
4-4: Import path switched to constants — LGTMFallback assignment to DailyLocationType on failure remains intact.
packages/lib/defaultEvents.ts (1)
3-3: Import path switched to constants — LGTMNo logic changes in defaults.
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
5-5: Import path switched to constants — LGTMKeeps location checks consistent across handlers.
packages/trpc/server/routers/viewer/bookings/editLocation.schema.ts (1)
3-3: Import path switched to constants — LGTMSchema fallback to DailyLocationType remains unchanged.
packages/features/bookings/lib/handleCancelBooking.ts (1)
4-4: Import path switched to constants — LGTMCancellation flow and credential fallback are unaffected.
packages/lib/server/getDefaultLocations.ts (1)
2-2: Import path switched to constants — LGTMDefaulting to DailyLocationType when needed is preserved.
packages/app-store/locations.ts (1)
16-17: Centralized DailyLocationType import/re-export — LGTMImporting from ./constants and re-exporting here removes duplication and keeps a single source of truth.
Also applies to: 21-21
packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts (1)
4-4: Import path update — LGTMSwitching to @calcom/app-store/constants aligns tests with the new single source of truth.
packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.ts (1)
1-1: Import path update — LGTMConsistent with the refactor to centralize constants.
E2E results are ready! |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts (1)
521-524: Replace Prisma include with select (repo guideline)Follow project rule: never use include; use select. This preserves payload while aligning with guidelines.
- const workflows = await ctx.prisma.workflow.findMany({ + const workflows = await ctx.prisma.workflow.findMany({ where: { activeOn: { some: { eventTypeId: input.id, }, }, }, - include: { - steps: true, - }, + select: { + steps: true, + // add other fields explicitly if needed by allowDisabling* helpers + }, });
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts (1)
549-555: Avoid console.log in server handlers; use loggerUse the existing logger for consistent levels/transport and to avoid noisy stdout.
- console.log("multiplePrivateLinks", multiplePrivateLinks); + logger.debug({ multiplePrivateLinks }, "multiplePrivateLinks"); // Handle multiple private links using the service const privateLinksRepo = HashedLinkRepository.create(); const connectedLinks = await privateLinksRepo.findLinksByEventTypeId(input.id); - console.log("connectedLinks", connectedLinks); + logger.debug({ connectedLinks }, "connectedLinks");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Learnt from: nangelina
PR: calcom/cal.com#23486
File: packages/app-store/kyzon-space/config.json:5-5
Timestamp: 2025-09-01T07:31:00.963Z
Learning: In Cal.com's video integration system, credential types (e.g., "kyzon-space_video") are transformed to app names by removing underscores using `cred.type.split("_").join("")` in videoClient.ts line 28. This means the key in packages/app-store/index.ts should match the underscore-removed version (e.g., "kyzon-spacevideo") rather than the original type name. This same pattern is used in other parts of the system like getCalendar.ts.
⏰ 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: Production builds / Build Docs
🔇 Additional comments (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts (1)
717-717: Confirm return value: pre-update eventType vs updatedEventTypeHandler computes updatedEventType but returns the original eventType. If clients expect updated fields, consider returning updatedEventType.
|
|
||
| import type { appDataSchemas } from "@calcom/app-store/apps.schemas.generated"; | ||
| import { DailyLocationType } from "@calcom/app-store/locations"; | ||
| import { DailyLocationType } from "@calcom/app-store/constants"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Import moved to constants — good call
This avoids pulling heavy deps from locations; nice perf/treeshaking win.
Run to confirm repo-wide consistency and a safe transition:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Ensure no remaining imports from locations for DailyLocationType"
rg -nP 'import\s+{[^}]*\bDailyLocationType\b[^}]*}\s+from\s+["'\'']@calcom/app-store/locations["'\'']'
echo "2) Ensure constants exports DailyLocationType"
rg -nP '^export\s+.*\bDailyLocationType\b' packages/app-store/constants.ts
echo "3) If keeping a transitional re-export, verify it exists (or intentionally removed)"
rg -nP '\bDailyLocationType\b' packages/app-store/locations.ts -n -C2 || trueLength of output: 1095
Update test import for DailyLocationType (and MeetLocationType)
In packages/lib/server/getDefaultLocations.test.ts (line 7), change the import from
import { DailyLocationType, MeetLocationType } from "@calcom/app-store/locations";to
import { DailyLocationType, MeetLocationType } from "@calcom/app-store/constants";🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts around
line 5, the import for DailyLocationType is from the wrong module; update the
import source from "@calcom/app-store/locations" to
"@calcom/app-store/constants" (and do the same for MeetLocationType if present)
so the test and code use the correct exported enums/constants; ensure only the
constants module is referenced and remove any obsolete/duplicate imports.
2 participants
What does this PR do?
import { DailyLocationType } from "@calcom/app-store/locations";->import { DailyLocationType } from "@calcom/app-store/constants";locations.tsloads zod, appStoreMetadata, logger and more. As a result, we don't do that where there aren't needed.Mandatory Tasks (DO NOT REMOVE)