Advanced date override handling and timezone compatibility improvements#7
Advanced date override handling and timezone compatibility improvements#7everettbu 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>
WalkthroughThe changes introduce improved timezone handling for schedule overrides and availability checks. Adjustments ensure that override times are correctly mapped between organizer and invitee timezones. Function signatures and types are updated to support timezone context, and tests are added to verify consistent schedule output across different timezones. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server (getSchedule)
participant SlotsLib (getSlots)
participant Availability (checkIfIsAvailable)
Client->>Server (getSchedule): Request schedule (with user timezones)
Server->>SlotsLib (getSlots): Get slots (pass overrides, timezones)
SlotsLib-->>Server (getSlots): Return slots (timezone-adjusted)
loop For each slot
Server->>Availability (checkIfIsAvailable): Check slot (with overrides, working hours, organizerTimeZone)
Availability-->>Server: Slot available/unavailable
end
Server-->>Client: Return filtered schedule
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
|
@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: 3
📜 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 context used
🧬 Code Graph Analysis (2)
apps/web/test/lib/getSchedule.test.ts (1)
packages/trpc/server/routers/viewer/slots.ts (1)
getSchedule(337-635)
packages/trpc/server/routers/viewer/slots.ts (4)
packages/dayjs/index.ts (1)
Dayjs(25-25)packages/types/Calendar.d.ts (1)
EventBusyDate(46-50)packages/types/schedule.d.ts (1)
WorkingHours(16-21)packages/core/getUserAvailability.ts (1)
CurrentSeats(98-98)
🔇 Additional comments (3)
packages/types/schedule.d.ts (1)
1-6: LGTM! Good addition for timezone context.Adding the optional
timeZoneproperty toTimeRangeis a clean way to associate timezone information with date overrides. This maintains backward compatibility while enabling the timezone-aware scheduling improvements.apps/web/test/lib/getSchedule.test.ts (1)
788-804: Good test coverage for timezone consistency.This test effectively verifies that the schedule returns consistent UTC slots regardless of the invitee's timezone parameter, which is the expected behavior when the underlying UTC time range remains the same.
packages/trpc/server/routers/viewer/slots.ts (1)
102-136: Add null check and consider simplifying the date override logic.The code uses
organizerTimeZonewithout checking if it's defined. Also, the logic flow with multiple early returns is hard to follow.Consider:
- Add null check for
organizerTimeZone:- const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0; + const utcOffset = organizerTimeZone ? -dayjs.tz(date.start, organizerTimeZone).utcOffset() : 0;
- Simplify the logic flow to make it more readable. The current implementation has many branches and early returns that make it difficult to understand the intended behavior.
Likely an incorrect or invalid review comment.
| 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.
Add null check for optional timeZone property.
The code accesses override.timeZone without checking if it exists, but the TimeRange type defines timeZone as optional. This could cause issues if the timezone is not provided.
Consider adding a null check or default value:
const overrides = activeOverrides.flatMap((override) => {
- const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset();
+ const organizerTimeZone = override.timeZone || organizerTimeZone; // Use passed organizerTimeZone as fallback
+ const organizerUtcOffset = dayjs(override.start.toString()).tz(organizerTimeZone).utcOffset();
const inviteeUtcOffset = dayjs(override.start.toString()).tz(timeZone).utcOffset();
const offset = inviteeUtcOffset - organizerUtcOffset;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/lib/slots.ts around lines 211 to 225, the code accesses
override.timeZone without verifying if it exists, but timeZone is optional in
the TimeRange type. To fix this, add a null check or provide a default timezone
value when accessing override.timeZone to prevent runtime errors when it is
undefined.
| 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; | ||
| } |
There was a problem hiding this comment.
Critical bug: Incorrect end time calculation in working hours check.
The end variable is using the same calculation as start, which will always make them equal. This breaks the working hours validation logic.
Fix the calculation:
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;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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; | |
| } | |
| workingHours.find((workingHour) => { | |
| if (workingHour.days.includes(slotStartTime.day())) { | |
| const start = 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 139 to 145, the
variable 'end' is incorrectly calculated using the same expression as 'start',
causing both to be equal and breaking the working hours validation. Fix this by
calculating 'end' based on the slot end time instead of the slot start time,
ensuring it correctly represents the ending minute of the slot.
| time: slot.time, | ||
| ...schedule, | ||
| ...availabilityCheckProps, | ||
| organizerTimeZone: schedule.timeZone, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent null-safety when accessing timeZone properties.
The code inconsistently handles potentially undefined timeZone values:
- Line 492:
schedule.timeZone(no null check) - Line 517:
userSchedule.timeZone(no null check) - Line 585:
userSchedule?.timeZone(with optional chaining)
Consider using consistent null-safety throughout:
- organizerTimeZone: schedule.timeZone,
+ organizerTimeZone: schedule.timeZone || organizerTimeZone,- organizerTimeZone: userSchedule.timeZone,
+ organizerTimeZone: userSchedule.timeZone || organizerTimeZone,Also applies to: 517-517, 585-585
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/slots.ts at lines 492, 517, and 585, the
timeZone properties are accessed inconsistently regarding null safety. To fix
this, apply optional chaining or null checks uniformly when accessing
schedule.timeZone and userSchedule.timeZone to prevent runtime errors from
undefined values. Update lines 492 and 517 to use optional chaining like line
585 for consistent null-safe access.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 7
Summary by CodeRabbit
New Features
Bug Fixes
Tests