Cal.com / PR #6

Advanced date override handling and timezone compatibility

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

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Incorrect end time calculation: slotStartTime used instead of slotEndTime, making start and end identical in working hours boundaries HIGH ✓ Found Critical bug: The end variable is computed using slotStartTime instead of slotEndTime, making both start and end identical (slots.ts L145). FriendlyReviewer classified this as CRITICAL with a clear explanation of the impact on working hours boundary calculations.
#2 === used to compare dayjs objects → compares references, not values. Use .isSame() instead MEDIUM ✗ Missed FriendlyReviewer did not flag this specific dayjs comparison pattern. It is a reference equality check on two dayjs objects that happen to have the same value but different object identities — a subtle JavaScript pitfall.

Supplementary Findings (5 findings)

Additional issues found by FriendlyReviewer that are not part of the golden (reference) set.

Finding File Severity Legitimate?
Working hours validation logic inverted: find() stops at first match → slot marked unavailable due to unrelated working hour boundary slots.ts:140 HIGH ✓ Logic inversion
Date override boundary checks only consider slotStartTime.day(), ignore slotEndTime → midnight-crossing slots incorrectly assigned slots.ts:100 MEDIUM ✓ Timezone bug
organizerTimeZone without optional chaining → crash if userAvailability empty slots.ts:411 MEDIUM ✓ Runtime crash
Redundant busy time overlap checks + double .utc() unnecessary slots.ts:160 LOW ✓ Code quality
Misleading test comment: 'utc time' but timezone is Asia/Dhaka (UTC+6) getSchedule.test.ts:797 LOW ✓ Test clarity

Honest Analysis

FriendlyReviewer found 1 of 2 golden bugs (50% recall). Golden #1 (slotStartTime vs slotEndTime) was correctly identified with a clear explanation of why it causes start and end to be identical. Golden #2 (dayjs === comparison) was missed — it's a reference equality check on two dayjs objects that happen to have the same value but different object identities, a subtle JavaScript pitfall.

The inverted working hours validation logic (slots.ts L140) that FriendlyReviewer discovered is arguably more severe than either golden bug. The find() vs some() distinction means a slot may be marked unavailable based on a different working hour's boundary. This is a logic error that would produce incorrect availability results in production. FriendlyReviewer correctly identified it as CRITICAL.

The 2 MEDIUM and 2 LOW supplementary findings are thin but relevant for a 4-file, +111/-10 PR. The misleading test comment finding is the kind of pedantry that makes FriendlyReviewer feel noisy, but it's factually correct: calling UTC+6 'utc time' is wrong.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
2 high, 2 medium, 2 low

This merge request introduces advanced date override handling and timezone compatibility changes to the scheduling system. While the direction is correct for improving timezone support, the implementation introduces several critical bugs and logic errors that must be addressed before merging. The most significant issues include: (1) incorrect end time calculation using slotStartTime instead of slotEndTime, making start and end identical in working hours boundaries, (2) an inverted working hours validation logic using find() instead of some() that can mark slots unavailable based on unrelated working hour boundaries, (3) date override boundary checks that ignore slotEndTime.day() leading to incorrect midnight-crossing slot assignments, (4) a potential runtime crash when userAvailability is empty due to missing optional chaining on organizerTimeZone, (5) redundant busy time overlap checks and unnecessary double .utc() calls, and (6) a misleading test comment that labels Asia/Dhaka (UTC+6) as 'utc time'. The review recommends blocking the merge until these issues are resolved.

These might need a close look

  • 🔴 packages/core/slots.ts (L145)
    CRITICAL: The end variable is computed using slotStartTime instead of slotEndTime, making both start and end identical. This causes all working hours boundary comparisons to operate on the same value, breaking time range calculations entirely.
  • 🔴 packages/core/slots.ts (L140)
    CRITICAL: Working hours validation logic uses Array.find() which stops at the first match, instead of Array.some() or Array.every(). A slot can be marked as unavailable because an unrelated working hour's boundary fails validation. Use .some() to check across all working hours.

Worth checking

  • 🟡 packages/core/slots.ts (L100)
    Date override boundary checks only consider slotStartTime.day() and ignore slotEndTime. When a slot crosses midnight, the day index check for the end time is skipped, which can incorrectly assign availability for overnight bookings. Include slotEndTime.day() in the boundary comparison.
  • 🟡 packages/core/slots.ts (L411)
    organizerTimeZone is accessed without optional chaining after destructuring from userAvailability. If userAvailability is empty or malformed, this throws a runtime error. Add a guard or use optional chaining.

Small things (take or leave)

  • 🔵 packages/core/slots.ts (L160)
    Redundant busy time overlap checks: the same busy time entries are checked multiple times with unnecessary double .utc() conversion calls. This doesn't change behavior but adds cognitive overhead. Consolidate the conversion.
  • 🔵 packages/core/getSchedule.test.ts (L797)
    Misleading test comment: the comment reads 'utc time' but the timezone used is Asia/Dhaka (UTC+6). UTC+6 is not UTC. Update the comment to reflect the actual timezone being tested.