fix(calendar): respect configured timezone - #2427
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Code Review Summary
This incremental review covers the timezone propagation added across the calendar actions, server range resolution, calendar views, event cards, and drag interactions. The overall direction is sound: saved IANA timezone validation is centralized, date-only server ranges account for DST, and event instants are projected into the configured timezone for rendering. The focused tests reported by the agents pass, but the new coverage is primarily action-level and does not exercise browser-local timezone differences or DST interaction behavior.
Risk assessment: Standard (business/UI date-range and interaction logic).
Key findings
- 🔴 HIGH — Calendar boundary calculations still operate on
selectedDatein the browser timezone while event instants are projected into the configured timezone, so non-browser timezone users can get wrong ranges and positions. - 🔴 HIGH — Dragging/resizing through a spring-forward gap can independently convert a nonexistent local end time and persist a zero or incorrect duration.
- 🟡 MEDIUM — Adjacent-range prefetching still uses unzoned ranges and therefore does not warm the same query keys as the primary range.
The centralized settings helper and server-side validation/defaulting are good improvements, and the range tests cover a date-only request plus a DST boundary. The prefetch issue is included in the summary because its unchanged lines are not commentable in this diff.
🧪 Browser testing: Will run after this review (PR touches UI code)
| return { | ||
| from: startOfWeek(ms).toISOString(), | ||
| to: endOfWeek(me).toISOString(), | ||
| from: fromZonedTime(startOfWeek(ms), calendarTimezone).toISOString(), |
There was a problem hiding this comment.
🔴 Compute calendar boundaries in the configured timezone
startOfMonth/endOfMonth are applied directly to selectedDate, which is initialized and navigated as a browser-local Date, before the result is passed to fromZonedTime. When the browser and configured calendar zones differ, the selected calendar day/month boundary can be wrong. Convert the selected instant with toZonedTime(selectedDate, calendarTimezone) before deriving month/week/day boundaries, and keep the same representation for the child views.
| date: Date, | ||
| timezone: string, | ||
| ) { | ||
| const start = toZonedTime(event.start, timezone); |
There was a problem hiding this comment.
🔴 Use one timezone context for day event positioning
The event start/end are projected with toZonedTime, but dayStart/dayEnd are derived directly from date. With a browser timezone different from the configured timezone, differenceInMinutes compares incompatible wall-clock representations and places events at the wrong vertical time. Project date into timezone before calculating the day boundaries.
| onEventTimeChange(state.eventId, newStart, newEnd); | ||
| onEventTimeChange( | ||
| state.eventId, | ||
| fromZonedTime(newStart, timezone), |
There was a problem hiding this comment.
🔴 Preserve elapsed duration across DST gaps
The drag path performs wall-clock addMinutes arithmetic on a zoned pseudo-Date, then converts the new start and end independently. If the end lands in a spring-forward gap, fromZonedTime can collapse the nonexistent local time onto the transition and persist a zero or incorrect duration. Calculate the new instants from the original elapsed duration (or explicitly resolve DST gaps) and add a spring-forward drag test.
|
thanks @mpiniarski! some CI errors and some PR feedback here worth taking a look at |
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🔴
Review Details
Code Review Summary
This incremental review evaluated the latest PR head while excluding the three unresolved findings from the prior review. The timezone propagation remains directionally sound, and the centralized IANA validation/defaulting plus DST-aware server range tests are useful improvements. However, the latest implementation still has several newly identified edge cases in inventory pagination, month drag/drop, legacy settings compatibility, and timezone-projected rendering.
Risk assessment: Standard (business logic and UI date/time behavior).
Key findings
- 🔴 HIGH — Command palette can call
toZonedTimewith its optional timezone prop unset. - 🟡 MEDIUM — Inventory cursor preflight reconstructs a different range than the first page when saved and request timezones differ.
- 🟡 MEDIUM — Month drag/drop does not preserve displayed wall-clock time across a DST transition.
- 🟡 MEDIUM — Existing invalid saved timezone values now make event listing fail without a recovery path.
- 🟡 MEDIUM — Week view compares a timezone-shifted display proxy with the real current instant for past-event styling.
The previous comments remain open and were intentionally not reposted or resolved because their underlying code is still present.
🧪 Browser testing: Will run after this review (PR touches UI code)
| ): Promise<CalendarEventsResult> { | ||
| const email = getRequestUserEmail(); | ||
| if (!email) throw new Error("no authenticated user"); | ||
| const timezone = await getCalendarTimezone(email); |
There was a problem hiding this comment.
🟡 Use the saved timezone when validating inventory cursors
The first inventory page resolves its range with the saved calendar timezone, but the cursor preflight later calls resolveCalendarEventRange without timezone. For date-only or omitted bounds, page two can compute a different query key using the request timezone and reject the cursor as query-bound. Resolve and pass the same saved timezone in the cursor path.
| const evStart = e.allDay | ||
| ? parseISO(e.start) | ||
| : toZonedTime(e.start, timezone); |
There was a problem hiding this comment.
🟡 Preserve local time when moving events across DST
Timed events are now grouped by their configured-calendar wall clock, but CalendarView.handleEventDrop still changes the dates of the original UTC instants with browser-local setFullYear. Moving an event across a DST boundary therefore changes its displayed local time (for example 10:00 becomes 11:00 after the offset changes). Convert the event times to the calendar timezone before replacing the date and convert them back with fromZonedTime.
| timezone?: unknown; | ||
| } | null; | ||
| if (settings?.timezone === undefined) return getDefaultSettings().timezone; | ||
| if (!isCalendarTimezone(settings.timezone)) { |
There was a problem hiding this comment.
🟡 Provide recovery for legacy invalid timezone settings
Reads now strictly validate the saved timezone, but older settings could contain arbitrary strings because the previous update path accepted them. getCalendarTimezone throws for those accounts, so list-events fails before any events can load, while get-settings returns the invalid record and offers no repair path. Fall back to a valid timezone or expose a recoverable settings update path instead of making the calendar unusable.
| : null; | ||
| const start = parseISO(event.start); | ||
| const end = parseISO(event.end); | ||
| const start = toZonedTime(event.start, timezone); |
There was a problem hiding this comment.
🟡 Compare past status using the real event instant
end is now a toZonedTime wall-clock proxy used for grid geometry, but it is also compared directly with now at the past-status check. Those values are not on the same instant timeline, so an event can be marked past while it is still active (or vice versa) when the configured zone differs from the browser zone. Keep a separately parsed real end instant for this comparison.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Code Review Summary
This incremental review evaluated the latest PR head and skipped all seven unresolved findings from the prior review because they remain present and unchanged; none were resolved. The latest changes continue to propagate the configured timezone through calendar rendering and settings while adding focused action-level coverage. The dev server is healthy, but browser execution remains unavailable in this environment.
Risk assessment: Standard (calendar business logic and UI interaction behavior).
New findings
- 🔴 HIGH —
view-screenparses a bareYYYY-MM-DDnavigation date as a server-local date before applying the configured timezone, so agent screen context can report the wrong week when the server timezone differs. - 🔴 HIGH — The drag callbacks use
timezonebut omit it from their dependency arrays, leaving stale closures after a live timezone setting change.
The existing settings validation/defaulting and date-range tests remain useful, but browser-level verification is needed for timezone changes and drag interactions.
🧪 Browser testing: Attempted after this review; all 17 planned flows were blocked because Chrome/browser automation tools were unavailable in the executor environment.
| if (!email) throw new Error("no authenticated user"); | ||
| const timezone = await getCalendarTimezone(email); | ||
| const viewDate = nav?.date | ||
| ? parseISO(nav.date) |
There was a problem hiding this comment.
🔴 Parse navigation dates in the configured timezone
navigate documents date as a bare YYYY-MM-DD, but parseISO(nav.date) creates a server-local date before startOfWeek and fromZonedTime are applied. When the server timezone differs from the saved calendar timezone, view-screen can compute a shifted week and return context for the wrong dates; parse the date-only value as a wall-clock date in timezone instead.
| onEventTimeChange(state.eventId, newStart, newEnd); | ||
| onEventTimeChange( | ||
| state.eventId, | ||
| fromZonedTime(newStart, timezone), |
There was a problem hiding this comment.
🔴 Refresh drag callbacks when the timezone changes
The newly timezone-aware startDrag and onPointerUp callbacks use timezone, but their dependency arrays omit it. After a live settings change, a drag can retain the previous timezone closure and save the event using the old zone; include timezone in both callback dependencies.
|
thanks for resolving the PR feedback @mpiniarski - looks like we just need to resolve the merge conflicts then this looks good to go! |
|
@steve8708 i'm on it. |
handleEventDrop rewrote the date on the raw UTC instant with browser-local setFullYear, so dragging an event across a DST boundary shifted its displayed time in the configured calendar timezone. Move the date in the calendar zone and convert back with fromZonedTime.
startDrag and onPointerUp read the configured timezone but omitted it from their dependency arrays, so a drag started after a live settings change could keep the previous zone's closure and save the event in the old timezone.
# Conflicts: # templates/calendar/actions/get-settings.ts # templates/calendar/actions/update-settings.ts # templates/calendar/app/components/calendar/DayView.tsx # templates/calendar/app/components/calendar/MonthView.tsx # templates/calendar/app/components/calendar/WeekView.tsx # templates/calendar/app/hooks/use-event-drag.ts # templates/calendar/app/pages/CalendarView.tsx # templates/calendar/server/handlers/settings.ts # templates/calendar/server/lib/ical-fetcher.spec.ts
|
no worries @mpiniarski, glad to see you're back! |
getCalendarTimezone threw whenever the stored timezone was not a valid IANA zone, so an account saved by an older build that accepted arbitrary strings could not load events at all. normalizeCalendarSettings now validates the zone and takes the fallbacks a caller wants, so every read and write resolves to a usable one. Settings reads and writes move behind readCalendarSettings / readPublicCalendarSettings / saveCalendarSettings, leaving one place that normalizes: the grid and the settings page can no longer resolve an account to different zones, and saving an unrelated field no longer resets the timezone of an account that had none. Consolidates the IANA check into shared/timezone.ts, which also stops a non-RangeError fault from being reported as an invalid zone.
The template carried the same wall-clock-to-instant algorithm in three places and the same IANA validity check in seven. Each copy handled DST edges a little differently: list-events put a skipped midnight (Santiago) an hour into the previous day, and every private copy of the validity check reported a broken Intl as "invalid timezone". shared/timezone.ts now owns the conversion, the date-key helpers, and the check; find-time.ts and list-events.ts delegate to it under their existing names, so callers are unchanged. Formatters are cached per zone and option set because constructing one costs ~45us and resolving a wall clock probes the zone a dozen times: dateTimeInTimezoneToIso drops from ~813us to ~99us per call, and the two event labels that render once per event are faster than the date-fns-tz code they replaced. Also drops an unreachable fallback in saveCalendarSettings, parallelizes its two independent writes, and makes the update-settings spec exercise the action schema rather than the predicate directly.
|
@steve8708 I think it's ready now |
2 participants
Summary
Gmail does use configured timezone instead of browser timezone as well.
Bug
When the configured timezone differed from the browser timezone, the week grid used browser-local wall-clock values while event creation used the configured timezone. For example, clicking 7:00 AM in a New York calendar from Central Europe created an event displayed at 1:00 PM.
Testing
pnpm --dir templates/calendar test -- actions/list-events.spec.ts actions/update-settings.spec.tspnpm --dir templates/calendar typecheck