Advanced date override handling and timezone compatibility improvements - #5
Advanced date override handling and timezone compatibility improvements#5CodingKylo 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>
![re-entry-ai[bot]](https://avatars.githubusercontent.com/in/3466347?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 52/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟠 High |
| Detectability | 🟢 Low |
Summary
Code Review Summary
Risk Level: HIGH (Score: 53/100)
Files Changed: 0
High Risk Areas: 3
Key Concerns
- CONFIGURATION (critical): Repository history signals: 4 rarely-changed file(s)
- CONFIGURATION (high): Dependency changes detected
- INTENTCONFIDENCE (high): Branch name does not follow conventional naming
📝 This review includes 3 inline comments (2 warnings, 1 note)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| let dateOverrideExist = false; | ||
|
|
||
| if ( | ||
| dateOverrides.find((date) => { |
There was a problem hiding this comment.
💡 NOTE
The timezone adjustment is real, but the reviewer’s stated crash/corruption scenario is not grounded in this diff. The code converts override boundaries into the organizer’s local day before comparing against the slot, which is the intended direction for timezone-aware date overrides. The actual risk here is more subtle: the comparison logic mixes Date/Dayjs values and relies on repeated ad hoc offset math, so correctness depends on the override and slot being normalized the same way everywhere. I would treat this as a correctness risk only if a concrete mismatch can be demonstrated.
| // slot is not within the date override | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a real logic bug. dateOverrideExist is set as soon as any override matches the slot’s calendar day, and the function then returns true even when the slot is outside the override window. That means the later working-hours/busy checks are skipped for any day that has an override, so availability can be reported incorrectly. The root cause is that the code uses a day-level sentinel instead of returning based on the actual interval match.
| }) | ||
| ) { | ||
| // slot is outside of working hours | ||
| return false; |
There was a problem hiding this comment.
The reviewer correctly spotted a bug: end is computed from slotStartTime instead of slotStartTime.add(eventLength, ...), so the working-hours check never considers slot duration. As written, a slot can be accepted even when it extends past the end of working hours, because the comparison only checks the start minute twice. This is a correctness issue in the new working-hours gate.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 52/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟠 High |
| Detectability | 🟢 Low |
Intent
Update slot availability logic to correctly account for date overrides across timezones and improve compatibility with working-hours constraints.
Summary
The PR introduces timezone-aware date override handling in the viewer slot availability gate and propagates timezone metadata through schedule/override structures. However, the new override logic in viewer/slots.ts has a correctness flaw that can mark slots available even when the slot is not actually covered by the override, and it also contains a working-hours boundary bug where end is computed from the start time. Additionally, timezone conversion in packages/lib/slots.ts assumes override.timeZone and timeZone are always present, which can silently produce incorrect offsets when schedule data is missing. You should verify slot availability outcomes for (1) overrides that exist on the day but do not cover the slot, (2) working-hours enforcement for events that extend past closing, and (3) behavior when timezone fields are absent or mismatched.
🎯 Review Focus
The slot availability predicate in packages/trpc/server/routers/viewer/slots.ts—specifically the override coverage logic (current find()/mutation flow) and the working-hours end-time calculation—because these can directly and incorrectly change which slots are returned to users.
Key Findings
⚠️ [packages/lib/slots.ts:L208-L232] WARNING: Timezone conversion assumesoverride.timeZoneandtimeZoneare always defined; if either is missing,dayjs(...).tz(undefined)can yield incorrect offsets without a hard failure. Quote:const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset();andconst inviteeUtcOffset = dayjs(override.start.toString()).tz(timeZone).utcOffset();— Why:packages/types/schedule.d.tsmakestimeZone?: string, and the viewer router passesschedule.timeZoneintocheckIfIsAvailable. If any caller constructsTimeRange/override withouttimeZone, offset math becomes undefined-based and can silently shift slot times. Fix: add runtime guards and a deterministic fallback. Example:if (!override.timeZone || !timeZone) { throw new Error('Missing timeZone for override conversion'); }or default to UTC explicitly:const organizerUtcOffset = dayjs(override.start).utc().utcOffset();and document it. Also consider using the same timezone source consistently (organizer vs invitee) rather than mixingoverride.timeZoneandtimeZonewithout validation.
✅ Action Checklist
- [ ] WARNING [packages/lib/slots.ts:L208-L232] — Timezone conversion assumes
override.timeZoneandtimeZoneare always defined; if either is missing,dayjs(...).tz(undefined)can yield incorrect offsets without a hard failure. Quote:const organizerUtcOffset = dayjs(override.start.toString()).tz(override.timeZone).utcOffset();andconst inviteeUtcOffset = dayjs(override.start.toString()).tz(timeZone).utcOffset();— Why:packages/types/schedule.d.tsmakestimeZone?: string, and the viewer router passesschedule.timeZoneintocheckIfIsAvailable. If any caller constructsTimeRange/override withouttimeZone, offset math becomes undefined-based and can silently shift slot times. Fix: add runtime guards and a deterministic fallback. Example:if (!override.timeZone || !timeZone) { throw new Error('Missing timeZone for override conversion'); }or default to UTC explicitly:const organizerUtcOffset = dayjs(override.start).utc().utcOffset();and document it. Also consider using the same timezone source consistently (organizer vs invitee) rather than mixingoverride.timeZoneandtimeZonewithout validation. - [ ] SUGGESTION — Replace the
find()-with-side-effects logic inpackages/trpc/server/routers/viewer/slots.tswith a pure predicate (some) that directly answers “does this slot fall within any override window?”. Concretely: removedateOverrideExistand thefind()callback mutation; computeconst isCovered = dateOverrides.some((date) => { ... return slotStartTime.isSameOrAfter(start) && slotEndTime.isSameOrBefore(end); }); if (isCovered) return true;[packages/trpc/server/routers/viewer/slots.ts:L99-L132]. - [ ] SUGGESTION — Fix the working-hours boundary check to use
slotEndTimeand ensure timezone basis matchesworkingHour.startTime/endTime. IfworkingHouris stored in organizer-local minutes, convertslotStartTime/slotEndTimeinto that same local basis before comparing. [packages/trpc/server/routers/viewer/slots.ts:L149-L160]. - [ ] SUGGESTION — Add targeted tests that reproduce the two correctness bugs: (1) multiple overrides on the same day where only one covers the slot, and (2) an event whose start is within working hours but end exceeds closing. Place them alongside existing
getScheduletests inapps/web/test/lib/getSchedule.test.tsand assert the exact returned slot set. [apps/web/test/lib/getSchedule.test.ts]. - [ ] SUGGESTION — Harden timezone handling at the API boundary: since
TimeRange.timeZoneis optional, enforce presence when buildingdateOverrides/workingHoursfor the viewer router (or explicitly default to UTC). This prevents silent offset=0 behavior. [packages/trpc/server/routers/viewer/slots.ts:L348-L436] and [packages/types/schedule.d.ts:L2-L7].
Suggestions
- Replace the
find()-with-side-effects logic inpackages/trpc/server/routers/viewer/slots.tswith a pure predicate (some) that directly answers “does this slot fall within any override window?”. Concretely: removedateOverrideExistand thefind()callback mutation; computeconst isCovered = dateOverrides.some((date) => { ... return slotStartTime.isSameOrAfter(start) && slotEndTime.isSameOrBefore(end); }); if (isCovered) return true;[packages/trpc/server/routers/viewer/slots.ts:L99-L132]. - Fix the working-hours boundary check to use
slotEndTimeand ensure timezone basis matchesworkingHour.startTime/endTime. IfworkingHouris stored in organizer-local minutes, convertslotStartTime/slotEndTimeinto that same local basis before comparing. [packages/trpc/server/routers/viewer/slots.ts:L149-L160]. - Add targeted tests that reproduce the two correctness bugs: (1) multiple overrides on the same day where only one covers the slot, and (2) an event whose start is within working hours but end exceeds closing. Place them alongside existing
getScheduletests inapps/web/test/lib/getSchedule.test.tsand assert the exact returned slot set. [apps/web/test/lib/getSchedule.test.ts]. - Harden timezone handling at the API boundary: since
TimeRange.timeZoneis optional, enforce presence when buildingdateOverrides/workingHoursfor the viewer router (or explicitly default to UTC). This prevents silent offset=0 behavior. [packages/trpc/server/routers/viewer/slots.ts:L348-L436] and [packages/types/schedule.d.ts:L2-L7].
📝 This review includes 4 inline comments (1 critical, 3 warnings)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
|
|
||
| //check if date override for slot exists | ||
| let dateOverrideExist = false; | ||
|
|
There was a problem hiding this comment.
The use of find() here is not just stylistically odd: the callback is being used as a side-effect hook to set dateOverrideExist, while its return value is also used to decide whether the slot is outside the override. That makes the control flow hard to reason about and easy to mis-handle, especially because the callback returns undefined for the common in-range case. The root cause is that availability evaluation is mixing predicate logic with mutation instead of computing a single explicit overlap/containment result.
| // slot is not within the date override | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
🚨 CRITICAL
This branch can incorrectly mark a slot as available whenever any override exists for the day, even if the slot is not actually covered by that override. Because dateOverrideExist is set independently of the find() result, the function can return true after the loop without proving the slot falls inside the override window. That can expose slots that should remain blocked.
| } | ||
| }) | ||
| ) { | ||
| // slot is outside of working hours |
There was a problem hiding this comment.
end is computed from slotStartTime instead of slotEndTime, so the working-hours check ignores the event length entirely. A slot that starts inside working hours but extends past closing time will still be treated as valid. This is a correctness bug in the new availability gate.
| @@ -507,17 +576,19 @@ export async function getSchedule(input: z.infer<typeof getScheduleSchema>, ctx: | |||
| return false; | |||
There was a problem hiding this comment.
Passing userSchedule?.timeZone into checkIfIsAvailable means the override-to-slot timezone conversion can silently fall back to undefined when the matching user schedule is missing. In that case the new date-override logic uses a zero offset, which can mis-evaluate overrides for users whose schedule timezone is not present in userAvailability. The root issue is that the new timezone-aware path depends on optional data without a guaranteed fallback.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 55/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟠 High |
| Detectability | 🟡 Medium |
Intent
Enhance scheduling availability to respect organizer-specific date overrides and working-hours constraints across timezones.
Summary
The PR adds timezone-aware date override handling and working-hours filtering to the TRPC availability check, and updates slot generation to shift override ranges between organizer and invitee timezones. The highest risks are correctness bugs in the availability logic (override short-circuiting before busy conflict checks, and a working-hours end-time calculation that uses the start time twice) plus a likely sign/reference-frame error in the timezone offset conversion in packages/lib/slots.ts. You must verify that availability filtering and slot generation agree on the meaning of override.start/end (organizer-local vs UTC) and that busy conflicts are still enforced even when a date override matches.
🎯 Review Focus
The availability decision path in packages/trpc/server/routers/viewer/slots.ts must be audited end-to-end to ensure (1) date overrides do not bypass busy conflict checks, (2) working-hours comparisons use the slot end time (duration-aware), and (3) the timezone conversion contract for override.start/end matches the conversion used in packages/lib/slots.ts.
✅ Action Checklist
- [ ] SUGGESTION — [packages/trpc/server/routers/viewer/slots.ts:L99-L160] Add a single shared utility for “convert override range between timezones” and use it in both
viewer/slots.tsandpackages/lib/slots.tsso both layers interpretoverride.start/endidentically (organizer-local vs UTC, inclusive/exclusive day boundaries). Example:convertRangeToInviteeMinutes({start,end,fromTz,toTz})returning{startTime,endTime}minutes in invitee-local. - [ ] SUGGESTION — [packages/trpc/server/routers/viewer/slots.ts:L99-L160] Fix the working-hours logic to use the slot’s actual interval: compute
slotStartTimeandslotEndTimein the same timezone basis asworkingHour.startTime/endTime(minutes-of-day). Then comparestartMinutesandendMinutesagainst working-hours, including the case whereendMinutescrosses midnight if that’s allowed. - [ ] SUGGESTION — [apps/web/test/lib/getSchedule.test.ts:L784-L812] Expand test coverage beyond the single “same as UTC time” scenario: add DST boundary tests (spring forward/fall back), tests where
eventLengthcauses the slot to extend beyondworkingHour.endTime, and tests where a date override matches but the slot overlaps a busy booking (to ensure busy conflicts still block availability). - [ ] SUGGESTION — [packages/trpc/server/routers/viewer/slots.ts:L408-L436] Verify
organizerTimeZonederivation is consistent with wheredateOverridesoriginate. IfeventType.timeZone/schedule.timeZonecan be undefined, add a fail-fast or explicit fallback contract (and tests) soorganizerTimeZoneis never silently treated asundefined/0-offset when overrides are used.
Suggestions
- [packages/trpc/server/routers/viewer/slots.ts:L99-L160] Add a single shared utility for “convert override range between timezones” and use it in both
viewer/slots.tsandpackages/lib/slots.tsso both layers interpretoverride.start/endidentically (organizer-local vs UTC, inclusive/exclusive day boundaries). Example:convertRangeToInviteeMinutes({start,end,fromTz,toTz})returning{startTime,endTime}minutes in invitee-local. - [packages/trpc/server/routers/viewer/slots.ts:L99-L160] Fix the working-hours logic to use the slot’s actual interval: compute
slotStartTimeandslotEndTimein the same timezone basis asworkingHour.startTime/endTime(minutes-of-day). Then comparestartMinutesandendMinutesagainst working-hours, including the case whereendMinutescrosses midnight if that’s allowed. - [apps/web/test/lib/getSchedule.test.ts:L784-L812] Expand test coverage beyond the single “same as UTC time” scenario: add DST boundary tests (spring forward/fall back), tests where
eventLengthcauses the slot to extend beyondworkingHour.endTime, and tests where a date override matches but the slot overlaps a busy booking (to ensure busy conflicts still block availability). - [packages/trpc/server/routers/viewer/slots.ts:L408-L436] Verify
organizerTimeZonederivation is consistent with wheredateOverridesoriginate. IfeventType.timeZone/schedule.timeZonecan be undefined, add a fail-fast or explicit fallback contract (and tests) soorganizerTimeZoneis never silently treated asundefined/0-offset when overrides are used.
📝 This review includes 3 inline comments (3 critical)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| } | ||
| }) | ||
| ) { | ||
| // slot is not within the date override |
There was a problem hiding this comment.
🚨 CRITICAL
Quote: if (dateOverrideExist) { return true; }
Issue: If any date override matches the slot’s day, the function returns true immediately without checking busy conflicts. That means a slot could be marked available even if it overlaps existing busy bookings, as long as it falls within a matching override day.
Fix: Only short-circuit to true after confirming there is no conflict with busy. For example:
if (dateOverrideExist) {
// still validate against busy
return busy.every((busyTime) => { ...existing overlap logic... });
}Or incorporate override logic into the overlap checks rather than bypassing them.
| 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.
🚨 CRITICAL
Quote: const start = slotStartTime.hour() * 60 + slotStartTime.minute(); const end = slotStartTime.hour() * 60 + slotStartTime.minute();
Issue: start and end are computed from the same slotStartTime value, so start < workingHour.startTime and end > workingHour.endTime will be identical checks. This ignores eventLength (slot duration) and will misclassify slots that start within working hours but extend beyond workingHour.endTime.
Fix: Compute end using slotEndTime:
const start = slotStartTime.hour() * 60 + slotStartTime.minute();
const end = slotEndTime.hour() * 60 + slotEndTime.minute();
``` (see also L142)
---
_[re-entry.ai](https://re-entry.ai)_| 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; |
There was a problem hiding this comment.
🚨 CRITICAL
Quote: const offset = inviteeUtcOffset - organizerUtcOffset;
Issue: The offset sign may be reversed depending on how override.start/end are interpreted (organizer-local vs UTC). If override.start is already a Dayjs in a specific zone/UTC, subtracting offsets and then applying add(offset, 'minute') can shift times in the wrong direction, producing incorrect availability windows (classic off-by-one-hour/day around DST).
Fix: Make the conversion explicit by anchoring both sides in the same reference frame. For example, if override.start/override.end represent organizer-local times, convert organizer-local -> UTC -> invitee-local:
const startUtc = dayjs(override.start).tz(override.timeZone).utc();
const startInInvitee = startUtc.tz(timeZone);
// then compute minutes from startInInvitee.hour()/minute()Avoid deriving the shift from utcOffset() differences unless you can prove the sign and reference frame. (see also L213, L214, L216, L212)
2 participants
Martian Code Review Benchmark PR (mirrored from source #7)