From 61c28d013246c071a8957cec92f578d08973f4f9 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Thu, 27 Aug 2026 17:31:00 -0400 Subject: [PATCH 1/2] fix(trips): key a trip weather day in UTC, not the device timezone tripDayMillis built the key from a local DateTime(y, m, d), whose epoch value differs in every timezone. Two devices looking at the same trip day derived different keys, therefore different UUIDv5 row ids, and never converged: each stored and refetched its own copy. Divers cross timezones by definition, and one diver flying home is enough to trigger it. The key is now UTC midnight for the calendar day, taking the fields as given rather than converting: toUtc() would shift a late evening onto the next day, and the story hands over a date whose y/m/d is the day it means, matching the app's wall-clock-as-UTC convention for dive timestamps. Reading needed the inverse or the fix would have been half a fix. DateTime.fromMillisecondsSinceEpoch returns a LOCAL DateTime, so re-extracting y/m/d walked the day backwards on every negative offset. tripDayDate is that inverse, and the three read sites use it. The story view was computing the key inline instead of through the shared helper, so it would have looked up local midnight against UTC-keyed rows and silently rendered no badges at all. It goes through tripDayMillis now, which is what makes the lookup unable to drift from the write. Also updates the design doc, which still described the id as a v4 uuid and the date column as local midnight. --- ...6-08-26-trip-day-weather-storage-design.md | 13 +++++- .../trip_day_weather_repository.dart | 13 ++---- .../domain/entities/trip_day_weather.dart | 26 ++++++++++- .../widgets/story/trip_story_view.dart | 8 ++-- .../trip_day_weather_repository_test.dart | 36 ++++++++------- .../entities/trip_day_weather_test.dart | 44 +++++++++++++++++++ .../widgets/story/trip_story_view_test.dart | 2 +- 7 files changed, 109 insertions(+), 33 deletions(-) diff --git a/docs/superpowers/specs/2026-08-26-trip-day-weather-storage-design.md b/docs/superpowers/specs/2026-08-26-trip-day-weather-storage-design.md index 7173932653..7feed8bc67 100644 --- a/docs/superpowers/specs/2026-08-26-trip-day-weather-storage-design.md +++ b/docs/superpowers/specs/2026-08-26-trip-day-weather-storage-design.md @@ -63,9 +63,9 @@ New table `TripDayWeather` in `lib/core/database/database.dart`: | Column | Type | Notes | | --- | --- | --- | -| `id` | text, pk | uuid v4 | +| `id` | text, pk | deterministic UUIDv5 over (`tripId`, day), via `tripDayWeatherRowId` | | `tripId` | text | references `Trips(#id)` | -| `date` | int | epoch **milliseconds** at local midnight, matching what `ItineraryDayRepository` writes for `trip_itinerary_days.date` (the column comment there says "Unix timestamp", but the repository writes `millisecondsSinceEpoch`) | +| `date` | int | epoch **milliseconds** at **UTC** midnight for the calendar day, via `tripDayMillis`. Milliseconds because that is what `ItineraryDayRepository` writes for `trip_itinerary_days.date`, despite that column comment saying "Unix timestamp". UTC rather than local midnight because the value is part of the row identity: a local midnight epoch differs in every timezone, so two devices would key the same trip day differently and never converge | | `latitude` | real | the coordinate the lookup used | | `longitude` | real | the coordinate the lookup used | | `airTemp` | real, nullable | celsius | @@ -84,6 +84,15 @@ New table `TripDayWeather` in `lib/core/database/database.dart`: Unique index on (`tripId`, `date`). +The id is **not** a v4 uuid. It is derived from the day it describes, +`UUIDv5(namespace, "$tripId|$dayMillis")`, following the same convention as +`importedDiveComputerId` and `qualityFindingId`. A per-device v4 would let two +devices store the same day under different primary keys; the serializer upserts +by primary key, so the peer's row would miss the `ON CONFLICT` target and hit +the unique index instead, throwing inside the merge transaction and aborting +the whole sync pull. The repository derives the id itself and ignores whatever +a caller passes. + The stored field set is the full `WeatherData` payload, not just the three fields the day header renders. The API returns them in one response at no extra cost, dive rows already store exactly this set, and a later migration to diff --git a/lib/features/trips/data/repositories/trip_day_weather_repository.dart b/lib/features/trips/data/repositories/trip_day_weather_repository.dart index 046e2030c7..ffa4392cd4 100644 --- a/lib/features/trips/data/repositories/trip_day_weather_repository.dart +++ b/lib/features/trips/data/repositories/trip_day_weather_repository.dart @@ -10,7 +10,7 @@ import 'package:submersion/core/services/sync/sync_event_bus.dart'; import 'package:submersion/features/trips/domain/entities/trip_day_weather.dart' as domain; import 'package:submersion/features/trips/domain/entities/trip_day_weather.dart' - show tripDayMillis, tripDayWeatherRowId; + show tripDayDate, tripDayMillis, tripDayWeatherRowId; /// Reads and writes stored per-day trip weather. /// @@ -50,7 +50,7 @@ class TripDayWeatherRepository { final winners = {}; for (final row in rows) { - final day = _dayKey(DateTime.fromMillisecondsSinceEpoch(row.date)); + final day = _dayKey(tripDayDate(row.date)); final held = winners[day]; winners[day] = held == null ? row @@ -205,10 +205,7 @@ class TripDayWeatherRepository { _db.tripDayWeather, )..where((t) => t.tripId.equals(tripId))).get(); return rows - .where( - (r) => - _dayKey(DateTime.fromMillisecondsSinceEpoch(r.date)) == dayMillis, - ) + .where((r) => _dayKey(tripDayDate(r.date)) == dayMillis) .toList(); } @@ -249,9 +246,7 @@ class TripDayWeatherRepository { // written by an older build or an out-of-date peer can still carry a // time component, and handing that back would put time-bearing dates // into downstream logic. - date: DateTime.fromMillisecondsSinceEpoch( - _dayKey(DateTime.fromMillisecondsSinceEpoch(row.date)), - ), + date: tripDayDate(_dayKey(tripDayDate(row.date))), latitude: row.latitude, longitude: row.longitude, airTemp: row.airTemp, diff --git a/lib/features/trips/domain/entities/trip_day_weather.dart b/lib/features/trips/domain/entities/trip_day_weather.dart index 227b404b6b..7d16ac340c 100644 --- a/lib/features/trips/domain/entities/trip_day_weather.dart +++ b/lib/features/trips/domain/entities/trip_day_weather.dart @@ -8,14 +8,36 @@ import 'package:submersion/features/trips/domain/entities/trip_story_day.dart'; /// Never change: the ids already stored depend on it. const String kTripDayWeatherNamespace = '3f1c8a52-9e47-4d6b-8b3a-16c9d0f27e45'; -/// Local midnight for [date], as epoch milliseconds. +/// The calendar day of [date] as epoch milliseconds at UTC midnight. /// /// The day is the identity of a weather row, so this is what turns a /// DateTime into one. Shared rather than reimplemented per caller: the row /// id, the repository's storage key, and the map key reads come back under /// all have to agree, and three copies of the same two lines would drift. +/// +/// UTC, not local midnight, and the distinction is the whole point. A local +/// `DateTime(y, m, d)` has a different epoch value in every timezone, so two +/// devices looking at the same trip day would derive different keys, and +/// therefore different row ids, and never converge: each would store and +/// refetch its own copy of the day. Divers cross timezones by definition, and +/// one diver flying home is enough to trigger it. +/// +/// The calendar fields are taken as given rather than converted. `toUtc()` +/// would shift a late evening onto the following day; the trip story already +/// hands over a date whose y/m/d is the day it means, matching the app's +/// wall-clock-as-UTC convention for dive timestamps. int tripDayMillis(DateTime date) => - DateTime(date.year, date.month, date.day).millisecondsSinceEpoch; + DateTime.utc(date.year, date.month, date.day).millisecondsSinceEpoch; + +/// The calendar day [dayMillis] denotes, as a UTC DateTime at midnight. +/// +/// The inverse of [tripDayMillis], and the only correct way to read a stored +/// day back. `DateTime.fromMillisecondsSinceEpoch` without `isUtc` returns a +/// LOCAL DateTime, so re-extracting y/m/d from it reads the calendar fields in +/// the device's frame: on any negative UTC offset, UTC midnight is the +/// previous evening locally, and the day walks backwards on every round trip. +DateTime tripDayDate(int dayMillis) => + DateTime.fromMillisecondsSinceEpoch(dayMillis, isUtc: true); /// Deterministic row id for one trip day. /// diff --git a/lib/features/trips/presentation/widgets/story/trip_story_view.dart b/lib/features/trips/presentation/widgets/story/trip_story_view.dart index a426887884..c687480488 100644 --- a/lib/features/trips/presentation/widgets/story/trip_story_view.dart +++ b/lib/features/trips/presentation/widgets/story/trip_story_view.dart @@ -249,9 +249,11 @@ class _TripStoryViewState extends ConsumerState Map storedWeather, ) { final day = story.days[index]; - // Stored rows are keyed on local midnight millis. - final dayDate = DateTime(day.date.year, day.date.month, day.date.day); - final stored = storedWeather[dayDate.millisecondsSinceEpoch]; + // Keyed through the same helper the repository stores under, so the + // lookup cannot drift from the write. Computing the key inline here was + // how the two came apart: it silently found nothing and every badge + // disappeared. + final stored = storedWeather[tripDayMillis(day.date)]; final showTodayDivider = todayIndex != null && index == todayIndex; const divider = SliverPadding( padding: EdgeInsets.symmetric(horizontal: 16), diff --git a/test/features/trips/data/repositories/trip_day_weather_repository_test.dart b/test/features/trips/data/repositories/trip_day_weather_repository_test.dart index 4379f53a3d..6b314b5722 100644 --- a/test/features/trips/data/repositories/trip_day_weather_repository_test.dart +++ b/test/features/trips/data/repositories/trip_day_weather_repository_test.dart @@ -89,14 +89,15 @@ void main() { final stored = await repository.getForTrip(testTripId); expect(stored, hasLength(1)); - final row = stored[day1.millisecondsSinceEpoch]!; + final row = stored[tripDayMillis(day1)]!; expect(row.airTemp, 21.5); expect(row.cloudCover, CloudCover.clear); expect(row.windDirection, CurrentDirection.north); expect(row.weatherCode, 0); expect(row.weatherSource, WeatherSource.openMeteo); expect(row.latitude, 12.16); - expect(row.date, day1); + // A calendar day in a device-independent frame, so UTC. + expect(row.date, DateTime.utc(2026, 3, 8)); }); test('a null payload field round-trips as null', () async { @@ -104,7 +105,7 @@ void main() { final row = (await repository.getForTrip( testTripId, - ))[day1.millisecondsSinceEpoch]!; + ))[tripDayMillis(day1)]!; expect(row.airTemp, isNull); expect(row.cloudCover, isNull); @@ -120,21 +121,21 @@ void main() { final stored = await repository.getForTrip(testTripId); expect(stored, hasLength(1)); - expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 25); + expect(stored[tripDayMillis(day1)]!.airTemp, 25); }); - test('a date with a time component is stored under local midnight', () { + test('a date with a time component is stored under UTC midnight', () { // The repository owns the (trip, date) uniqueness invariant, so it // normalizes rather than trusting every caller to. A row keyed on a - // stray time would be invisible to midnight-keyed lookups and would - // refetch forever. + // stray time would be invisible to day-keyed lookups and would refetch + // forever. UTC, so the key does not move with the device timezone. return () async { await repository.upsert(sample(date: DateTime(2026, 3, 8, 17, 30))); final stored = await repository.getForTrip(testTripId); - expect(stored.keys.single, day1.millisecondsSinceEpoch); - expect(stored[day1.millisecondsSinceEpoch]!.date, day1); + expect(stored.keys.single, tripDayMillis(day1)); + expect(stored[tripDayMillis(day1)]!.date, DateTime.utc(2026, 3, 8)); }(); }); @@ -147,7 +148,7 @@ void main() { final stored = await repository.getForTrip(testTripId); expect(stored, hasLength(1)); - expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 25); + expect(stored[tripDayMillis(day1)]!.airTemp, 25); }); test('two different days both persist', () async { @@ -157,7 +158,7 @@ void main() { final stored = await repository.getForTrip(testTripId); expect(stored, hasLength(2)); - expect(stored[day2.millisecondsSinceEpoch]!.airTemp, 19); + expect(stored[tripDayMillis(day2)]!.airTemp, 19); }); test('getForTrip is scoped to one trip', () async { @@ -271,7 +272,7 @@ void main() { final rows = await allRows(); expect(rows, hasLength(1)); - expect(rows.single.date, day1.millisecondsSinceEpoch); + expect(rows.single.date, tripDayMillis(day1)); expect(rows.single.airTemp, 25); }); @@ -333,20 +334,23 @@ void main() { final stored = await repository.getForTrip(testTripId); expect(stored, hasLength(1)); - expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 25); + expect(stored[tripDayMillis(day1)]!.airTemp, 25); }, ); test('getForTrip falls back to the most recently updated stray', () async { + // Off-midnight strays within the same UTC day. "Same day" is a UTC + // question now: a row stored at 23:00 local on a negative offset falls + // on the following UTC day and is genuinely a different day's row. await insertRaw( id: 'peer-a', - date: DateTime(2026, 3, 8, 6), + date: DateTime.utc(2026, 3, 8, 6), airTemp: 10, updatedAt: 100, ); await insertRaw( id: 'peer-b', - date: DateTime(2026, 3, 8, 23), + date: DateTime.utc(2026, 3, 8, 23), airTemp: 20, updatedAt: 200, ); @@ -354,7 +358,7 @@ void main() { final stored = await repository.getForTrip(testTripId); expect(stored, hasLength(1)); - expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 20); + expect(stored[tripDayMillis(day1)]!.airTemp, 20); }); test('getForTrip does not write while resolving strays', () async { diff --git a/test/features/trips/domain/entities/trip_day_weather_test.dart b/test/features/trips/domain/entities/trip_day_weather_test.dart index 5b9334fd53..c3a8bb8f38 100644 --- a/test/features/trips/domain/entities/trip_day_weather_test.dart +++ b/test/features/trips/domain/entities/trip_day_weather_test.dart @@ -28,6 +28,50 @@ void main() { ); } + group('tripDayMillis', () { + test('is the calendar day in UTC, not the device local midnight', () { + // The key must not depend on where the device is standing. A local + // DateTime(y, m, d) has a different epoch value in every timezone, so + // two devices would derive different row ids for the same calendar day + // and never converge. + expect( + tripDayMillis(DateTime(2026, 3, 8, 17, 30)), + DateTime.utc(2026, 3, 8).millisecondsSinceEpoch, + ); + }); + + test('takes the calendar fields as given, never shifting the day', () { + // Guards the wrong fix: converting with toUtc() would move a late + // evening local time onto the following calendar day. + expect( + tripDayMillis(DateTime(2026, 3, 8, 23, 59)), + tripDayMillis(DateTime.utc(2026, 3, 8, 0, 1)), + ); + }); + + test('distinct days stay distinct', () { + expect( + tripDayMillis(DateTime(2026, 3, 8)), + isNot(tripDayMillis(DateTime(2026, 3, 9))), + ); + }); + }); + + group('tripDayWeatherRowId', () { + test('is stable for one calendar day regardless of the time given', () { + expect( + tripDayWeatherRowId( + tripId: 't1', + dayMillis: tripDayMillis(DateTime(2026, 3, 8, 1)), + ), + tripDayWeatherRowId( + tripId: 't1', + dayMillis: tripDayMillis(DateTime(2026, 3, 8, 22)), + ), + ); + }); + }); + group('hasRenderableWeather', () { test('air temperature alone counts', () { expect(weather(airTemp: 24).hasRenderableWeather, isTrue); diff --git a/test/features/trips/presentation/widgets/story/trip_story_view_test.dart b/test/features/trips/presentation/widgets/story/trip_story_view_test.dart index b7792afa8e..8083c1df94 100644 --- a/test/features/trips/presentation/widgets/story/trip_story_view_test.dart +++ b/test/features/trips/presentation/widgets/story/trip_story_view_test.dart @@ -426,7 +426,7 @@ void main() { tester, story, tripDayWeather: { - surfaceDate.millisecondsSinceEpoch: TripDayWeather( + tripDayMillis(surfaceDate): TripDayWeather( id: 'w1', tripId: trip.id, date: surfaceDate, From 70ca2ba23a0cdbb4a2c9a04d9a4cfaefcab20644 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Thu, 27 Aug 2026 17:40:11 -0400 Subject: [PATCH 2/2] docs(trips): correct the day-key comments the UTC move left behind tripDayDate was documented as returning "a UTC DateTime at midnight". It returns the instant it is given, read in UTC, and normalizes nothing, so it is the inverse of tripDayMillis only for a value tripDayMillis produced. The repository deliberately calls it on raw stored dates that can carry a time component, so the claim was not merely loose. It now says what the function does and tells callers to run the result through tripDayMillis when they want the day rather than the instant. Three more comments still described the local-midnight scheme this branch replaced: tripDayWeatherRowId told callers the day key "must already be normalized to local midnight", which is the exact mistake the branch exists to fix. A caller who followed it would derive a per-timezone id and lose the convergence the deterministic id buys. TripDayWeatherRepository._dayKey was still headed "Local midnight for [date]" while delegating to a UTC helper. _rowsForDay justified filtering in Dart by claiming SQLite cannot derive local midnight without the zone and its DST history. That reason died with the UTC move: the day is now integer arithmetic on the stored millis. The filter stays in Dart because a trip holds a few dozen rows and one Dart function cannot drift from tripDayMillis, which is the honest reason. No behaviour change. --- .../trip_day_weather_repository.dart | 12 +++++---- .../domain/entities/trip_day_weather.dart | 26 +++++++++++++------ 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/lib/features/trips/data/repositories/trip_day_weather_repository.dart b/lib/features/trips/data/repositories/trip_day_weather_repository.dart index ffa4392cd4..cf2b826d4c 100644 --- a/lib/features/trips/data/repositories/trip_day_weather_repository.dart +++ b/lib/features/trips/data/repositories/trip_day_weather_repository.dart @@ -22,7 +22,7 @@ class TripDayWeatherRepository { final SyncRepository _syncRepository = SyncRepository(); final _log = LoggerService.forClass(TripDayWeatherRepository); - /// Local midnight for [date], as epoch milliseconds. + /// The UTC-midnight day key for [date], as epoch milliseconds. /// /// The day is the identity, so normalizing here is what actually enforces /// the (trip, date) uniqueness intent. A caller that passes a DateTime with @@ -193,10 +193,12 @@ class TripDayWeatherRepository { /// Every stored row for this trip that falls on [dayMillis]'s calendar day. /// - /// Filtered in Dart rather than SQL: local midnight is not something SQLite - /// can derive from the stored epoch millis without knowing the zone and its - /// DST history. A trip holds one row per day, so the scan is a few dozen - /// rows at most. + /// Filtered in Dart rather than SQL. It could be pushed down now that the + /// key is UTC, since the day is plain integer arithmetic on the stored + /// millis with no zone or DST to consult, but there is nothing to gain: a + /// trip holds one row per day, so the scan is a few dozen rows, and keeping + /// the rule in one Dart function is what stops it drifting from + /// [tripDayMillis]. Future> _rowsForDay({ required String tripId, required int dayMillis, diff --git a/lib/features/trips/domain/entities/trip_day_weather.dart b/lib/features/trips/domain/entities/trip_day_weather.dart index 7d16ac340c..c8fb0108c1 100644 --- a/lib/features/trips/domain/entities/trip_day_weather.dart +++ b/lib/features/trips/domain/entities/trip_day_weather.dart @@ -29,13 +29,21 @@ const String kTripDayWeatherNamespace = '3f1c8a52-9e47-4d6b-8b3a-16c9d0f27e45'; int tripDayMillis(DateTime date) => DateTime.utc(date.year, date.month, date.day).millisecondsSinceEpoch; -/// The calendar day [dayMillis] denotes, as a UTC DateTime at midnight. +/// The instant [dayMillis] denotes, read in UTC. /// -/// The inverse of [tripDayMillis], and the only correct way to read a stored -/// day back. `DateTime.fromMillisecondsSinceEpoch` without `isUtc` returns a -/// LOCAL DateTime, so re-extracting y/m/d from it reads the calendar fields in -/// the device's frame: on any negative UTC offset, UTC midnight is the -/// previous evening locally, and the day walks backwards on every round trip. +/// The only correct way to read a stored day back. `fromMillisecondsSinceEpoch` +/// without `isUtc` returns a LOCAL DateTime, so re-extracting y/m/d from it +/// reads the calendar fields in the device's frame: on any negative UTC +/// offset, UTC midnight is the previous evening locally, and the day walks +/// backwards on every round trip. +/// +/// It reads the value it is given and normalizes nothing, so it is the +/// inverse of [tripDayMillis] only for a value [tripDayMillis] produced. A +/// stored `date` is not guaranteed to be one: rows written before this branch +/// derived ids can carry a time component, and the repository deliberately +/// calls this on those raw values. Run the result back through +/// [tripDayMillis] whenever you need the normalized day rather than the +/// instant as stored. DateTime tripDayDate(int dayMillis) => DateTime.fromMillisecondsSinceEpoch(dayMillis, isUtc: true); @@ -49,8 +57,10 @@ DateTime tripDayDate(int dayMillis) => /// target entirely and hits the index instead. That throws inside the merge /// transaction and aborts the whole sync pull. /// -/// [dayMillis] must already be normalized to local midnight; pass it through -/// [tripDayMillis]. +/// [dayMillis] must already be the normalized UTC-midnight day key; pass it +/// through [tripDayMillis]. Local midnight would defeat the purpose: its epoch +/// value differs in every timezone, so the derived id would too and the two +/// devices this exists to converge would not. String tripDayWeatherRowId({required String tripId, required int dayMillis}) => const Uuid().v5(kTripDayWeatherNamespace, '$tripId|$dayMillis');