Skip to content

Advanced date override handling and timezone compatibility improvements#6

Draft
ShashankFC wants to merge 1 commit into
date-algorithm-basefrom
date-algorithm-enhanced
Draft

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

Conversation

@ShashankFC

Copy link
Copy Markdown
Collaborator

Test 7nn---nReplicated from ai-code-review-evaluation/cal.com-greptile#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>
@github-actions

Copy link
Copy Markdown
Contributor

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Advanced date override handling and timezone compatibility improvements". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@ShashankFC

Copy link
Copy Markdown
Collaborator Author

@claude

@claude

claude Bot commented Jan 7, 2026

Copy link
Copy Markdown

Claude finished @ShashankFC's task in 1m 45s —— View job


Code Review Complete

I've reviewed this PR's advanced date override handling and timezone compatibility improvements. Here are my findings:


Issues Found

🔴 Critical Issues

1. Bug in working hours validation logic (packages/trpc/server/routers/viewer/slots.ts:142)

The start and end variables are identical - both calculate the slot start time, not the end time:

const start = slotStartTime.hour() * 60 + slotStartTime.minute();
const end = slotStartTime.hour() * 60 + slotStartTime.minute(); // ❌ Should use slotEndTime

This means the working hours check only validates the start time, not whether the entire slot fits within working hours. Fix this →

2. Incorrect equality comparison in date override check (packages/trpc/server/routers/viewer/slots.ts:114)

if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) {
  return true;
}

This uses === for object comparison, which will always be false for different dayjs instances. Should use .isSame() method. Fix this →

3. Missing null safety check (packages/trpc/server/routers/viewer/slots.ts:212)

The code assumes override.timeZone exists but it's an optional field (per TimeRange type):

const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset();

If timeZone is undefined, this could cause runtime errors. Need to provide a fallback.


⚠️ Logic & Design Issues

4. Inconsistent timezone offset calculation (packages/lib/slots.ts:212-214)

The offset calculation in packages/lib/slots.ts differs from the one in the slots router:

  • In slots.ts: offset = inviteeUtcOffset - organizerUtcOffset
  • In slots router: utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0

The multiplier by -1 in the router suggests different conventions. These should be consistent across the codebase.

5. Incorrect comment (packages/trpc/server/routers/viewer/slots.ts:102)

//check if date override for slot exists

Missing space after //. Should be // check if date override for slot exists. Same issue on line 137.

6. Logic flow issue with dateOverrideExist flag (packages/trpc/server/routers/viewer/slots.ts:133-151)

The logic is confusing:

  • If a date override exists and the slot IS within the override (function returns early with false at line 130)
  • If a date override exists and the slot is NOT within the override, return true at line 134
  • Then check working hours if no override exists

This seems backwards. If an override exists but the slot doesn't match it, we should still fall through to check working hours, not immediately return true.

7. Missing parameters in multiple checkIfIsAvailable calls (packages/trpc/server/routers/viewer/slots.ts:489, 497, 521, 579)

The new dateOverrides and workingHours parameters are not being passed to checkIfIsAvailable in several places. This means the new validation logic won't run for those cases. You're only passing organizerTimeZone but not the actual override/working hours data.


📝 Code Quality Issues

8. Inconsistent spacing in comments (packages/trpc/server/routers/viewer/slots.ts:102, 137)

Comments should have a space after // per standard conventions.

9. Redundant .utc() calls (packages/trpc/server/routers/viewer/slots.ts:99-100, 154)

const slotEndTime = time.add(eventLength, "minutes").utc();
const slotStartTime = time.utc();
// ... later
const startTime = dayjs.utc(busyTime.start).utc(); // redundant .utc()

The dayjs.utc() already returns a UTC dayjs object, so chaining .utc() again is redundant.

10. Whitespace inconsistency (packages/trpc/server/routers/viewer/slots.ts:493)

Empty line removed between return statements without clear reason (see diff at line 159-169).


✅ Positive Aspects

  • Good test coverage: The new test case for different timezone scenarios is valuable
  • Type safety improvement: Adding timeZone to TimeRange type improves type safety
  • Timezone awareness: The overall approach of handling organizer vs invitee timezones is sound

Additional Notes

PR Title: The GitHub Actions bot correctly flagged that this PR title doesn't follow Conventional Commits. It should start with a prefix like feat:, fix:, or refactor:.

Suggested title: feat: Advanced date override handling and timezone compatibility improvements


Summary

This PR attempts to improve timezone handling for date overrides, which is important functionality. However, there are critical bugs that would cause incorrect behavior:

  1. The working hours end time calculation bug will allow slots outside working hours
  2. The dayjs equality comparison will never work as intended
  3. The logic flow for date override validation appears inverted
  4. Missing parameters mean the new validation won't run in several code paths

These issues should be addressed before merging.


@github-actions

Copy link
Copy Markdown
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions Bot added the Stale label Jan 22, 2026
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.

2 participants