fix: Faulty logic around shouldServeCache#24465
Conversation
keithwillcode
added
core
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Add tests for CacheService.getShouldServeCache to verify it returns false when no teamId is provided (instead of undefined) - Add tests for GoogleCalendarService.getFreeBusyResult to verify the falsey check properly handles undefined, null, 0, empty string, and false values - All tests verify the fix in PR #24465 which changes undefined return to false and changes === false check to !shouldServeCache Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
- Add tests for CacheService.getShouldServeCache to verify it returns false when no teamId is provided (instead of undefined) - Add tests for GoogleCalendarService.getFreeBusyResult to verify the falsey check properly handles undefined, null, 0, empty string, and false values - All tests verify the fix in PR #24465 which changes undefined return to false and changes === false check to !shouldServeCache Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
56-56: Consider using named export instead of default export.The coding guidelines recommend named exports over default exports for better tree-shaking, easier refactoring, and clearer imports.
As per coding guidelines.
Apply this diff to convert to a named export:
-export default class GoogleCalendarService implements Calendar { +export class GoogleCalendarService implements Calendar {Then update imports across the codebase from:
import GoogleCalendarService from "./CalendarService";to:
import { GoogleCalendarService } from "./CalendarService";
📜 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 (4)
packages/app-store/googlecalendar/lib/CalendarService.ts(1 hunks)packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts(1 hunks)packages/features/calendar-cache/lib/getShouldServeCache.test.ts(1 hunks)packages/features/calendar-cache/lib/getShouldServeCache.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/calendar-cache/lib/getShouldServeCache.test.tspackages/features/calendar-cache/lib/getShouldServeCache.tspackages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.tspackages/app-store/googlecalendar/lib/CalendarService.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/calendar-cache/lib/getShouldServeCache.test.tspackages/features/calendar-cache/lib/getShouldServeCache.tspackages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.tspackages/app-store/googlecalendar/lib/CalendarService.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/features/calendar-cache/lib/getShouldServeCache.test.tspackages/features/calendar-cache/lib/getShouldServeCache.tspackages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.tspackages/app-store/googlecalendar/lib/CalendarService.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/app-store/googlecalendar/lib/CalendarService.ts
🧠 Learnings (1)
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
packages/app-store/googlecalendar/lib/CalendarService.ts
⏰ 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 (4)
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
469-469: LGTM: Falsy check correctly handles all non-cache cases.The change from
shouldServeCache === falseto!shouldServeCachecorrectly handles all falsy values (includingundefinedreturned bygetShouldServeCachewhen no teamId is provided). This ensures the cache is properly bypassed when it shouldn't be served.packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts (1)
1-135: LGTM: Comprehensive test coverage for falsy value handling.The test suite thoroughly validates that
getFreeBusyResultimmediately callsfetchAvailabilityfor all falsyshouldServeCachevalues (false, undefined, null, 0, ""), confirming the fix works as intended.packages/features/calendar-cache/lib/getShouldServeCache.test.ts (1)
1-90: LGTM: Excellent test coverage for all scenarios.The test suite comprehensively validates:
- Explicit boolean values take precedence over teamId checks
undefinedwith no/falsy teamId returnsfalsewithout repository calls- Valid teamId triggers feature flag check via repository
- Return value matches repository result
This confirms the bug fix and ensures correct behavior across all edge cases.
packages/features/calendar-cache/lib/getShouldServeCache.ts (1)
12-12: Approved: explicitfalsereturn for!teamIdis correct—tests verifyteamId = 0returnsfalse.
- Fix private credential property access by using type assertion - Add missing IFeaturesRepository methods to mock Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts (4)
10-34: Optional: Consider a cleaner mock setup pattern.The current approach of creating an empty object, manually attaching methods, and binding the prototype works but is somewhat unconventional. For better type safety and clarity, consider using a partial mock or creating a minimal test double with the required properties.
Example alternative:
- calendarService = {} as CalendarService; - - const mockFetchAvailability = vi.fn().mockResolvedValue({ + const mockFetchAvailability = vi.fn().mockResolvedValue({ calendars: { "test@example.com": { busy: [ @@ -25,11 +23,11 @@ }, }, }); - calendarService.fetchAvailability = mockFetchAvailability; + calendarService = { + fetchAvailability: mockFetchAvailability, + credential: { id: 1, userId: 1 }, + getFreeBusyResult: CalendarService.prototype.getFreeBusyResult, + } as CalendarService; fetchAvailabilitySpy = mockFetchAvailability; - - calendarService.getFreeBusyResult = CalendarService.prototype.getFreeBusyResult.bind(calendarService); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (calendarService as any).credential = { id: 1, userId: 1 };
75-93: Reconsider testing non-boolean falsy values.Tests for
null,0, and""asshouldServeCachevalues require type assertions to bypass TypeScript, indicating these are not part of the actual type contract (likelyboolean | undefined). Testing exotic falsy values that cannot occur in production adds maintenance burden without improving coverage.Consider removing these tests or consolidating them into a single edge-case test if there's a specific runtime scenario where non-boolean values might occur.
Also applies to: 95-113, 115-133
37-133: Reduce test duplication with parameterized tests.All five test cases follow an identical structure with only the
shouldServeCachevalue varying. Vitest supportstest.eachfor parameterized tests, which would significantly reduce duplication and improve maintainability.Example refactor:
test.each([ { value: false, description: "explicitly false" }, { value: undefined, description: "undefined (falsey)" }, ])("should call fetchAvailability immediately when shouldServeCache is $description", async ({ value }) => { const args = { timeMin: new Date().toISOString(), timeMax: new Date().toISOString(), items: [{ id: "test@example.com" }], }; const result = await calendarService.getFreeBusyResult(args, value); expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args); expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1); expect(result.calendars?.["test@example.com"]?.busy).toEqual([ { start: "2023-12-01T20:00:00Z", end: "2023-12-01T21:00:00Z", }, ]); });
39-40: Use fixed timestamps for test reproducibility.Using
new Date().toISOString()generates different timestamps on each test run, which can complicate debugging and make tests less reproducible. Since the specific date values don't affect the test logic, prefer fixed timestamps.Example:
- const args = { - timeMin: new Date().toISOString(), - timeMax: new Date().toISOString(), + const args = { + timeMin: "2023-12-01T10:00:00Z", + timeMax: "2023-12-01T12:00:00Z", items: [{ id: "test@example.com" }], };Also applies to: 58-59, 77-78, 97-98, 117-118
📜 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/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts(1 hunks)packages/features/calendar-cache/lib/getShouldServeCache.test.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/googlecalendar/lib/__tests__/getFreeBusyResult.test.tspackages/features/calendar-cache/lib/getShouldServeCache.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/googlecalendar/lib/__tests__/getFreeBusyResult.test.tspackages/features/calendar-cache/lib/getShouldServeCache.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.tspackages/features/calendar-cache/lib/getShouldServeCache.test.ts
⏰ 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). (8)
- GitHub Check: Production builds / Build Docs
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build API v2
- GitHub Check: Tests / Unit
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build API v1
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (1)
packages/features/calendar-cache/lib/getShouldServeCache.test.ts (1)
1-92: LGTM! Excellent test coverage.The test suite is comprehensive, well-structured, and properly validates all the key scenarios:
- Explicit boolean values bypassing feature checks
- Falsy teamId values (undefined, null, 0) correctly returning false
- Valid teamId delegation to the feature repository
- Precedence of explicit
shouldServeCacheover teamId checksThe tests align perfectly with the PR objectives of handling undefined values and ensuring correct cache-serving logic.
E2E results are ready! |
5 participants
What does this PR do?
getShouldServeCachewas returningundefinedwhen noteamIdwas passedshouldServeCache === falsecheck to!shouldServeCacheto handle all falsey values