Repository navigation
[benchmark][TypeScript] Advanced date override handling and timezone compatibility improvements - #10
Conversation
| if (currentSeats?.some((booking) => booking.startTime.toISOString() === time.toISOString())) { | ||
| return true; | ||
| const slotEndTime = time.add(eventLength, "minutes").utc(); | ||
| const slotStartTime = time.utc(); |
There was a problem hiding this comment.
🟠 Сравнение dayjs-объектов через === вместо .isSame() — проверка нулевого переопределения никогда не срабатывает
В slots.ts:30 два независимо созданных экземпляра dayjs сравниваются оператором ===, который проверяет ссылочную идентичность, а не временное равенство. Выражение dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes") всегда равно false, из-за чего ветка раннего возврата для переопределений нулевой длительности становится недостижимой. Следует использовать .isSame() с соответствующей единицей измерения или сравнивать Unix-таймстампы через .valueOf().
| } | ||
| if (slotStartTime.isAfter(dayjs(date.end).add(utcOffset, "minutes"))) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🟠 Проверка конца рабочих часов использует slotStartTime вместо slotEndTime
В slots.ts:55 обе переменные start и end вычисляются из slotStartTime, из-за чего они идентичны. Проверка должна определять, начинается ли слот до начала рабочего дня ИЛИ заканчивается ли он после конца рабочего дня. Из-за ошибки слот, который начинается в рабочее время, но выходит за его конец, ошибочно проходит проверку и помечается как доступный. Переменная end должна вычисляться из slotEndTime.
| ...userSchedule, | ||
| ...availabilityCheckProps, | ||
| organizerTimeZone: userSchedule.timeZone, | ||
| }); |
There was a problem hiding this comment.
🟠 Дублирование ошибки с slotStartTime вместо slotEndTime в новом guard'е рабочих часов
В slots.ts:137 в новом блоке проверки рабочих часов обе переменные start и end вычисляются из slotStartTime: const start = slotStartTime.hour() * 60 + slotStartTime.minute() и const end = slotStartTime.hour() * 60 + slotStartTime.minute(). Значение end должно вычисляться из slotEndTime (уже рассчитанного выше как time.add(eventLength, 'minutes').utc()). Это та же ошибка, что и в slots.ts:55, но в другом месте — вероятно, результат копирования кода.
| eventType.timeZone || eventType?.schedule?.timeZone || userAvailability?.[0]?.timeZone; | ||
|
|
||
| for ( | ||
| let currentCheckedTime = startTime; |
There was a problem hiding this comment.
🟡 Сравнение dayjs через === в slots.ts:110 — мёртвый код для нулевых переопределений
В slots.ts:110 проверка dayjs(date.start).add(utcOffset, 'minutes') === dayjs(date.end).add(utcOffset, 'minutes') сравнивает два различных экземпляра dayjs строгим равенством, что всегда даёт false. Ветка становится недостижимым кодом. Это тот же класс ошибки, что и в slots.ts:30. Заменить на .isSame() или сравнение .valueOf().
| ); | ||
| } | ||
| time: slot.time, | ||
| ...schedule, |
There was a problem hiding this comment.
🟡 organizerTimeZone может быть undefined при отсутствии пользователя в userAvailability
В slots.ts:120 вызов userAvailability.find() может вернуть undefined, если ни один пользователь не соответствует slotUserId. В этом случае organizerTimeZone становится undefined, и последующий вызов dayjs.tz(date.start, organizerTimeZone) в checkIfIsAvailable получит неопределённую таймзону. В зависимости от поведения плагина timezone это может вызвать исключение или тихо произвести неверный расчёт смещения (fallback на 0). Следует явно обрабатывать случай отсутствия пользователя.
| userId: availability.user.id, | ||
| timeZone: availability.timeZone, | ||
| ...override, | ||
| })) |
There was a problem hiding this comment.
🟡 Флаг dateOverrideExist мутируется как побочный эффект внутри Array.prototype.find
В slots.ts:99 dateOverrides.find(...) используется в первую очередь ради побочного эффекта — установки внешнего флага dateOverrideExist = true. Колбэк find возвращает true в ветках 'вне переопределения' (что заставляет внешний if вернуть false), и неявно возвращает undefined в остальных случаях. Такая логика трудна для понимания и сопровождения: семантика find смешивается с управлением потоком через побочные эффекты. Рекомендуется разделить поиск и проверку условий, либо использовать for...of с явным break.
|
🔵 Одинаковое смещённое значение dayjs пересчитывается четыре раза на каждое переопределение В |
| 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.
🟡 Риск DST: смещение вычисляется один раз из override.start и применяется к override.end
В lib/slots.ts:5 смещение UTC вычисляется на основе override.start для организатора и приглашённого. Если переопределение охватывает момент перехода на летнее/зимнее время, смещение на момент override.end может отличаться от смещения на момент override.start. Применение одного смещения к обеим границам может сдвинуть конец окна, сделав переопределение короче или длиннее ожидаемого. Следует вычислять смещение отдельно для каждой границы.
|
🔵 Парсинг В |
|
🔵 В |
🔍 Автоматическое ревью кодаКраткий итогСинтез ревью: 11 уникальных групп замечаний по 2 исходн. выполнениям; 10 для публикации (критических: 0, высоких: 3, средних: 4, низких: 3, мелких: 0). Замечания🟠 Важно
💡 Дополнительно
Автоматическое ревью от Review Engine v2.0 |
| busy: EventBusyDate[]; | ||
| eventLength: number; | ||
| dateOverrides?: { | ||
| start: Date; |
There was a problem hiding this comment.
🟠 Сравнение dayjs-объектов через === всегда возвращает false (строка 20)
Каждый вызов dayjs(...) создаёт новый экземпляр объекта, поэтому === сравнивает ссылки и всегда возвращает false. Данная ветка фактически является мёртвым кодом. Предположительно, намерение состояло в проверке, совпадают ли моменты начала и окончания переопределения, для чего требуется .isSame().
| 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.
🟠 Сравнение dayjs-объектов через === (сравнение по ссылке) всегда даёт false
Выражение if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) сравнивает два только что созданных экземпляра dayjs по ссылке. Каждый вызов dayjs(...) создаёт новый объект, поэтому === всегда возвращает false, и ветка обработки нулевых/мгновенных переопределений дат становится мёртвым кодом. Окружающая логика доступности полагается на эту ветку для обработки переопределений с совпадающим началом и концом. Следует использовать .isSame().
| const slotStartTime = time.utc(); | ||
|
|
||
| //check if date override for slot exists | ||
| let dateOverrideExist = false; |
There was a problem hiding this comment.
🟠 Логика рабочих часов помечает слот недоступным, если он не попадает хотя бы в один из интервалов
Колбэк find возвращает truthy, если слот выходит за пределы конкретного рабочего часа. Если find возвращает truthy для любого рабочего часа, слот помечается недоступным. Это означает, что слот должен попадать во ВСЕ записи рабочих часов одновременно. При наличии нескольких интервалов в один день (например, утренний блок 9–12 и дневной блок 13–17) слот в 10:00 будет некорректно признан недоступным, так как он не попадает в дневной блок. Логику следует изменить: слот доступен, если он попадает хотя бы в один рабочий интервал.
| const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0; | ||
|
|
||
| if ( | ||
| dayjs(date.start).add(utcOffset, "minutes").format("YYYY MM DD") === |
There was a problem hiding this comment.
🟠 Проверка рабочих часов использует slotStartTime для вычисления как начала, так и окончания слота
Переменная end вычисляется из slotStartTime вместо slotEndTime. Из-за этого условие end > workingHour.endTime сравнивает только время начала слота с концом рабочего часа. Слоты, которые начинаются в пределах рабочих часов, но выходят за их конец (например, 60-минутный слот, начинающийся в 16:30 при окончании рабочих часов в 17:00), некорректно проходят фильтр и помечаются как доступные. Переменная end должна вычисляться из slotEndTime.
| 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.
🟠 Фильтр рабочих часов использует время начала слота для обеих границ
В новом фильтре рабочих часов const start = slotStartTime.hour() * 60 + slotStartTime.minute(); и const end = slotStartTime.hour() * 60 + slotStartTime.minute(); вычисляются из одного и того же slotStartTime. Переменная end должна вычисляться из slotEndTime (начало слота + eventLength). Поскольку end отражает время начала слота, условие end > workingHour.endTime срабатывает только тогда, когда начало слота уже за пределами рабочего часа, пропуская слоты, которые выходят за конец рабочего времени.
| 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.
🟡 override.timeZone опционален, но передаётся без проверки в dayjs .tz()
Вызов dayjs(override.start.toString()).tz(override.timeZone) использует override.timeZone, тип которого timeZone?: string (опциональное поле в schedule.d.ts). Когда timeZone равно undefined (например, доступность организатора без указанного часового пояса), dayjs(...).tz(undefined) молча откатывается к локальному времени сервера, и organizerUtcOffset вычисляется относительно локального пояса сервера, а не UTC. Это может привести к некорректным результатам доступности. Следует добавить проверку на undefined и значение по умолчанию (например, 'UTC').
|
|
||
| if ( | ||
| dateOverrides.find((date) => { | ||
| const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0; |
There was a problem hiding this comment.
🟡 Повторные вычисления dayjs(date...).add(utcOffset,'minutes') избыточны и подвержены ошибкам
Выражения dayjs(date.start).add(utcOffset, "minutes") и dayjs(date.end).add(utcOffset, "minutes") пересоздаются 5+ раз в одном колбэке (строки 40, 44, 44, 48, 49, 53, 53). Каждый вызов выделяет новый объект dayjs, что усложняет чтение логики и повышает риск рассинхронизации — одна ошибка в выборе базового значения молча изменит поведение. Рекомендуется вычислить значения один раз и переиспользовать.
| //check if date override for slot exists | ||
| let dateOverrideExist = false; | ||
|
|
||
| if ( |
There was a problem hiding this comment.
🟡 dateOverrides.find с побочным эффектом через флаг dateOverrideExist некорректно классифицирует слоты при нескольких переопределениях в один день
Проверка переопределений использует dateOverrides.find(...). Внутри колбэка, когда день слота совпадает с переопределением, но слот находится ВНУТРИ окна переопределения, dateOverrideExist устанавливается в true, а колбэк возвращает undefined (falsy), и find переходит к следующему переопределению. Если последующее переопределение в тот же день находит слот вне своего окна, find возвращает этот элемент, и внешний if выполняет ветку недоступности, даже если слот уже был корректно обработан первым переопределением. Это приводит к ошибочной маркировке доступных слотов как недоступных.
| }); | ||
| }); | ||
| return slot; | ||
| return false; |
There was a problem hiding this comment.
🟡 organizerTimeZone может быть undefined, если userSchedule не найден
Если userAvailability.find(...) возвращает undefined (пользователь слота не найден в списке доступности), userSchedule?.timeZone вычисляется как undefined. Это значение передаётся как organizerTimeZone, из-за чего checkIfIsAvailable устанавливает utcOffset в 0, пропуская корректировку часового пояса для переопределений дат. Это может привести к некорректным результатам доступности для слотов с не привязанным организатором. Следует явно обрабатывать случай отсутствующего расписания.
| return { | ||
| userIds: override.userId ? [override.userId] : [], | ||
| startTime: | ||
| dayjs(override.start).utc().add(offset, "minute").hour() * 60 + |
There was a problem hiding this comment.
🔵 dayjs(override.start).utc().add(offset,'minute') вычисляется повторно четыре раза
Выражение dayjs(override.start).utc().add(offset, "minute") вычисляется дважды для startTime.hour() и .minute(), и аналогично для endTime — всего четыре раза. Результат идентичен в каждой паре и может быть вычислен один раз и сохранён в переменную, что улучшит читаемость и производительность.
| const scheduleForEventOnADayWithDateOverrideDifferentTimezone = await getSchedule( | ||
| { | ||
| eventTypeId: 1, | ||
| eventTypeSlug: "", |
There was a problem hiding this comment.
🔵 Новый тест покрывает только корректный путь часового пояса; нет покрытия для багованных веток
Добавленный тест проверяет корректный рендеринг слотов для часового пояса приглашённого +6:00, но не затрагивает новую логику верхней границы рабочих часов (слот, выходящий за конец рабочих часов) и ветку нулевого переопределения дат, обе из которых содержат дефекты. Также файл теста не заканчивается символом новой строки (\ No newline at end of file).
🔍 Автоматическое ревью кодаКраткий итогСинтез ревью: 13 уникальных групп замечаний по 2 исходн. выполнениям; 12 для публикации (критических: 0, высоких: 5, средних: 4, низких: 3, мелких: 0). Замечания🟠 Важно
💡 Дополнительно
Автоматическое ревью от Review Engine v2.0 |
| 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.
🔵 Хрупкое преобразование Date.toString() перед парсингом dayjs
override.start.toString() преобразует Date в строковое представление, зависящее от локали и системного часового пояса, затем повторно парсится dayjs. Это зависит от окружения и излишне, так как dayjs может принимать объекты Date напрямую. В серверных окружениях с не-UTC часовым поясом это может внести тонкие ошибки смещения. Рекомендуется передавать Date напрямую: dayjs(override.start).
| const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0; | ||
|
|
||
| if ( | ||
| dayjs(date.start).add(utcOffset, "minutes").format("YYYY MM DD") === |
There was a problem hiding this comment.
🟠 Проверка рабочих часов использует slotStartTime для обеих границ вместо slotEndTime для конца
В функции checkIfIsAvailable переменная end вычисляется как slotStartTime.hour() * 60 + slotStartTime.minute() — копия start. Должно использоваться slotEndTime, чтобы проверить, что конец слота находится в пределах рабочих часов. В текущем виде слот, начинающийся в рабочее время, но заканчивающийся после workingHour.endTime, ошибочно проходит проверку и помечается как доступный.
| dateOverrides?: { | ||
| start: Date; | ||
| end: Date; | ||
| }[]; |
There was a problem hiding this comment.
🟡 dayjs-объекты сравниваются через === (сравнение по ссылке) — условие всегда false
Выражение dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes") сравнивает два разных экземпляра dayjs по ссылке. dayjs никогда не возвращает один и тот же объект, поэтому условие всегда ложно. Ветка раннего возврата для пустого переопределения (равные start и end) никогда не срабатывает, и логика проваливается в последующие проверки.
| 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.
🟡 dayjs().tz(override.timeZone) вызывается с возможно undefined значением timeZone
Поле timeZone в типе DateOverride сделано optional (timeZone?: string), а в viewer/slots.ts оно берётся из availability.timeZone, который также может быть undefined. Вызов dayjs(override.start.toString()).tz(override.timeZone) с undefined тихо подставляет часовой пояс по умолчанию dayjs вместо часового пояса организатора, что приводит к неверному расчёту organizerUtcOffset и неправильным диапазонам переопределений.
| // flattens availability of multiple users | ||
| const dateOverrides = userAvailability.flatMap((availability) => | ||
| availability.dateOverrides.map((override) => ({ userId: availability.user.id, ...override })) | ||
| availability.dateOverrides.map((override) => ({ |
There was a problem hiding this comment.
🟡 organizerTimeZone становится undefined при неудачном userAvailability.find(), молча отключая коррекцию смещения
Если userAvailability.find() возвращает undefined (например, ID пользователя слота не найден в данных доступности), organizerTimeZone становится undefined. В checkIfIsAvailable это приводит к utcOffset = 0, и логика сопоставления дат переопределения использует сырую UTC-дату вместо локальной даты организатора. Слоты могут быть ошибочно включены или исключены из доступности.
| let dateOverrideExist = false; | ||
|
|
||
| if ( | ||
| dateOverrides.find((date) => { |
There was a problem hiding this comment.
🔵 Проверка доступности переопределений полагается на побочные эффекты внутри callback'а find()
Callback dateOverrides.find(...) мутирует внешний флаг dateOverrideExist и одновременно возвращает true/false, управляя как find, так и последующей веткой if (dateOverrideExist). Смешение побочных эффектов с truthiness-логикой find затрудняет понимание потока управления: двойной смысл возвращаемого значения («вне переопределения» vs «совпадение по дате») неочевиден и легко ломается при рефакторинге.
🔍 Автоматическое ревью кодаКраткий итогСинтез ревью: 10 уникальных групп замечаний по 2 исходн. выполнениям; 5 для публикации (критических: 0, высоких: 1, средних: 3, низких: 1, мелких: 0). Замечания🟠 Важно
💡 Дополнительно
Автоматическое ревью от Review Engine v2.0 |
1 participant
Benchmark fixture for cal_dot_com PR #8330.\n\nGolden comments:
benchmark/golden_comments/cal_dot_com.json— PR8330.