perf: optimize getLuckyUser for single user scenarios #19503 #22384
Conversation
|
@ajayjha1 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/10/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/10/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
bef71d1 to
c1456a9
Compare
There was a problem hiding this comment.
@ajayjha1 can you please try to address the unit tests failing. Marking it draft until then. Feel free to rfr
|
@Devanshusharma2005 I only changed the getLuckyUser.ts file, and all the tests for that passed. The failing test case seems unrelated to my changes. However, I think it might be due to some edge cases not covered, possibly because of different time zones. Let me know if you’d like me to look into and resolve that issue. |
There was a problem hiding this comment.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
|
|
||
| it("returns the single user immediately if only one user is available", async () => { | ||
| const singleUser = buildUser({ | ||
| id: 42, | ||
| username: "singleuser", | ||
| name: "Single User", | ||
| email: "singleuser@example.com", | ||
| bookings: [], | ||
| }); | ||
|
|
||
| // Mocks should not be called for data fetching in this case | ||
| const spyCalendar = vi.spyOn(CalendarManagerMock, "getBusyCalendarTimes"); | ||
| const spyPrismaUser = vi.spyOn(prismaMock.user, "findMany"); | ||
| const spyPrismaHost = vi.spyOn(prismaMock.host, "findMany"); | ||
| const spyPrismaBooking = vi.spyOn(prismaMock.booking, "findMany"); | ||
|
|
||
| await expect( | ||
| getLuckyUser({ | ||
| availableUsers: [singleUser], | ||
| eventType: { | ||
| id: 1, | ||
| isRRWeightsEnabled: false, | ||
| team: { rrResetInterval: RRResetInterval.MONTH, rrTimestampBasis: RRTimestampBasis.CREATED_AT }, | ||
| }, | ||
| allRRHosts: [], | ||
| routingFormResponse: null, | ||
| }) | ||
| ).resolves.toStrictEqual(singleUser); | ||
|
|
||
| // Ensure no unnecessary data fetching occurred | ||
| expect(spyCalendar).not.toHaveBeenCalled(); | ||
| expect(spyPrismaUser).not.toHaveBeenCalled(); | ||
| expect(spyPrismaHost).not.toHaveBeenCalled(); | ||
| expect(spyPrismaBooking).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
when early return does not happen, this test throws error instead of failing gracefully.
There was a problem hiding this comment.
The test is still not failing gracefully. The issue is still not resolved.
There was a problem hiding this comment.
cubic found 1 issue across 2 files. Review it in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| }); | ||
|
|
||
| // Mocks should not be called for data fetching in this case | ||
| const spyCalendar = vi.spyOn(CalendarManagerMock, "getBusyCalendarTimes"); |
There was a problem hiding this comment.
Spies created in this test are never restored, which can leak mocked implementations or call history into other tests executed later in the same process and cause flaky behaviour. Call mockRestore() (or vi.restoreAllMocks()) in an afterEach/finally block.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the """ WalkthroughThe changes refactor the logic for optimizing the "single user" scenario in the lucky user selection process. Early-return logic is now implemented within the data-fetching layer to prevent unnecessary data retrieval when only one user is available. The test suite is updated to verify this behavior, ensuring no redundant data-fetching occurs. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/lib/server/getLuckyUser.ts (2)
511-514: Consider removing commented code instead of keeping it.The commented-out early return logic should be removed rather than kept as comments, as it's no longer needed and adds noise to the codebase.
- // Early return if only one available user to avoid unnecessary data fetching - // if (getLuckyUserParams.availableUsers.length === 1) { - // return getLuckyUserParams.availableUsers[0]; - // }
573-576: Consider removing commented code instead of keeping it.Similar to the previous comment, this commented-out early return logic should be removed to keep the code clean.
- // Early return if only one user is available - // if (availableUsers.length === 1) { - // return { luckyUser: availableUsers[0], usersAndTheirBookingShortfalls: [] }; - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/lib/server/getLuckyUser.test.ts(3 hunks)packages/lib/server/getLuckyUser.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/lib/server/getLuckyUser.test.ts (2)
packages/lib/test/builder.ts (1)
buildUser(286-340)packages/lib/server/getLuckyUser.ts (1)
getLuckyUser(505-539)
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
🔇 Additional comments (3)
packages/lib/server/getLuckyUser.ts (1)
634-646: LGTM! Excellent optimization implementation.The early return logic at the data-fetching level is well-implemented. It correctly returns empty data structures for all expected return values when only one user is available, effectively avoiding expensive database queries and external API calls. This approach is more efficient than performing the check after data has been fetched.
packages/lib/server/getLuckyUser.test.ts (2)
29-31: LGTM! Proper mock restoration implemented.The
afterEachhook correctly addresses the mock restoration issue mentioned in past review comments by callingvi.restoreAllMocks()after each test. This ensures test isolation and prevents mocked implementations from leaking between tests.
1487-1520: Excellent test coverage for the single-user optimization.This test case effectively verifies that the optimization works as intended:
- Correct setup: Creates a single user scenario
- Proper verification: Uses spies to ensure no external calls are made
- Comprehensive coverage: Checks all relevant mock functions (calendar, user, host, booking queries)
- Clear assertions: Verifies both the return value and the absence of external calls
The test correctly validates that the performance optimization prevents unnecessary data fetching operations in single-user scenarios.
|
@kart1ka can you check it now? I’ve made the updates. |
|
|
||
| it("returns the single user immediately if only one user is available", async () => { | ||
| const singleUser = buildUser({ | ||
| id: 42, | ||
| username: "singleuser", | ||
| name: "Single User", | ||
| email: "singleuser@example.com", | ||
| bookings: [], | ||
| }); | ||
|
|
||
| // Mocks should not be called for data fetching in this case | ||
| const spyCalendar = vi.spyOn(CalendarManagerMock, "getBusyCalendarTimes"); | ||
| const spyPrismaUser = vi.spyOn(prismaMock.user, "findMany"); | ||
| const spyPrismaHost = vi.spyOn(prismaMock.host, "findMany"); | ||
| const spyPrismaBooking = vi.spyOn(prismaMock.booking, "findMany"); | ||
|
|
||
| await expect( | ||
| getLuckyUser({ | ||
| availableUsers: [singleUser], | ||
| eventType: { | ||
| id: 1, | ||
| isRRWeightsEnabled: false, | ||
| team: { rrResetInterval: RRResetInterval.MONTH, rrTimestampBasis: RRTimestampBasis.CREATED_AT }, | ||
| }, | ||
| allRRHosts: [], | ||
| routingFormResponse: null, | ||
| }) | ||
| ).resolves.toStrictEqual(singleUser); | ||
|
|
||
| // Ensure no unnecessary data fetching occurred | ||
| expect(spyCalendar).not.toHaveBeenCalled(); | ||
| expect(spyPrismaUser).not.toHaveBeenCalled(); | ||
| expect(spyPrismaHost).not.toHaveBeenCalled(); | ||
| expect(spyPrismaBooking).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The test is still not failing gracefully. The issue is still not resolved.
|
Hi @kart1ka, Do you mean:
|
43db475 to
d1022f4
Compare
When early return is missing, the code just crashes as the error thrown is not handled in the test. |
94036de to
84795d8
Compare
|
@kart1ka Please check it. test runs successfully in both the cases whether the early return present or not and that should be the expected case ig. |
This PR optimizes the getLuckyUser function to handle single-user scenarios more efficiently by adding an early return before expensive operations are performed.
Performance Impact:
Before: Single user scenarios performed 6+ database queries, calendar API calls, and complex calculations
After: Single user scenarios return in microseconds with zero external calls
Fixes #19503 (Performance optimization for single user round-robin scenarios)
Summary by cubic
Optimized getLuckyUser to return immediately for single-user scenarios, skipping all database and API calls for faster performance.