fix: Selected calendar delegation credentials#24190
Conversation
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/server/repository/__tests__/SelectedCalendarRepository.test.ts (1)
59-79: Fix test data inconsistency.The test mock returns a calendar with nested
credential.delegationCredentialdata (lines 60-68, 70), and the assertion expects this data (line 78). However, the actualfindByIdimplementation no longer includes credential relations in its query.The test should mock and assert only the data that
findByIdactually returns (a plainSelectedCalendarwithout nested credentials).Apply this diff to fix the test:
test("should find selected calendar by id with credential delegation", async () => { - const mockCalendarWithCredential = { - ...mockSelectedCalendar, - credential: { - delegationCredential: { - id: "delegation-id", - key: { client_email: "test@service.com" }, - }, - }, - }; - - vi.mocked(mockPrismaClient.selectedCalendar.findUnique).mockResolvedValue(mockCalendarWithCredential); + vi.mocked(mockPrismaClient.selectedCalendar.findUnique).mockResolvedValue(mockSelectedCalendar); const result = await repository.findById("test-calendar-id"); expect(mockPrismaClient.selectedCalendar.findUnique).toHaveBeenCalledWith({ where: { id: "test-calendar-id" }, }); - expect(result).toEqual(mockCalendarWithCredential); + expect(result).toEqual(mockSelectedCalendar); });Also consider updating the test description on line 59 since it mentions "with credential delegation" but the method no longer includes credentials.
🧹 Nitpick comments (2)
packages/features/calendar-subscription/lib/cache/CalendarCacheWrapper.ts (1)
63-65: Remove commented parameter.The commented
_shouldServeCache?: booleanparameter on line 64 should be removed entirely rather than left as a comment. Dead code in method signatures can cause confusion for future maintainers.Apply this diff to clean up the signature:
async getAvailability( dateFrom: string, dateTo: string, - selectedCalendars: IntegrationCalendar[] - // _shouldServeCache?: boolean + selectedCalendars: IntegrationCalendar[] // _fallbackToPrimary?: boolean ): Promise<EventBusyDate[]> {packages/lib/server/repository/SelectedCalendarRepository.ts (1)
8-12: Consider using explicitselectif not all fields are needed.The query now correctly avoids
includefor relations. However, per coding guidelines for Prisma queries, you should "only select data you need; never useinclude, always useselect".If consumers of this method don't require all
SelectedCalendarfields, consider adding aselectclause to fetch only necessary fields.Example if only core fields are needed:
async findById(id: string) { return this.prismaClient.selectedCalendar.findUnique({ where: { id }, + select: { + id: true, + userId: true, + integration: true, + externalId: true, + credentialId: true, + delegationCredentialId: true, + // ... other required fields + }, }); }
📜 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)
packages/features/calendar-subscription/lib/CalendarSubscriptionService.ts(6 hunks)packages/features/calendar-subscription/lib/__mocks__/delegationCredential.ts(1 hunks)packages/features/calendar-subscription/lib/__tests__/CalendarSubscriptionService.test.ts(6 hunks)packages/features/calendar-subscription/lib/cache/CalendarCacheWrapper.ts(1 hunks)packages/lib/delegationCredential/server.ts(2 hunks)packages/lib/server/repository/SelectedCalendarRepository.interface.ts(1 hunks)packages/lib/server/repository/SelectedCalendarRepository.ts(1 hunks)packages/lib/server/repository/__tests__/SelectedCalendarRepository.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/lib/server/repository/__tests__/SelectedCalendarRepository.test.tspackages/lib/server/repository/SelectedCalendarRepository.interface.tspackages/features/calendar-subscription/lib/__mocks__/delegationCredential.tspackages/lib/server/repository/SelectedCalendarRepository.tspackages/lib/delegationCredential/server.tspackages/features/calendar-subscription/lib/__tests__/CalendarSubscriptionService.test.tspackages/features/calendar-subscription/lib/cache/CalendarCacheWrapper.tspackages/features/calendar-subscription/lib/CalendarSubscriptionService.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/lib/server/repository/__tests__/SelectedCalendarRepository.test.tspackages/lib/server/repository/SelectedCalendarRepository.interface.tspackages/features/calendar-subscription/lib/__mocks__/delegationCredential.tspackages/lib/server/repository/SelectedCalendarRepository.tspackages/lib/delegationCredential/server.tspackages/features/calendar-subscription/lib/__tests__/CalendarSubscriptionService.test.tspackages/features/calendar-subscription/lib/cache/CalendarCacheWrapper.tspackages/features/calendar-subscription/lib/CalendarSubscriptionService.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/lib/server/repository/__tests__/SelectedCalendarRepository.test.tspackages/lib/server/repository/SelectedCalendarRepository.interface.tspackages/features/calendar-subscription/lib/__mocks__/delegationCredential.tspackages/lib/server/repository/SelectedCalendarRepository.tspackages/lib/delegationCredential/server.tspackages/features/calendar-subscription/lib/__tests__/CalendarSubscriptionService.test.tspackages/features/calendar-subscription/lib/cache/CalendarCacheWrapper.tspackages/features/calendar-subscription/lib/CalendarSubscriptionService.ts
**/*Repository.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/lib/server/repository/SelectedCalendarRepository.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/features/calendar-subscription/lib/CalendarSubscriptionService.ts
🧠 Learnings (1)
📚 Learning: 2025-09-08T07:27:42.903Z
Learnt from: vijayraghav-io
PR: calcom/cal.com#16878
File: packages/app-store/feishucalendar/api/callback.ts:72-79
Timestamp: 2025-09-08T07:27:42.903Z
Learning: In the Cal.com codebase, some calendar integrations like google-calendar already use SelectedCalendarRepository.create for selectedCalendar creation, which automatically triggers reconnection logic, while others like feishucalendar use direct prisma.selectedCalendar.create calls that bypass the repository hooks.
Applied to files:
packages/lib/server/repository/SelectedCalendarRepository.ts
🧬 Code graph analysis (6)
packages/lib/server/repository/SelectedCalendarRepository.interface.ts (1)
packages/types/Calendar.d.ts (1)
SelectedCalendar(320-323)
packages/features/calendar-subscription/lib/__mocks__/delegationCredential.ts (1)
packages/lib/delegationCredential/server.ts (1)
getCredentialForSelectedCalendar(659-680)
packages/lib/delegationCredential/server.ts (3)
packages/types/Calendar.d.ts (1)
SelectedCalendar(320-323)packages/lib/server/repository/credential.ts (1)
CredentialRepository(28-288)packages/lib/delegationCredential/clientAndServer.ts (1)
buildNonDelegationCredential(23-41)
packages/features/calendar-subscription/lib/__tests__/CalendarSubscriptionService.test.ts (2)
packages/features/calendar-subscription/lib/__mocks__/delegationCredential.ts (1)
getCredentialForSelectedCalendar(3-8)packages/lib/delegationCredential/server.ts (1)
getCredentialForSelectedCalendar(659-680)
packages/features/calendar-subscription/lib/cache/CalendarCacheWrapper.ts (1)
packages/types/Calendar.d.ts (1)
IntegrationCalendar(245-254)
packages/features/calendar-subscription/lib/CalendarSubscriptionService.ts (3)
packages/types/Calendar.d.ts (1)
SelectedCalendar(320-323)packages/features/calendar-subscription/lib/CalendarSubscriptionPort.interface.ts (1)
CalendarCredential(42-42)packages/lib/delegationCredential/server.ts (1)
getCredentialForSelectedCalendar(659-680)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/lib/server/repository/SelectedCalendarRepository.interface.ts (1)
5-9: LGTM!The method rename from
findByIdWithCredentialstofindByIdappropriately reflects that credentials are no longer fetched with the query. The interface change aligns cleanly with the implementation.packages/features/calendar-subscription/lib/__mocks__/delegationCredential.ts (1)
3-12: LGTM!The mock function rename from
getCredentialForCalendarCachetogetCredentialForSelectedCalendarcorrectly aligns with the new API introduced inpackages/lib/delegationCredential/server.ts. The mock return value structure is appropriate for testing the credential retrieval flow.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/lib/server/repository/credential.ts (1)
29-36: Consider making all methods instance methods for consistency.The class now mixes static methods (using the global
prismaimport) with an instance method (using injectedprimaClient). This creates two different usage patterns and makes testing more complex.For better consistency and testability, consider either:
- Converting all static methods to instance methods using the injected client, or
- Making this new method static like the existing
findByIdIncludeDelegationCredential(line 117)
📜 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 (2)
packages/lib/delegationCredential/server.ts(2 hunks)packages/lib/server/repository/credential.ts(3 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/lib/server/repository/credential.tspackages/lib/delegationCredential/server.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/lib/server/repository/credential.tspackages/lib/delegationCredential/server.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/lib/server/repository/credential.tspackages/lib/delegationCredential/server.ts
🧠 Learnings (1)
📚 Learning: 2025-09-01T08:56:14.071Z
Learnt from: nangelina
PR: calcom/cal.com#23486
File: packages/app-store/kyzon-space/lib/tokenManager.ts:25-31
Timestamp: 2025-09-01T08:56:14.071Z
Learning: In token refresh utilities like tokenManager.ts, accessing credential.key from Prisma is legitimate and necessary for OAuth token refresh flows. These internal utilities need stored credentials to refresh tokens and don't expose them in API responses.
Applied to files:
packages/lib/server/repository/credential.ts
🧬 Code graph analysis (2)
packages/lib/server/repository/credential.ts (2)
packages/prisma/index.ts (1)
PrismaClient(84-84)packages/prisma/selects/credential.ts (1)
credentialForCalendarServiceSelect(3-17)
packages/lib/delegationCredential/server.ts (3)
packages/types/Calendar.d.ts (1)
SelectedCalendar(320-323)packages/lib/server/repository/credential.ts (1)
CredentialRepository(28-297)packages/lib/delegationCredential/clientAndServer.ts (1)
buildNonDelegationCredential(23-41)
🔇 Additional comments (3)
packages/lib/server/repository/credential.ts (1)
169-169: LGTM!Minor formatting improvement.
packages/lib/delegationCredential/server.ts (2)
11-12: LGTM!Import changes correctly use named imports and add the necessary
SelectedCalendartype.
659-663: Ignore type mismatch concern The PrismaSelectedCalendarmodel (in packages/prisma/schema.prisma) definesdelegationCredentialId, so the function’sPartial<SelectedCalendar>signature is correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
packages/lib/server/repository/credential.ts(3 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/lib/server/repository/credential.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/lib/server/repository/credential.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/lib/server/repository/credential.ts
🧠 Learnings (1)
📚 Learning: 2025-09-01T08:56:14.071Z
Learnt from: nangelina
PR: calcom/cal.com#23486
File: packages/app-store/kyzon-space/lib/tokenManager.ts:25-31
Timestamp: 2025-09-01T08:56:14.071Z
Learning: In token refresh utilities like tokenManager.ts, accessing credential.key from Prisma is legitimate and necessary for OAuth token refresh flows. These internal utilities need stored credentials to refresh tokens and don't expose them in API responses.
Applied to files:
packages/lib/server/repository/credential.ts
🧬 Code graph analysis (1)
packages/lib/server/repository/credential.ts (2)
packages/prisma/index.ts (1)
PrismaClient(84-84)packages/prisma/selects/credential.ts (1)
credentialForCalendarServiceSelect(3-17)
E2E results are ready! |
* Fix selected calendar delegation credentials
4 participants
What does this PR do?