fix: Manual RR reassign location change - Cal Video/Zoom/GoogleMeet to Zoom/Google Meet/Cal Video #22840
Conversation
WalkthroughA new Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
0ea3c7b to
4603c9c
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
4603c9c to
c1b0737
Compare
E2E results are ready! |
8ced440 to
d04a16c
Compare
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/04/25)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/app-store/webex/api/add.ts (1)
27-31: Inconsistent redirect-URI encoding between “add” and “callback” handlersHere the raw string is passed to
querystring.stringify, which already performs percent-encoding.
Incallback.tsthe same URI is wrapped inencodeURIbefore being sent throughURLSearchParams, causing different (and potentially double-encoded) behaviour.No change needed in this file, but align both handlers by removing the superfluous
encodeURIincallback.ts(see suggested diff there).packages/app-store/webex/api/callback.ts (1)
18-23: Drop redundantencodeURIto avoid double-encoding
URLSearchParamsencodes each value internally, so pre-encoding the URI is unnecessary and can lead to an over-encodedredirect_uri.- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
76-78: Simplify the find condition.The condition
result.type.includes("_video") && result.type === "daily_video"is redundant. Ifresult.type === "daily_video", it will always include "_video".- const calVideoResult = results.find( - (result) => result.type.includes("_video") && result.type === "daily_video" - ); + const calVideoResult = results.find((result) => result.type === "daily_video");packages/lib/server/service/bookingLocationService.test.ts (1)
459-476: Consider making the test more explicit about behavior.The comment on line 466 mentions that
getLocationValueForDBpicks the last matching credential due toforEach. This behavior dependency makes the test fragile if the implementation changes.Consider adding a more explicit test name or assertion comment to clarify that this tests the "last-one-wins" behavior when multiple locations have the same type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-store/locations.ts(3 hunks)packages/app-store/webex/api/add.ts(2 hunks)packages/app-store/webex/api/callback.ts(2 hunks)packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)packages/lib/server/service/bookingLocationService.test.ts(1 hunks)packages/lib/server/service/bookingLocationService.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/webex/api/callback.tspackages/app-store/locations.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.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/webex/api/callback.tspackages/app-store/locations.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
**/*Service.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/bookingLocationService.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/webex/api/callback.tspackages/app-store/webex/api/add.ts
📚 Learning: in cal.com's embed system, internal events like "__scrollbydistance" are fired by cal.com's own code...
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
Applied to files:
packages/app-store/locations.ts
📚 Learning: in cal.com's calendar integration, both google calendar and outlook calendar are designed to allow m...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Applied to files:
packages/app-store/locations.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the multipleprivatelinkscontroller component (packages/features/eventtypes/components/multiplepri...
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the `currentLink.maxUsageCount ?? 1` fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: for e2e integration tests that require real third-party service credentials (like outlook calendar),...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts
🧬 Code Graph Analysis (4)
packages/app-store/webex/api/callback.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/app-store/webex/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/lib/server/service/bookingLocationService.ts (1)
packages/app-store/locations.ts (4)
LocationObject(202-212)OrganizerDefaultConferencingAppType(70-70)CalVideoLocationType(64-64)getLocationValueForDB(379-408)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
⏰ Context from checks skipped due to timeout of 90000ms. 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 (15)
packages/app-store/webex/api/callback.ts (1)
3-4: LGTM – consistent constant importSwitching to
WEBAPP_URL_FOR_OAUTHmatches the change in the “add” endpoint and keeps the OAuth flow consistent.packages/app-store/locations.ts (3)
1-3: Good documentation of technical debt.The TODO comment clearly indicates the need for future consolidation work and test coverage. This aligns well with the introduction of
BookingLocationService.
64-64: Good abstraction for Cal Video location type.Creating a semantic alias
CalVideoLocationTypeimproves code readability while maintaining backward compatibility withDailyLocationType.
384-384: Good type safety improvement.Explicitly typing
conferenceCredentialIdasnumber | undefinedimproves type safety and code clarity.packages/features/ee/round-robin/roundRobinManualReassignment.ts (3)
158-172: Good refactoring to use BookingLocationService.The migration to
BookingLocationService.getLocationForHostproperly centralizes the location determination logic and correctly handles both booking location and conference credential ID extraction.
223-233: Good handling of credential ID extraction for unchanged organizer.The code correctly extracts
conferenceCredentialIdeven when the organizer hasn't changed, ensuring the credential ID is properly propagated in all reassignment scenarios.
306-306: Correct propagation of conferenceCredentialId.The credential ID is properly included in the CalendarEvent, with appropriate null-to-undefined conversion for API compatibility.
packages/lib/server/service/bookingLocationService.test.ts (1)
1-558: Excellent test coverage!The test suite is comprehensive, well-structured, and covers all major scenarios including edge cases. The use of mocks is appropriate and the test organization follows best practices.
packages/lib/server/service/bookingLocationService.ts (1)
1-247: Excellent service implementation!The
BookingLocationServiceis well-designed with:
- Clear separation of concerns
- Comprehensive JSDoc documentation
- Strong TypeScript typing with discriminated unions
- Proper error handling with null returns
- Follows the Service naming convention as required
This successfully centralizes the booking location logic as intended by the PR objectives.
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (6)
20-29: LGTM! Clean mock setupThe new imports and mock configuration follow the established patterns correctly.
30-50: Well-structured type definitionsThe type definitions are clear and properly structured, providing good type safety for the test data.
52-137: Excellent test data builder implementationThe factory pattern implementation significantly improves test maintainability and readability. The builders provide sensible defaults while allowing flexible overrides, reducing code duplication across tests.
161-234: Comprehensive mock implementation for EventManagerThe mock helpers provide excellent coverage for different conferencing scenarios including static links, dynamic conferencing apps, and error cases. The implementation correctly simulates real-world behavior.
Note: The
eslint-disable-next-line @typescript-eslint/no-explicit-anycomments are justified here due to the need to mock private methods.
239-526: Thorough test coverage for core reassignment scenariosThe refactored tests effectively utilize the new test builders and provide comprehensive coverage for:
- Basic reassignment with workflow updates
- Conference credential ID propagation
- Fixed host scenarios
- Cancellation email handling
The tests properly verify both the integration points (EventManager calls) and the resulting state changes.
528-875: Excellent test coverage for location change scenariosThis new test suite directly addresses the PR objectives by thoroughly testing location updates during round-robin reassignment. The tests cover:
- Dynamic conferencing app transitions (Google Meet ↔ Zoom)
- Static video link handling
- Fallback behavior when no default app is configured
- Error handling for failed conferencing setup
The test structure with dedicated helpers (
createConferencingUsers,mockAppStore) makes the tests maintainable and easy to understand.
packages/features/ee/round-robin/roundRobinManualReassignment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/app-store/webex/api/add.ts (1)
29-33: Minor encoding mismatch between auth & token requestsIn this file the
redirect_urivalue is implicitly URL-encoded byquerystring.stringify(which callsencodeURIComponent) whereas the token-exchange route (callback.ts) first appliesencodeURIand thenURLSearchParams, resulting in slightly different encodings (e.g. slashes encoded vs. not). Most providers tolerate this, but Webex’ spec requires the token request use exactly the sameredirect_urithat was supplied on the authorize call.Consider aligning both sides to a single helper:
-redirect_uri: `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`, +redirect_uri: buildWebexRedirectUri(config.slug),and in a shared util:
export const buildWebexRedirectUri = (slug: string) => `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${slug}/callback`;Then let each caller rely on the transport (
querystring/URLSearchParams) to perform one consistent round ofencodeURIComponent.packages/app-store/webex/api/callback.ts (1)
20-27: Potential double-encoding ofredirect_uriHere
redirectUriis first processed byencodeURI, after whichURLSearchParamswill runencodeURIComponentagain. For harmless characters (e.g./,:) this simply results in once-encoded output, but any space or reserved char would become double-encoded (%20→%2520). To stay future-proof and fully match the value sent in the initial/authorizecall, you can skip the manualencodeURI:-const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); +const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;The subsequent
URLSearchParamscall will perform the correct single encoding.packages/app-store/locations.ts (1)
1-3: Track the TODO consolidation taskThe TODO indicates plans to consolidate this file with BookingLocationService. Consider creating a tracking issue to ensure this technical debt is addressed.
Would you like me to create a tracking issue for this consolidation task?
packages/lib/server/service/bookingLocationService.test.ts (1)
466-466: Document the multiple credential ID behaviorThe comment indicates that
getLocationValueForDBpicks the last matching location when there are multiple with the same type. This behavior should be documented in the service implementation to prevent confusion.Consider adding a comment in the BookingLocationService or getLocationValueForDB implementation to document this behavior explicitly.
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (2)
454-455: Consider using a more type-safe approach for BookingRepositoryInstead of casting
prismaMockwithas any, consider using proper typing or a mock-specific type to maintain type safety in tests.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - const bookingRepo = new BookingRepository(prismaMock as any); + const bookingRepo = new BookingRepository(prismaMock);
675-676: Remove commented codeThis commented line appears to be leftover from development and should be removed.
- // (await mockGetAppFromSlug()).mockReturnValue(undefined); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-store/locations.ts(3 hunks)packages/app-store/webex/api/add.ts(2 hunks)packages/app-store/webex/api/callback.ts(2 hunks)packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)packages/lib/server/service/bookingLocationService.test.ts(1 hunks)packages/lib/server/service/bookingLocationService.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/webex/api/add.tspackages/app-store/locations.tspackages/app-store/webex/api/callback.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/lib/server/service/bookingLocationService.test.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/webex/api/add.tspackages/app-store/locations.tspackages/app-store/webex/api/callback.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/lib/server/service/bookingLocationService.test.ts
**/*Service.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/bookingLocationService.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/webex/api/add.tspackages/app-store/webex/api/callback.ts
📚 Learning: in cal.com's embed system, internal events like "__scrollbydistance" are fired by cal.com's own code...
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
Applied to files:
packages/app-store/locations.ts
📚 Learning: in cal.com's calendar integration, both google calendar and outlook calendar are designed to allow m...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Applied to files:
packages/app-store/locations.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: for e2e integration tests that require real third-party service credentials (like outlook calendar),...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: applies to **/*.{service,repository}.ts : avoid dot-suffixes like `.service.ts` or `.repository.ts` ...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{service,repository}.ts : Avoid dot-suffixes like `.service.ts` or `.repository.ts` for new files; reserve `.test.ts`, `.spec.ts`, `.types.ts` for their specific purposes
Applied to files:
packages/lib/server/service/bookingLocationService.test.ts
🧬 Code Graph Analysis (5)
packages/app-store/webex/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/app-store/webex/api/callback.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/lib/server/service/bookingLocationService.ts (1)
packages/app-store/locations.ts (4)
LocationObject(202-212)OrganizerDefaultConferencingAppType(70-70)CalVideoLocationType(64-64)getLocationValueForDB(379-408)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
packages/lib/server/service/bookingLocationService.test.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
⏰ Context from checks skipped due to timeout of 90000ms. 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 (9)
packages/app-store/webex/api/add.ts (1)
4-4: Consistent constant for OAuth redirect – looks goodSwitching to
WEBAPP_URL_FOR_OAUTHkeeps local-dev and staging flows working without hard-coding localhost. No further action needed.packages/app-store/webex/api/callback.ts (1)
3-3: Consistent constant import – looks goodThe callback now uses the same
WEBAPP_URL_FOR_OAUTHconstant as the authorisation endpoint, avoiding environment drift.packages/app-store/locations.ts (1)
384-384: Good type clarity improvementExplicitly typing
conferenceCredentialIdasnumber | undefinedimproves code clarity and type safety.packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
158-172: Excellent refactoring using BookingLocationServiceThe refactoring properly centralizes location determination logic and correctly handles conference credential IDs for both organizer change scenarios. This aligns with the PR objectives to fix location updates during reassignment.
Also applies to: 223-233, 306-306
packages/lib/server/service/bookingLocationService.test.ts (1)
1-558: Excellent comprehensive test coverageThe test suite thoroughly covers all methods of BookingLocationService with proper edge case handling, mock setup, and clear test organization. Great work!
packages/lib/server/service/bookingLocationService.ts (1)
1-247: Well-designed service with excellent documentationThe BookingLocationService is well-structured with:
- Clear separation of concerns
- Comprehensive JSDoc documentation
- Proper TypeScript typing
- Defensive programming with null returns instead of throwing errors
- Correct handling of both dynamic and static link conferencing apps
The implementation aligns perfectly with the PR objectives to fix location updates during round-robin reassignment.
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (3)
51-117: Excellent test data builder pattern implementationThe test data builders provide a clean and maintainable way to create test fixtures with sensible defaults. This pattern significantly improves test readability and reduces duplication across test cases.
527-875: Comprehensive test coverage for location changesThis test suite thoroughly covers the location change scenarios mentioned in the PR objectives, including:
- Transitions between different conferencing apps (Cal Video → Google Meet)
- Static video link handling
- Fallback behaviors when hosts lack default conferencing apps
- Error scenarios for Cal Video failures
The tests effectively validate the fix for issue PRI-298.
320-373: Critical test for conferenceCredentialId handlingThis test validates that
conferenceCredentialIdis properly included during reassignment, which is essential for teams using installed conferencing apps. This directly addresses one of the key issues mentioned in the PR objectives.
packages/features/ee/round-robin/handleRescheduleEventManager.ts
Outdated
Show resolved
Hide resolved
packages/features/ee/round-robin/roundRobinManualReassignment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
packages/app-store/webex/api/add.ts (1)
26-33: State parameter is an empty string – defeats CSRF protectionOAuth best-practice requires a non-guessable
statevalue that is verified on the callback.
decodeOAuthStateis called later but an empty string means no verification happens. At minimum generate a random nonce and persist it in the session:+import crypto from "crypto"; ... - const params = { + const state = crypto.randomBytes(16).toString("hex"); + const params = { response_type: "code", client_id, redirect_uri: encodeURI( `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback` ), - scope: "spark:kms meeting:schedules_read meeting:schedules_write", //should be "A space-separated list of scopes being requested by your integration" - state: "", + scope: + "spark:kms meeting:schedules_read meeting:schedules_write", // space-separated list of scopes + state, };Remember to save
state(e.g.req.session.oauthState = state) and validate it insidedecodeOAuthState.packages/app-store/webex/api/callback.ts (1)
20-26: Potential double-encoding ofredirect_uriHere the redirect URI is first wrapped with
encodeURIand then passed throughURLSearchParams, which internally appliesencodeURIComponent. That results in%25sequences (e.g.https%3A%2F%2F…becomeshttps%253A%252F%252F…).
The authorization server usually rejects such double-encoded values.Options:
- Drop the manual
encodeURIand rely onURLSearchParams:- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;
- Keep the manual encode here and in the
/addendpoint but pass the already-encoded value toURLSearchParamsusing the second argumentencode: false(node-18 feature) ― less portable.Given the simplicity, option 1 is recommended; it also resolves the inconsistency highlighted in
add.ts.
🧹 Nitpick comments (2)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (2)
320-320: Fix typo in test descriptionThere's a duplicate word "with" in the test description.
- test("should include conferenceCredentialId when reassigning booking with with fixed host as organizer", async () => { + test("should include conferenceCredentialId when reassigning booking with fixed host as organizer", async () => {
675-675: Remove commented codeThis commented line should be removed as it doesn't provide any value.
- // (await mockGetAppFromSlug()).mockReturnValue(undefined); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-store/locations.ts(3 hunks)packages/app-store/webex/api/add.ts(2 hunks)packages/app-store/webex/api/callback.ts(2 hunks)packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)packages/lib/server/service/bookingLocationService.test.ts(1 hunks)packages/lib/server/service/bookingLocationService.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/webex/api/add.tspackages/app-store/webex/api/callback.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.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/webex/api/add.tspackages/app-store/webex/api/callback.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
**/*Service.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/bookingLocationService.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/webex/api/add.tspackages/app-store/webex/api/callback.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in cal.com's embed system, internal events like "__scrollbydistance" are fired by cal.com's own code...
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
Applied to files:
packages/app-store/locations.ts
📚 Learning: in cal.com's calendar integration, both google calendar and outlook calendar are designed to allow m...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Applied to files:
packages/app-store/locations.ts
📚 Learning: applies to **/*.{service,repository}.ts : avoid dot-suffixes like `.service.ts` or `.repository.ts` ...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{service,repository}.ts : Avoid dot-suffixes like `.service.ts` or `.repository.ts` for new files; reserve `.test.ts`, `.spec.ts`, `.types.ts` for their specific purposes
Applied to files:
packages/lib/server/service/bookingLocationService.test.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: for e2e integration tests that require real third-party service credentials (like outlook calendar),...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts
🧬 Code Graph Analysis (4)
packages/app-store/webex/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/app-store/webex/api/callback.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/lib/server/service/bookingLocationService.ts (1)
packages/app-store/locations.ts (4)
LocationObject(202-212)OrganizerDefaultConferencingAppType(70-70)CalVideoLocationType(64-64)getLocationValueForDB(379-408)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
⏰ Context from checks skipped due to timeout of 90000ms. 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 (16)
packages/app-store/webex/api/add.ts (1)
4-4: Constant swap looks correctChanging to
WEBAPP_URL_FOR_OAUTHaligns the file with the new OAuth-specific constant already used elsewhere. No further action needed here.packages/app-store/webex/api/callback.ts (1)
3-3: Consistent constant usage – good catchSwitching to
WEBAPP_URL_FOR_OAUTHkeeps both endpoints in sync.packages/app-store/locations.ts (3)
1-3: Good practice to track technical debtAdding this TODO comment helps track the need for future consolidation and test coverage.
64-64: Good alias for improved readabilityThe
CalVideoLocationTypealias provides a clearer, more descriptive name while maintaining backward compatibility.
384-384: Improved type safety with explicit typingMaking the type explicit as
number | undefinedimproves code clarity and type safety.packages/features/ee/round-robin/handleRescheduleEventManager.ts (2)
76-88: Good error handling for video conferencing failuresThis error handling appropriately catches Cal Video creation failures and provides clear feedback that the meeting was rescheduled even though the video link failed. This aligns with the PR's objective of improving conferencing app reliability.
169-169: Minor formatting improvementpackages/features/ee/round-robin/roundRobinManualReassignment.ts (4)
30-30: Proper import of the new serviceThe BookingLocationService import is correctly placed and follows the project structure.
158-172: Excellent refactoring using the centralized serviceThe refactoring properly centralizes the location determination logic using
BookingLocationService.getLocationForHost. This addresses the PR objective of correctly updating locations during reassignment and ensures proper handling of conference credentials.
223-234: Good addition to handle credential extraction without organizer changeThis logic ensures that
conferenceCredentialIdis properly extracted even when the organizer hasn't changed, addressing the missing credential issue mentioned in the PR objectives.
306-306: Proper propagation of conference credential IDAdding
conferenceCredentialIdto the CalendarEvent ensures proper credential handling throughout the booking flow.packages/lib/server/service/bookingLocationService.test.ts (1)
1-558: Excellent comprehensive test coverageThe test file provides thorough coverage of all BookingLocationService methods with well-structured tests covering:
- Dynamic and static conferencing apps
- Edge cases and error scenarios
- Proper mocking and test isolation
- Clear, descriptive test names
This level of test coverage ensures the service's reliability.
packages/lib/server/service/bookingLocationService.ts (4)
1-87: Well-structured type definitions with excellent documentationThe type definitions are comprehensive and use discriminated unions effectively for type safety. The inline documentation clearly explains the purpose and usage of each type.
107-140: Robust implementation with proper error handlingThe method safely handles metadata parsing and gracefully returns null for invalid cases. Good separation of concerns between static and dynamic conferencing apps.
153-212: Well-designed location determination logicThe method implements a clear fallback chain and properly respects event type restrictions. Good reuse of
getLocationDetailsFromTypefor consistent credential extraction.
236-246: Clean abstraction with consistent return valuesThe method provides a clean API by wrapping
getLocationValueForDBand ensuring consistent null values instead of undefined.
packages/features/ee/round-robin/roundRobinManualReassignment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/app-store/webex/api/add.ts (1)
26-30: Double-check redirect-URI encoding parity between auth & token calls
In this request theredirect_urivalue is passed toquerystring.stringifywithout prior encoding, while the token-exchange step (callback.tsLine 20) pre-encodes the same value withencodeURIbeforeURLSearchParamsapplies its own encoding.
AlthoughencodeURIcurrently leaves the string unchanged (no spaces/UTF-8 chars), the two-step encoding in the token call could drift from the value used here if the path ever contains unsafe characters, breaking the OAuth “exact match” requirement.Consider removing the redundant
encodeURIin the token exchange for symmetry (preferred) or applying the same call here — but avoid a situation where one side is double-encoded and the other is not.- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;packages/app-store/webex/api/callback.ts (1)
20-20: RedundantencodeURImay lead to silent drift
URLSearchParamsalready performs percent-encoding on values; wrapping the URL inencodeURIbeforehand adds no benefit for ASCII paths and risks double-encoding if the base constant ever contains encoded characters. Removing it simplifies the flow and guarantees the raw string matches the value sent in the initial authorization request.- const redirectUri = encodeURI(`${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`); + const redirectUri = `${WEBAPP_URL_FOR_OAUTH}/api/integrations/${config.slug}/callback`;packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
76-88: Simplify redundant condition in Cal Video result checkThe condition
result.type.includes("_video") && result.type === "daily_video"is redundant since checking for exact equality to "daily_video" already ensures it includes "_video".const calVideoResult = results.find( - (result) => result.type.includes("_video") && result.type === "daily_video" + (result) => result.type === "daily_video" );packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (2)
94-96: Consider extracting date calculationThe date calculation is repeated in the test. Consider extracting it to a helper or passing it as a parameter for better reusability.
320-320: Fix grammatical error in test descriptionThe test description has a duplicate "with" word.
- test("should include conferenceCredentialId when reassigning booking with with fixed host as organizer", async () => { + test("should include conferenceCredentialId when reassigning booking with fixed host as organizer", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-store/locations.ts(3 hunks)packages/app-store/webex/api/add.ts(2 hunks)packages/app-store/webex/api/callback.ts(2 hunks)packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)packages/lib/server/service/bookingLocationService.test.ts(1 hunks)packages/lib/server/service/bookingLocationService.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/webex/api/callback.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.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/webex/api/callback.tspackages/app-store/webex/api/add.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/app-store/locations.tspackages/lib/server/service/bookingLocationService.test.tspackages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
**/*Service.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/bookingLocationService.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/webex/api/callback.tspackages/app-store/webex/api/add.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in cal.com's embed system, internal events like "__scrollbydistance" are fired by cal.com's own code...
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
Applied to files:
packages/app-store/locations.ts
📚 Learning: in cal.com's calendar integration, both google calendar and outlook calendar are designed to allow m...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Applied to files:
packages/app-store/locations.ts
📚 Learning: applies to **/*.{service,repository}.ts : avoid dot-suffixes like `.service.ts` or `.repository.ts` ...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{service,repository}.ts : Avoid dot-suffixes like `.service.ts` or `.repository.ts` for new files; reserve `.test.ts`, `.spec.ts`, `.types.ts` for their specific purposes
Applied to files:
packages/lib/server/service/bookingLocationService.test.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/bookingLocationService.tspackages/features/ee/round-robin/roundRobinManualReassignment.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.test.ts
📚 Learning: for e2e integration tests that require real third-party service credentials (like outlook calendar),...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts
🧬 Code Graph Analysis (5)
packages/app-store/webex/api/callback.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/app-store/webex/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL_FOR_OAUTH(22-22)
packages/lib/server/service/bookingLocationService.test.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
packages/lib/server/service/bookingLocationService.ts (1)
packages/app-store/locations.ts (4)
LocationObject(202-212)OrganizerDefaultConferencingAppType(70-70)CalVideoLocationType(64-64)getLocationValueForDB(379-408)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
packages/lib/server/service/bookingLocationService.ts (1)
BookingLocationService(89-247)
🔇 Additional comments (13)
packages/app-store/webex/api/add.ts (1)
4-4: Consistent constant usage looks good
Switching toWEBAPP_URL_FOR_OAUTHaligns this endpoint with the new OAuth-specific base URL introduced in@calcom/lib/constants.packages/app-store/webex/api/callback.ts (1)
3-3: Import update acknowledged
UsingWEBAPP_URL_FOR_OAUTHkeeps this file consistent with the auth-step change.packages/app-store/locations.ts (2)
1-3: Good documentation of technical debtThe TODO comment clearly indicates the planned consolidation with BookingLocationService, which aligns with the PR's goal of centralizing location management logic.
64-64: Good improvements to type safety and exports
- Exporting
CalVideoLocationTypeprovides a clearer semantic name for the Daily location type- Explicit typing of
conferenceCredentialIdasnumber | undefinedimproves type safetyAlso applies to: 384-384
packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
169-169: Good formatting improvementAdding a blank line before cloning references improves code readability by separating logical sections.
packages/features/ee/round-robin/roundRobinManualReassignment.ts (2)
159-172: Excellent integration of BookingLocationServiceThe implementation properly:
- Determines event type characteristics (managed/team)
- Uses the centralized service for location determination
- Handles both static links and dynamic conferencing apps correctly
224-233: Good handling of conferenceCredentialId extractionProperly extracts the conferenceCredentialId even when the organizer hasn't changed, ensuring consistent handling across all reassignment scenarios.
packages/lib/server/service/bookingLocationService.test.ts (1)
1-558: Excellent test coverageThe test suite provides comprehensive coverage for all BookingLocationService methods with:
- Well-organized test structure
- Thorough edge case testing
- Proper mock setup and cleanup
Consider adding a test case for when
getLocationValueForDBreturns an empty string (triggering the fallback to DailyLocationType), though this might be an edge case handled internally.packages/lib/server/service/bookingLocationService.ts (1)
1-247: Well-designed service implementationThe BookingLocationService is excellently implemented with:
- Clear separation of concerns between the three methods
- Comprehensive JSDoc documentation with examples
- Proper null handling and defensive programming
- Strong type safety with well-defined interfaces
The service successfully centralizes the booking location logic as intended by the PR objectives.
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (4)
30-50: Well-structured type definitions for test dataThe type definitions provide good type safety and clarity for the test data structures.
139-159: Clean assertion helper for EventManager verificationGood abstraction that makes test assertions more readable and maintainable.
177-229: Comprehensive mocking of EventManager reschedule behaviorExcellent mock implementation that covers various scenarios including static links, different conferencing apps, and failure cases. This provides realistic test conditions.
528-875: Excellent test coverage for location change scenariosThe new test suite provides comprehensive coverage of location change functionality during round robin reassignment. The tests cover:
- Different conferencing app transitions
- Fallback behavior
- Static video links
- Error handling
The helper functions are well-structured and promote test maintainability.
packages/features/ee/round-robin/roundRobinManualReassignment.ts
Outdated
Show resolved
Hide resolved
d04a16c to
bb6367b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (1)
844-849: Fix duplicate event type ID.Event type ID 3 is used in multiple tests, which could cause conflicts or confusion. Each test should use unique IDs to avoid potential issues.
- id: 3, + id: 5,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(8 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/features/ee/round-robin/handleRescheduleEventManager.ts
- packages/features/ee/round-robin/roundRobinManualReassignment.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/features/ee/round-robin/roundRobinManualReassignment.test.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/features/ee/round-robin/roundRobinManualReassignment.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. 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 (5)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (5)
20-137: Excellent refactoring with structured test data builders.The introduction of factory functions for test data creation significantly improves test maintainability and readability. The type definitions are well-structured, and the builders follow good patterns with sensible defaults and flexible overrides.
139-234: Well-designed mock and assertion helpers.The helper functions provide excellent abstractions that make the tests more readable and maintainable. The
mockEventManagerReschedulefunction particularly well simulates real-world conferencing scenarios including failure cases, which is crucial for comprehensive testing.
239-526: Comprehensive test coverage with improved structure.The refactored tests maintain excellent coverage of the reassignment functionality while benefiting from the new structured approach. The tests properly verify all aspects including workflow updates, conference credentials, and email notifications.
528-669: Excellent coverage for location change scenarios.This new test suite directly addresses the core issue described in the PR objectives. The tests comprehensively verify that location updates work correctly when reassigning between hosts with different default conferencing apps, which was the main problem being solved.
831-874: Proper error handling test for conferencing failures.This test correctly verifies that the system handles conferencing setup failures gracefully while still completing the meeting rescheduling process. The error message assertion ensures proper user feedback in failure scenarios.
What does this PR do?
Tech Debt Payment
Fixes PRI-298
Visual Demo (For contributors especially)
Before After loom - https://www.loom.com/share/342131fed0f6447383801b81a2279c7d
Mandatory Tasks (DO NOT REMOVE)