Automated Test: date-algorithm-enhanced #374
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>
📝 WalkthroughWalkthroughThis pull request enhances timezone handling across the scheduling system by introducing timezone-aware boundary recalculation for date overrides, propagating timezone information through the slot generation pipeline, extending the TimeRange type to include optional timezone metadata, and adding corresponding test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/lib/slots.ts`:
- Around line 211-225: activeOverrides handling can call
dayjs(...).tz(override.timeZone) when override.timeZone is undefined, producing
wrong offsets; change the computation in the block that builds overrides (the
activeOverrides flatMap) to first fall back to the organizer timezone (or skip
offset adjustment) when override.timeZone is missing before calling dayjs.tz,
and replace flatMap with map since each override returns a single object; update
references in this block (override.timeZone,
organizerUtcOffset/inviteeUtcOffset/offset, and the activeOverrides flatMap →
map) so offset calculations only run when a valid timeZone exists or use
organizer timezone as the fallback.
In `@packages/trpc/server/routers/viewer/slots.ts`:
- Around line 114-116: The condition in slots.ts is comparing two Dayjs
instances by reference causing it to always be false; replace the strict
equality check between dayjs(date.start).add(utcOffset, "minutes") and
dayjs(date.end).add(utcOffset, "minutes") with a value comparison using the
Dayjs method .isSame(), i.e. call .isSame(...) on one of those Dayjs objects
(optionally with a unit like "minute" if needed) and keep the existing return
true when they match.
- Around line 141-143: The boundary check wrongly computes end from
slotStartTime causing start and end to be identical; update the calculation that
sets end so it uses slotEndTime (i.e., replace the second use of
slotStartTime.hour()/minute() with slotEndTime.hour()/minute()) in the block
that defines start and end (look for the variables start, end in the slots.ts
logic that compares to workingHour.startTime and workingHour.endTime) so slots
that extend past working hours are correctly filtered.
- Around line 102-151: Replace the two misused find() calls with some() and
remove the mutable dateOverrideExist side-effect: use dateOverrides.some(...) to
first check whether any override falls on the same day as slotStartTime
(respecting organizerTimeZone), then separately check if any matching override
contains or excludes the slot times and return accordingly; similarly replace
the workingHours.find(...) with workingHours.some(...), compute start =
slotStartTime.hour()*60 + slotStartTime.minute() and end = slotEndTime.hour()*60
+ slotEndTime.minute() (fixing the current bug where end is set from
slotStartTime), and return false when some() indicates the slot is outside
working hours. Reference symbols: dateOverrides, organizerTimeZone,
slotStartTime, slotEndTime, dateOverrideExist (remove), workingHours.
🧹 Nitpick comments (1)
packages/lib/slots.ts (1)
218-223: Redundantdayjsinstantiation for the same offset-adjusted time.
dayjs(override.start).utc().add(offset, "minute")is computed twice forstartTime(and likewise forendTime). Extract to a local variable for clarity and to avoid wasted work.Proposed refactor
+ const adjustedStart = dayjs(override.start).utc().add(offset, "minute"); + const adjustedEnd = dayjs(override.end).utc().add(offset, "minute"); 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(), + startTime: adjustedStart.hour() * 60 + adjustedStart.minute(), + endTime: adjustedEnd.hour() * 60 + adjustedEnd.minute(), };
| 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.
override.timeZone may be undefined, causing incorrect offset calculation.
TimeRange.timeZone is optional. If a date override lacks a timeZone value, dayjs(...).tz(undefined) will produce unpredictable results — likely falling back to the local/system timezone rather than the organizer's timezone.
Add a fallback to the organizer timezone (or skip offset adjustment entirely when timeZone is absent):
Proposed fix
const overrides = activeOverrides.flatMap((override) => {
- const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset();
+ const overrideTimeZone = override.timeZone || organizerTimeZone;
+ const organizerUtcOffset = dayjs(override.start.toString()).tz(overrideTimeZone).utcOffset();
const inviteeUtcOffset = dayjs(override.start.toString()).tz(timeZone).utcOffset();
const offset = inviteeUtcOffset - organizerUtcOffset;Additionally, flatMap is misleading here since each override produces exactly one object — map is more appropriate.
🤖 Prompt for AI Agents
In `@packages/lib/slots.ts` around lines 211 - 225, activeOverrides handling can
call dayjs(...).tz(override.timeZone) when override.timeZone is undefined,
producing wrong offsets; change the computation in the block that builds
overrides (the activeOverrides flatMap) to first fall back to the organizer
timezone (or skip offset adjustment) when override.timeZone is missing before
calling dayjs.tz, and replace flatMap with map since each override returns a
single object; update references in this block (override.timeZone,
organizerUtcOffset/inviteeUtcOffset/offset, and the activeOverrides flatMap →
map) so offset calculations only run when a valid timeZone exists or use
organizer timezone as the fallback.
| //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; | ||
| } | ||
|
|
||
| //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.
find() callbacks should always return a value — use some() or add explicit returns.
Both find() calls (lines 106 and 139) rely on implicit undefined returns for non-matching branches, which the linter flags and makes the intent unclear. Since you're only checking for existence (not using the found element), some() is the right method.
Additionally, the mutable side-effect pattern (dateOverrideExist set inside find()) is fragile. Consider restructuring to separate the "does an override exist for this day" check from the "is the slot within that override" check.
Proposed refactor for the find→some pattern
- if (
- dateOverrides.find((date) => {
+ if (
+ dateOverrides.some((date) => {
const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0;
// ...
+ return false;
})
) {- if (
- workingHours.find((workingHour) => {
+ if (
+ workingHours.some((workingHour) => {
if (workingHour.days.includes(slotStartTime.day())) {
// ...
+ return start < workingHour.startTime || end > workingHour.endTime;
}
+ return false;
})
) {🧰 Tools
🪛 Biome (2.3.13)
[error] 106-106: This callback passed to find() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
[error] 139-139: This callback passed to find() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/slots.ts` around lines 102 - 151, Replace
the two misused find() calls with some() and remove the mutable
dateOverrideExist side-effect: use dateOverrides.some(...) to first check
whether any override falls on the same day as slotStartTime (respecting
organizerTimeZone), then separately check if any matching override contains or
excludes the slot times and return accordingly; similarly replace the
workingHours.find(...) with workingHours.some(...), compute start =
slotStartTime.hour()*60 + slotStartTime.minute() and end = slotEndTime.hour()*60
+ slotEndTime.minute() (fixing the current bug where end is set from
slotStartTime), and return false when some() indicates the slot is outside
working hours. Reference symbols: dateOverrides, organizerTimeZone,
slotStartTime, slotEndTime, dateOverrideExist (remove), workingHours.
| if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Bug: Dayjs object identity comparison will always be false.
dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes") compares two distinct Dayjs object references, not their underlying time values. This condition will never be true, even when start and end are identical.
Use .isSame() for value comparison:
Proposed fix
- 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;
}📝 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.
| if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) { | |
| return true; | |
| } | |
| 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 114 - 116, The
condition in slots.ts is comparing two Dayjs instances by reference causing it
to always be false; replace the strict equality check between
dayjs(date.start).add(utcOffset, "minutes") and dayjs(date.end).add(utcOffset,
"minutes") with a value comparison using the Dayjs method .isSame(), i.e. call
.isSame(...) on one of those Dayjs objects (optionally with a unit like "minute"
if needed) and keep the existing return true when they match.
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| if (start < workingHour.startTime || end > workingHour.endTime) { |
There was a problem hiding this comment.
Bug: end is computed from slotStartTime instead of slotEndTime, making the boundary check ineffective.
Both start and end resolve to the same value (slotStartTime.hour() * 60 + slotStartTime.minute()), so the end > workingHour.endTime check is equivalent to start > workingHour.endTime. Slots that start within working hours but extend past them won't be filtered out.
Proposed fix
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) {📝 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.
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| if (start < workingHour.startTime || end > workingHour.endTime) { | |
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| const end = slotEndTime.hour() * 60 + slotEndTime.minute(); | |
| if (start < workingHour.startTime || end > workingHour.endTime) { |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/slots.ts` around lines 141 - 143, The
boundary check wrongly computes end from slotStartTime causing start and end to
be identical; update the calculation that sets end so it uses slotEndTime (i.e.,
replace the second use of slotStartTime.hour()/minute() with
slotEndTime.hour()/minute()) in the block that defines start and end (look for
the variables start, end in the slots.ts logic that compares to
workingHour.startTime and workingHour.endTime) so slots that extend past working
hours are correctly filtered.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests