fix:optimized data loading in locations.ts#24271
fix:optimized data loading in locations.ts#24271thetanav wants to merge 2 commits intocalcom:mainfrom thetanav:fix/faster-dev-server
Conversation
|
|
github-actions
bot
added
$2K
app-store
|
@thetanav is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
performance
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
dosubot
bot
added
the
🧹 Improvements
![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 (2)
packages/app-store/getNormalizedAppMetadata.ts (1)
10-17: Fix unresolvedAppStoreMetaDatatype
AppStoreMetaDatawas deleted in this refactor, so this cast now references an undefined type. The file currently fails to compile under TypeScript. Cast to the existingAppMetashape instead so the build passes.- const metadata = { - appData: null, - dirName, - __template: "", - ...appMeta, - } as Omit<AppStoreMetaData[keyof AppStoreMetaData], "dirName"> & { dirName: string }; + const metadata: AppMeta = { + appData: null, + __template: "", + ...appMeta, + dirName, + };packages/app-store-cli/src/build.ts (1)
246-260: Lazy-importing metadata breaks synchronous consumersSetting
lazyImport: truehere makes every entry in the generatedappStoreMetadatamap aPromise. Callers (seeapps/web/test/utils/bookingScenario/bookingScenario.tsand many others) treat those entries as plain objects and immediately read fields like.type, so this change turns into runtime failures (undefinedaccess / promise objects). Until the callers are refactored toawaitthe metadata, we need to keep this export synchronous.- ], - lazyImport: true, + ],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/web/test/utils/bookingScenario/bookingScenario.ts(1 hunks)packages/app-store-cli/src/build.ts(1 hunks)packages/app-store/_appRegistry.ts(2 hunks)packages/app-store/appStoreMetaData.ts(1 hunks)packages/app-store/getNormalizedAppMetadata.ts(1 hunks)packages/app-store/locations.ts(2 hunks)packages/app-store/utils.ts(3 hunks)scripts/seed-app-store.ts(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/seed-app-store.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/locations.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/app-store/appStoreMetaData.tspackages/app-store-cli/src/build.tspackages/app-store/utils.tspackages/app-store/_appRegistry.tspackages/app-store/getNormalizedAppMetadata.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/locations.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/app-store/appStoreMetaData.tspackages/app-store-cli/src/build.tspackages/app-store/utils.tspackages/app-store/_appRegistry.tspackages/app-store/getNormalizedAppMetadata.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/locations.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/app-store/appStoreMetaData.tspackages/app-store-cli/src/build.tspackages/app-store/utils.tspackages/app-store/_appRegistry.tspackages/app-store/getNormalizedAppMetadata.ts
🧬 Code graph analysis (2)
packages/app-store/appStoreMetaData.ts (1)
packages/app-store/getNormalizedAppMetadata.ts (1)
getNormalizedAppMetadata(5-21)
packages/app-store/_appRegistry.ts (1)
packages/app-store/appStoreMetaData.ts (1)
getAppMetadata(9-31)
⏰ 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
| // Since this is server-side and the module should be available, use require | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const { appStoreMetadata } = require("@calcom/app-store/bookerAppsMetaData"); | ||
| processAppMetadata(appStoreMetadata); | ||
| } catch (error) { | ||
| console.warn("Failed to load app metadata, app locations may not be available", error); | ||
| locationsFromApps = []; | ||
| } | ||
|
|
||
| return locationsFromApps; | ||
| }; | ||
|
|
||
| const processAppMetadata = (appStoreMetadata: any) => { | ||
| locationsFromApps = []; | ||
|
|
||
| for (const [appName, meta] of Object.entries(appStoreMetadata)) { | ||
| const location = meta.appData?.location; | ||
| if (location) { | ||
| // TODO: This template variable replacement should happen once during app-store:build. | ||
| for (const [key, value] of Object.entries(location)) { | ||
| if (typeof value === "string") { | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| location[key] = value.replace(/{SLUG}/g, meta.slug).replace(/{TITLE}/g, meta.name); | ||
| } | ||
| } | ||
| } | ||
| const newLocation = { | ||
| ...location, | ||
| messageForOrganizer: location.messageForOrganizer || `Set ${location.label} link`, | ||
| iconUrl: meta.logo, | ||
| // For All event location apps, locationLink is where we store the input | ||
| // TODO: locationLink and link seems redundant. We can modify the code to keep just one of them. | ||
| variable: location.variable || "locationLink", | ||
| defaultValueVariable: location.defaultValueVariable || "link", | ||
| }; | ||
|
|
||
| // Static links always require organizer to input | ||
| if (newLocation.linkType === "static") { | ||
| newLocation.organizerInputType = location.organizerInputType || "text"; | ||
| if (newLocation.organizerInputPlaceholder?.match(/https?:\/\//)) { | ||
| // HACK: Translation ends up removing https? if it's in the beginning :( | ||
| newLocation.organizerInputPlaceholder = ` ${newLocation.organizerInputPlaceholder}`; | ||
| const newLocation = { | ||
| ...location, | ||
| messageForOrganizer: location.messageForOrganizer || `Set ${location.label} link`, | ||
| iconUrl: meta.logo, | ||
| // For All event location apps, locationLink is where we store the input | ||
| // TODO: locationLink and link seems redundant. We can modify the code to keep just one of them. | ||
| variable: location.variable || "locationLink", | ||
| defaultValueVariable: location.defaultValueVariable || "link", | ||
| }; | ||
|
|
||
| // Static links always require organizer to input | ||
| if (newLocation.linkType === "static") { | ||
| newLocation.organizerInputType = location.organizerInputType || "text"; | ||
| if (newLocation.organizerInputPlaceholder?.match(/https?:\/\//)) { | ||
| // HACK: Translation ends up removing https? if it's in the beginning :( | ||
| newLocation.organizerInputPlaceholder = ` ${newLocation.organizerInputPlaceholder}`; | ||
| } | ||
| } else { | ||
| newLocation.organizerInputType = null; | ||
| } | ||
| } else { | ||
| newLocation.organizerInputType = null; | ||
| } | ||
|
|
||
| AppStoreLocationType[appName] = newLocation.type; | ||
| AppStoreLocationType[appName] = newLocation.type; | ||
|
|
||
| locationsFromApps.push({ | ||
| ...newLocation, | ||
| }); | ||
| locationsFromApps.push({ | ||
| ...newLocation, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Blocking: lazy loader still relies on appStoreMetadata Proxy
bookerAppsMetaData no longer exposes a concrete appStoreMetadata; in this PR it’s now a Proxy that throws on property access to prevent the old eager pattern. Destructuring it here trips that guard, we fall into the catch block, and locationsFromApps is permanently set to []. Every downstream lookup (getLocationFromApp, LocationType, etc.) now loses all app-provided locations, breaking conferencing apps across the product. Please switch this loader over to the new lazy API (e.g. hydrate via getAppMetadata / the new registry) before caching so we still populate real metadata.
| // For now, return empty arrays since we can't synchronously load all apps | ||
| // This needs to be changed to load apps from database or a static list | ||
| ALL_APPS_CACHE = []; | ||
| ALL_APPS_MAP_CACHE = {}; | ||
| return { apps: ALL_APPS_CACHE, appsMap: ALL_APPS_MAP_CACHE }; | ||
| } | ||
|
|
||
| export type CredentialDataWithTeamName = CredentialForCalendarService & { | ||
| team?: { | ||
| name: string; | ||
| } | null; | ||
| }; | ||
|
|
||
| export const ALL_APPS = Object.values(ALL_APPS_MAP); | ||
| // For backward compatibility, provide synchronous access | ||
| // But this will be empty until we implement proper loading | ||
| export const ALL_APPS: AppMeta[] = []; | ||
| const ALL_APPS_MAP: Record<string, AppMeta> = {}; |
There was a problem hiding this comment.
Blocking regression: ALL_APPS/slug lookups now always return empty
loadAllApps currently seeds both caches with empty structures and nothing ever repopulates them, yet ALL_APPS stays the public source for getApps, hasIntegrationInstalled, location options, etc. Coupled with getAppFromSlug returning undefined, every consumer now sees “no apps installed,” wiping out integrations UI/logic. We need to keep hydrating these exports—either by reusing the generated metadata module or by wiring in the new lazy getAppMetadata/registry pipeline—before flipping the contract to async. Until the real loader is in place this change cannot ship.
Also applies to: 139-143
🤖 Prompt for AI Agents
packages/app-store/utils.ts lines 31-47 (and also address similar logic at
139-143): the current change seeds ALL_APPS/ALL_APPS_MAP (and
ALL_APPS_CACHE/ALL_APPS_MAP_CACHE) with empty values causing all slug lookups
and getApps consumers to see no integrations; restore synchronous hydration by
loading the existing generated app metadata module (or synchronously iterate the
registry/getAppMetadata results) and populate ALL_APPS, ALL_APPS_MAP,
ALL_APPS_CACHE and ALL_APPS_MAP_CACHE before exporting so getAppFromSlug and
other sync consumers continue to work; ensure the map is keyed by slug and that
getAppFromSlug reads from the populated ALL_APPS_MAP, and do not convert the
public contract to async until a full async loader is implemented.
2 participants
I am still workingon this pr if you have a suggestion to me pls add a comment to this PR, as i am first time contributor of this project
What does this PR do?
/claim #23104
Visual Demo (For contributors especially)
add in future working on this pr
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
npm run lintAre there environment variables that should be set?
What are the minimal test data to have?
What is expected (happy path) to have (input and output)?
Any other important info that could help to test that PR