Advanced date override handling and timezone compatibility improvements#6
Advanced date override handling and timezone compatibility improvements#6ShashankFC 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>
|
@cubic-dev-ai review this pull request |
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/trpc/server/routers/viewer/slots.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/slots.ts:114">
P1: Bug: Comparing two `dayjs` objects with `===` always returns `false` (reference equality). Use `.isSame()` for value comparison.</violation>
<violation number="2" location="packages/trpc/server/routers/viewer/slots.ts:142">
P1: Bug: `end` is computed from `slotStartTime` instead of `slotEndTime`, making it always equal to `start`. This means slots extending past working hours won't be filtered out.</violation>
</file>
<file name="packages/lib/slots.ts">
<violation number="1" location="packages/lib/slots.ts:212">
P2: `dayjs(override.start.toString())` is fragile — `Date.toString()` produces an implementation-dependent format. Use `dayjs(override.start)` directly, which is what the same code does two lines below. This also aligns with a known dayjs issue (#2720) where passing a Date object vs its string representation can produce different timezone behavior.</violation>
<violation number="2" location="packages/lib/slots.ts:212">
P1: `override.timeZone` is optional (`timeZone?: string` in `TimeRange` type) and may be `undefined`. Calling `dayjs().tz(undefined)` can throw or produce incorrect offset calculations. Add a fallback, e.g., to `organizerTimeZone`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| workingHours.find((workingHour) => { | ||
| if (workingHour.days.includes(slotStartTime.day())) { | ||
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); |
There was a problem hiding this comment.
P1: Bug: end is computed from slotStartTime instead of slotEndTime, making it always equal to start. This means slots extending past working hours won't be filtered out.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/slots.ts, line 142:
<comment>Bug: `end` is computed from `slotStartTime` instead of `slotEndTime`, making it always equal to `start`. This means slots extending past working hours won't be filtered out.</comment>
<file context>
@@ -89,6 +99,57 @@ const checkIfIsAvailable = ({
+ 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;
</file context>
| 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")) { |
There was a problem hiding this comment.
P1: Bug: Comparing two dayjs objects with === always returns false (reference equality). Use .isSame() for value comparison.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/slots.ts, line 114:
<comment>Bug: Comparing two `dayjs` objects with `===` always returns `false` (reference equality). Use `.isSame()` for value comparison.</comment>
<file context>
@@ -89,6 +99,57 @@ const checkIfIsAvailable = ({
+ slotStartTime.format("YYYY MM DD")
+ ) {
+ dateOverrideExist = true;
+ if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) {
+ return true;
+ }
</file context>
| 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"))) { |
| endTime: override.end.getUTCHours() * 60 + override.end.getUTCMinutes(), | ||
| })); | ||
| const overrides = activeOverrides.flatMap((override) => { | ||
| const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset(); |
There was a problem hiding this comment.
P1: override.timeZone is optional (timeZone?: string in TimeRange type) and may be undefined. Calling dayjs().tz(undefined) can throw or produce incorrect offset calculations. Add a fallback, e.g., to organizerTimeZone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/lib/slots.ts, line 212:
<comment>`override.timeZone` is optional (`timeZone?: string` in `TimeRange` type) and may be `undefined`. Calling `dayjs().tz(undefined)` can throw or produce incorrect offset calculations. Add a fallback, e.g., to `organizerTimeZone`.</comment>
<file context>
@@ -208,11 +208,22 @@ const getSlots = ({
- endTime: override.end.getUTCHours() * 60 + override.end.getUTCMinutes(),
- }));
+ 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;
</file context>
| endTime: override.end.getUTCHours() * 60 + override.end.getUTCMinutes(), | ||
| })); | ||
| const overrides = activeOverrides.flatMap((override) => { | ||
| const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset(); |
There was a problem hiding this comment.
P2: dayjs(override.start.toString()) is fragile — Date.toString() produces an implementation-dependent format. Use dayjs(override.start) directly, which is what the same code does two lines below. This also aligns with a known dayjs issue (#2720) where passing a Date object vs its string representation can produce different timezone behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/lib/slots.ts, line 212:
<comment>`dayjs(override.start.toString())` is fragile — `Date.toString()` produces an implementation-dependent format. Use `dayjs(override.start)` directly, which is what the same code does two lines below. This also aligns with a known dayjs issue (#2720) where passing a Date object vs its string representation can produce different timezone behavior.</comment>
<file context>
@@ -208,11 +208,22 @@ const getSlots = ({
- endTime: override.end.getUTCHours() * 60 + override.end.getUTCMinutes(),
- }));
+ 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;
</file context>
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)*