feat: A new feature auto-referesh slots on timezone change #22455
Conversation
|
@Vansh5632 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/13/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/13/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
cubic found 1 issue across 4 files. Review it in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.ts
Outdated
Show resolved
Hide resolved
…ChangeDetection.ts Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
Hey @Vansh5632, Thanks for the PR. Could you please add a loom video showing that your changes work? |
WalkthroughThis change introduces timezone change detection to the booking flow. A new React hook, Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/features/bookings/Booker/components/TimezoneIntegration.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 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). (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I have added the video showing the changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.ts (2)
17-28: Remove dead code from useEffect.The ref is initialized with the current timezone value on line 15, so the null check on line 19 will never be true. This code is unreachable and should be removed.
useEffect(() => { - // Initialize the previous timezone on first render - if (previousTimezoneRef.current === null) { - previousTimezoneRef.current = timezone; - return; - } - - // Update the previous timezone when timezone changes - if (previousTimezoneRef.current !== timezone) { - previousTimezoneRef.current = timezone; - } + // Update the previous timezone when timezone changes + previousTimezoneRef.current = timezone; }, [timezone]);
30-40: Consider caching the shouldRefreshSlots result.The
shouldRefreshSlotsfunction is called on every render, but its result only changes when timezone or event data changes. Consider memoizing this calculation for better performance.+import { useEffect, useRef, useMemo } from "react"; - const shouldRefreshSlots = () => { + const shouldRefreshSlots = useMemo(() => { const hasTimezoneChanged = previousTimezoneRef.current !== timezone; const hasRestrictionSchedule = !!eventData?.restrictionScheduleId; const isUsingBookerTimezone = !!eventData?.useBookerTimezone; // Only refresh slots when: // 1. Timezone has changed // 2. Event has a restriction schedule // 3. Event is configured to use booker's timezone return hasTimezoneChanged && hasRestrictionSchedule && isUsingBookerTimezone; - }; + }, [timezone, eventData?.restrictionScheduleId, eventData?.useBookerTimezone]); return { - shouldRefreshSlots: shouldRefreshSlots(), + shouldRefreshSlots, currentTimezone: timezone, previousTimezone: previousTimezoneRef.current, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/features/bookings/Booker/Booker.tsx(2 hunks)packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.ts(1 hunks)packages/features/bookings/types.ts(1 hunks)packages/features/eventtypes/lib/getPublicEvent.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/features/bookings/Booker/Booker.tsx (1)
packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.ts (1)
useTimezoneChangeDetection(13-47)
🔇 Additional comments (4)
packages/features/bookings/types.ts (1)
65-66: LGTM! Type extension correctly supports the new timezone detection feature.The addition of
restrictionScheduleIdanduseBookerTimezoneproperties to theBookerEventtype properly supports the timezone change detection functionality. The optional and nullable typing is appropriate for these database fields.packages/features/eventtypes/lib/getPublicEvent.ts (1)
155-156: LGTM! Database selection correctly includes the new timezone-related fields.The addition of
restrictionScheduleIdanduseBookerTimezoneto the Prisma selection ensures these fields are fetched from the database and available for the timezone change detection logic.packages/features/bookings/Booker/Booker.tsx (2)
39-39: LGTM! Clean import of the new timezone detection hook.
133-151: LGTM! Proper integration of timezone change detection.The implementation correctly:
- Extracts the required event data properties for timezone detection
- Uses the hook to determine when slots should be refreshed
- Invalidates the schedule when timezone changes meet the specified conditions
The conditional logic aligns with the PR requirements to only refresh slots when both a restriction schedule is active and the "use booker's timezone" option is enabled.
|
Hey @Vansh5632 , if you need support fixing the failing tests or addressing any remaining feedback, happy to collaborate! |
|
@Vansh5632 Thank you for the PR. Could you please add tests to confirm and ensure that your changes work 🙏 |
- Add unit tests for useTimezoneChangeDetection hook (10 test cases) - Add type definition tests for BookerEvent interface (3 test cases) - Add integration tests for Booker component timezone handling (6 test cases) - Add detailed documentation in TIMEZONE_TESTS.md - Fix hook logic to prevent refresh on initial render - Total coverage: 19 passing tests across unit, type, and integration levels
|
@alishaz-polymath i have added the required test cases for the changes |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/features/bookings/types.test.ts (1)
8-41: Consider expanding test coverage for type safety.While the current tests verify basic property assignment, consider adding tests for:
- Type constraints and validation
- Integration with other BookerEvent properties
- Edge cases with different data types
Example additional test case:
it("should maintain type safety with all required properties", () => { const completeEvent: BookerEvent = { // ... all required properties restrictionScheduleId: 123, useBookerTimezone: true, // This would help catch type mismatches }; expect(completeEvent).toBeDefined(); });packages/features/bookings/Booker/components/TimezoneIntegration.test.tsx (1)
81-315: Comprehensive component-level integration tests!This test suite provides excellent coverage of the Booker component's timezone integration functionality. The tests validate:
- Initial render behavior without triggering slot refresh
- Proper slot refresh triggering when conditions are met
- Event data structure handling and extraction
- Graceful handling of null/missing event data
- Various restriction schedule configurations
The
MockBookercomponent effectively simulates the actual component behavior for testing purposes.Consider consolidating with similar integration tests.
There appears to be some overlap between this test file and
TimezoneIntegration.test.tsx. Consider whether both files are necessary or if they can be consolidated to reduce maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/features/bookings/Booker/TimezoneIntegration.test.tsx(1 hunks)packages/features/bookings/Booker/components/TimezoneIntegration.test.tsx(1 hunks)packages/features/bookings/Booker/components/hooks/TIMEZONE_TESTS.md(1 hunks)packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.test.ts(1 hunks)packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.ts(1 hunks)packages/features/bookings/types.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/features/bookings/Booker/components/hooks/TIMEZONE_TESTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
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.
packages/features/bookings/Booker/components/TimezoneIntegration.test.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
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.
🧬 Code Graph Analysis (1)
packages/features/bookings/types.test.ts (1)
packages/features/bookings/types.ts (1)
BookerEvent(34-70)
⏰ 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 (2)
packages/features/bookings/Booker/components/hooks/useTimezoneChangeDetection.test.ts (1)
19-310: Excellent comprehensive test coverage!This test suite thoroughly covers the
useTimezoneChangeDetectionhook with proper scenarios including:
- Initial state and first render behavior
- Timezone change detection with proper conditions
- Graceful handling of null/undefined event data
- Edge cases with missing restriction schedules
- Rapid timezone changes and various formats
- Proper ref updates for previous timezone tracking
The mocking strategy and test structure are well-implemented and provide confidence in the hook's reliability.
packages/features/bookings/Booker/TimezoneIntegration.test.tsx (1)
97-252: Well-structured integration tests!This test suite effectively validates the integration between event data and timezone detection functionality. The tests cover:
- Proper event data extraction and passing to the hook
- Handling of various event data states (null, missing, with restrictions)
- Correct slot refresh triggering based on timezone changes
- Graceful handling of edge cases
The simplified
TimezoneIntegrationTestcomponent approach effectively isolates the integration logic for testing.
- Remove duplicate TimezoneIntegration.test.tsx from incorrect location - Fix import paths in components/TimezoneIntegration.test.tsx to use correct relative paths - All 7 integration tests now pass successfully
|
@alishaz-polymath can you please review it and tell me further changes |
|
This PR is being marked as stale due to inactivity. |
|
can anyone check this PR and tell me the changes required to make in this |
|
@Vansh5632 Yes, We will review. |
There was a problem hiding this comment.
I don't think this file is needed.
There was a problem hiding this comment.
okay removing this
What does this PR do?
Fixes an issue where available time slots were not refreshing when users changed their timezone in events with restriction schedules enabled.
##Video
Screencast.from.2025-07-15.15-07-39.mp4
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
restrictionScheduleIdset)useBookerTimezone: true)useBookerTimezone: false(should NOT refreshSummary by cubic
Time slots now refresh automatically when users change their timezone on booking pages with restriction schedules and "use booker's timezone" enabled, matching CAL-6060 requirements.