Skip to content

Advanced date override handling and timezone compatibility improvements#7

Open
everettbu wants to merge 1 commit into
date-algorithm-basefrom
date-algorithm-enhanced
Open

Advanced date override handling and timezone compatibility improvements#7
everettbu wants to merge 1 commit into
date-algorithm-basefrom
date-algorithm-enhanced

Conversation

@everettbu

Copy link
Copy Markdown

Test 7

* 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>

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR enhances timezone compatibility for date override functionality in the Cal.com scheduling system. The changes span across type definitions, slot calculation logic, and availability checking.

The core changes include:

  1. Type System Enhancement: Added an optional timeZone field to the TimeRange type in packages/types/schedule.d.ts, enabling timezone context to be stored with date overrides.

  2. Slot Calculation Improvements: Modified packages/lib/slots.ts to implement sophisticated timezone offset calculations when processing date overrides. The system now calculates the difference between organizer and invitee UTC offsets and adjusts override times accordingly.

  3. Availability Checking Logic: Enhanced packages/trpc/server/routers/viewer/slots.ts with new parameters (dateOverrides, workingHours, organizerTimeZone) to the checkIfIsAvailable function. This includes complex date override validation with timezone offset calculations and working hours validation as a fallback.

  4. Test Coverage: Added a comprehensive test case in apps/web/test/lib/getSchedule.test.ts to verify that date overrides maintain UTC consistency when requested from different timezones.

These changes address the critical need for consistent timezone handling in a global scheduling application, ensuring that availability calculations remain accurate regardless of the timezone differences between organizers and invitees. The implementation focuses on maintaining UTC consistency while properly handling timezone-specific date overrides.

Confidence score: 1/5

• This PR has critical bugs that will cause immediate production issues and should not be merged.
• The timezone calculation logic contains fundamental errors, including incomplete implementations, mathematical mistakes in UTC offset calculations, and a critical bug in working hours validation that could break availability checking.
• All modified files need significant attention, particularly packages/lib/slots.ts and packages/trpc/server/routers/viewer/slots.ts which contain the most critical issues.

4 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

workingHours.find((workingHour) => {
if (workingHour.days.includes(slotStartTime.day())) {
const start = slotStartTime.hour() * 60 + slotStartTime.minute();
const end = slotStartTime.hour() * 60 + slotStartTime.minute();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Bug: end should use slotEndTime instead of slotStartTime. This will incorrectly validate working hours as both start and end use the same time.

Suggested change
const end = slotStartTime.hour() * 60 + slotStartTime.minute();
const end = slotEndTime.hour() * 60 + slotEndTime.minute();

slotStartTime.format("YYYY MM DD")
) {
dateOverrideExist = true;
if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Potential issue: Using === for dayjs comparison may not work as expected. Consider using .isSame() method instead.

Suggested change
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"))) {

@github-actions

Copy link
Copy Markdown
Contributor

This PR is being marked as stale due to inactivity.

@GitHoobar

Copy link
Copy Markdown

Review Summary

🏷️ Draft Comments (7)

Skipped posting 7 draft comments that were valid but scored below your review threshold (>=13/15). Feel free to update them here.

apps/web/test/lib/getSchedule.test.ts (1)

234-1166: getSchedule.test.ts contains very large, monolithic test cases with repeated scenario setup logic, making maintenance and future extension difficult and error-prone.

📊 Impact Scores:

  • Production Impact: 1/5
  • Fix Specificity: 2/5
  • Urgency Impact: 2/5
  • Total Score: 5/15

🤖 AI Agent Prompt (Copy & Paste Ready):

Refactor the test suite in apps/web/test/lib/getSchedule.test.ts (lines 234-1166) to break up large, monolithic test cases into smaller, focused tests. Extract repeated scenario setup logic (such as booking scenario creation and busy time injection) into reusable helper functions. This will significantly improve maintainability, readability, and reduce the risk of errors as the test suite grows.

packages/lib/slots.ts (2)

211-224: offset calculation for date overrides uses override.start.toString(), which may yield incorrect results if override.start is a Date object not in the expected timezone, leading to wrong slot times for invitees in different timezones.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 4/5
  • Urgency Impact: 3/5
  • Total Score: 11/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In packages/lib/slots.ts, lines 211-224, the calculation of the offset for date overrides uses `override.start.toString()` with dayjs, which can yield incorrect results if the Date object is not in the expected timezone. This can cause slot times to be wrong for invitees in different timezones. Please update the code to use `dayjs.tz(override.start, override.timeZone)` and `dayjs.tz(override.start, timeZone)` to ensure correct timezone interpretation, and use these objects for all subsequent calculations. Also, ensure the same is done for `override.end`.

231-240: computedLocalAvailability.findIndex inside a while loop (lines 231-240) results in O(n^2) time complexity for large numbers of overrides and availabilities, causing major performance degradation as data scales.

📊 Impact Scores:

  • Production Impact: 3/5
  • Fix Specificity: 2/5
  • Urgency Impact: 2/5
  • Total Score: 7/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In packages/lib/slots.ts, lines 231-240, the code uses a while loop with findIndex to remove matching availabilities, resulting in O(n^2) time complexity. Refactor this block to a single reverse for loop that removes matching entries in O(n) time. Replace the while/findIndex/indexes logic with a reverse for loop that checks the condition and splices directly.

packages/trpc/server/routers/viewer/slots.ts (4)

377-407: Promise.all(users.map(...getUserAvailability)) in getSchedule can cause N+1 query/memory issues for large user lists, leading to high latency and resource usage.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 2/5
  • Urgency Impact: 3/5
  • Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In packages/trpc/server/routers/viewer/slots.ts, lines 377-407, the code uses `Promise.all(users.map(...getUserAvailability))` which can cause N+1 query and memory issues for large user lists, leading to high latency and resource usage. Refactor this to batch fetch all user availabilities in a single call to `getUserAvailability`, passing the entire user list and relevant parameters, so that the function can optimize queries and reduce resource consumption.

504-522,566-590: Multiple .filter(...).map(...) and .map(...).filter(...) chains on large availableTimeSlots arrays (lines 504-522, 566-590) cause unnecessary iterations and memory churn, degrading performance at scale.

📊 Impact Scores:

  • Production Impact: 3/5
  • Fix Specificity: 4/5
  • Urgency Impact: 2/5
  • Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In packages/trpc/server/routers/viewer/slots.ts, lines 504-522 and 566-590, the code chains `.map(...).filter(...)` on large `availableTimeSlots` arrays, causing unnecessary iterations and memory churn. Refactor these to use a single `.reduce(...)` that both filters and maps in one pass, significantly improving performance for large datasets.

75-181: The function checkIfIsAvailable is large and complex (over 80 lines), making it difficult to maintain and reason about, increasing risk of bugs and performance regressions.

📊 Impact Scores:

  • Production Impact: 2/5
  • Fix Specificity: 2/5
  • Urgency Impact: 2/5
  • Total Score: 6/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In packages/trpc/server/routers/viewer/slots.ts, lines 75-181, the function `checkIfIsAvailable` is large and complex, making it hard to maintain and reason about. Refactor this function by extracting logical sections (date override check, working hours check, busy check) into smaller helper functions, and have `checkIfIsAvailable` delegate to them. This will improve maintainability and reduce risk of performance regressions.

82-93,107: organizerTimeZone is used directly in time calculations without validation, allowing attacker-controlled or malformed time zone strings to cause logic errors or DoS via invalid time zone input.

📊 Impact Scores:

  • Production Impact: 3/5
  • Fix Specificity: 3/5
  • Urgency Impact: 3/5
  • Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In packages/trpc/server/routers/viewer/slots.ts, lines 82-93 and 107, the `organizerTimeZone` parameter is used directly in time calculations without validation. This allows attacker-controlled or malformed time zone strings to cause logic errors or denial of service. Add validation to ensure `organizerTimeZone` is a string and is a valid IANA time zone using `dayjs.tz.zone`. Throw an error if validation fails.

🔍 Comments beyond diff scope (1)
packages/trpc/server/routers/viewer/slots.ts (1)

531-531: typeof availabilityCheckProps.currentSeats !== undefined is always true; should be !== 'undefined' or direct undefined check, else logic for updating currentSeats is broken.
Category: correctness


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants