fix: resolve circular dependencies in app-store package#23535
fix: resolve circular dependencies in app-store package#23535joeauyeung wants to merge 12 commits intomainfrom
Conversation
- Create @calcom/app-store-types package for shared metadata types - Extract routingFormResponseInDbSchema to @calcom/lib/zod/routingFormResponse - Update CredentialDataWithTeamName type to match CredentialForCalendarService - Fix import statements to separate type and value imports - Remove circular dependency warnings from utils.ts - Update package dependencies and exports This resolves circular import issues between app-store, lib, and prisma packages while maintaining existing functionality and type safety. Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Walkthrough
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
|
keithwillcode
added
core
- Move all types from external @calcom/app-store-types package into app-store/types.d.ts - Add missing LocationOption, CredentialDataWithTeamName, TDependencyData types - Remove dependency on external @calcom/app-store-types package - Keep all types within app-store package as requested by user - Resolves TypeScript errors for IntegrationOAuthCallbackState and CredentialOwner Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
- Remove all files from packages/app-store-types directory - Update remaining imports in _appRegistry.ts and utils.ts to use internal types - Complete consolidation of all types into app-store/types.d.ts - Eliminates external package dependency as requested by user Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
- Move defaultVideoAppCategories from types.d.ts to constants.ts - Update imports in utils.ts and server.ts to use constants.ts - Resolves module resolution error causing 60+ unit test failures - TypeScript .d.ts files are for type declarations only, not runtime values Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
- Auto-formatted by pre-commit hooks during previous commit Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
- Move schema from @calcom/lib/zod/routingFormResponse to routing-forms/zod.ts - Update import in booking.ts to use routing-forms package location - Remove unused routingFormResponse.ts file from lib - Keeps routing-form related schemas consolidated in one location Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
This change doesn't solve any circular dependency and was unnecessary consolidation. Keep types where they logically belong. Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
…ils.ts These type moves don't solve any circular dependencies and were unnecessary consolidation. Keep types in their original locations: - LocationOption and CredentialDataWithTeamName back to utils.ts - MeetLocationType and MSTeamsLocationType back to utils.ts - Remove constants.ts file and update all imports accordingly Only keep changes that actually resolve circular dependency issues. Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
…r dependency - Create constants.ts with defaultVideoAppCategories, MeetLocationType, MSTeamsLocationType - Update imports in utils.ts, googlecalendar, office365calendar, and locations.ts - This separates runtime values from type declarations to fix module resolution errors - Revert routing-forms lint fixes that don't solve actual circular dependencies Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
…ategories - Fixes type check error: Module '@calcom/app-store/utils' has no exported member 'defaultVideoAppCategories' - This was the last remaining import that needed to be updated after moving runtime values to constants.ts Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
| import { createFallbackRoute } from "../lib/createFallbackRoute"; | ||
| import { getSerializableForm } from "../lib/getSerializableForm"; | ||
| import { isFallbackRoute } from "../lib/isFallbackRoute"; | ||
| import { isFormCreateEditAllowed } from "../lib/isFormCreateEditAllowed"; |
| import { MembershipRole } from "@calcom/prisma/enums"; | ||
| import type { TrpcSessionUser } from "@calcom/trpc/server/types"; | ||
|
|
||
| import { TRPCError } from "@trpc/server"; |
|
|
||
| export const MSTeamsLocationType = "integrations:office365_video"; | ||
|
|
||
| export const defaultVideoAppCategories: AppCategories[] = [ |
There was a problem hiding this comment.
Having this in utils.ts was causing a circular dependency
dosubot
bot
added
app-store
![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/utils.ts (2)
171-171: Convert default export to named export and update imports
Removingexport default getApps;will break all existing default imports in these files (and others):
- packages/trpc/server/routers/viewer/apps/updateUserDefaultConferencingApp.handler.ts:3
- packages/trpc/server/routers/viewer/apps/appById.handler.ts:1
- packages/platform/libraries/app-store.ts:1
- packages/lib/server/getDefaultLocations.ts:3
- packages/lib/apps/getEnabledAppsFromCredentials.ts:4
- packages/lib/EventManager.ts:11
- packages/lib/CalendarManager.ts:6
Update eachimport getApps from "@calcom/app-store/utils"toimport { getApps } from "@calcom/app-store/utils".
56-79: Scrub credential.key from getApps output — in packages/app-store/utils.ts, changecredentials: appCredentialsto
credentials: appCredentials.map(getPiiFreeCredential)so no tRPC/HTTP route ever returns a raw
key.
♻️ Duplicate comments (1)
packages/app-store/constants.ts (1)
7-7: Ack: moving from utils breaks the cycleCentralizing defaultVideoAppCategories here fixes the utils-induced cycle.
🧹 Nitpick comments (4)
packages/app-store/routing-forms/trpc/permissions.ts (1)
12-18: Make fallbackRoles optional with a safe defaultMinor resilience: default to [] so callers don’t need to pass it explicitly.
export async function checkPermissionOnExistingRoutingForm({ formId, userId, permission, - fallbackRoles, + fallbackRoles = [], }: { formId: string; userId: number; permission: PermissionString; - fallbackRoles: MembershipRole[]; + fallbackRoles?: MembershipRole[]; }) {packages/app-store/server.ts (1)
39-47: Select only what you need from org lookupSmall efficiency win: only id is used.
- const org = await prisma.team.findFirst({ - where: { + const org = await prisma.team.findFirst({ + select: { id: true }, + where: { children: { some: { id: teamId, }, }, }, });packages/app-store/constants.ts (1)
7-12: Freeze the list at type-level to prevent mutationMake it readonly while preserving AppCategories checking.
-export const defaultVideoAppCategories: AppCategories[] = [ - "messaging", - "conferencing", - // Legacy name for conferencing - "video", -]; +export const defaultVideoAppCategories = + ["messaging", "conferencing", /* Legacy */ "video"] as const + satisfies readonly AppCategories[];packages/app-store/utils.ts (1)
147-165: Minor: use a Set for category membershipSlight micro-optimization and readability.
- return !appCategories.some( - (category) => - category === "calendar" || - (defaultVideoAppCategories.includes(category as AppCategories) && !concurrentMeetings) - ); + const videoCats = new Set(defaultVideoAppCategories); + return !appCategories.some( + (c) => c === "calendar" || (videoCats.has(c as AppCategories) && !concurrentMeetings) + );
📜 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 (7)
packages/app-store/constants.ts(1 hunks)packages/app-store/routing-forms/trpc/formMutation.handler.ts(0 hunks)packages/app-store/routing-forms/trpc/formQuery.handler.ts(0 hunks)packages/app-store/routing-forms/trpc/permissions.ts(1 hunks)packages/app-store/server.ts(1 hunks)packages/app-store/utils.ts(2 hunks)packages/prisma/zod/custom/booking.ts(0 hunks)
💤 Files with no reviewable changes (3)
- packages/app-store/routing-forms/trpc/formQuery.handler.ts
- packages/app-store/routing-forms/trpc/formMutation.handler.ts
- packages/prisma/zod/custom/booking.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/app-store/routing-forms/trpc/permissions.tspackages/app-store/constants.tspackages/app-store/server.tspackages/app-store/utils.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/routing-forms/trpc/permissions.tspackages/app-store/constants.tspackages/app-store/server.tspackages/app-store/utils.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/routing-forms/trpc/permissions.tspackages/app-store/constants.tspackages/app-store/server.tspackages/app-store/utils.ts
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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.
Applied to files:
packages/app-store/server.tspackages/app-store/utils.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/app-store/server.tspackages/app-store/utils.ts
🧬 Code graph analysis (1)
packages/app-store/constants.ts (1)
packages/app-store/locations.ts (2)
MeetLocationType(18-18)MSTeamsLocationType(19-19)
⏰ 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/app-store/routing-forms/trpc/permissions.ts (1)
4-4: Good: type-only enum importSwitching MembershipRole to a type-only import avoids runtime coupling and helps prevent cycles.
packages/app-store/server.ts (2)
67-84: credentialForCalendarServiceSelect correctly omits credential.key
The select only includesid,appId, andtype; nokeyfield is returned, satisfying the credential exposure policy.
4-4: Import path consolidation verified
No remaining imports ofdefaultVideoAppCategoriesfrom@calcom/app-store/utilswere found—everything now correctly references@calcom/app-store/constants.packages/app-store/constants.ts (1)
1-1: Confirm AppCategories enum parity
Runtime definitions for AppCategories (@calcom/prisma/enums) aren’t present in the repo—manually verify that the generated enum in your published wrapper exactly matches the Prisma schema’s values (calendar,messaging) to avoid mismatches.packages/app-store/utils.ts (2)
4-4: Good: import constant from constantsThis aligns imports and avoids the prior utils self-reference.
21-25: Verify existence of@calcom/app-store-typesand its export
I couldn’t locate apackages/app-store-typesdirectory or an export namedCredentialDataWithTeamNamein the repo—please confirm that the@calcom/app-store-typespackage and theCredentialDataWithTeamNameexport exist before re-exporting.
| @@ -1,3 +1,12 @@ | |||
| import type { AppCategories } from "@prisma/client"; | |||
There was a problem hiding this comment.
| import type { AppCategories } from "@prisma/client"; | |
| import type { AppCategories } from "@calcom/prisma/enums"; |
hariombalhara
left a comment
hariombalhara
left a comment
There was a problem hiding this comment.
Small suggestion, otherwise good !!
|
This PR is being marked as stale due to inactivity. |
Udit-takkar
left a comment
Udit-takkar
left a comment
There was a problem hiding this comment.
@joeauyeung Can you please fix all the conflicts? and do we still need this PR?
|
This PR is being marked as stale due to inactivity. |
4 participants
What does this PR do?
This PR resolves circular dependencies in the app-store package that were causing TypeScript compilation issues and architectural problems. The main changes include:
@calcom/app-store-typespackage to house shared types (CredentialDataWithTeamName,LocationOption,TDependencyData,defaultVideoAppCategories) that were causing circular dependencies between app-store, lib, and prisma packagesroutingFormResponseInDbSchemafrom routing-forms to@calcom/lib/zod/routingFormResponseto break the circular dependency between app-store and prisma packagesCredentialForCalendarServiceandCredentialDataWithTeamNameby making fields properly nullable/optional to match the actual Prisma schemaLink to Devin run: https://app.devin.ai/sessions/d5c40dff618e42ef89edfc3e3e205fb6
Requested by: @joeauyeung
How should this be tested?
yarn type-check:ci --forceto verify no TypeScript errorsyarn app-store:buildto ensure CLI still generates files correctlyyarn buildto ensure no dependency or compilation issuesExpected behavior: All existing app-store functionality should work unchanged, but without circular dependency warnings/errors.
Mandatory Tasks (DO NOT REMOVE)
Please pay special attention to:
CredentialDataWithTeamNametype changes (makingdelegatedToIdoptional and several fields nullable) actually match the real data structure returned by Prisma queries@calcom/app-store-typespackage is correctly configured and exportedChecklist