Advanced date override handling and timezone compatibility improvements#5
Advanced date override handling and timezone compatibility improvements#5akshayutture-augment wants to merge 1 commit into
Conversation
* fix date override for fixed round robin + time zone in date override * check if slot is within working hours of fixed hosts * add test for date override in different time zone * fix date overrides for not fixed hosts (round robin) * code clean up * fix added test * use the correct timezone of user for date overrides --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce timezone-aware slot availability checking across the scheduling system. Timezone information is propagated through override calculations, availability checks, and slot generation logic. Type definitions are enhanced to support timezone metadata on time ranges, and test coverage validates timezone-consistent UTC slot emission. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getSchedule
participant checkIfIsAvailable
participant Availability Store
Caller->>getSchedule: request with timezone info
Note over getSchedule: Propagate organizerTimeZone
getSchedule->>getSchedule: Augment dateOverrides with<br/>userId & timeZone
loop For each time slot
getSchedule->>checkIfIsAvailable: slot, organizerTimeZone,<br/>dateOverrides, workingHours
alt Slot aligned with override
checkIfIsAvailable->>checkIfIsAvailable: Apply timezone offset<br/>adjustment to override times
checkIfIsAvailable-->>getSchedule: Valid (within override)
else Slot outside working hours
checkIfIsAvailable->>checkIfIsAvailable: Check workingHours<br/>constraints
checkIfIsAvailable-->>getSchedule: Valid/Invalid
else Slot not in override
checkIfIsAvailable->>Availability Store: Check availability
Availability Store-->>checkIfIsAvailable: availability data
checkIfIsAvailable-->>getSchedule: Valid/Invalid
end
end
getSchedule-->>Caller: Available time slots<br/>(UTC-normalized)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Key areas requiring attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/slots.ts (1)
75-152: Fix date override equality and working-hours end-time logic incheckIfIsAvailableTwo critical correctness issues confirmed:
Dayjs equality using
===(line 114)
Dayjs===checks object identity, not date value — two distinct Dayjs instances with the same moment are never===. The conditionif (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes"))will never be true and must use.isSame()instead.Working-hours end-time ignores event length (lines 139–144)
Bothstartandendare computed identically fromslotStartTime, whileslotEndTime(correctly defined at line 99) is never used for the end calculation. A slot extending beyondworkingHour.endTimewill not be caught.Apply the provided diff to fix both issues, and consider refactoring timezone handling to consistently use
.tz(organizerTimeZone)to avoid subtle off-by-one-day issues around DST.
🧹 Nitpick comments (1)
packages/lib/slots.ts (1)
210-246: Verify override offset math and handle missingoverride.timeZonedefensivelyThe new override mapping:
const overrides = activeOverrides.flatMap((override) => { const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset(); const inviteeUtcOffset = dayjs(override.start.toString()).tz(timeZone).utcOffset(); const offset = inviteeUtcOffset - organizerUtcOffset; return { userIds: override.userId ? [override.userId] : [], startTime: dayjs(override.start).utc().add(offset, "minute").hour() * 60 + dayjs(override.start).utc().add(offset, "minute").minute(), endTime: dayjs(override.end).utc().add(offset, "minute").hour() * 60 + dayjs(override.end).utc().add(offset, "minute").minute(), }; });concerns:
Fragility if
override.timeZoneis undefined
TimeRange.timeZoneis optional, andgetSlotsis exported; other callers might pass overrides withouttimeZone. In that case,dayjs(...).tz(override.timeZone)will fall back to a default zone, which is likely not the organizer’s actual tz and will skew offsets.A safer pattern is to fall back to
organizerTimeZone(or even the invitee tz) whenoverride.timeZoneis missing:
- const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset();
- const organizerUtcOffset = dayjs(override.start.toString())
- .tz(override.timeZone || organizerTimeZone)
- .utcOffset();
2. **Offset arithmetic is hard to reason about** You first derive an offset difference (`inviteeUtcOffset - organizerUtcOffset`) and then apply it to `dayjs(override.start).utc()`. This is non-obvious and easy to get wrong, especially around DST transitions. A more direct and self-explanatory approach is to compute the override start/end directly in the invitee’s timezone and then take hour/minute, e.g.: ```ts const startInInviteeTz = dayjs(override.start) .tz(override.timeZone || organizerTimeZone) .tz(timeZone); const endInInviteeTz = dayjs(override.end) .tz(override.timeZone || organizerTimeZone) .tz(timeZone); const startTime = startInInviteeTz.hour() * 60 + startInInviteeTz.minute(); const endTime = endInInviteeTz.hour() * 60 + endInInviteeTz.minute();This makes it explicit that we’re expressing the override window in the invitee’s local day, and avoids manual offset math.
Given how central this is to availability, I’d strongly recommend adding/expanding tests that cover:
- Organizer and invitee in different non-DST timezones (already partially covered).
- A DST transition day where organizer and invitee observe DST differently.
to validate that the minute-of-day windows line up as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/test/lib/getSchedule.test.ts(1 hunks)packages/lib/slots.ts(1 hunks)packages/trpc/server/routers/viewer/slots.ts(9 hunks)packages/types/schedule.d.ts(1 hunks)
🔇 Additional comments (6)
packages/types/schedule.d.ts (1)
1-6:TimeRange.timeZoneaddition matches new override usageAdding
timeZone?: stringonTimeRangealigns with how date overrides are now consumed (e.g., inpackages/lib/slots.ts) and keeps it optional for backward compatibility. No change needed.packages/trpc/server/routers/viewer/slots.ts (4)
22-22: ImportingWorkingHourstype is consistent with new usageThe new
WorkingHoursimport correctly types theworkingHoursparameter incheckIfIsAvailableand keeps this helper aligned with@calcom/types/schedule.
483-499: Per-userorganizerTimeZonein availability checksIn both fixed-host and loose-host paths you now pass the user’s own timezone into
checkIfIsAvailable:const isAvailable = checkIfIsAvailable({ time: slot.time, ...schedule, ...availabilityCheckProps, organizerTimeZone: schedule.timeZone, }); … return checkIfIsAvailable({ time: slot.time, ...userSchedule, ...availabilityCheckProps, organizerTimeZone: userSchedule.timeZone, });This makes the date override logic in
checkIfIsAvailableuser-specific, which is what we want for heterogeneous teams. Once the bugs in that helper are fixed, this wiring looks correct.Also applies to: 505-523
579-586:checkIfIsAvailablecall for selected-slot busy times ignores overrides by designHere you:
const userSchedule = userAvailability.find(({ user: { id: userId } }) => userId === slotUserId); return checkIfIsAvailable({ time: slot.time, busy, ...availabilityCheckProps, organizerTimeZone: userSchedule?.timeZone, });By omitting
workingHoursanddateOverrides, this path reuses existing availability-filtered slots and only layers on additional busy intervals fromselectedSlots, which is a sensible separation of concerns.Once
checkIfIsAvailable’s override/working-hours logic is corrected, this call site should behave as intended.
410-417: Add runtime guard for missingorganizerTimeZoneThe
organizerTimeZonederivation at lines 439–440 can result inundefined(all three fallback sources falsy), but theGetSlotstype requires it as a non-optionalstring. Currently there is no guard.Add the suggested check:
const organizerTimeZone = eventType.timeZone || eventType?.schedule?.timeZone || userAvailability?.[0]?.timeZone; if (!organizerTimeZone) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Missing organizer timezone for getSchedule", }); }Additionally, verify in
packages/lib/slots.tsline 212 that the override adjustment (override.timeZone) handles legacy data safely. The new decorator (lines 410–414) now addstimeZonefromavailability, but edge cases with missingtimeZoneon overrides should be tested with mixed-host teams and older events.apps/web/test/lib/getSchedule.test.ts (1)
742-805: New date-override test across timezones validates core behaviorThe added block:
const scheduleForEventOnADayWithDateOverrideDifferentTimezone = await getSchedule({ eventTypeId: 1, eventTypeSlug: "", startTime: `${plus1DateString}T18:30:00.000Z`, endTime: `${plus2DateString}T18:29:59.999Z`, timeZone: Timezones["+6:00"], }, ctx); // it should return the same as this is the utc time expect(scheduleForEventOnADayWithDateOverrideDifferentTimezone).toHaveTimeSlots( ["08:30:00.000Z", "09:30:00.000Z", "10:30:00.000Z", "11:30:00.000Z"], { dateString: plus2DateString } );nicely asserts that, with the same underlying override, changing invitee timezone from
+5:30to+6:00leaves the emitted UTC slots unchanged. This is exactly the invariant your new override logic is supposed to guarantee.If you want to further tighten this, you could also assert equality of the two full
slotsstructures, but the current expectation is already strong and focused.
2 participants
Test 7
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/cal.com-coderabbit#7