refactor: Remove app store index#23449
Conversation
WalkthroughThis change removes the app-store registry and its default export by deleting packages/app-store/index.ts, and eliminates the global test mock at tests/libs/mocks/app-store.ts. Tests are updated to stop using appStoreMock. In bookingScenario test utilities, video integrations now use a registry-based approach (mockVideoAdapterRegistry with VideoApiAdapterMap). CRM integrations are mocked directly via vi.doMock for the @calcom/app-store/crm.apps.generated module, supplying a MockCrmService and wiring CrmServiceMap (including metadataLookupKey) to the mock implementation. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
|
|
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: |
keithwillcode
added
core
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| //@ts-ignore | ||
| appStoreMock.default[appStoreLookupKey as keyof typeof appStoreMock.default].mockImplementation(() => { |
There was a problem hiding this comment.
The video adapter is mocked directly from the video adapter registry
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| //@ts-ignore | ||
| appStoreMock.default[appStoreLookupKey].mockImplementation(() => { |
There was a problem hiding this comment.
The video adapter is mocked directly from the video adapter registry
| ownerEmail: string; | ||
| }[] = []; | ||
| const eventsCreated: boolean[] = []; | ||
| const app = appStoreMetadata[metadataLookupKey as keyof typeof appStoreMetadata]; |
There was a problem hiding this comment.
Instead of mocking the whole CRM app we just mock the CrmService
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: 2
📜 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 (3)
apps/web/test/utils/bookingScenario/bookingScenario.ts(1 hunks)packages/app-store/index.ts(0 hunks)tests/libs/__mocks__/app-store.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/app-store/index.ts
- tests/libs/mocks/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:
apps/web/test/utils/bookingScenario/bookingScenario.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:
apps/web/test/utils/bookingScenario/bookingScenario.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:
apps/web/test/utils/bookingScenario/bookingScenario.ts
🧠 Learnings (1)
📚 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:
apps/web/test/utils/bookingScenario/bookingScenario.ts
🔇 Additional comments (1)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
2171-2176: Mock ordering is correct – thevi.doMock("@calcom/app-store/crm.apps.generated", …)call precedes any import of that module in this file, so no cache reset or registry rewrite is needed.Likely an incorrect or invalid review comment.
| vi.doMock("@calcom/app-store/crm.apps.generated", async (importOriginal) => { | ||
| const original = await importOriginal<typeof import("@calcom/app-store/crm.apps.generated")>(); | ||
|
|
||
| getContacts(email: string) { | ||
| if (crmData?.getContacts) { | ||
| contactsQueried = crmData?.getContacts; | ||
| const contactsOfEmail = contactsQueried.filter((contact) => contact.email === email); | ||
| class MockCrmService { | ||
| constructor() { | ||
| log.debug("Create CrmService"); | ||
| } | ||
|
|
||
| return Promise.resolve(contactsOfEmail); | ||
| } | ||
| } | ||
| createContact() { | ||
| if (crmData?.createContacts) { | ||
| contactsCreated = crmData.createContacts; | ||
| return Promise.resolve(crmData?.createContacts); | ||
| } | ||
| return Promise.resolve([]); | ||
| } | ||
|
|
||
| createEvent() { | ||
| eventsCreated.push(true); | ||
| return Promise.resolve({}); | ||
| } | ||
| }, | ||
| getContacts(email: string) { | ||
| if (crmData?.getContacts) { | ||
| contactsQueried = crmData?.getContacts; | ||
| const contactsOfEmail = contactsQueried.filter((contact) => contact.email === email); | ||
| return Promise.resolve(contactsOfEmail); | ||
| } | ||
| return Promise.resolve([]); | ||
| } | ||
|
|
||
| createEvent() { | ||
| eventsCreated.push(true); | ||
| return Promise.resolve({}); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| ...original, | ||
| CrmServiceMap: { | ||
| ...original.CrmServiceMap, | ||
| [metadataLookupKey]: Promise.resolve({ | ||
| default: MockCrmService, | ||
| }), | ||
| }, | ||
| }); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid vi.doMock timing pitfalls; switch to a registry-based mock (mirrors video adapters)
vi.doMock must run before the first import; repeated calls override the whole module and drop earlier overrides. Use a shared registry + top-level vi.mock, then register per-key services here.
- // Mock the CrmServiceMap directly instead of using the old app-store index approach
- vi.doMock("@calcom/app-store/crm.apps.generated", async (importOriginal) => {
- const original = await importOriginal<typeof import("@calcom/app-store/crm.apps.generated")>();
-
- class MockCrmService {
- constructor() {
- log.debug("Create CrmService");
- }
- ...
- }
-
- return {
- ...original,
- CrmServiceMap: {
- ...original.CrmServiceMap,
- [metadataLookupKey]: Promise.resolve({
- default: MockCrmService,
- }),
- },
- };
- });
+ class MockCrmService {
+ constructor() {
+ log.debug("Create CrmService");
+ }
+ ...
+ }
+ // register into the shared registry (defined at module scope)
+ mockCrmServiceRegistry[metadataLookupKey] = Promise.resolve({ default: MockCrmService });Add at module scope (near the video registry):
const mockCrmServiceRegistry: Record<string, any> = {};
vi.mock("@calcom/app-store/crm.apps.generated", () => ({
CrmServiceMap: new Proxy(
{},
{
get(_t, prop) {
if (typeof prop === "string" && mockCrmServiceRegistry[prop]) {
return mockCrmServiceRegistry[prop];
}
return Promise.resolve({ default: vi.fn() });
},
}
),
}));| contactsCreated = crmData.createContacts; | ||
| return Promise.resolve(crmData?.createContacts); | ||
| } | ||
| return Promise.resolve([]); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix reassignment bug: returned arrays never update
contactsCreated and contactsQueried are reassigned, so the arrays returned from mockCrmApp() won’t reflect updates. Mutate in place to preserve references.
- createContact() {
- if (crmData?.createContacts) {
- contactsCreated = crmData.createContacts;
- return Promise.resolve(crmData?.createContacts);
- }
- return Promise.resolve([]);
- }
+ async createContact() {
+ if (crmData?.createContacts) {
+ contactsCreated.length = 0;
+ contactsCreated.push(...crmData.createContacts);
+ return contactsCreated;
+ }
+ return [];
+ }
- getContacts(email: string) {
- if (crmData?.getContacts) {
- contactsQueried = crmData?.getContacts;
- const contactsOfEmail = contactsQueried.filter((contact) => contact.email === email);
- return Promise.resolve(contactsOfEmail);
- }
- return Promise.resolve([]);
- }
+ async getContacts(email: string) {
+ if (crmData?.getContacts) {
+ contactsQueried.length = 0;
+ contactsQueried.push(...crmData.getContacts);
+ const contactsOfEmail = contactsQueried.filter((contact) => contact.email === email);
+ return contactsOfEmail;
+ }
+ return [];
+ }Also applies to: 2187-2194
🤖 Prompt for AI Agents
In apps/web/test/utils/bookingScenario/bookingScenario.ts around lines 2180-2185
and 2187-2194, the issue is that contactsCreated and contactsQueried are being
reassigned to new arrays so callers holding the original references won't see
subsequent updates; instead mutate the existing arrays in place and return the
original array reference (e.g., clear the target array and push new items or use
splice to replace contents) so that mockCrmApp() always returns the same array
object while its contents are updated; keep returning a resolved Promise of that
array reference.
2 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist