fix(calendar): use local timezone for agenda day boundaries#443
fix(calendar): use local timezone for agenda day boundaries#443anshul-garg27 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
Previously, --today and --tomorrow computed day boundaries using UTC epoch arithmetic, so after local midnight \!= UTC midnight the wrong day's events were returned. Now uses chrono::Local to derive midnight in the user's timezone. Also fixes --today which had no explicit branch and fell through to the generic "N days from now" path.
🦋 Changeset detectedLatest commit: ef0c212 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
googleworkspace-bot
added
the
area: core
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the calendar agenda feature where day boundaries were incorrectly calculated using UTC, leading to users seeing events for the wrong day in non-UTC timezones. The changes refactor the time range determination logic to leverage the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly changes the calendar agenda logic to use the local timezone instead of UTC, fixing a bug for users in non-UTC timezones. The implementation is mostly correct, but I've found a critical issue related to Daylight Saving Time transitions that could lead to incorrect agenda boundaries. The new test also contains duplicated logic with the same issue. My review includes suggestions to make the timezone handling more robust.
| let today_start_local = Local | ||
| .from_local_datetime(&today_start) | ||
| .single() | ||
| .unwrap_or(local_now); |
There was a problem hiding this comment.
The fallback unwrap_or(local_now) can lead to incorrect behavior. If from_local_datetime results in an ambiguous or non-existent time (e.g., during a DST transition), single() will return None, and today_start_local will become local_now. For commands like --today, this means the agenda would start from the current time instead of the beginning of the day, which is not the intended behavior.
To handle ambiguous times correctly, you can use .earliest(). For the rare case of a non-existent midnight, panicking with expect() is safer than silently producing incorrect results.
| let today_start_local = Local | |
| .from_local_datetime(&today_start) | |
| .single() | |
| .unwrap_or(local_now); | |
| let today_start_local = Local | |
| .from_local_datetime(&today_start) | |
| .earliest() | |
| .expect("Failed to determine local midnight; this can happen during a DST transition where midnight does not exist."); |
| let today_start_local = Local | ||
| .from_local_datetime(&today_start) | ||
| .single() | ||
| .unwrap_or(local_now); |
There was a problem hiding this comment.
This logic for determining the start of the day is duplicated from the handle_agenda function and shares the same potential bug with unwrap_or(local_now).
To improve maintainability and correctness, this logic should be extracted into a helper function and used in both handle_agenda and this test. For now, I'll suggest the same fix as for the main logic to ensure the test is correct and robust against timezone edge cases.
| let today_start_local = Local | |
| .from_local_datetime(&today_start) | |
| .single() | |
| .unwrap_or(local_now); | |
| let today_start_local = Local | |
| .from_local_datetime(&today_start) | |
| .earliest() | |
| .expect("Failed to determine local midnight; this can happen during a DST transition where midnight does not exist."); |
2 participants
Summary
gws calendar +agenda --todayand--tomorrownow use the local timezone for day boundaries instead of UTCRoot cause
Day boundaries were computed by dividing Unix epoch seconds by 86400, which yields UTC midnight. The RFC3339 timestamps were also formatted in UTC. Users in non-UTC timezones got the wrong day's events.
Before (PST, 8pm local = UTC midnight):
--todayreturns tomorrow's events--tomorrowreturns the day after tomorrow's eventsAfter:
--todayreturns today's events using local midnight boundaries--tomorrowreturns tomorrow's events using local midnight boundariesChanges
chrono::Localday boundary calculation--todaybranch (previously fell through to generic "N days from now")epoch_to_rfc3339helper (now usingchrono::DateTime::to_rfc3339()directly)-07:00)Test plan
Closes #259