State the trip day-key contract where callers actually read it - #1338
Merged
Merged
Conversation
Three doc comments still said the stored-weather map is keyed by date.millisecondsSinceEpoch, and the entity still called its date field local midnight. Both were true before the key moved to UTC and are now exactly the wrong thing to believe: local-midnight millis differ in every timezone, so a caller keying a lookup that way finds nothing and the day renders no badge. Nothing throws, which is what makes a stale comment here worth fixing rather than tolerating. TripDayWeatherTarget.date genuinely is local midnight and stays that way: localNoon reads its calendar fields to ask the archive for noon at the dive site. Its doc now says why that is unrelated to identity, since it sits one call away from the key derivation and is the easiest place to conflate the two.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates documentation comments to accurately state the UTC-based “trip day key” contract used for stored trip-day weather lookups, preventing silent mismatches when callers use local-midnight epoch millis.
Changes:
- Clarify that trip-day weather is keyed by
tripDayMillis(date)(UTC-midnight day key), notdate.millisecondsSinceEpoch. - Explain why local-midnight keys silently miss stored rows on non-UTC devices.
- Distinguish between “local midnight is fine for archive lookup inputs” vs “UTC-midnight is required for storage identity”.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/features/trips/presentation/providers/trip_day_weather_providers.dart | Updates provider doc comment to state the UTC key contract and the failure mode when mis-keyed. |
| lib/features/trips/domain/services/trip_day_weather_backfill.dart | Clarifies why the backfill target date is local-midnight for archive querying but not identity. |
| lib/features/trips/domain/entities/trip_day_weather.dart | Updates entity doc comment for the day/identity contract (noting UTC normalization). |
| lib/features/trips/data/repositories/trip_day_weather_repository.dart | Updates repository doc comment to state the UTC key contract and silent-miss behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The entity's date doc asserted the field is UTC midnight, which the type does not enforce and one in-tree writer does not honour: the backfill builds a row with the target's local DateTime(y, m, d), and only the repository hands back a normalized UTC instant. A comment stating an invariant the code does not hold is worse than none, because it is what a reader keys their next lookup off. It now states the contract that is actually enforced. Identity is tripDayMillis(date), which reads y/m/d and pins them to UTC midnight, and every path that stores, keys, or looks a row up derives it that way, so the differing isUtc flag is invisible to all of them. With one exception, now named: date is in props, and DateTime equality compares the epoch value and isUtc, so two entities for the same day compare unequal across provenance. That is the same silent shape as the key bug this branch fixed. A test asserting entity equality across a repository read and a freshly fetched row fails for a reason that looks nothing like a timezone problem, so the doc says to compare day keys. Comments only. flutter analyze clean, 634 trips tests pass.
Contributor
|
📦 Build artifacts for this PR · commit
Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the Updated automatically on each push. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-project-automation
Bot
moved this from In review
to Done
in Submersion Release Tracker
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the remaining review comments on #1319. Stacked on
worktree-trip-day-weather. Comments only, no behaviour change.What was stale
Moving the day key to UTC left three doc comments describing the old contract:
tripDayWeatherProvider: "keyed bydate.millisecondsSinceEpoch"TripDayWeatherRepository.getForTrip: the sameTripDayWeather.date: "Local midnight for the day this describes"All three were true before the key moved and are now exactly the wrong thing to believe. This is worth fixing rather than tolerating because the failure is silent: a caller that keys a lookup with local-midnight millis finds nothing on any device not running UTC, and the day just renders no badge. Nothing throws. That is the same shape as the bug caught during the UTC change, where the story view computed the key inline and would have dropped every badge.
Each comment now states the contract and why it is UTC.
One that was correct and stays
TripDayWeatherTarget.dategenuinely is local midnight, and should be:localNoonreads its calendar fields to ask the archive for noon at the dive site. But it sits one call away from the key derivation, so it is the easiest place to conflate the two. Its doc now says why local is right there and irrelevant to identity: nothing keys a stored row off that instant, andtripDayMillistakes the same calendar fields and pins them to UTC.Already fixed
The fourth comment, on the backfill test seeding
storedwith local-midnight millis, was already addressed on the branch by "fix(trips): ask for a stored day with the key it was stored under". That fixture keys throughtripDayMillisnow and carries a comment explaining why any other key would assert a contract production never offers. Nothing further needed.634 trips tests pass.
flutter analyzeclean.