fix(calendar): use local day boundaries for agenda date filters#369
fix(calendar): use local day boundaries for agenda date filters#369sonwr wants to merge 2 commits intogoogleworkspace:mainfrom
Conversation
|
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 date filtering, where previous implementations led to incorrect results in non-UTC timezones. By refactoring the date range computation to use local day boundaries and 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
|
![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 refactors the agenda date filtering logic to respect local timezones, fixing a bug where events could be shown on the wrong day. The new implementation is moved into separate, testable functions, and new unit tests have been added to verify the behavior with different timezone offsets. The changes are a significant improvement.
My review includes one suggestion to handle an edge case where a user might provide --days 0, which would currently lead to an API error. Adding a validation check for this would improve the user experience.
| .get_one::<String>("days") | ||
| .and_then(|s| s.parse::<u64>().ok()) | ||
| { | ||
| return local_date_range(&now.timezone(), local_date, days); |
There was a problem hiding this comment.
The code doesn't handle the case where a user provides --days 0. This will result in time_min and time_max being identical, which is invalid for the Google Calendar API (it requires timeMin to be smaller than timeMax). This will lead to an API error. It would be better to validate this input and return a user-friendly error.
A check should be added to ensure the number of days is positive.
if days == 0 {
return Err(GwsError::Validation("Number of days for agenda must be a positive integer.".to_string()));
}
return local_date_range(&now.timezone(), local_date, days);
sonwr
force-pushed
the
fix/agenda-local-day-boundaries
branch
from
573c978 to
236e70a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue with date filters in calendar +agenda by using local timezone boundaries instead of UTC-based calculations, which resolves incorrect behavior in non-UTC timezones. The logic has been cleanly refactored into new, well-tested functions. I have one high-severity suggestion to improve user experience by adding validation to enforce that the date-related command-line flags are mutually exclusive. This will prevent confusing behavior and provide clearer feedback to the user.
| use chrono::Days; | ||
|
|
||
| let local_date = now.date_naive(); |
There was a problem hiding this comment.
The date-related flags (--today, --tomorrow, --week, --days) are not defined as mutually exclusive in the clap argument setup. The current implementation creates an implicit precedence order (tomorrow > today > week > days) and silently ignores any lower-precedence flags if multiple are provided. This could be confusing for users.
To provide clearer feedback, I recommend adding validation at the beginning of this function to ensure only one of these flags is used at a time. If more than one is present, returning a GwsError::Validation error would inform the user about the invalid combination of arguments.
| use chrono::Days; | |
| let local_date = now.date_naive(); | |
| use chrono::Days; | |
| let flag_count = [ | |
| matches.get_flag("tomorrow"), | |
| matches.get_flag("today"), | |
| matches.get_flag("week"), | |
| matches.get_one::<String>("days").is_some(), | |
| ] | |
| .into_iter() | |
| .filter(|&f| f) | |
| .count(); | |
| if flag_count > 1 { | |
| return Err(GwsError::Validation( | |
| "The arguments --today, --tomorrow, --week, and --days are mutually exclusive." | |
| .to_string(), | |
| )); | |
| } | |
| let local_date = now.date_naive(); |
2 participants
This fixes
calendar +agendadate filters so they respect local day boundaries instead of using UTC/current-time windows.Fixes #259
What changed:
--todaynow queries from local midnight to the next local midnight--tomorrownow queries the next local calendar day--weeknow starts at local midnight today--days Nnow starts at local midnight today and spansNlocal calendar days+agendabehavior without date flags remains unchanged (nowtonow + 24h)Why:
The previous implementation used:
now..now+86400for--today/--days--tomorrowThat produced incorrect results in non-UTC timezones, especially later in the day, where
--todaycould include tomorrow’s events.Tests added:
test_compute_agenda_range_today_uses_local_midnight_boundariestest_compute_agenda_range_tomorrow_uses_next_local_daytest_compute_agenda_range_days_starts_at_local_midnight