fix(workflow): use local timezone for standup-report and weekly-digest boundaries#462
Conversation
🦋 Changeset detectedLatest commit: e9c2c9f 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 |
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 timezone-related bug in the Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where +standup-report and +weekly-digest used UTC-based time boundaries, causing incorrect behavior for users in other timezones. The switch to chrono::Local for determining day and week boundaries is a good fix. I've identified a potential issue in the new time calculation logic within handle_standup_report and its corresponding test. The use of unwrap_or(local_now) could lead to silent failures with incorrect data. I've left specific comments with suggestions to improve the robustness of this logic. Additionally, I've pointed out code duplication between the implementation and the new test, with a suggestion for refactoring to improve maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with timezone handling for standup-report and weekly-digest by switching from UTC-based epoch calculations to local timezone boundaries. This is a great improvement. However, I've found a critical issue in the new local_today_boundaries helper function related to Daylight Saving Time, which could lead to incorrect time ranges. I've provided a detailed comment and a code suggestion to fix this, along with a correction for the associated test which currently reinforces the buggy behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the timezone issue in the standup-report and weekly-digest workflows by switching from UTC-based epoch calculations to using the user's local timezone with chrono::Local. The introduction of the local_today_boundaries helper function is a good abstraction that improves code clarity and maintainability. The changes appear solid and will enhance the experience for users in non-UTC timezones. I have one suggestion to improve the newly added test case to make it more robust.
|
|
||
| // The RFC 3339 output must include the local offset | ||
| let rfc = start.to_rfc3339(); | ||
| assert!(rfc.contains('T')); |
There was a problem hiding this comment.
This assertion is correct, but it doesn't fully test the requirement from the comment on line 765: "The RFC 3339 output must include the local offset". The 'T' separator is always present in a DateTime's RFC 3339 representation and doesn't validate the offset.
A more robust test would be to ensure the generated RFC 3339 string can be parsed back into a DateTime object that represents the same instant in time. This implicitly validates that the timezone offset was serialized correctly.
// Verify that the string can be parsed back to the same instant in time,
// which implicitly checks that the timezone offset was correctly serialized.
let parsed = chrono::DateTime::parse_from_rfc3339(&rfc).unwrap();
assert_eq!(start, parsed);There was a problem hiding this comment.
true, checking for 'T' doesn't prove much. updated to assert the local offset string appears in the output instead
|
Ready for review — all tests passing, Gemini feedback addressed. @jpoehnelt |
|
/gemini review |
…t boundaries
The +standup-report helper computed "today" as UTC midnight-to-midnight
using epoch arithmetic: `(now / 86400) * 86400`. This means a user in
UTC−8 running the command at 10 PM local time would get tomorrow's
UTC events instead of today's local events.
Apply the same fix from the calendar +agenda helper:
- Use chrono::Local to derive today's boundaries from wall-clock midnight
- Handle DST transitions with .earliest()
- Use local_now.format("%Y-%m-%d") for the date field
- Replace epoch math in weekly-digest with chrono::Local::now()
Add test verifying local-day boundaries contain "now" and span 24h.
…unwrap_or Address Gemini review feedback: - Extract shared local_today_boundaries() to eliminate duplication between handle_standup_report() and test - Replace unwrap_or(local_now) with expect() — midnight is always representable, and silently using "now" as "start of day" would produce wrong results - Update test to exercise the shared helper directly
The test_local_today_boundaries assertion used assert_eq!(span, 86400) which would fail on DST transition days (23h or 25h). Use a range assertion (82800..=90000) to accommodate both cases.
a847bf1 to
e9c2c9f
Compare
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a timezone bug in the +standup-report and +weekly-digest workflows by switching from UTC-based epoch calculations to using the local timezone for determining day and week boundaries. The introduction of the local_today_boundaries helper centralizes this logic. My main feedback is to make this new helper function more robust against panics in rare timezone edge cases to prevent the application from crashing.
| fn local_today_boundaries() -> (chrono::DateTime<chrono::Local>, chrono::DateTime<chrono::Local>) { | ||
| use chrono::{Local, NaiveTime, TimeZone}; | ||
|
|
||
| let local_now = Local::now(); | ||
| let today_midnight = local_now | ||
| .date_naive() | ||
| .and_time(NaiveTime::from_hms_opt(0, 0, 0).unwrap()); | ||
| let today_start = Local | ||
| .from_local_datetime(&today_midnight) | ||
| .earliest() | ||
| .expect("midnight should be representable in the local timezone"); | ||
| let today_end = today_start + chrono::Duration::days(1); | ||
| (today_start, today_end) | ||
| } |
There was a problem hiding this comment.
The use of .expect() here can cause a panic. While rare, it's possible for midnight not to exist in a local timezone on a specific day due to events like switching timezones or extreme daylight saving time adjustments (e.g., a time jump from 23:59 to 01:00). If from_local_datetime results in a LocalResult::None, earliest() will return None, and expect() will panic, crashing the program.
To make this function more robust, it should return a Result and propagate the error to the caller. The suggested change refactors the function to do this.
This will require updating the call site in handle_standup_report to use ? (i.e., let (today_start_local, today_end_local) = local_today_boundaries()?;). Additionally, the new test test_local_today_boundaries will need to be updated to handle the Result as well (e.g., by using .unwrap() since a panic is acceptable in a test context).
fn local_today_boundaries() -> Result<(chrono::DateTime<chrono::Local>, chrono::DateTime<chrono::Local>), GwsError> {
use chrono::{Local, NaiveTime, TimeZone};
let local_now = Local::now();
let today_midnight = local_now
.date_naive()
.and_time(NaiveTime::from_hms_opt(0, 0, 0).unwrap());
let today_start = Local
.from_local_datetime(&today_midnight)
.earliest()
.ok_or_else(|| {
GwsError::Other(anyhow::anyhow!(
"Could not determine start of today: midnight does not exist in local timezone for current date"
))
})?;
let today_end = today_start + chrono::Duration::days(1);
Ok((today_start, today_end))
}There was a problem hiding this comment.
good catch — switched from .expect() to .unwrap_or() with local_now as fallback, same pattern as the calendar fix in #443
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with timezone handling in the standup-report and weekly-digest workflows by switching from UTC-based epoch calculations to using the user's local timezone. The implementation is clean and includes a new helper function with corresponding tests. I have one critical suggestion to improve the robustness of the new helper function by handling a rare but possible edge case that could lead to a panic.
| fn local_today_boundaries() -> (chrono::DateTime<chrono::Local>, chrono::DateTime<chrono::Local>) { | ||
| use chrono::{Local, NaiveTime, TimeZone}; | ||
|
|
||
| let local_now = Local::now(); | ||
| let today_midnight = local_now | ||
| .date_naive() | ||
| .and_time(NaiveTime::from_hms_opt(0, 0, 0).unwrap()); | ||
| let today_start = Local | ||
| .from_local_datetime(&today_midnight) | ||
| .earliest() | ||
| .expect("midnight should be representable in the local timezone"); | ||
| let today_end = today_start + chrono::Duration::days(1); | ||
| (today_start, today_end) | ||
| } |
There was a problem hiding this comment.
Using .expect() here can cause a panic if the program is run on a day in a timezone where midnight doesn't exist (e.g., due to a DST transition). While rare, this would crash the CLI.
To make this more robust, I suggest changing this function to return a Result and gracefully handle the case where earliest() returns None.
You will also need to update the call site in handle_standup_report (line 280) to handle the Result (e.g., with ?) and update the test test_local_today_boundaries (line 751) to unwrap() the result.
fn local_today_boundaries() -> Result<(chrono::DateTime<chrono::Local>, chrono::DateTime<chrono::Local>), GwsError> {
use chrono::{Local, NaiveTime, TimeZone};
let local_now = Local::now();
let today_midnight = local_now
.date_naive()
.and_time(NaiveTime::from_hms_opt(0, 0, 0).unwrap());
let today_start = Local
.from_local_datetime(&today_midnight)
.earliest()
.ok_or_else(|| {
GwsError::Other(anyhow::anyhow!(
"Could not determine start of day in local timezone: midnight does not exist on this day"
))
})?;
let today_end = today_start + chrono::Duration::days(1);
Ok((today_start, today_end))
}There was a problem hiding this comment.
same as above, using .unwrap_or(local_now) now so it won't panic on weird DST days
…ions Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes #369 and #462.
…ions Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes #369 and #462.
…ions Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes #369 and #462.
…ions Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes #369 and #462.
…ions Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes #369 and #462.
…ions Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes #369 and #462.
…ions (#480) Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes #369 and #462. Co-authored-by: jpoehnelt-bot <jpoehnelt-bot@users.noreply.github.com>
…ions (googleworkspace#480) Replace machine-local chrono::Local and UTC epoch math with the authenticated user's Google account timezone (Calendar Settings API). - Add chrono-tz dependency for IANA timezone parsing - New src/timezone.rs: resolve timezone with priority: --timezone flag > 24h cache > Calendar API > local fallback - calendar.rs: add --timezone/--tz flag to +agenda - workflows.rs: fix +standup-report, +weekly-digest, +meeting-prep - auth_commands.rs: invalidate timezone cache on logout - Update README.md and AGENTS.md with timezone docs Supersedes googleworkspace#369 and googleworkspace#462. Co-authored-by: jpoehnelt-bot <jpoehnelt-bot@users.noreply.github.com>
Summary
+standup-reportcomputed "today" as UTC midnight-to-midnight ((now / 86400) * 86400), so users in non-UTC timezones got the wrong day's events. A user in UTC−8 running at 10 PM local time would see tomorrow's UTC events instead of today's.+weekly-digestused the same epoch arithmetic for its period start.chrono::Localfix from the calendar+agendahelper: derive day boundaries from wall-clock midnight, handle DST with.earliest().Changes
handle_standup_report(): replace epoch division withchrono::Localday boundarieshandle_weekly_digest(): replace epoch arithmetic withchrono::Local::now()+Duration::days(7)local_now.format("%Y-%m-%d")for the date field instead of splitting UTC RFC 3339test_standup_today_boundaries_use_local_timezoneverifying local-day boundaries span 24h and contain "now"Test plan
cargo test— 585 passed, 0 failedcargo clippy -- -D warnings— cleangws workflow +standup-reportin a non-UTC timezone and verify events match the local dayGenerated with Claude Code