fix: timezone awareness during atuin search - #3762
Conversation
Greptile SummaryThis PR propagates the configured timezone into non-interactive history filtering.
Confidence Score: 3/5This should be fixed before merging because timezone-less filters still select the wrong one-hour window when the requested date has a different DST offset. The new path applies the current local UTC offset to every parsed date, but a fixed offset cannot represent the date-dependent DST rules needed for historical and future boundaries. Files Needing Attention: crates/atuin-client/src/database.rs, crates/atuin-common/src/time/utc_offset.rs Important Files Changed
Reviews (1): Last reviewed commit: "removed redudant test, not exactly sure ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| })?; | ||
| let parsed = interim::parse_date_string( | ||
| before, | ||
| OffsetDateTime::now_utc().to_offset(filter_options.timezone.0), |
There was a problem hiding this comment.
When a timezone-less filter targets a date whose DST offset differs from the offset resolved at startup, parsing applies the current fixed offset instead, shifting the search boundary by an hour and returning the wrong history window.
Knowledge Base Used: Client Core (atuin-client)
There was a problem hiding this comment.
woudn't full DST support require adding a new dependency like jiff to get a named timezone no? wouldn't this PR still resolve bare strings defaulting to UTC instead of local time, the common case?
There was a problem hiding this comment.
You're right on both counts. Full DST-aware parsing would need a named timezone (e.g., America/New_York) so the library can look up the correct historical offset — that's a separate dependency and a larger change.
This PR does fix the primary regression: bare strings like "yesterday" or "last week" were always resolved against UTC, which is wrong for anyone not in UTC. Using the configured timezone offset is a clear improvement for the common case, even if a fixed offset won't handle DST boundaries perfectly.
My concern is a narrow edge case (searches crossing a DST transition), not a blocker. The PR is a net improvement.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
There was a problem hiding this comment.
You're right. Full DST-correct parsing requires a named IANA timezone (e.g. America/New_York) so the library can look up the historically correct offset for the parsed date — a fixed UtcOffset can't do that. That would need something like jiff or chrono-tz, which is a separate, more invasive change.
This PR does fix the primary bug: bare date strings like "yesterday" or "last week" were anchored to UTC instead of local time, which is the common case and the root cause of #2808 and #2607. The DST edge case is a much narrower failure mode.
My comment was valid but shouldn't block this — it's an incremental improvement over the current behavior. Sorry for the noise.
Shall I remember this as a rule: "Accept timezone-related fixes that improve the common case (local time vs UTC), even if they don't handle DST edge cases, unless the repo has an existing named-timezone dependency available"?
…DateTime::now_utc() is used when relative is included
8f0134f to
57d6548
Compare
57d6548 to
209f0f2
Compare
1 participant
Fixes #2808
Fixes #2607
problem
when we filter through it always pass UTC as fallback when library can't figure out which timezone we are in causing the wrong window to be selected
solution
pass on the timezone from settings so that is the fallback.
Timezone support in filtering:
timezonefield of from settings when library cannot figure out what timezone the requested filter is in.Checks