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..cf2b826d4c 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. /// @@ -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 @@ -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 @@ -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, @@ -205,10 +207,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 +248,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..c8fb0108c1 100644 --- a/lib/features/trips/domain/entities/trip_day_weather.dart +++ b/lib/features/trips/domain/entities/trip_day_weather.dart @@ -8,14 +8,44 @@ 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 instant [dayMillis] denotes, read in UTC. +/// +/// 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); /// Deterministic row id for one trip day. /// @@ -27,8 +57,10 @@ int tripDayMillis(DateTime date) => /// 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'); 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,