Advanced date override handling and timezone compatibility improvements#5
Advanced date override handling and timezone compatibility improvements#5ShashankFC 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 |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughTimezone-aware scheduling availability checking is implemented across multiple modules. Date override computation now adjusts for timezone differences between organizer and invitee. The availability check function is enhanced to accept and validate against date overrides and working hours parameters with timezone context. Changes
Sequence Diagram(s)sequenceDiagram
participant Router as Slots Router
participant Slots as slots.ts
participant Availability as Availability Check
participant TZ as Timezone Calc
Router->>Slots: getTimeSlots(organizerTimeZone)
Slots->>TZ: Get organizer UTC offset
Slots->>TZ: Get invitee UTC offset
TZ->>Slots: Return offset delta
Slots->>Slots: Apply offset to dateOverrides
Slots->>Availability: checkIfIsAvailable(time, busy, dateOverrides, workingHours, organizerTimeZone)
Availability->>Availability: Validate slot within dateOverride range
Availability->>Availability: Check slot within workingHours
Availability-->>Slots: Return slot validity
Slots-->>Router: Return filtered time slots
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/lib/slots.ts:
- Around line 211-225: The code uses override.timeZone which is optional and may
be undefined when computing organizerUtcOffset in the activeOverrides flatMap;
update the computation to fall back to organizerTimeZone when override.timeZone
is missing (i.e., use override.timeZone || organizerTimeZone) and apply the same
fallback wherever override.timeZone is passed into dayjs(...).tz() for
calculating inviteeUtcOffset/offset so timezone conversions are safe; ensure you
reference the activeOverrides flatMap, organizerUtcOffset, inviteeUtcOffset, and
offset calculations when making the change.
In @packages/trpc/server/routers/viewer/slots.ts:
- Around line 102-135: The find() callback in the dateOverrides check
miscompares Dayjs objects with === and mixes returns with side effects via
dateOverrideExist; fix by replacing the === comparison with
dayjs(...).isSame(...) for value equality, compute adjustedStart and adjustedEnd
once inside the dateOverrides.find callback (using organizerTimeZone
adjustments), and ensure the callback always returns a boolean (return true when
the slot is outside the override: adjustedStart.isSame(adjustedEnd) OR
slotEndTime.isSameOrBefore(adjustedStart) OR slotStartTime.isAfter(adjustedEnd);
otherwise return false). Remove or stop relying on the side-effect variable
dateOverrideExist—use the boolean result of dateOverrides.find (or a dedicated
some()/every() call) to decide the subsequent returns instead of setting
dateOverrideExist inside the callback.
- Around line 137-151: The callback that checks workingHours incorrectly
computes end using slotStartTime, making start and end identical; update the
calculation of end to use slotEndTime (e.g., compute end = slotEndTime.hour() *
60 + slotEndTime.minute()) inside the workingHours.find callback and ensure the
predicate returns a boolean (true when start < workingHour.startTime || end >
workingHour.endTime) so slots that extend past workingHour.endTime are correctly
flagged.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/test/lib/getSchedule.test.tspackages/lib/slots.tspackages/trpc/server/routers/viewer/slots.tspackages/types/schedule.d.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/lib/slots.ts (1)
apps/web/lib/clock.ts (1)
timeZone(54-54)
🔇 Additional comments (8)
packages/types/schedule.d.ts (1)
1-6: LGTM!The addition of the optional
timeZoneproperty toTimeRangeis a clean, backward-compatible change that enables timezone-aware scheduling across the codebase.packages/trpc/server/routers/viewer/slots.ts (6)
75-94: LGTM on signature changes.The extended function signature properly handles backward compatibility with default values for new parameters.
410-416: LGTM!The flattening logic correctly propagates
userIdandtimeZonefrom the user's availability context into each date override entry.
439-440: Verify fallback behavior when all timezone sources are undefined.The fallback chain is reasonable, but if
eventType.timeZone,eventType?.schedule?.timeZone, anduserAvailability?.[0]?.timeZoneare all undefined,organizerTimeZonewill beundefined. Verify that downstream consumers (e.g.,getTimeSlots,checkIfIsAvailable) handle this gracefully.
488-498: LGTM!The
organizerTimeZoneis correctly derived from each user's schedule timezone when checking availability for fixed hosts.
513-518: LGTM!Consistent propagation of
organizerTimeZonefor loose (round-robin) host availability checks.
579-586: LGTM!The user schedule lookup and timezone propagation for selected slots availability check is implemented correctly.
apps/web/test/lib/getSchedule.test.ts (1)
787-804: LGTM!Good test coverage for timezone consistency. This correctly verifies that UTC-based slot times remain stable when the invitee's timezone differs from the organizer's timezone, which is essential for the date override handling.
| 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(), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for dateOverrides and where they are populated/created
rg -n "dateOverrides|activeOverrides" packages/lib/slots.ts -B 3 -A 3 | head -150Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1123
🏁 Script executed:
#!/bin/bash
# Search for override type definition or interface
rg -n "override.*timeZone|interface.*override|type.*override" packages/lib/ -i | head -50Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 440
🏁 Script executed:
#!/bin/bash
# Find where activeOverrides is assigned in slots.ts
rg -n "activeOverrides\s*=" packages/lib/slots.ts -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 365
🏁 Script executed:
#!/bin/bash
# Search for where dateOverrides or override objects are created
rg -n "dateOverrides\s*=|new.*Override|Override\s*\{" packages/lib/ -A 5 | head -100Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Find and examine the TimeRange type definition
find . -path "*/types/schedule*" -type f | head -20Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for TimeRange type definition
rg -n "type TimeRange|interface TimeRange" --type ts | head -20Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Read the TimeRange type definition from both files
echo "=== packages/types/schedule.d.ts ===" && cat -n packages/types/schedule.d.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 820
🏁 Script executed:
#!/bin/bash
# Also check the other schedule.ts file
echo "=== apps/web/lib/types/schedule.ts ===" && cat -n apps/web/lib/types/schedule.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 728
🏁 Script executed:
#!/bin/bash
# Check which @calcom package/alias is being used in the project
find . -name "tsconfig.json" -o -name "tsconfig.*.json" | xargs grep -l "calcom" | head -5Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check the actual import source for @calcom/types
cat -n tsconfig.json 2>/dev/null || cat -n apps/web/tsconfig.json 2>/dev/null || cat -n packages/tsconfig.json 2>/dev/null | head -50Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 854
🏁 Script executed:
#!/bin/bash
# Look for path mappings or package.json setup
rg -n '"@calcom' tsconfig.json -A 1 | head -20Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Let's verify what happens when dayjs().tz() is called with undefined
# Create a test to check dayjs behavior
node << 'EOF'
const dayjs = require('dayjs');
require('dayjs/plugin/utc');
require('dayjs/plugin/timezone');
dayjs.extend(require('dayjs/plugin/utc'));
dayjs.extend(require('dayjs/plugin/timezone'));
console.log("Testing dayjs().tz(undefined):");
const result = dayjs('2024-01-15').tz(undefined);
console.log("Result:", result.toString());
console.log("UTC Offset:", result.utcOffset());
console.log("\nTesting dayjs().tz() with no argument:");
const result2 = dayjs('2024-01-15').tz();
console.log("Result:", result2.toString());
console.log("UTC Offset:", result2.utcOffset());
EOFRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 922
🏁 Script executed:
#!/bin/bash
# Check the exact usage context around line 212 - see if there's any validation before the filter
sed -n '200,230p' packages/lib/slots.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1382
🏁 Script executed:
#!/bin/bash
# Search for how timeZone property is used elsewhere in the codebase
rg -n "\.timeZone" packages/lib/slots.ts -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 453
🏁 Script executed:
#!/bin/bash
# Check if there's any validation or fallback for timeZone in the codebase
rg -n "override\.timeZone|dateOverride.*timeZone" packages/lib/ -B 3 -A 3Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 617
🏁 Script executed:
#!/bin/bash
# Check how dayjs().tz() is used elsewhere in the codebase to understand expected behavior
rg -n "\.tz\(" packages/lib/slots.ts -B 1 -A 1 | head -30Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1001
Add validation or fallback for override.timeZone which is optional.
The timeZone property in TimeRange (the type for date overrides) is optional, so override.timeZone can be undefined. Passing undefined to dayjs().tz() on line 212 will not correctly handle the timezone conversion. Add a fallback to organizerTimeZone when timeZone is missing:
const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone || organizerTimeZone).utcOffset();
🤖 Prompt for AI Agents
In @packages/lib/slots.ts around lines 211 - 225, The code uses
override.timeZone which is optional and may be undefined when computing
organizerUtcOffset in the activeOverrides flatMap; update the computation to
fall back to organizerTimeZone when override.timeZone is missing (i.e., use
override.timeZone || organizerTimeZone) and apply the same fallback wherever
override.timeZone is passed into dayjs(...).tz() for calculating
inviteeUtcOffset/offset so timezone conversions are safe; ensure you reference
the activeOverrides flatMap, organizerUtcOffset, inviteeUtcOffset, and offset
calculations when making the change.
| //check if date override for slot exists | ||
| let dateOverrideExist = false; | ||
|
|
||
| if ( | ||
| dateOverrides.find((date) => { | ||
| const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0; | ||
|
|
||
| if ( | ||
| dayjs(date.start).add(utcOffset, "minutes").format("YYYY MM DD") === | ||
| slotStartTime.format("YYYY MM DD") | ||
| ) { | ||
| dateOverrideExist = true; | ||
| if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) { | ||
| return true; | ||
| } | ||
| if ( | ||
| slotEndTime.isBefore(dayjs(date.start).add(utcOffset, "minutes")) || | ||
| slotEndTime.isSame(dayjs(date.start).add(utcOffset, "minutes")) | ||
| ) { | ||
| return true; | ||
| } | ||
| if (slotStartTime.isAfter(dayjs(date.end).add(utcOffset, "minutes"))) { | ||
| return true; | ||
| } | ||
| } | ||
| }) | ||
| ) { | ||
| // slot is not within the date override | ||
| return false; | ||
| } | ||
|
|
||
| if (dateOverrideExist) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Critical bug: Dayjs object comparison uses === instead of .isSame().
Line 114 compares two Dayjs objects using ===, which checks reference equality rather than value equality. This will always be false unless the objects are the same instance.
Additionally, the find() callback has inconsistent return behavior — it sometimes returns true and sometimes returns nothing (implicitly undefined), relying on side effects via dateOverrideExist. This makes the logic harder to follow and maintain.
🐛 Proposed fix for the Dayjs comparison
- if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) {
+ if (dayjs(date.start).add(utcOffset, "minutes").isSame(dayjs(date.end).add(utcOffset, "minutes"))) {
return true;
}🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/slots.ts around lines 102 - 135, The
find() callback in the dateOverrides check miscompares Dayjs objects with ===
and mixes returns with side effects via dateOverrideExist; fix by replacing the
=== comparison with dayjs(...).isSame(...) for value equality, compute
adjustedStart and adjustedEnd once inside the dateOverrides.find callback (using
organizerTimeZone adjustments), and ensure the callback always returns a boolean
(return true when the slot is outside the override:
adjustedStart.isSame(adjustedEnd) OR slotEndTime.isSameOrBefore(adjustedStart)
OR slotStartTime.isAfter(adjustedEnd); otherwise return false). Remove or stop
relying on the side-effect variable dateOverrideExist—use the boolean result of
dateOverrides.find (or a dedicated some()/every() call) to decide the subsequent
returns instead of setting dateOverrideExist inside the callback.
| //if no date override for slot exists check if it is within normal work hours | ||
| if ( | ||
| workingHours.find((workingHour) => { | ||
| if (workingHour.days.includes(slotStartTime.day())) { | ||
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| if (start < workingHour.startTime || end > workingHour.endTime) { | ||
| return true; | ||
| } | ||
| } | ||
| }) | ||
| ) { | ||
| // slot is outside of working hours | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Critical bug: end variable incorrectly uses slotStartTime instead of slotEndTime.
Line 142 computes end using slotStartTime, making it identical to start. This defeats the purpose of checking if the slot's end time exceeds working hours. Slots that extend past workingHour.endTime will incorrectly be considered valid.
🐛 Proposed fix
if (
workingHours.find((workingHour) => {
if (workingHour.days.includes(slotStartTime.day())) {
const start = slotStartTime.hour() * 60 + slotStartTime.minute();
- const end = slotStartTime.hour() * 60 + slotStartTime.minute();
+ const end = slotEndTime.hour() * 60 + slotEndTime.minute();
if (start < workingHour.startTime || end > workingHour.endTime) {
return true;
}
}
})
) {🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/slots.ts around lines 137 - 151, The
callback that checks workingHours incorrectly computes end using slotStartTime,
making start and end identical; update the calculation of end to use slotEndTime
(e.g., compute end = slotEndTime.hour() * 60 + slotEndTime.minute()) inside the
workingHours.find callback and ensure the predicate returns a boolean (true when
start < workingHour.startTime || end > workingHour.endTime) so slots that extend
past workingHour.endTime are correctly flagged.
2 participants
Test 7nn
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#7](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/7)*