Cal.com / PR #26

Advanced date override handling and timezone compatibility improvements

calcom/cal.com#8330
Golden recall: 50% Extra findings: 5

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Incorrect end time calculation using slotStartTime instead of slotEndTime MEDIUM ✓ Found "end is computed from slotStartTime instead of slotEndTime (copy-paste error), making start and end identical. The right-hand bound test (end > workingHour.endTime) thus compares the slot start against the range end, so a slot that begins before but extends past the range end is not rejected. Currently masked by getSlots never emitting overflowing slots, this is still a latent defect" (packages/trpc/server/routers/viewer/slots.ts L142)
#2 Using === for dayjs object comparison will always return false as it compares object references, not values. Use .isSame() method instead: dayjs(date.start).add(utcOffset, 'minutes').isSame(dayjs(date.end).add(utcOffset, minutes)) MEDIUM ✗ Not found The review critiques slots.ts at length — the short-circuit before the busy check (L133), the inverted workingHours check (L143), the end-time copy-paste (L142) — and lib/slots.ts (DST offset L219, optional timeZone L212), but no candidate mentions the comparison by reference: start === end on two dayjs objects compares object references and always returns false, where .isSame() is required. See packages/trpc/server/routers/viewer/slots.ts:114

Supplementary Findings (5 findings)

Finding File Severity Legitimate?
Slots inside a date override short-circuit before the busy check: when dateOverrideExist is true, return true exits before busy.every(...) runs — a fixed host with an existing booking overlapping their own override is still offered as available, a double-booking hazard for round-robin events slots.ts:133 HIGH ✓ Real — double-booking hazard; the most severe finding of the review
The workingHours check uses find and marks the slot unavailable if it violates ANY range, instead of requiring it to be inside at least one range — hosts with split working hours lose valid slots; regression affects all personal/fixed/loose hosts, not only override days slots.ts:143 HIGH ✓ Real — inverted check removes valid slots for split-schedule hosts
The date-override timezone shift computes offset once from offsets at override.start only — wrong if the window spans a DST transition; hour() * 60 + minute() also silently wraps a window crossing midnight (e.g. 23:30 + 60 min → 00:30) lib/slots.ts:219 MEDIUM ✓ Real — DST and midnight rollover in the shifted coordinate space
The added test only exercises a personal event (single user, no ROUND_ROBIN, hosts: []) — the PR's primary scenario (round-robin with fixed host + loose host and a date override) is not covered getSchedule.test.ts:739 MEDIUM ✓ Real — the headline scenario of the PR is untested
override.timeZone is accessed unconditionally while TimeRange.timeZone is optional in the public GetSlots type — any future caller without it would call .tz(undefined) and misbehave slots.ts:212 LOW ✓ Borderline: defensive nit; the only current caller always passes timeZone

Honest Analysis

FriendlyReviewer found 1 of the 2 golden comments of this PR — recall 50%. Golden #1 (the slotStartTime/slotEndTime copy-paste) is found at slots.ts L142 with the full mechanics: start and end are identical, the right-hand bound test compares the slot start against the range end, and the defect is currently masked by getSlots never emitting overflowing slots. Golden #2 (comparing two dayjs objects with ===) is missed.

The miss says a lot about where the review's attention went. It critiques the same file at length — the short-circuit before the busy check (L133), the inverted workingHours check (L143), the timezone-shift math (lib/slots.ts L219), the optional timeZone access (L212) — but no candidate examines the semantics of the equality comparison: start === end on two dayjs objects compares references and always returns false, where .isSame() is needed. The two high findings it did produce are genuine and arguably more severe than either golden: the L133 short-circuit can double-book a fixed host, and the L143 inverted find-based check removes valid slots for any host with split working hours — a regression affecting all hosts, not just override days.

The most useful supplementary item is the test-coverage finding (getSchedule.test.ts L739): the PR's headline scenario — a round-robin event with a fixed host and a date override — is never exercised by the new test, which only covers a personal event. That is exactly the behavior the review's own high findings put at risk. Overall the review is strong on control flow and arithmetic but blind to value-comparison semantics, which is precisely where golden #2 sits.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
2 high, 3 medium, 1 low 1 issue, 1 mitigated

The MR correctly threads timeZone into date overrides and adds a router-side guard to exclude fixed hosts' slots outside their override window, and the new tests cover a timezone-shift case. However, the new availability logic in the router contains three defects — a short-circuit that skips the busy check (double-booking risk), an inverted workingHours check that removes valid slots, and an end-time copy-paste error — and the test suite does not cover the headline round-robin fixed-host scenario. These issues should be addressed before merge.

These might need a close look

  • 🔴 packages/trpc/server/routers/viewer/slots.ts (L133)
    Slots inside a date override short-circuit before the busy check: when dateOverrideExist is true, return true exits before busy.every(...) runs, so a fixed host with an existing calendar/Cal booking overlapping their own override is still offered as available — a double-booking hazard for round-robin events. A date override should only override working hours, not suppress the busy-time check.
  • 🔴 packages/trpc/server/routers/viewer/slots.ts (L143)
    The workingHours check uses find and marks the slot unavailable if it violates ANY range, instead of requiring it to be inside at least one range. For hosts with split working hours on the same day (e.g. 09:00–13:00 and 14:00–18:00), a slot in the second range is wrongly removed. This regression affects all personal/fixed/loose hosts, not only override days.

Worth checking

  • 🟡 packages/trpc/server/routers/viewer/slots.ts (L142)
    end is computed from slotStartTime instead of slotEndTime (copy-paste error), making start and end identical. The right-hand bound test (end > workingHour.endTime) thus compares the slot start against the range end, so a slot that begins before but extends past the range end is not rejected. Currently masked by getSlots never emitting overflowing slots, this is still a latent defect.
  • 🟡 packages/lib/slots.ts (L219)
    The date-override timezone shift computes offset once from offsets at override.start only, which is wrong if the override window spans a DST transition in either zone. Also hour() * 60 + minute() rolls over after 24:00, silently wrapping a window that crosses midnight in the shifted coordinate space (e.g. 23:30 + 60 min → 00:30), producing an empty/truncated/mis-placed window. Consider clamping/segmenting across midnight rather than mod-wrapping.
  • 🟡 apps/web/test/lib/getSchedule.test.ts (L739)
    The added test only exercises a personal event (single user, no ROUND_ROBIN, hosts: []). The PR's primary scenario — a round-robin event with a fixed host + loose host, a date override on the fixed host, and an assertion that slots where the fixed host is unavailable are removed — is not covered. Please add a ROUND_ROBIN test with isFixed host and date override.
Small things (take or leave)
  • 🔵 packages/lib/slots.ts (L212)
    override.timeZone is accessed unconditionally while TimeRange.timeZone is optional in the public GetSlots type. Today the only caller passing dateOverrides always supplies timeZone, but any future caller without it would call .tz(undefined) and misbehave. Either make timeZone required on the override type or guard the access. Relatedly, the router's checkIfIsAvailable signature types dateOverrides as { start: Date; end: Date }[], omitting userId/timeZone actually present — align the type with TimeRange.

✅ Feature-level checklist

Not there yet

  • ❌ Date overrides of fixed hosts are taken into account for round-robin events, preventing round-robin events being booked where the fixed hosts are unavailable.
    The new checkIfIsAvailable guard short-circuits on override slots before the busy check, so a fixed host with an existing booking overlapping their override can still be offered — a double-booking hazard. The workingHours check is also inverted and the end time is computed from slotStartTime, which can wrongly remove or fail to reject slots. The test suite does not exercise the round-robin fixed-host scenario.

Partially covered

  • ⚠️ Date overrides adjust to different timezones (available slots on the booking page change according to the invitee's timezone).
    The implementation threads timeZone and applies an offset, and a Dhaka +6:00 test is covered. However, the offset is computed only at override start, which may be incorrect across DST transitions, and past-midnight rollover is not handled. Also override.timeZone is accessed unconditionally while the type is optional.